pacific: librbd: fix regressions in ObjectListSnapsRequest #54860
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Since commit 73f50a1 ("rbd-mirror: use generalized deep copy for image sync"), the only user of calc_snap_set_diff() immediately unsets end_size otherwise. calc_snap_set_diff() semantics are clearer if end_size is set together with end_exists and clone_end_snap_id. Signed-off-by: Ilya Dryomov <idryomov@gmail.com> (cherry picked from commit c074792)
Despite being added in commit 66dd53d ("librbd: optionally return full object extent for any snapshot deltas") ostensibly to test the new LIST_SNAPS_FLAG_WHOLE_OBJECT code, it surely doesn't do that because the flag isn't even passed to MockObjectListSnapsRequest::create(). I can only guess, but it looks like snap ID 3 was intended to be a starting point. Otherwise, with 0 and CEPH_NOSNAP passed as snap IDs, the overlap that is set up for the clone wouldn't affect the computation in any way. Use snap ID 3 as a starting point and run both with and without LIST_SNAPS_FLAG_WHOLE_OBJECT on the same snapset to pinpoint the difference. Signed-off-by: Ilya Dryomov <idryomov@gmail.com> (cherry picked from commit bd52297)
Bundling read_whole_object and LIST_SNAPS_FLAG_WHOLE_OBJECT cases together is wrong: - In read_whole_object case, calc_snap_set_diff() sets just read_whole_object. Everything else is zeroed out and may require resetting to fit with the rest of ObjectListSnapsRequest logic. - In LIST_SNAPS_FLAG_WHOLE_OBJECT case, only the diff should be expanded. Everything else is set by calc_snap_set_diff() and should be used as is. This goes for end_size in particular -- if it's reset to object size, bogus zero extents may be returned as the object would appear to have grown. This is a regression introduced in commit 4429ed4 ("librbd: switch diff iterate API to use new snaps list dispatch methods") by way of commit 66dd53d ("librbd: optionally return full object extent for any snapshot deltas"). Fixes: https://tracker.ceph.com/issues/63654 Signed-off-by: Ilya Dryomov <idryomov@gmail.com> (cherry picked from commit 8f86d80)
Originally, in commit 2be4840 ("librados/snap_set_diff: don't assert on empty snapset"), exists was set to true. This didn't make ObjectListSnapsRequest, causing the following deep-copy tests to fail when run against calc_snap_set_diff() rigged to return "whole object" as described in [1]: TestDeepCopy.Snaps TestDeepCopy.SnapDiscard TestDeepCopy.CloneHideParent TestDeepCopy.Snaps_LargerDstObjSize TestDeepCopy.Snaps_SmallerDstObjSize This is a regression introduced in commit cc87a8b ("librbd: deep-copy object utilizes image-extent IO methods") by way of commit 11923e2 ("librbd: generic object list snapshot request"). [1] ceph#20648 (comment) Fixes: https://tracker.ceph.com/issues/63654 Signed-off-by: Ilya Dryomov <idryomov@gmail.com> (cherry picked from commit 0a1f633)
scribble()-based DiffIterate tests are too weak: at least two regressions that should been caught by DiffIterate.DiffIterate or DiffIterate.DiffIterateStress were missed [1][2]. Aside from the randomness which can be both a good and a bad thing, asserts there ensure only that the returned diff covers all changes that were made. If the returned diff is too excessive or otherwise bogus, this isn't detected [3]. Add a deterministic test to systematically cover the most common cases that don't involve discards. A similar test for discards will be added with the fix for [4]. Comment out debug log in vector_iterate_cb() like it's done in iterate_cb(). [1] https://tracker.ceph.com/issues/50787 [2] https://tracker.ceph.com/issues/63654 [3] https://tracker.ceph.com/issues/63719 [4] https://tracker.ceph.com/issues/53897 Signed-off-by: Ilya Dryomov <idryomov@gmail.com> (cherry picked from commit f5e3f26)
This was added to integration test [1], separate from the fix which
went in only with unit test adjustments. It's duplicated by several
cases in DiffIterateTest.DiffIterateDeterministic now. Specifically,
the issue could be reproduced by any of:
(3) snap2 -> HEAD
(4) snap3 -> HEAD
(7) snap2 -> snap3
[1] https://tracker.ceph.com/issues/50787
Signed-off-by: Ilya Dryomov <idryomov@gmail.com>
(cherry picked from commit 356ac6a)
This was added to test [1]. It's duplicated by several cases in
DiffIterateTest.DiffIterateDeterministicPP now. Specifically, the
issue could be reproduced by any of:
(8) beginning of time -> snap2
(9) snap1 -> snap2
(10) beginning of time -> snap1
[1] https://tracker.ceph.com/issues/6926
Signed-off-by: Ilya Dryomov <idryomov@gmail.com>
(cherry picked from commit 93ff7fe)
... to avoid valgrind reporting a memory leak on ImageCtx. Signed-off-by: Ilya Dryomov <idryomov@gmail.com> (cherry picked from commit be40bbb)
ajarr
approved these changes
Dec 11, 2023
trociny
approved these changes
Dec 11, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
backport tracker: https://tracker.ceph.com/issues/63736
backport of #54755 and #54848
parent tracker: https://tracker.ceph.com/issues/63654