New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

jewel: rbd: object-map: batch updates during trim operation #15460

Merged
merged 2 commits into from Aug 28, 2017

Conversation

Projects
None yet
5 participants
@smithfarm
Contributor

smithfarm commented Jun 4, 2017

@smithfarm smithfarm self-assigned this Jun 4, 2017

@smithfarm smithfarm added this to the jewel milestone Jun 4, 2017

@smithfarm smithfarm changed the title from jewel: object-map: batch updates during trim operation to jewel: rbd: object-map: batch updates during trim operation Jul 12, 2017

@@ -331,11 +331,14 @@ class AioObjectRemove : public AbstractAioObjectWrite {
class AioObjectTrim : public AbstractAioObjectWrite {
public:
// we'd need to only conditionally specify if a post object map

This comment has been minimized.

@amitkumar50

amitkumar50 Aug 16, 2017

Contributor

@smithfarm
I believe multi-line comment should be inside /* */
Please replace we'd with we would

@amitkumar50

amitkumar50 Aug 16, 2017

Contributor

@smithfarm
I believe multi-line comment should be inside /* */
Please replace we'd with we would

@smithfarm

This comment has been minimized.

Show comment
Hide comment
@smithfarm

smithfarm Aug 16, 2017

Contributor

@amitkumar50 Backports are cherry-picked from master, so any changes would have to go into master, first.

Regarding contractions, see the discussion in #16705

Contributor

smithfarm commented Aug 16, 2017

@amitkumar50 Backports are cherry-picked from master, so any changes would have to go into master, first.

Regarding contractions, see the discussion in #16705

@smithfarm smithfarm requested review from dillaman and trociny Aug 25, 2017

@smithfarm

This comment has been minimized.

Show comment
Hide comment
@smithfarm
Contributor

smithfarm commented Aug 25, 2017

@trociny

This comment has been minimized.

Show comment
Hide comment
@trociny

trociny Aug 25, 2017

Contributor

All tests failed for ceph_test_librbd_fsx like below:

2017-08-25T03:23:10.028 INFO:teuthology.orchestra.run.smithi193.stdout:428 clone        6 order 18 su 1024 sc 6
2017-08-25T03:23:10.871 INFO:teuthology.orchestra.run.smithi193.stdout:truncating image image_client.0-clone5 from 0x37660d2 (overlap 0x47dc2d) to 0x23ee16
2017-08-25T03:23:11.063 INFO:teuthology.orchestra.run.smithi193.stdout:rbd_get_flags() indicates object map is invalid
2017-08-25T03:23:11.103 INFO:teuthology.orchestra.run.smithi193.stdout:do_clone: ops->resize: Invalid argument

When I revert this PR in wip-jewel-backports branch the tests start to pass locally. So the failures are most likely related to this PR.

Contributor

trociny commented Aug 25, 2017

All tests failed for ceph_test_librbd_fsx like below:

2017-08-25T03:23:10.028 INFO:teuthology.orchestra.run.smithi193.stdout:428 clone        6 order 18 su 1024 sc 6
2017-08-25T03:23:10.871 INFO:teuthology.orchestra.run.smithi193.stdout:truncating image image_client.0-clone5 from 0x37660d2 (overlap 0x47dc2d) to 0x23ee16
2017-08-25T03:23:11.063 INFO:teuthology.orchestra.run.smithi193.stdout:rbd_get_flags() indicates object map is invalid
2017-08-25T03:23:11.103 INFO:teuthology.orchestra.run.smithi193.stdout:do_clone: ops->resize: Invalid argument

When I revert this PR in wip-jewel-backports branch the tests start to pass locally. So the failures are most likely related to this PR.

@smithfarm smithfarm changed the title from jewel: rbd: object-map: batch updates during trim operation to [DNM] jewel: rbd: object-map: batch updates during trim operation Aug 25, 2017

@trociny

This comment has been minimized.

Show comment
Hide comment
@trociny

trociny Aug 25, 2017

Contributor

@smithfarm The problem with this PR is that in the master this "batch update" change [1] was merged before "order concurrent updates" [2], while in jewel "order concurrent updates" is already backported [3]. Due to vice verse order, when backporting "order concurrent updates" Jason skipped changes in "TrimRequest.cc" that should be applied to send_pre_copyup and send_post_copyup functions (introduced by "batch update" change). So your cherry-pick misses this changes too and you need to add them manually.

Do you want to work on this? Or I can do this if you wish.

[1] #11510
[2] #12420
[3] #12909
[4] c53df37

Contributor

trociny commented Aug 25, 2017

@smithfarm The problem with this PR is that in the master this "batch update" change [1] was merged before "order concurrent updates" [2], while in jewel "order concurrent updates" is already backported [3]. Due to vice verse order, when backporting "order concurrent updates" Jason skipped changes in "TrimRequest.cc" that should be applied to send_pre_copyup and send_post_copyup functions (introduced by "batch update" change). So your cherry-pick misses this changes too and you need to add them manually.

Do you want to work on this? Or I can do this if you wish.

[1] #11510
[2] #12420
[3] #12909
[4] c53df37

@smithfarm

This comment has been minimized.

Show comment
Hide comment
@smithfarm

smithfarm Aug 26, 2017

Contributor

@trociny Thanks for debugging this. I think you have given me enough hints that I'll be able to track down and apply the needed changes myself. I'll ping you for review if/when that happens.

Contributor

smithfarm commented Aug 26, 2017

@trociny Thanks for debugging this. I think you have given me enough hints that I'll be able to track down and apply the needed changes myself. I'll ping you for review if/when that happens.

librbd: batch ObjectMap updations upon trim
Shrinking a clone which has snapshots and does not share
majority of objects with its parent (i.e., there are less
objects to be copied up) involves huge number of object
map updates -- two (pre, post) per object. This results
in lots of requests to be send to OSDs especially when
trimming a gigantus image. This situation can be optimized
by sending batch ObjectMap updates for an object range
thereby significantly cutting down OSD traffic resulting
in faster trim times.

Fixes: http://tracker.ceph.com/issues/17356
Signed-off-by: Venky Shankar <vshankar@redhat.com>
(cherry picked from commit 05653b7)
@smithfarm

This comment has been minimized.

Show comment
Hide comment
@smithfarm

smithfarm Aug 26, 2017

Contributor

@trociny Thanks for debugging this. Thanks to your analysis, I think I was able to track down and apply the needed changes myself (see the new commit).

Contributor

smithfarm commented Aug 26, 2017

@trociny Thanks for debugging this. Thanks to your analysis, I think I was able to track down and apply the needed changes myself (see the new commit).

@trociny

+1

@smithfarm smithfarm changed the title from [DNM] jewel: rbd: object-map: batch updates during trim operation to jewel: rbd: object-map: batch updates during trim operation Aug 26, 2017

librbd: clean up object map update interface, revisited
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] #11510
[2] #12420
[3] #12909

Signed-off-by: Mykola Golub <mgolub@mirantis.com>
Signed-off-by: Nathan Cutler <ncutler@suse.com>
@smithfarm

This comment has been minimized.

Show comment
Hide comment
@smithfarm

smithfarm Aug 26, 2017

Contributor

The last force-push just tweaks the commit message.

Contributor

smithfarm commented Aug 26, 2017

The last force-push just tweaks the commit message.

@smithfarm

This comment has been minimized.

Show comment
Hide comment
@smithfarm

smithfarm Aug 26, 2017

Contributor

test this please

Contributor

smithfarm commented Aug 26, 2017

test this please

@smithfarm

This comment has been minimized.

Show comment
Hide comment
@smithfarm

smithfarm Aug 28, 2017

Contributor

@dillaman This passed an rbd suite at http://tracker.ceph.com/issues/20613#note-31 (there was 1 failure that passed on rerun).

The rados run is putting up a bit of a fight; there are a couple failures but they don't appear to be reliably reproducible or related to rbd: http://tracker.ceph.com/issues/20613#note-25

I also ran three different upgrade suites and all the failures were either not reproducible or were "hand-waved away".

Please review.

Contributor

smithfarm commented Aug 28, 2017

@dillaman This passed an rbd suite at http://tracker.ceph.com/issues/20613#note-31 (there was 1 failure that passed on rerun).

The rados run is putting up a bit of a fight; there are a couple failures but they don't appear to be reliably reproducible or related to rbd: http://tracker.ceph.com/issues/20613#note-25

I also ran three different upgrade suites and all the failures were either not reproducible or were "hand-waved away".

Please review.

@dillaman

lgtm

@smithfarm smithfarm merged commit 0916352 into ceph:jewel Aug 28, 2017

4 checks passed

Docs: build check OK - docs built
Details
Signed-off-by all commits in this PR are signed
Details
Unmodified Submodules submodules for project are unmodified
Details
make check make check succeeded
Details

@smithfarm smithfarm deleted the smithfarm:wip-17843-jewel branch Aug 28, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment