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.

Sign up for GitHub

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

Jump to bottom

osd: set min_version to newest version in maybe_force_recovery #17752

Merged
merged 2 commits into from Oct 2, 2017
Merged

osd: set min_version to newest version in maybe_force_recovery #17752

merged 2 commits into from Oct 2, 2017

Conversation

XinzeChi
Copy link
Contributor

No description provided.

@XinzeChi
osd: set min_version to newest version in maybe_force_recovery
dd08098 
Signed-off-by: Xinze Chi <xinze@xsky.com>
@XinzeChi
Copy link
Contributor Author

@tchaikov ping

tchaikov
tchaikov reviewed Sep 18, 2017
View reviewed changes
src/osd/PrimaryLogPG.cc Outdated
@@ -728,7 +728,7 @@ void PrimaryLogPG::maybe_force_recovery()
// find the oldest missing object
version_t min_version = pg_log.get_log().head.version;
hobject_t soid;
if (!pg_log.get_missing().get_items().empty()) {
if (!pg_log.get_missing().get_rmissing().empty()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rmissing != missing in backfill

why?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in prep_backfill_object_push func, we know the needed version of all backfilling object is zero

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how is this connected to the conclusion of missing.empty() != rmissing.empty()?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because we use rmissing iterator instead of missing iterator, rmissing.empty() looks more correctly

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so, i think it's more of a cleanup? am i right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you update the commit message accordingly?

src/osd/PrimaryLogPG.cc
@@ -726,7 +726,7 @@ void PrimaryLogPG::maybe_force_recovery()
return;

// find the oldest missing object
version_t min_version = 0;
version_t min_version = pg_log.get_log().head.version;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you elaborate a little bit the rationale of this change in the commit message?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if min_version is 0, if (min_version > xxx) always return false

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the question is why pg_log.get_log().head.version is correct, but 0 is not.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what we want to get is the minimum of the objects' versions. so we should initialize to as head version and compare it with other version

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oic, could you amend the commit message with the explanation above?

tchaikov
tchaikov requested changes Sep 28, 2017
View reviewed changes
Copy link
Contributor

@tchaikov tchaikov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rmissing != missing in backfill

this commit message is wrong and misleading.

@xiexingguo
Copy link
Member

xiexingguo commented Sep 28, 2017 •
edited

instead of resetting min_version at the very beginning (and change these codes significantly), I think you can:

--- a/src/osd/PrimaryLogPG.cc
+++ b/src/osd/PrimaryLogPG.cc
@@ -723,7 +723,8 @@ void PrimaryLogPG::maybe_force_recovery()
     pg_shard_t peer = *it;
     if (peer_missing.count(peer) &&
        !peer_missing[peer].get_items().empty() &&
-       min_version > peer_missing[peer].get_rmissing().begin()->first) {
+        (!min_version ||
+       min_version > peer_missing[peer].get_rmissing().begin()->first)) {
       min_version = peer_missing[peer].get_rmissing().begin()->first;
       soid = peer_missing[peer].get_rmissing().begin()->second;
     }

@tchaikov
Copy link
Contributor

@xiexingguo why make 0 a special case? in the sense of correctness, i think to use the maximum possible max eversion as the initial min_version does make sense.

@xiexingguo
Copy link
Member

use the maximum possible max eversion as the initial min_version does make sense.

Fine with me, kefu:-)

tchaikov
tchaikov reviewed Sep 28, 2017
View reviewed changes
src/osd/PrimaryLogPG.cc Outdated
min_version > peer_missing[peer].get_rmissing().begin()->first) {
min_version = peer_missing[peer].get_rmissing().begin()->first;
soid = peer_missing[peer].get_rmissing().begin()->second;
!peer_missing[peer].get_items().empty()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

might want to drop this change or split it into another separate "refactor only" commit.

@XinzeChi
osd: using get_rmissing instread of missing as the judgement
96a3cb6 
the below code using peer_missing iterator

Signed-off-by: Xinze Chi <xinze@xsky.com>
@tchaikov
Copy link
Contributor

tchaikov commented Oct 2, 2017

http://pulpito.ceph.com/kchai-2017-10-01_17:38:10-rados-wip-kefu-testing-2017-10-01-2202-distro-basic-mira/

@tchaikov tchaikov merged commit 0024679 into ceph:master Oct 2, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tchaikov tchaikov tchaikov approved these changes

@wonzhq wonzhq Awaiting requested review from wonzhq

Assignees
No one assigned
Labels
needs-qa wip-kefu-testing
Projects
None yet
Milestone
No milestone
3 participants
@XinzeChi @xiexingguo @tchaikov