crimson/mgr: prevent pg stats report pile-up in overloaded store

When AlienStore's thread pool is saturated (e.g. during PG splitting),
pool_statfs() calls inside OSD::get_stats() can get stuck behind
hundreds of queued transactions in the free_slots semaphore. Because
Client::report() dispatched get_stats() unconditionally every 5 seconds,
hundreds of stuck pool_statfs() calls accumulated, further exhausting
the semaphore and making recovery impossible.

Fix by tracking whether a get_stats() call is already in flight and
skipping new dispatches until the current one completes. Emit a
cluster-log warning every ~60 seconds when stats reporting is stuck.

This limits the damage from thread pool starvation to a single consumed
free_slot instead of an unbounded pile-up.

Fixes: https://tracker.ceph.com/issues/77930
Signed-off-by: Ronen Friedman <rfriedma@redhat.com>

diff --git a/src/crimson/mgr/client.cc b/src/crimson/mgr/client.cc
index 3980df47f9e2b7dcfe1e17e1d0b5053d2ccb3957..790f7453fe977e0a4637e778bf759b10c2c5cfa1 100644 (file)
--- a/src/crimson/mgr/client.cc
+++ b/src/crimson/mgr/client.cc
@@ -26,12 +26,14 @@ namespace crimson::mgr
 Client::Client(crimson::net::Messenger& msgr,
               WithStats& with_stats,
               set_perf_queries_cb_t cb_set,
-              get_perf_report_cb_t cb_get)
+              get_perf_report_cb_t cb_get,
+              stats_warning_cb_t stats_warning_cb)
   : msgr{msgr},
     with_stats{with_stats},
     report_timer{[this] {report();}},
     set_perf_queries_cb(cb_set),
-    get_perf_report_cb(cb_get)
+    get_perf_report_cb(cb_get),
+    stats_warning_cb(std::move(stats_warning_cb))
 {}
 
 seastar::future<> Client::start()
@@ -214,11 +216,31 @@ void Client::report()
   LOG_PREFIX(Client::report);
   DEBUGDPP("", *this);
   _send_report();
+  if (stats_in_flight) {
+    ++stats_skip_count;
+    if (stats_skip_count >= STATS_ABANDON_THRESHOLD) {
+      WARNDPP("pg stats send stuck for {} ticks, abandoning and retrying",
+             *this, stats_skip_count);
+      stats_in_flight = false;
+    } else {
+      WARNDPP("pg stats send still in flight, skipped (count={})",
+             *this, stats_skip_count);
+      if (stats_warning_cb &&
+         stats_skip_count % STATS_WARN_INTERVAL == 0) {
+       stats_warning_cb(stats_skip_count);
+      }
+      return;
+    }
+  }
+  stats_in_flight = true;
+  stats_skip_count = 0;
   gates.dispatch_in_background(__func__, *this, [this, FNAME] {
     DEBUGDPP("dispatching in background", *this);
     return with_stats.get_stats(
     ).then([this](auto &&pg_stats) {
       return send(std::move(pg_stats));
+    }).finally([this] {
+      stats_in_flight = false;
     });
   });
 }

diff --git a/src/crimson/mgr/client.h b/src/crimson/mgr/client.h
index 1ae104f0f7ef99c023ac2916d5496b56dc3c7b3e..ddcec46fee575da5f15e38ae2e599ac57fce1ea7 100644 (file)
--- a/src/crimson/mgr/client.h
+++ b/src/crimson/mgr/client.h
@@ -35,11 +35,13 @@ class Client : public crimson::net::Dispatcher {
   using get_perf_report_cb_t = std::function<seastar::future<MetricPayload> ()>;
   using set_perf_queries_cb_t =
     std::function<seastar::future<> (const ConfigPayload &)>;
+  using stats_warning_cb_t = std::function<void(uint32_t skips)>;
 public:
   Client(crimson::net::Messenger& msgr,
         WithStats& with_stats,
         set_perf_queries_cb_t cb_set,
-        get_perf_report_cb_t cb_get);
+        get_perf_report_cb_t cb_get,
+        stats_warning_cb_t stats_warning_cb);
   seastar::future<> start();
   seastar::future<> stop();
   seastar::future<> send(MessageURef msg);
@@ -75,6 +77,14 @@ private:
 
   std::vector<DaemonHealthMetric> daemon_health_metrics;
 
+  bool stats_in_flight = false;
+  uint32_t stats_skip_count = 0;
+  // emit a cluster-log warning every this many skipped reports (~60s)
+  static constexpr uint32_t STATS_WARN_INTERVAL = 12;
+  // abandon a stuck in-flight stats send after this many skips (~5min)
+  static constexpr uint32_t STATS_ABANDON_THRESHOLD = 5 * STATS_WARN_INTERVAL;
+  stats_warning_cb_t stats_warning_cb;
+
   void _send_report();
 };
 

diff --git a/src/crimson/osd/osd.cc b/src/crimson/osd/osd.cc
index d742eef94be19be731b9216bc29db2d4fc041f89..3ef85860fbb4cd364beee5efc218f704a6a6d273 100644 (file)
--- a/src/crimson/osd/osd.cc
+++ b/src/crimson/osd/osd.cc
@@ -106,11 +106,19 @@ OSD::OSD(int id, uint32_t nonce,
     mgrc{new crimson::mgr::Client{
       *public_msgr,
       *this,
+      // one lambda to perf-query them all
       [this](const ConfigPayload &config_payload) {
        return set_perf_queries(config_payload);
       },
+      // one lambda to help _send_report() report them all
       [this] {
        return get_perf_reports();
+      },
+      // one lambda to log-warn if pg stats report is stuck
+      [this](uint32_t skips) {
+       clog->warn() << fmt::format(
+         "pg stats report stuck for ~{}s, store may be overloaded",
+         skips * 5);
       }
     }},
     store{store},