Skip to content
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

ceph-objectstore-tool: do not populate snapmapper with missing clones #15787

Merged
merged 3 commits into from Jun 21, 2017

Conversation

Projects
None yet
4 participants
@liewegas
Copy link
Member

liewegas commented Jun 20, 2017

Cache pools may have clones represented in SnapSet that are not stored
locally. This is fine as long as ceph-objectstore-tool doesn't go and
add them when it shouldn't.

Fixes: http://tracker.ceph.com/issues/19943
Signed-off-by: Sage Weil sage@redhat.com

@liewegas liewegas force-pushed the liewegas:wip-19943-fix branch from da4a741 to c480e51 Jun 20, 2017

@liewegas liewegas force-pushed the liewegas:wip-19943-fix branch 3 times, most recently from a61c397 to bad925c Jun 20, 2017

ceph-objectstore-tool: do not populate snapmapper with missing clones
Cache pools may have clones represented in SnapSet that are not stored
locally.  This is fine as long as ceph-objectstore-tool doesn't go and
add them when it shouldn't.

Fixes: http://tracker.ceph.com/issues/19943
Signed-off-by: Sage Weil <sage@redhat.com>

@liewegas liewegas force-pushed the liewegas:wip-19943-fix branch from bad925c to 867ada5 Jun 20, 2017

@gregsfortytwo
Copy link
Member

gregsfortytwo left a comment

ceph_objectstore_tool looks good to me.
The SnapMapper should wrap the for loops in dout so we don't do per-key iteration if we're not going to do anything with them.
Didn't look at BlueStore.

liewegas added some commits Jun 13, 2017

os/bluestore: debug OmapIteratorImpl
Signed-off-by: Sage Weil <sage@redhat.com>

@liewegas liewegas force-pushed the liewegas:wip-19943-fix branch from b68e0b1 to 87224fd Jun 20, 2017

@liewegas

This comment has been minimized.

Copy link
Member Author

liewegas commented Jun 20, 2017

Good catch, updated. bluestore's douts are all one-offs, no loops.

@@ -240,6 +255,11 @@ void SnapMapper::add_oid(
++i) {
to_add.insert(to_raw(make_pair(*i, oid)));
}
if (g_conf->subsys.should_gather(ceph_subsys_osd, 20)) {

This comment has been minimized.

Copy link
@gregsfortytwo

gregsfortytwo Jun 20, 2017

Member

Aside: I think in the past you've done this exclusion with some macro shenanigans, enclosing the whole loop in a dout/dendl pair. This format here is perfectly (perhaps more) readable, but did you switch for any particular reason?

@gregsfortytwo

This comment has been minimized.

Copy link
Member

gregsfortytwo commented Jun 20, 2017

Reviewed-by: Greg Farnum gfarnum@redhat.com
...on the non-bluestore bits. (I'm just not sticking my nose in there :))

@liewegas

This comment has been minimized.

Copy link
Member Author

liewegas commented Jun 20, 2017

set<snapid_t> snaps(p.second.begin(), p.second.end());
if (!store->exists(coll, clone)) {

This comment has been minimized.

Copy link
@dzafman

dzafman Jun 21, 2017

Member

You could clean-up last_clones since it is not used. last_clones is cleared when we import the head object which would be the point where it could have been useful. It is better to check the store since you might import into a pg that already has existing objects.

This comment has been minimized.

Copy link
@dzafman

dzafman Jun 21, 2017

Member

You can remove last_head too. Just replace "hold == last_head" test with hold.is_head().

This comment has been minimized.

Copy link
@dzafman

dzafman Jun 21, 2017

Member

241f8ad has the change you can cherry-pick here.

This comment has been minimized.

Copy link
@liewegas

liewegas Jun 21, 2017

Author Member

Let's put this in a separate pr, since this one already went through testing and passed?

@liewegas liewegas merged commit a683138 into ceph:master Jun 21, 2017

4 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodified Submodules submodules for project are unmodified
Details
arm64 make check arm64 make check succeeded
Details
make check make check succeeded
Details

@liewegas liewegas deleted the liewegas:wip-19943-fix branch Jun 21, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.