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

Jump to bottom

mon: mark OSD beacons and pg_create messages as no_reply #20517

Merged
merged 1 commit into from Mar 6, 2018
Merged

mon: mark OSD beacons and pg_create messages as no_reply #20517

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/mon/OSDMonitor.cc
View file
Open in desktop
Expand Up @@ -2637,6 +2637,7 @@ bool OSDMonitor::preprocess_pg_created(MonOpRequestRef op)
auto m = static_cast<MOSDPGCreated*>(op->get_req());
dout(10) << __func__ << " " << *m << dendl;
auto session = m->get_session();
mon->no_reply(op);
Copy link
Contributor

@tchaikov tchaikov Feb 22, 2018 •
edited

Choose a reason for hiding this comment

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

both the leader and the proxy (peon) monitor will call this method when handling a pg-created. how about calling mon->no_reply(op) in OSDMonitor::prepare_pg_created() and OSDMonitor::prepare_beacon() instead ?

Copy link
Member

Choose a reason for hiding this comment

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

Given the function itself even mentions (at the bottom) we "always forward" to the leader, then I agree with @tchaikov that we should have this only on the prepare functions (which are essentially only called on the leader), for both the pg create and the osd beacon.

Copy link
Member Author

Choose a reason for hiding this comment

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

I considered it but actually don’t like that:

  1. marking it only in prepare means any future changes to the function that should apply to the follower mons won’t.
  2. you’ll notice there are some error cases where we actually handle the op in preprocess, and if that turns up in the leader but not the follower we will still leak. (Yes, this would indicate another bug as well, but still.)
  3. if we always mark no_reply in preprocess it’s less cognitive overhead to make sure it’s set correctly.

Copy link
Contributor

Choose a reason for hiding this comment

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

marking it only in prepare means any future changes to the function that should apply to the follower mons won’t.

i don't follow you. mon->no_reply(op) will reply the proxy mon, which will in turn remove the the routed request from its routed_requests map. if there will be any future message which we don't need reply to the party who sent the request. we should call mon->no_reply(op) in the leader mon.

you’ll notice there are some error cases where we actually handle the op in preprocess, and if that turns up in the leader but not the follower we will still leak. (Yes, this would indicate another bug as well, but still.)

yeah, i did notice the error case handling in the preprocess. there are two cases

  • it's the leader who received the original MOSDPGCreated message: since the message is not routed, it is not added to the routed_requests map, and the message's life cycle is managed by MonOpRequestRef, which is able to take care of itself just fine, even mon->no_reply() does nothing in this case other than mark_event("no_reply").
  • it's the peon who received the original MOSDPGCreated message: the message is routed, and it's added to the routed_requests map. if mon->no_reply(op) is called in OSDMonitor::prepare_pg_created() by the leader, the peon who forwarded the request will get an MRoute message. so no leak is caused.

if we always mark no_reply in preprocess it’s less cognitive overhead to make sure it’s set correctly.

well, i'd say it's a subjective assertion. my point is that no_reply() only takes effect on leader, and we only do prepare on leader. if we can reach a consensus on this, probably it would make more sense to call mon->no_reply() in the prepare methods.

Copy link
Member Author

Choose a reason for hiding this comment

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

I mean that we may change the code in the future, so that no_reply() does something on follower monitors as well. In which case we'd have to change them.

And again, you're right that with the current error cases in preprocess, they will not bail out on the leader if the message was forwarded.

But the cost of calling no_reply() in preprocess is...very low. And putting it at the top of that function, instead of down in the bottom of the call stack where only the leader can see it, is a very simple way to practice defensive programming so that we don't leak forwarded messages due to changes in the future.

Copy link
Contributor

Choose a reason for hiding this comment

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

okay, but i'd prefer reasoning based on what we have instead of the hypotheses.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is reasoning based on what we have. It is simple and obviously correct. You are trying to optimize away a function call on a path that involves passing multiple messages over the wire, because you have chased down at least three different things that must be true for that optimization to be safe and determined that they are. It’s a false economy.

Copy link
Member

Choose a reason for hiding this comment

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

eh, this kind of blew out of proportion; but given @gregsfortytwo's arguments a wee-bit above, I kinda agree with him. We are losing nothing by marking the op no_reply() early in preprocess, afaict, and we are ensuring that the proxy mon gets to drop its routed request as soon as possible. The only thing that tickles me the wrong way, but it's objectively irrelevant I think, is that we are replying to a message (in this case to the proxy mon) before the message has been handled, which is not the way we typically do things in the monitor and likely why we have been preferring replying at a later stage in the function call instead of at the beginning.

if (!session) {
dout(10) << __func__ << ": no monitor session!" << dendl;
return true;
Expand Down Expand Up @@ -2900,6 +2901,7 @@ bool OSDMonitor::preprocess_beacon(MonOpRequestRef op)
auto beacon = static_cast<MOSDBeacon*>(op->get_req());
// check caps
auto session = beacon->get_session();
mon->no_reply(op);
if (!session) {
dout(10) << __func__ << " no monitor session!" << dendl;
return true;
Expand Down