osd: refcounting chunks for snapshotted manifest object #29283
Merged
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
myoungwon
force-pushed
the
wip-refcount-snap
branch
5 times, most recently
from
e57060f
to
b496bf3
Compare
|
@liewegas Before starting scrub work for snapshotted manifest object, I want to get a feedback to remove misunderstanding about the concept. These commit present the reference tracking based on clone info as @gregsfortytwo's comment. Note that he key idea behind of reference counting is false-positive, which means (manifest object (no ref), chunk object(has ref)) can be possible instead of (manifest object (has ref), chunk 1(no ref)). Am I missing something? |
|
@liewegas Ping. |
liewegas
reviewed
Aug 6, 2019
src/osd/PrimaryLogPG.cc
Outdated
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
This part of the design worries me. It means that the first time we touch a snapshotted object and need to make a clone, the op is blocked while we go bump a bunch of ref counts.
myoungwon
force-pushed
the
wip-refcount-snap
branch
3 times, most recently
from
f23ca9c
to
54bf4b2
Compare
|
@liewegas Make sense? |
Merged
3 tasks
|
@liewegas Ping. |
liewegas
reviewed
Sep 6, 2019
src/osd/PrimaryLogPG.cc
Outdated
| interval_set<uint64_t> clone_overlap = newest_overlap; | ||
| interval_set<uint64_t> chunk_range; | ||
| chunk_range.insert(p.first, p.second.length); | ||
| clone_overlap.intersection_of(chunk_range); |
There was a problem hiding this comment.
i think here you can replace the last 4 lines with just if (newest_overlap.intersects(p.first, p.second.length)) { ...
liewegas
reviewed
Sep 6, 2019
liewegas
reviewed
Sep 6, 2019
myoungwon
force-pushed
the
wip-refcount-snap
branch
2 times, most recently
from
290d83e
to
95c0df2
Compare
|
@liewegas Fixed. Could you take a look? |
myoungwon
changed the title
liewegas
reviewed
Sep 9, 2019
src/osd/PrimaryLogPG.cc
Outdated
| if (!has_reference) { | ||
| return false; | ||
| } | ||
| return true; |
liewegas
reviewed
Sep 9, 2019
src/osd/PrimaryLogPG.cc
Outdated
| // check if the references is still used | ||
| object_info_t& oi = cobc->obs.oi; | ||
| if (oi.has_manifest()) { | ||
| coi.manifest.build_non_intersection_set(oi.manifest.chunk_map, no_refs, NULL); |
There was a problem hiding this comment.
The way build_non_intersection_set is written, it will only add things that don't overlap. So calling it inside a loop won't find things that don't intersect with all other clones.. instead, it'll find things that don't intersect with at least one other clone, which isn't very useful.
- I think this only needs to check the before and after clones.
- I think it needs to do the first pass, where it finds the intersection, on both of those clones, and then do the second pass.
myoungwon
force-pushed
the
wip-refcount-snap
branch
from
95c0df2
to
64c0709
Compare
|
@liewegas I addressed your comments. Please review. |
liewegas
reviewed
Sep 9, 2019
src/osd/PrimaryLogPG.cc
Outdated
| object_info_t& coi = cobc->obs.oi; | ||
| oi.manifest.build_intersection_set(coi.manifest.chunk_map, refs, &newest_overlap); | ||
|
|
||
| if (refs.size() > 0) { |
|
other than that if, i think this looks right! |
myoungwon
force-pushed
the
wip-refcount-snap
branch
from
64c0709
to
f11fa97
Compare
|
@athanatos @myoungwon i will include this PR in my next batch. |
|
@athanatos The test result looks like there is no fails which have to do with this PR. What do you think this? |
|
@myoungwon What caused the two rados/test.sh failures? They persisted into the second run and could be related. |
they timedout. in which |
|
rerunning the failed tests in the second run at https://pulpito.ceph.com/kchai-2020-07-08_03:34:45-rados-wip-kefu-testing-2020-07-06-1016-distro-basic-smithi/ |
|
This seems like a bug in dec_all_refcount_manifest(). api_tier_pp never finish because vargrind reports an error And this caused by Based on valgrind's report, in dec_all_refcount_manifest(), dec_refcount is called with unreferenced ctx due to move(ctx) in AwaitAsyncWork::react(). So, to fix this, replace above code with Make sense? |
ctx will be moved, so replace it with the value Signed-off-by: Myoungwon Oh <ohmyoungwon@gmail.com>
|
@myoungwon probably you could rerun the failed tests with your latest fix to verify it? |
|
@tchaikov I cannot access the sepia yet because my update request for login credentials is in-progress. When that request is competed, I can rerun this. |
|
retest this please |
src/osd/osd_types.cc
Outdated
| if (i == manifest.chunk_map.end() || current != i->first) { | ||
| return nullptr; | ||
| } else { | ||
| // We advance the iterator iff we consider the chunk_map on this iteration |
There was a problem hiding this comment.
This comment was not actually helpful in understanding what's going on here. The way you're advancing through the loop is elegant but "hiding" the iter advancement inside of a lambda function is messy, so you should explain the lambda in light of the overall algorithm, especially since its very clear name only describes a portion of its purpose.
|
Those functions look fine @athanatos other than the one bit needing a better comment. |
|
@myoungwon Top commit on https://github.com/athanatos/ceph/commits/sjust/wip-fix-comment (athanatos@1c65b0a) has an improvement to address @gregsfortytwo's comment. |
Signed-off-by: Samuel Just <sjust@redhat.com>
|
retest this please |
|
retest this please |
|
jenkins retest this please |
|
@athanatos Can you look over this PR? QA result looks good. Do I need to rerun the tests? |
athanatos
approved these changes
Jul 14, 2020
3 tasks
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.
https://lists.ceph.io/hyperkitty/list/dev@ceph.io/thread/7G222W5F7MD7X5MJS3BF6YDJ76RKJGN5/
To prevent removing a manifest object in use,
chunk maps in clone object are introduced.
Signed-off-by: Myoungwon Oh myoungwon.oh@samsung.com