mgr: locking fixes #17309
Merged
mgr: locking fixes #17309
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jcsp
force-pushed
the
wip-daemonserver-locks
branch
from
0e46dcd
to
9aa680a
Compare
tchaikov
reviewed
Aug 29, 2017
src/mgr/PyModules.cc
Outdated
|
|
||
| if (svc_type == "") { | ||
| states = daemon_state.get_all(); | ||
| daemons = daemon_state.get_all(); |
There was a problem hiding this comment.
could also:
daemons = std::move(daemon_state.get_all());to save a copy ctor call.
src/mgr/PyModules.cc
Outdated
| } else if (svc_id.empty()) { | ||
| states = daemon_state.get_by_service(svc_type); | ||
| daemons = daemon_state.get_by_service(svc_type); |
src/mgr/PyModules.cc
Outdated
| auto key = statepair.first; | ||
| auto state = statepair.second; | ||
|
|
||
| Mutex::Locker(state->lock); |
There was a problem hiding this comment.
this is not necessary to create a temporary Locker object to lock and then unlock immediately.
| @@ -385,6 +390,7 @@ bool DaemonServer::handle_report(MMgrReport *m) | |||
| return true; | |||
| } | |||
|
|
|||
| // Look up the DaemonState | |||
There was a problem hiding this comment.
shall we guard the access to daemon_state with lock here also?
There was a problem hiding this comment.
It's not necessary until we are doing some updates (in the "Update the DaemonState" block below).
In the "constructing new DaemonState" path we are making updates before inserting it into the index, so nobody else can see it yet.
jcsp
force-pushed
the
wip-daemonserver-locks
branch
from
9aa680a
to
2f8d191
Compare
|
Updated |
liewegas
reviewed
Sep 14, 2017
src/mgr/DaemonServer.cc
Outdated
| @@ -267,6 +268,8 @@ bool DaemonServer::ms_dispatch(Message *m) | |||
|
|
|||
| void DaemonServer::maybe_ready(int32_t osd_id) | |||
| { | |||
| Mutex::Locker l(lock); | |||
|
|
|||
| if (!pgmap_ready && reported_osds.find(osd_id) == reported_osds.end()) { | |||
There was a problem hiding this comment.
It seems worth making pgmap_ready an atomic so that you can avoid taking the lock each time here? It's only false during mgr startup, right?
|
@jcsp need rebase |
jcsp
force-pushed
the
wip-daemonserver-locks
branch
from
2f8d191
to
e905389
Compare
added 3 commits
September 18, 2017 05:44
Various things here were dangerously operating outside locks. Additionally switch to a RWLock because this lock will be relatively read-hot when it's taken every time a MMgrReport is handled, to look up the DaemonState for the sender. Fixes: http://tracker.ceph.com/issues/21158 Signed-off-by: John Spray <john.spray@redhat.com>
The DaemonStateIndex locking is sufficient to make all the report processing safe: holding DaemonServer::lock through all ms_dispatch was unnecessarily serializing dispatch. Signed-off-by: John Spray <john.spray@redhat.com>
Signed-off-by: John Spray <john.spray@redhat.com>
jcsp
force-pushed
the
wip-daemonserver-locks
branch
from
4942fc2
to
d209157
Compare
|
Rebased |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes: http://tracker.ceph.com/issues/21158