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

mon:C_AckMarkedDown has not handled the Callback Arguments #29624

Merged
merged 1 commit into from Aug 23, 2019
Merged

mon:C_AckMarkedDown has not handled the Callback Arguments #29624

merged 1 commit into from Aug 23, 2019

Conversation

NancySu05
Copy link
Contributor

@NancySu05 NancySu05 commented Aug 13, 2019

in C_AckMarkedDown, we add a process that handle _finish(int) Callback Arguments

Fixes: https://tracker.ceph.com/issues/41217
Signed-off-by: NancySu05 su_nan@inspur.com

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.

@NancySu05 thanks for your fix! could you revise the title of your commit message? as it reads like a bug report instead of the summary of your change.

see https://github.com/ceph/ceph/blob/master/SubmittingPatches.rst#3-describe-your-changes

also could you add a space after "Fixes:"?

i'd suggest use your full name as your user.name of your git config. currently it's NancySu05.

src/mon/OSDMonitor.cc Outdated Show resolved Hide resolved
src/mon/OSDMonitor.cc Outdated Show resolved Hide resolved
osdmon->dispatch(op);
} else if(ECANCELED == r) {
return;
}else {
Copy link
Contributor

Choose a reason for hiding this comment

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

a space before else.

@athanatos
Copy link
Contributor

Is dropping the message on ECANCELED ok? The osd is waiting for a reply to shutdown, right? Also, I don't see anywhere where waiting_for_finished_proposal contexts get an ECANCELED.

@athanatos
Copy link
Contributor

The EAGAIN case looks right though I think.

@tchaikov
Copy link
Contributor

The osd is waiting for a reply to shutdown, right?

no. it does not wait for a reply.

@athanatos
Copy link
Contributor

There's a timeout set in OSDService::prepare_to_stop to ensure that it shuts down on its own eventually, but it looks to me like OSD::ms_dispatch does handle the MSG_OSD_MARK_ME_DOWN reply from the mon by calling OSDService::got_stop_ack which moves the OSD on to STOPPING allowing it to finish shutting down more quickly. I'd guess that if ECANCELED is possible, some form of retry or reply to the OSD would be appropriate.

@tchaikov
Copy link
Contributor

oh, sorry! i was wrong. was checking MonClient, thought that it was not a MMonCommand.

@tchaikov
Copy link
Contributor

Also, I don't see anywhere where waiting_for_finished_proposal contexts get an ECANCELED.

neither can i. probably we can add an assert()?

@NancySu05
Copy link
Contributor Author

Also, I don't see anywhere where waiting_for_finished_proposal contexts get an ECANCELED.

neither can i. probably we can add an assert()?

you idea is better,I have revised the submission

@NancySu05
Copy link
Contributor Author

Also, I don't see anywhere where waiting_for_finished_proposal contexts get an ECANCELED.

neither can i. probably we can add an assert()?

you idea is better,I have revised the submission

@NancySu05 NancySu05 closed this Aug 20, 2019
@NancySu05 NancySu05 reopened this Aug 20, 2019
@NancySu05
Copy link
Contributor Author

Also, I don't see anywhere where waiting_for_finished_proposal contexts get an ECANCELED.

neither can i. probably we can add an assert()?

I have made changes as suggested
Do you have any questions in mind about the PR?

src/mon/OSDMonitor.cc Outdated Show resolved Hide resolved
src/mon/OSDMonitor.cc Show resolved Hide resolved
in C_AckMarkedDown,we add a process that handle _finish(int) Callback Arguments

Fixes: https://tracker.ceph.com/issues/41217
Signed-off-by: NancySu05 <su_nan@inspur.com>
false)); // ACK itself does not request an ack
void _finish(int r) override {
if (r == 0) {
MOSDMarkMeDown *m = static_cast<MOSDMarkMeDown*>(op->get_req());
Copy link
Contributor

@tchaikov tchaikov Aug 20, 2019

Choose a reason for hiding this comment

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

use auto. less repeating this way. and could use the template variant of get_req(). i.e.,

auto m = op->get_req<MOSDMarkMeDown>();

@tchaikov tchaikov self-assigned this Aug 20, 2019
@tchaikov tchaikov removed their assignment Aug 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants