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

mon/OSDMonitor: do not alter the "created" epoch of a pg #14849

Merged
merged 2 commits into from Apr 29, 2017

Conversation

Projects
None yet
2 participants
@tchaikov
Contributor

tchaikov commented Apr 27, 2017

Fixes: http://tracker.ceph.com/issues/19787
Signed-off-by: Kefu Chai kchai@redhat.com

// scan_for_creating_pgs() because we don't have the updated pg
// mapping by then.
created = mapping.get_epoch();
mapped = mapping.get_epoch();

This comment has been minimized.

@liewegas

liewegas Apr 27, 2017

Member

I think here we also need to remove the pg from it's old position.. pgs_by_epoch.second.erase(pgid)?

This comment has been minimized.

@liewegas

liewegas Apr 27, 2017

Member

oh, nevermind, you rebuild teh whole thing!

This comment has been minimized.

@tchaikov

tchaikov Apr 27, 2017

Contributor

yeah, assuming the set of pg being created should be smallish in practice.

// scan_for_creating_pgs() because we don't have the updated pg
// mapping by then.
created = mapping.get_epoch();
mapped = mapping.get_epoch();
}

This comment has been minimized.

@liewegas

liewegas Apr 27, 2017

Member

else {
mapped = pgs_by_epoch.first;
}

This comment has been minimized.

@tchaikov

tchaikov Apr 27, 2017

Contributor

no, it's not necessary. mapped is pg.second.first from the very beginning, if it's not changed after that.

This comment has been minimized.

@tchaikov

tchaikov Apr 27, 2017

Contributor

oh, i was wrong. will fix.

@liewegas

liewegas requested changes Apr 27, 2017 edited

Hmm, this whole thing is something like O(n*m*log(n/m)) (m = epochs, n = pgs). I guess that's okay, but I wonder if this would be better off modifying the map instead of rebuilding it from scratch.

@tchaikov

This comment has been minimized.

Contributor

tchaikov commented Apr 27, 2017

@liewegas can we fix this issue first and then think about how to improve the performance or if it's necessary?

the creating_pgs_by_osd_epoch is organized in a different way than pg_creatings, i am afraid that updating the map instead rebuilding it from scratch would be much more complicated: two passes: 1) to iterate in the map to trim the pgs not exist in creating_pg, 2) to iterate in pg_creatings to add/update pg being created. or we can walk thru the pools and cross check creating_pgs_by_osd_epoch and creating_pgs to find stale or non-existent pgs.

i am not sure it is worthwhile introducing the complicity.

@liewegas

This comment has been minimized.

Member

liewegas commented Apr 27, 2017

@tchaikov tchaikov requested a review from liewegas Apr 27, 2017

@liewegas liewegas added the needs-qa label Apr 27, 2017

@liewegas

This comment has been minimized.

Member

liewegas commented Apr 27, 2017

looks good, except the last (emplace) commit has the hunk fixing the mapped value mixed into it.

@tchaikov

This comment has been minimized.

Contributor

tchaikov commented Apr 27, 2017

i just tried to update the pgs_by_osd_epoch in-place, the size of this function tripled. if we find it's a bottleneck. i will trade the simplicity for the performance.

tchaikov added some commits Apr 27, 2017

mon/OSDMonitor: use emplace() to avoid creating temporary elements
Signed-off-by: Kefu Chai <kchai@redhat.com>
@tchaikov

This comment has been minimized.

Contributor

tchaikov commented Apr 27, 2017

@liewegas fixed and repushed.

@liewegas

This comment has been minimized.

Member

liewegas commented Apr 27, 2017

@tchaikov

This comment has been minimized.

Contributor

tchaikov commented Apr 28, 2017

/home/jenkins-build/build/workspace/ceph-pull-requests/src/test/mon/misc.sh:169: jq_success:  return 1
/home/jenkins-build/build/workspace/ceph-pull-requests/src/test/mon/misc.sh:251: TEST_mon_features:  return 1

retest this please.

@liewegas liewegas merged commit b81caa0 into ceph:master Apr 29, 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

@tchaikov tchaikov deleted the tchaikov:wip-19787 branch Apr 29, 2017

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