test: fix rbd unit test cases w/ striping feature #13196
Conversation
src/test/librbd/test_librbd.cc
Outdated
| @@ -3419,7 +3423,7 @@ TEST_F(TestLibRBD, FlattenNoEmptyObjects) | |||
| librbd::RBD rbd; | |||
| std::string parent_name = get_temp_image_name(); | |||
| uint64_t size = 4 << 20; | |||
| int order = 12; // smallest object size is 4K | |||
| int order = 0; | |||
There was a problem hiding this comment.
W/ the default order (4MB) and an image size of 4MB, would you expect any empty objects?
There was a problem hiding this comment.
Thanks for catching this. Will fix and update.
There was a problem hiding this comment.
Sorry for the delay - will send out the fix in a while.
src/librbd/AioImageRequestWQ.cc
Outdated
| @@ -284,6 +288,10 @@ void AioImageRequestWQ::set_require_lock_on_read() { | |||
| ldout(cct, 20) << __func__ << dendl; | |||
|
|
|||
| RWLock::WLocker locker(m_lock); | |||
| if (m_require_lock_on_read) { | |||
There was a problem hiding this comment.
Nit: doesn't really protect against anything :-p
src/librbd/AioImageRequestWQ.cc
Outdated
| @@ -111,6 +111,10 @@ void AioImageRequestWQ::aio_read(AioCompletion *c, uint64_t off, uint64_t len, | |||
|
|
|||
| RWLock::RLocker owner_locker(m_image_ctx.owner_lock); | |||
|
|
|||
| if (m_image_ctx.clone_copy_on_read && is_lock_required()) { | |||
There was a problem hiding this comment.
Probably better to combine this with the existing logic in librbd::image::RefreshRequest to avoid having this flag cleared should journaling be disabled while a read IO is queued. Probably can avoid the other changes in this file if that change is made.
vshankar
force-pushed
the
rbd-ut-fix
branch
2 times, most recently
from
dc852a6
to
7553d29
Compare
src/librbd/ImageCtx.cc
Outdated
| @@ -596,7 +596,7 @@ struct C_InvalidateCache : public Context { | |||
| bool ImageCtx::test_features(uint64_t in_features, | |||
| const RWLock &in_snap_lock) const | |||
| { | |||
| assert(snap_lock.is_locked()); | |||
| assert(in_snap_lock.is_locked()); | |||
There was a problem hiding this comment.
The convention of passing in a reference of a lock is just a way to indicate that this method requires that you have already locked the lock. The assertion just verifies that fact. With this change, I could now invoke test_features(XYZ, owner_lock) and the assert will pass if I have ImageCtx::owner_lock locked without locking the correct ImageCtx::snap_lock.
There was a problem hiding this comment.
Thanks for the explanation. It looked like a harmless typo to me given that in_snap_lock is never used in the function. Plus it looked a bit unnecessary to accept a lock as a parameter and check if its the lock is already acquired. (I had put up a comment in github regarding this but never published it)
I'll revert this change and resubmit.
Fixes: http://tracker.ceph.com/issues/18888 Signed-off-by: Venky Shankar <vshankar@redhat.com>
Signed-off-by: Venky Shankar <vshankar@redhat.com>
Signed-off-by: Venky Shankar <vshankar@redhat.com>
|
@dillaman resent the PR with the unwanted commit removed. |
dillaman
changed the title
Run RBD unit tests additionally with RBD_FEATURES=127 (including STRIPINGV2).
Bonus fix for grabbing exclusive-lock during copy on read (required for running unit tests with changes in this pr).