From: John Spray Date: Thu, 24 Aug 2017 16:53:24 +0000 (-0400) Subject: mgr: clean up DaemonStateIndex locking X-Git-Tag: v12.2.2~61^2~90 X-Git-Url: http://git-server-git.apps.pok.os.sepia.ceph.com/?a=commitdiff_plain;h=d70fae092db920e2db77c7c8b044cfb4d9687992;p=ceph.git mgr: clean up DaemonStateIndex locking 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 (cherry picked from commit 806f10847cefe5c7a78fc319b1b130d372197dd3) --- diff --git a/src/mgr/DaemonServer.cc b/src/mgr/DaemonServer.cc index 298ed4413f6e..3602881ee252 100644 --- a/src/mgr/DaemonServer.cc +++ b/src/mgr/DaemonServer.cc @@ -332,7 +332,7 @@ bool DaemonServer::handle_open(MMgrOpen *m) if (daemon) { dout(20) << "updating existing DaemonState for " << m->daemon_name << dendl; Mutex::Locker l(daemon->lock); - daemon_state.get(key)->perf_counters.clear(); + daemon->perf_counters.clear(); } if (m->service_daemon) { diff --git a/src/mgr/DaemonState.cc b/src/mgr/DaemonState.cc index f304b18c6fcb..8da321747357 100644 --- a/src/mgr/DaemonState.cc +++ b/src/mgr/DaemonState.cc @@ -20,7 +20,7 @@ void DaemonStateIndex::insert(DaemonStatePtr dm) { - Mutex::Locker l(lock); + RWLock::WLocker l(lock); if (all.count(dm->key)) { _erase(dm->key); @@ -32,7 +32,7 @@ void DaemonStateIndex::insert(DaemonStatePtr dm) void DaemonStateIndex::_erase(const DaemonKey& dmk) { - assert(lock.is_locked_by_me()); + assert(lock.is_wlocked()); const auto to_erase = all.find(dmk); assert(to_erase != all.end()); @@ -49,7 +49,7 @@ void DaemonStateIndex::_erase(const DaemonKey& dmk) DaemonStateCollection DaemonStateIndex::get_by_service( const std::string& svc) const { - Mutex::Locker l(lock); + RWLock::RLocker l(lock); DaemonStateCollection result; @@ -65,7 +65,7 @@ DaemonStateCollection DaemonStateIndex::get_by_service( DaemonStateCollection DaemonStateIndex::get_by_server( const std::string &hostname) const { - Mutex::Locker l(lock); + RWLock::RLocker l(lock); if (by_server.count(hostname)) { return by_server.at(hostname); @@ -76,14 +76,14 @@ DaemonStateCollection DaemonStateIndex::get_by_server( bool DaemonStateIndex::exists(const DaemonKey &key) const { - Mutex::Locker l(lock); + RWLock::RLocker l(lock); return all.count(key) > 0; } DaemonStatePtr DaemonStateIndex::get(const DaemonKey &key) { - Mutex::Locker l(lock); + RWLock::RLocker l(lock); auto iter = all.find(key); if (iter != all.end()) { @@ -98,7 +98,7 @@ void DaemonStateIndex::cull(const std::string& svc_name, { std::vector victims; - Mutex::Locker l(lock); + RWLock::WLocker l(lock); auto begin = all.lower_bound({svc_name, ""}); auto end = all.end(); for (auto &i = begin; i != end; ++i) { diff --git a/src/mgr/DaemonState.h b/src/mgr/DaemonState.h index 98853a2ec2cb..7dbdeb657cda 100644 --- a/src/mgr/DaemonState.h +++ b/src/mgr/DaemonState.h @@ -20,7 +20,7 @@ #include #include -#include "common/Mutex.h" +#include "common/RWLock.h" #include "msg/msg_types.h" @@ -133,38 +133,52 @@ typedef std::map DaemonStateCollection; class DaemonStateIndex { private: + mutable RWLock lock = {"DaemonStateIndex", true, true, true}; + std::map by_server; DaemonStateCollection all; - std::set updating; - mutable Mutex lock; + void _erase(const DaemonKey& dmk); public: - - DaemonStateIndex() : lock("DaemonState") {} + DaemonStateIndex() {} // FIXME: shouldn't really be public, maybe construct DaemonState // objects internally to avoid this. PerfCounterTypes types; void insert(DaemonStatePtr dm); - void _erase(const DaemonKey& dmk); - bool exists(const DaemonKey &key) const; DaemonStatePtr get(const DaemonKey &key); + + // Note that these return by value rather than reference to avoid + // callers needing to stay in lock while using result. Callers must + // still take the individual DaemonState::lock on each entry though. DaemonStateCollection get_by_server(const std::string &hostname) const; DaemonStateCollection get_by_service(const std::string &svc_name) const; - - const DaemonStateCollection &get_all() const {return all;} - const std::map &get_all_servers() const - { - return by_server; + DaemonStateCollection get_all() const {return all;} + + template + auto with_daemons_by_server(Callback&& cb, Args&&... args) const -> + decltype(cb(by_server, std::forward(args)...)) { + RWLock::RLocker l(lock); + + return std::forward(cb)(by_server, std::forward(args)...); } - void notify_updating(const DaemonKey &k) { updating.insert(k); } - void clear_updating(const DaemonKey &k) { updating.erase(k); } - bool is_updating(const DaemonKey &k) { return updating.count(k) > 0; } + void notify_updating(const DaemonKey &k) { + RWLock::WLocker l(lock); + updating.insert(k); + } + void clear_updating(const DaemonKey &k) { + RWLock::WLocker l(lock); + updating.erase(k); + } + bool is_updating(const DaemonKey &k) { + RWLock::RLocker l(lock); + return updating.count(k) > 0; + } /** * Remove state for all daemons of this type whose names are diff --git a/src/mgr/PyModules.cc b/src/mgr/PyModules.cc index e014b382fd35..f7da497a58a8 100644 --- a/src/mgr/PyModules.cc +++ b/src/mgr/PyModules.cc @@ -105,14 +105,16 @@ PyObject *PyModules::list_servers_python() dout(10) << " >" << dendl; PyFormatter f(false, true); - const auto &all = daemon_state.get_all_servers(); - for (const auto &i : all) { - const auto &hostname = i.first; + daemon_state.with_daemons_by_server([this, &f] + (const std::map &all) { + for (const auto &i : all) { + const auto &hostname = i.first; - f.open_object_section("server"); - dump_server(hostname, i.second, &f); - f.close_section(); - } + f.open_object_section("server"); + dump_server(hostname, i.second, &f); + f.close_section(); + } + }); return f.get(); } @@ -741,35 +743,34 @@ PyObject* PyModules::get_perf_schema_python( Mutex::Locker l(lock); PyEval_RestoreThread(tstate); - DaemonStateCollection states; + DaemonStateCollection daemons; if (svc_type == "") { - states = daemon_state.get_all(); + daemons = std::move(daemon_state.get_all()); } else if (svc_id.empty()) { - states = daemon_state.get_by_service(svc_type); + daemons = std::move(daemon_state.get_by_service(svc_type)); } else { auto key = DaemonKey(svc_type, svc_id); // so that the below can be a loop in all cases auto got = daemon_state.get(key); if (got != nullptr) { - states[key] = got; + daemons[key] = got; } } PyFormatter f; f.open_object_section("perf_schema"); - // FIXME: this is unsafe, I need to either be inside DaemonStateIndex's - // lock or put a lock on individual DaemonStates - if (!states.empty()) { - for (auto statepair : states) { - std::ostringstream daemon_name; + if (!daemons.empty()) { + for (auto statepair : daemons) { auto key = statepair.first; auto state = statepair.second; - Mutex::Locker l(state->lock); + + std::ostringstream daemon_name; daemon_name << key.first << "." << key.second; f.open_object_section(daemon_name.str().c_str()); + Mutex::Locker l(state->lock); for (auto typestr : state->perf_counters.declared_types) { f.open_object_section(typestr.c_str()); auto type = state->perf_counters.types[typestr];