From 61aa7e2e0230bff49e757c32932d1db4bcad5f67 Mon Sep 17 00:00:00 2001 From: Sage Weil Date: Mon, 26 Nov 2018 14:54:00 -0600 Subject: [PATCH] mgr: revise locking in getter paths There were many places where with_* methods were blocking on locks while holding the GIL. Signed-off-by: John Spray --- src/mgr/ActivePyModules.cc | 114 ++++++++++++++++++++----------------- 1 file changed, 62 insertions(+), 52 deletions(-) diff --git a/src/mgr/ActivePyModules.cc b/src/mgr/ActivePyModules.cc index 9f57ed9b0ce66..6f75a7b6586b3 100644 --- a/src/mgr/ActivePyModules.cc +++ b/src/mgr/ActivePyModules.cc @@ -103,13 +103,13 @@ PyObject *ActivePyModules::get_server_python(const std::string &hostname) PyObject *ActivePyModules::list_servers_python() { PyThreadState *tstate = PyEval_SaveThread(); - std::lock_guard l(lock); - PyEval_RestoreThread(tstate); dout(10) << " >" << dendl; PyFormatter f(false, true); - daemon_state.with_daemons_by_server([this, &f] + daemon_state.with_daemons_by_server([this, &f, &tstate] (const std::map &all) { + PyEval_RestoreThread(tstate); + for (const auto &i : all) { const auto &hostname = i.first; @@ -162,26 +162,30 @@ PyObject *ActivePyModules::get_daemon_status_python( PyObject *ActivePyModules::get_python(const std::string &what) { + PyFormatter f; + + // Drop the GIL, as most of the following blocks will block on + // a mutex -- they are all responsible for re-taking the GIL before + // touching the PyFormatter instance or returning from the function. PyThreadState *tstate = PyEval_SaveThread(); - std::lock_guard l(lock); - PyEval_RestoreThread(tstate); if (what == "fs_map") { - PyFormatter f; - cluster_state.with_fsmap([&f](const FSMap &fsmap) { + cluster_state.with_fsmap([&f, &tstate](const FSMap &fsmap) { + PyEval_RestoreThread(tstate); fsmap.dump(&f); }); return f.get(); } else if (what == "osdmap_crush_map_text") { bufferlist rdata; - cluster_state.with_osdmap([&rdata](const OSDMap &osd_map){ - osd_map.crush->encode(rdata, CEPH_FEATURES_SUPPORTED_DEFAULT); + cluster_state.with_osdmap([&rdata, &tstate](const OSDMap &osd_map){ + PyEval_RestoreThread(tstate); + osd_map.crush->encode(rdata, CEPH_FEATURES_SUPPORTED_DEFAULT); }); std::string crush_text = rdata.to_str(); return PyString_FromString(crush_text.c_str()); } else if (what.substr(0, 7) == "osd_map") { - PyFormatter f; - cluster_state.with_osdmap([&f, &what](const OSDMap &osd_map){ + cluster_state.with_osdmap([&f, &what, &tstate](const OSDMap &osd_map){ + PyEval_RestoreThread(tstate); if (what == "osd_map") { osd_map.dump(&f); } else if (what == "osd_map_tree") { @@ -192,7 +196,7 @@ PyObject *ActivePyModules::get_python(const std::string &what) }); return f.get(); } else if (what.substr(0, 6) == "config") { - PyFormatter f; + PyEval_RestoreThread(tstate); if (what == "config_options") { g_conf().config_options(&f); } else if (what == "config") { @@ -200,24 +204,25 @@ PyObject *ActivePyModules::get_python(const std::string &what) } return f.get(); } else if (what == "mon_map") { - PyFormatter f; cluster_state.with_monmap( - [&f](const MonMap &monmap) { + [&f, &tstate](const MonMap &monmap) { + PyEval_RestoreThread(tstate); monmap.dump(&f); } ); return f.get(); } else if (what == "service_map") { - PyFormatter f; cluster_state.with_servicemap( - [&f](const ServiceMap &service_map) { + [&f, &tstate](const ServiceMap &service_map) { + PyEval_RestoreThread(tstate); service_map.dump(&f); } ); return f.get(); } else if (what == "osd_metadata") { - PyFormatter f; auto dmc = daemon_state.get_by_service("osd"); + PyEval_RestoreThread(tstate); + for (const auto &i : dmc) { std::lock_guard l(i.second->lock); f.open_object_section(i.first.second.c_str()); @@ -229,9 +234,10 @@ PyObject *ActivePyModules::get_python(const std::string &what) } return f.get(); } else if (what == "pg_summary") { - PyFormatter f; cluster_state.with_pgmap( - [&f](const PGMap &pg_map) { + [&f, &tstate](const PGMap &pg_map) { + PyEval_RestoreThread(tstate); + std::map > osds; std::map > pools; std::map all; @@ -275,68 +281,70 @@ PyObject *ActivePyModules::get_python(const std::string &what) ); return f.get(); } else if (what == "pg_status") { - PyFormatter f; cluster_state.with_pgmap( - [&f](const PGMap &pg_map) { + [&f, &tstate](const PGMap &pg_map) { + PyEval_RestoreThread(tstate); pg_map.print_summary(&f, nullptr); } ); return f.get(); } else if (what == "pg_dump") { - PyFormatter f; cluster_state.with_pgmap( - [&f](const PGMap &pg_map) { + [&f, &tstate](const PGMap &pg_map) { + PyEval_RestoreThread(tstate); pg_map.dump(&f); } ); return f.get(); } else if (what == "devices") { - PyFormatter f; + PyEval_RestoreThread(tstate); f.open_array_section("devices"); daemon_state.with_devices([&f] (const DeviceState& dev) { - f.dump_object("device", dev); - }); + f.dump_object("device", dev); + }); + + PyEval_RestoreThread(tstate); f.close_section(); return f.get(); } else if (what.size() > 7 && what.substr(0, 7) == "device ") { string devid = what.substr(7); - PyFormatter f; - daemon_state.with_device(devid, [&f] (const DeviceState& dev) { + daemon_state.with_device(devid, [&f, &tstate] (const DeviceState& dev) { + PyEval_RestoreThread(tstate); f.dump_object("device", dev); }); return f.get(); } else if (what == "io_rate") { - PyFormatter f; cluster_state.with_pgmap( - [&f](const PGMap &pg_map) { + [&f, &tstate](const PGMap &pg_map) { + PyEval_RestoreThread(tstate); pg_map.dump_delta(&f); } ); return f.get(); } else if (what == "df") { - PyFormatter f; - - cluster_state.with_pgmap([this, &f](const PGMap &pg_map) { - cluster_state.with_osdmap( - [&pg_map, &f](const OSDMap &osd_map) { + cluster_state.with_osdmap_and_pgmap( + [this, &f, &tstate]( + const OSDMap& osd_map, + const PGMap &pg_map) { + PyEval_RestoreThread(tstate); pg_map.dump_cluster_stats(nullptr, &f, true); pg_map.dump_pool_stats_full(osd_map, nullptr, &f, true); }); - }); return f.get(); } else if (what == "osd_stats") { - PyFormatter f; cluster_state.with_pgmap( - [&f](const PGMap &pg_map) { + [&f, &tstate](const PGMap &pg_map) { + PyEval_RestoreThread(tstate); pg_map.dump_osd_stats(&f); }); return f.get(); } else if (what == "osd_pool_stats") { int64_t poolid = -ENOENT; string pool_name; - PyFormatter f; - cluster_state.with_osdmap_and_pgmap([&](const OSDMap& osdmap, const PGMap& pg_map) { + cluster_state.with_osdmap_and_pgmap([&](const OSDMap& osdmap, + const PGMap& pg_map) { + PyEval_RestoreThread(tstate); f.open_array_section("pool_stats"); for (auto &p : osdmap.get_pools()) { poolid = p.first; @@ -346,7 +354,6 @@ PyObject *ActivePyModules::get_python(const std::string &what) }); return f.get(); } else if (what == "health" || what == "mon_status") { - PyFormatter f; bufferlist json; if (what == "health") { json = cluster_state.get_health(); @@ -355,16 +362,19 @@ PyObject *ActivePyModules::get_python(const std::string &what) } else { ceph_abort(); } + + PyEval_RestoreThread(tstate); f.dump_string("json", json.to_str()); return f.get(); } else if (what == "mgr_map") { - PyFormatter f; - cluster_state.with_mgrmap([&f](const MgrMap &mgr_map) { + cluster_state.with_mgrmap([&f, &tstate](const MgrMap &mgr_map) { + PyEval_RestoreThread(tstate); mgr_map.dump(&f); }); return f.get(); } else { derr << "Python module requested unknown data '" << what << "'" << dendl; + PyEval_RestoreThread(tstate); Py_RETURN_NONE; } } @@ -518,6 +528,7 @@ PyObject *ActivePyModules::get_store_prefix(const std::string &module_name, { PyThreadState *tstate = PyEval_SaveThread(); std::lock_guard l(lock); + std::lock_guard lock(module_config.lock); PyEval_RestoreThread(tstate); const std::string base_prefix = PyModule::config_prefix @@ -527,8 +538,6 @@ PyObject *ActivePyModules::get_store_prefix(const std::string &module_name, PyFormatter f; - std::lock_guard lock(module_config.lock); - for (auto p = store_cache.lower_bound(global_prefix); p != store_cache.end() && p->first.find(global_prefix) == 0; ++p) { @@ -811,15 +820,16 @@ PyObject *construct_with_capsule( PyObject *ActivePyModules::get_osdmap() { - PyThreadState *tstate = PyEval_SaveThread(); - std::lock_guard l(lock); - PyEval_RestoreThread(tstate); - OSDMap *newmap = new OSDMap; - cluster_state.with_osdmap([&](const OSDMap& o) { - newmap->deepish_copy_from(o); - }); + PyThreadState *tstate = PyEval_SaveThread(); + { + std::lock_guard l(lock); + cluster_state.with_osdmap([&](const OSDMap& o) { + newmap->deepish_copy_from(o); + }); + } + PyEval_RestoreThread(tstate); return construct_with_capsule("mgr_module", "OSDMap", (void*)newmap); } -- 2.39.5