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: mark OSD beacons and pg_create messages as no_reply #20517

Merged

Conversation

Projects
None yet
4 participants
@gregsfortytwo
Copy link
Member

commented Feb 21, 2018

Fixes: http://tracker.ceph.com/issues/22114
Reported-by: Hongpeng Lu ludehp@163.com

Signed-off-by: Greg Farnum gfarnum@redhat.com

mon: mark OSD beacons and pg_create messages as no_reply
Fixes: http://tracker.ceph.com/issues/22114
Reported-by: Hongpeng Lu <ludehp@163.com>

Signed-off-by: Greg Farnum <gfarnum@redhat.com>
@@ -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);

This comment has been minimized.

Copy link
@tchaikov

tchaikov Feb 22, 2018

Contributor

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 ?

This comment has been minimized.

Copy link
@jecluis

jecluis Feb 22, 2018

Member

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.

This comment has been minimized.

Copy link
@gregsfortytwo

gregsfortytwo Feb 22, 2018

Author Member

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.

This comment has been minimized.

Copy link
@tchaikov

tchaikov Feb 27, 2018

Contributor

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.

This comment has been minimized.

Copy link
@gregsfortytwo

gregsfortytwo Feb 27, 2018

Author Member

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.

This comment has been minimized.

Copy link
@tchaikov

tchaikov Mar 1, 2018

Contributor

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

This comment has been minimized.

Copy link
@gregsfortytwo

gregsfortytwo Mar 3, 2018

Author Member

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.

This comment has been minimized.

Copy link
@jecluis

jecluis Mar 5, 2018

Member

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.

@@ -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);

This comment has been minimized.

Copy link
@jecluis

jecluis Feb 22, 2018

Member

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.

@gregsfortytwo

This comment has been minimized.

Copy link
Member Author

commented Mar 3, 2018

Jenkins test this please
(Looks like it died while running)

@jecluis

jecluis approved these changes Mar 5, 2018

@gregsfortytwo gregsfortytwo merged commit 7b9a715 into ceph:master Mar 6, 2018

5 checks passed

Docs: build check OK - docs built
Details
Signed-off-by all commits in this PR are signed
Details
Unmodified Submodules submodules for project are unmodified
Details
make check make check succeeded
Details
make check (arm64) make check succeeded
Details

@gregsfortytwo gregsfortytwo deleted the gregsfortytwo:wip-22114-beacon-blocking-2 branch Mar 6, 2018

tchaikov added a commit to tchaikov/ceph that referenced this pull request Mar 27, 2018

mon: mark mgr reports as no_reply
see also: ceph#20517

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

tchaikov added a commit to tchaikov/ceph that referenced this pull request Mar 27, 2018

mon: mark mgr reports and osd_failure as no_reply
see also: ceph#20517

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

theanalyst added a commit to pdvian/ceph that referenced this pull request Mar 29, 2018

mon: mark mgr reports and osd_failure as no_reply
see also: ceph#20517

Fixes: http://tracker.ceph.com/issues/22114
Signed-off-by: Kefu Chai <kchai@redhat.com>
(cherry picked from commit 0daccfb)

tspmelo added a commit to tspmelo/ceph that referenced this pull request Apr 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.