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: small clear up and optimize on _recover_now and should_share_map function #13476
Conversation
@branch-predictor Hi! Have a look? |
src/osd/OSD.cc
Outdated
@@ -8434,8 +8436,8 @@ void OSD::_remove_pg(PG *pg) | |||
// and handle_notify_timeout | |||
pg->on_removal(&rmt); | |||
|
|||
service.cancel_pending_splits_for_parent(pg->info.pgid); | |||
|
|||
int tr = service.cancel_pending_splits_for_parent(pg->info.pgid); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cancel_pending_splits_for_parent()
does not return a value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tchaikov Thank you! Sorry i mistake it.
src/osd/OSD.cc
Outdated
@@ -8476,6 +8477,16 @@ void OSDService::_maybe_queue_recovery() { | |||
|
|||
bool OSDService::_recover_now(uint64_t *available_pushes) | |||
{ | |||
if (ceph_clock_now() < defer_recovery_until) { | |||
dout(15) << "_recover_now defer until " << defer_recovery_until << dendl; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit, could use __func__
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tchaikov Thank you tchaikov!I will change it later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tchaikov Done!
src/osd/OSD.cc
Outdated
} | ||
|
||
if (recovery_paused) { | ||
dout(15) << "_recover_now paused" << dendl; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
and the title of your commit could be more specific. "small change and optimization" is way too vague, imo. |
src/osd/OSD.cc
Outdated
@@ -962,6 +962,7 @@ bool OSDService::should_share_map(entity_name_t name, Connection *con, | |||
<< " versus osdmap epoch " << osdmap->get_epoch() << dendl; | |||
if (*sent_epoch_p < osdmap->get_epoch()) { | |||
should_send = true; | |||
return should_send; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's just drop should_send entirely and return true directly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@liewegas Done.
cdf515c
to
18906e6
Compare
jenkins test this please |
4250314
to
c295dd9
Compare
@@ -8489,15 +8499,6 @@ bool OSDService::_recover_now(uint64_t *available_pushes) | |||
if (available_pushes) | |||
*available_pushes = max - recovery_ops_active - recovery_ops_reserved; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Returning early will cause this function to return correct result, but leave *available_pushes unchanged, this may break code that call _recover_now and rely on available_pushes set regardless of result returned.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@branch-predictor Agree with it good eyes man. and i change it.
@@ -8489,15 +8499,6 @@ bool OSDService::_recover_now(uint64_t *available_pushes) | |||
if (available_pushes) | |||
*available_pushes = max - recovery_ops_active - recovery_ops_reserved; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Returning early will cause this function to return correct result, but leave *available_pushes unchanged, this may break code that call _recover_now and rely on available_pushes set regardless of result returned.
e2fa678
to
8e309aa
Compare
… function Signed-off-by: song baisen <song.baisen@zte.com.cn>
osd: small clear up and optimize on _recover_now and should_share_map function
Signed-off-by: song baisen song.baisen@zte.com.cn