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 <john.spray@redhat.com>

diff --git a/src/mgr/ActivePyModules.cc b/src/mgr/ActivePyModules.cc
index 9f57ed9b0ce66d68d807bf2b1f672d27fe4867c8..6f75a7b6586b3204188627dd3032db46971e246e 100644 (file)
--- 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<std::string, DaemonStateCollection> &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<std::string, std::map<std::string, uint32_t> > osds;
           std::map<std::string, std::map<std::string, uint32_t> > pools;
           std::map<std::string, uint32_t> 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);
 }