librbd: properly order concurrent updates to the object map #12420
Conversation
librbd: properly order concurrent updates to the object map #12420
| }; | ||
|
|
||
| TEST_F(TestIOBlockGuard, NonDetainedOps) { | ||
| OpBlockGuard op_block_uard(m_cct); |
There was a problem hiding this comment.
super minor: typo? op_block_guard?
There was a problem hiding this comment.
@vshankar thx -- regex replace fail apparently
| @@ -0,0 +1,272 @@ | |||
| // -*- mode:C; tab-width:8; c-basic-offset:2; indent-tabs-mode:t -*- | |||
There was a problem hiding this comment.
@dillaman super minor (not worth to repush, may be a separate commit someday to fix all the affected files):
It should be 'mode:C++' (causes some troubles to me when editing these files in Emacs).
Also, while we are here, 'indent-tabs-mode:t' makes my Emacs to use tabs, while I see the tabs are converted to spaces after your edit. I like spaces more too, though this statement overrides my emacs config...
There was a problem hiding this comment.
I'll fix it -- I always just copy-n-paste those from another file, so obviously there is some cleanup that needs to be done.
|
|
||
| // alert the caller that the IO was detained | ||
| *cell = nullptr; | ||
| return 1; |
There was a problem hiding this comment.
May be return block_operations.size() instead (might be useful for debugging, e.g could be printed in detained_aio_update)?
Also, your implementation does not return an error. Why declare this in the interface? It looks like it only adds unnecessary check in detained_aio_update.
There was a problem hiding this comment.
I based this on another implementation in my image cache branch that actually can restrict the number of in-flight ops (returns -ENOMEM). I figured eventually the other (more complicated) version will come back, so I might as well support it now instead of having to fix it later.
|
|
||
| Operation op3; | ||
| BlockGuardCell *cell3; | ||
| ASSERT_EQ(1, op_block_guard.detain({0, 2}, &op3, &cell3)); |
| @@ -0,0 +1,98 @@ | |||
| // -*- mode:C; tab-width:8; c-basic-offset:2; indent-tabs-mode:t -*- | |||
There was a problem hiding this comment.
And then this would be nice to fix too
Concurrent IO to the same object would previously result in the first IO pausing to update the object map while the other IO would proceed to directly update the object before the object map state was properly updated. Fixes: http://tracker.ceph.com/issues/16176 Signed-off-by: Jason Dillaman <dillaman@redhat.com>
Signed-off-by: Jason Dillaman <dillaman@redhat.com>
Signed-off-by: Jason Dillaman <dillaman@redhat.com>
Signed-off-by: Jason Dillaman <dillaman@redhat.com>
…ject Signed-off-by: Jason Dillaman <dillaman@redhat.com>
|
@trociny done |
|
retest this please |
|
@ceph-jenkins retest this please |
In master, the "batch update" change [1] was merged before the "order concurrent updates" [2], while in jewel the latter is already backported [3]. A partial backport of [1] was attempted, but the automated cherry-pick missed some parts of it which this commit is adding manually. [1] ceph#11510 [2] ceph#12420 [3] ceph#12909 Signed-off-by: Mykola Golub <mgolub@mirantis.com> Signed-off-by: Nathan Cutler <ncutler@suse.com>
In master, the "batch update" change [1] was merged before the "order concurrent updates" [2], while in jewel the latter is already backported [3]. A backport of [1] to jewel was attempted, and was necessarily applied on top of [3] - i.e. in the reverse order compared to how the commits went into master. This reverse ordering caused the automated cherry-pick to miss some parts of [1] which this commit is adding manually. [1] ceph#11510 [2] ceph#12420 [3] ceph#12909 Signed-off-by: Mykola Golub <mgolub@mirantis.com> Signed-off-by: Nathan Cutler <ncutler@suse.com>
No description provided.