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

osd/PG: restrict want_acting to up+acting on recovery completion #13420

Merged
merged 1 commit into from Feb 18, 2017

Conversation

Projects
None yet
3 participants
@liewegas
Member

liewegas commented Feb 14, 2017

http://tracker.ceph.com/issues/18929

On recovery completion we recalculate want_acting to see if we
should add recently backfilled osds into acting. However, at
this point we may have gotten infos from others OSDs outside
of up/acting that could be used for want_acting. We currently
assert that only up/acting osds are used in
PG::RecoveryState::Active::react(const AdvMap&), so we must
restrict want_acting to up/acting here.

We could remove this restriction, but it would mean

  1. checking on every map change that want_acting hasn't been
    invalidated, and if so, recalculating want_acting and requesting
    a new pg_temp. Also, presumably

  2. on each new info, checking whether we can construct a better
    want_acting, and if so, doing it.

That would be a good thing, but is a more complicated change. In
reality this case comes up very rarely, so simply make our
post-recovery want_acting calculation limit itself to up+acting.

See 1db67c4 for the assertion.

Signed-off-by: Sage Weil sage@redhat.com

@liewegas liewegas requested a review from athanatos Feb 14, 2017

// TODO: do this someday!
vector<int> new_want;
for (size_t i = 0; i < pg->want_acting.size(); ++i) {
int osd = pg->want_acting[i];

This comment has been minimized.

@athanatos

athanatos Feb 14, 2017

Contributor

Need to skip CRUSH_ITEM_NONE here.

osd/PG: restrict want_acting to up+acting on recovery completion
On recovery completion we recalculate want_acting to see if we
should add recently backfilled osds into acting.  However, at
this point we may have gotten infos from others OSDs outside
of up/acting that could be used for want_acting.  We currently
assert that only up/acting osds are used in
PG::RecoveryState::Active::react(const AdvMap&), so we must
restrict want_acting to up/acting here.

We could remove this restriction, but it would mean

1) checking on every map change that want_acting hasn't been
invalidated, and if so, recalculating want_acting and requesting
a new pg_temp.  Also, presumably

2) on each new info, checking whether we can construct a better
want_acting, and if so, doing it.

That would be a good thing, but is a more complicated change.  In
reality this case comes up very rarely, so simply make our
post-recovery want_acting calculation limit itself to up+acting.

See 1db67c4 for the assertion.

Signed-off-by: Sage Weil <sage@redhat.com>

@liewegas liewegas added the needs-qa label Feb 14, 2017

@yuriw

This comment has been minimized.

Contributor

yuriw commented Feb 15, 2017

test this please

@liewegas liewegas merged commit 2a4be50 into ceph:master Feb 18, 2017

3 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodifed Submodules submodules for project are unmodified
Details
default Build finished.
Details

@liewegas liewegas deleted the liewegas:wip-18929 branch Feb 18, 2017

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