crimson/osd: fix backfill deadlock in BackfillState::Waiting

BackfillState::Enqueuing unconditionally posts RequestWaiting{}
when budget available return false. Waiting can exit only via
ObjectPushed which requires an in-flight push. If the budget
is exhausted with no pushes in flight no objectPush will ever
arrive and the PG stalls permanently.

Solution: Introduce a dedicated BudgetBlocked state. When budget_available()
is false and tracked_objects_completed()==true post RequestBudgetBlocked{}
instead of RequestWaiting{}.BudgetBlocked calls get_backfill_throttle()
to suspend until a slots free up, then fires BudgetAvailable{} which
transitions directly back to Enqueuing to dispatch the next batch of pushes.

Fixes: https://tracker.ceph.com/issues/77804
Signed-off-by: Mohit Agrawal <moagrawa@redhat.com>

diff --git a/src/crimson/osd/backfill_state.cc b/src/crimson/osd/backfill_state.cc
index 295113c2f1601ac6294566a2783a732e2dcaf55e..e2b68c7ec49ee7fcdd7e266858b308539e50519a 100644 (file)
--- a/src/crimson/osd/backfill_state.cc
+++ b/src/crimson/osd/backfill_state.cc
@@ -398,7 +398,13 @@ BackfillState::Enqueuing::Enqueuing(my_context ctx)
 
   do {
     if (!backfill_listener().budget_available()) {
-      post_event(RequestWaiting{});
+      if (backfill_state().progress_tracker->tracked_objects_completed()) {
+        INFODPP("backfill budget unavailable with nothing in flight, "
+                "entering BudgetBlocked", pg());
+        post_event(RequestBudgetBlocked{});
+      } else {
+        post_event(RequestWaiting{});
+      }
       return;
     } else if (should_rescan_replicas(backfill_state().peer_backfill_info,
                                      primary_bi)) {
@@ -653,6 +659,51 @@ BackfillState::Waiting::react(Triggered evt)
   return discard_event();
 }
 
+// -- BudgetBlocked
+BackfillState::BudgetBlocked::BudgetBlocked(my_context ctx)
+  : my_base(ctx)
+{
+  LOG_PREFIX(BackfillState::BudgetBlocked::BudgetBlocked);
+  DEBUGDPP("budget unavailable with nothing in flight, "
+           "waiting for throttle slot to become available", pg());
+  backfill_listener().request_budget_retry();
+}
+
+boost::statechart::result
+BackfillState::BudgetBlocked::react(SuspendBackfill evt)
+{
+  LOG_PREFIX(BackfillState::BudgetBlocked::react::SuspendBackfill);
+  DEBUGDPP("suspended within BudgetBlocked", pg());
+  backfill_state().on_suspended();
+  return discard_event();
+}
+
+boost::statechart::result
+BackfillState::BudgetBlocked::react(Triggered evt)
+{
+  LOG_PREFIX(BackfillState::BudgetBlocked::react::Triggered);
+  ceph_assert(backfill_state().is_suspended());
+  if (backfill_state().on_resumed()) {
+    DEBUGDPP("Backfill resumed, going Enqueuing", pg());
+    return transit<Enqueuing>();
+  }
+  return discard_event();
+}
+
+boost::statechart::result
+BackfillState::BudgetBlocked::react(BudgetAvailable evt)
+{
+  LOG_PREFIX(BackfillState::BudgetBlocked::react::BudgetAvailable);
+  DEBUGDPP("BudgetBlocked::react() on BudgetAvailable", pg());
+  if (!backfill_state().is_suspended()) {
+    return transit<Enqueuing>();
+  } else {
+    DEBUGDPP("backfill suspended, not going Enqueuing", pg());
+    backfill_state().go_enqueuing_on_resume();
+  }
+  return discard_event();
+}
+
 // -- Done
 BackfillState::Done::Done(my_context ctx)
   : my_base(ctx)

diff --git a/src/crimson/osd/backfill_state.h b/src/crimson/osd/backfill_state.h
index 5870859abefef5ac70d185b1b83e068af00fde48..c47db0c9ad031becd4c85261c413f5da704623eb 100644 (file)
--- a/src/crimson/osd/backfill_state.h
+++ b/src/crimson/osd/backfill_state.h
@@ -63,6 +63,12 @@ struct BackfillState {
   struct SuspendBackfill : sc::event<SuspendBackfill> {
   };
 
+  struct RequestBudgetBlocked : sc::event<RequestBudgetBlocked> {
+  };
+
+  struct BudgetAvailable : sc::event<BudgetAvailable> {
+  };
+
 private:
   // internal events
   struct RequestPrimaryScanning : sc::event<RequestPrimaryScanning> {
@@ -84,6 +90,7 @@ public:
   struct ReplicasScanning;
   struct Waiting;
   struct Done;
+  struct BudgetBlocked;
 
   struct BackfillMachine : sc::state_machine<BackfillMachine, Initial> {
     BackfillMachine(BackfillState& backfill_state,
@@ -157,6 +164,7 @@ public:
       sc::transition<RequestPrimaryScanning, PrimaryScanning>,
       sc::transition<RequestReplicasScanning, ReplicasScanning>,
       sc::transition<RequestWaiting, Waiting>,
+      sc::transition<RequestBudgetBlocked, BudgetBlocked>,
       sc::transition<sc::event_base, Crashed>>;
     explicit Enqueuing(my_context);
 
@@ -281,6 +289,19 @@ public:
     }
   };
 
+  struct BudgetBlocked : sc::state<BudgetBlocked, BackfillMachine>,
+                         StateHelper<BudgetBlocked> {
+    using reactions = boost::mpl::list<
+      sc::custom_reaction<BudgetAvailable>,
+      sc::custom_reaction<SuspendBackfill>,
+      sc::custom_reaction<Triggered>,
+      sc::transition<sc::event_base, Crashed>>;
+    explicit BudgetBlocked(my_context ctx);
+    sc::result react(BudgetAvailable);
+    sc::result react(SuspendBackfill);
+    sc::result react(Triggered);
+  };
+
   BackfillState(BackfillListener& backfill_listener,
                 std::unique_ptr<PeeringFacade> peering_state,
                 std::unique_ptr<PGFacade> pg);
@@ -387,6 +408,8 @@ struct BackfillState::BackfillListener {
 
   virtual void backfilled() = 0;
 
+  virtual void request_budget_retry() = 0;
+
   virtual ~BackfillListener() = default;
 };
 
@@ -487,5 +510,7 @@ public:
 #if FMT_VERSION >= 90000
 template <> struct fmt::formatter<crimson::osd::BackfillState::PGFacade>
   : fmt::ostream_formatter {};
+template <> struct fmt::formatter<crimson::osd::BackfillState::BudgetBlocked>
+  : fmt::ostream_formatter {};
 #endif
 

diff --git a/src/crimson/osd/pg_recovery.cc b/src/crimson/osd/pg_recovery.cc
index 7b252e212cfbc2720746a29385a81bf233e39d4f..62014cfeb4d4dda0ab55d32869f2cc0a68c6dce0 100644 (file)
--- a/src/crimson/osd/pg_recovery.cc
+++ b/src/crimson/osd/pg_recovery.cc
@@ -593,6 +593,9 @@ void PGRecovery::enqueue_push(
   if (!added)
     return;
   peering_state.prepare_backfill_for_missing(obj, v, peers);
+  // release the budget_retry_releaser now that we're dispatching a real
+  // push -- recover_object_with_throttle will acquire its own slot
+  budget_retry_releaser.reset();
   std::ignore = recover_object_with_throttle(obj, v).\
   handle_exception_interruptible([] (auto) {
     ceph_abort_msg("got exception on backfill's push");
@@ -618,6 +621,10 @@ void PGRecovery::enqueue_drop(
 {
   LOG_PREFIX(PGRecovery::update_peers_last_backfill);
   DEBUGDPP("obj={} v={} target={}", *pg->get_dpp(), obj, v, target);
+  // release the budget_retry_releaser now that we're dispatching work
+  // (same as enqueue_push) -- slot was held to guarantee Enqueuing
+  // found budget available, drops don't need their own throttle slot
+  budget_retry_releaser.reset();
   // allocate a pair if target is seen for the first time
   auto& req = backfill_drop_requests[target];
   if (!req) {
@@ -705,9 +712,46 @@ bool PGRecovery::budget_available() const
   return ss.throttle_available();
 }
 
+PGRecovery::interruptible_future<>
+PGRecovery::do_request_budget_retry()
+{
+  LOG_PREFIX(PGRecovery::do_request_budget_retry);
+  DEBUGDPP("budget unavailable with nothing in flight, "
+           "waiting for throttle slot", *pg->get_dpp());
+  auto releaser = co_await get_backfill_throttle();
+  budget_retry_in_flight = false;
+  if (!backfill_state) {
+    DEBUGDPP("backfill_state is null, skipping BudgetAvailable "
+             "(pg cleaned or interval changed)", *pg->get_dpp());
+    co_return;
+  }
+  // hold the releaser so the slot stays acquired until Enqueuing
+  budget_retry_releaser.emplace(std::move(releaser));
+  if (backfill_state->is_triggered()) {
+    backfill_state->post_event(
+      BackfillState::BudgetAvailable{}.intrusive_from_this());
+  } else {
+    backfill_state->process_event(
+      BackfillState::BudgetAvailable{}.intrusive_from_this());
+  }
+}
+
+void PGRecovery::request_budget_retry()
+{
+  LOG_PREFIX(PGRecovery::request_budget_retry);
+  if (budget_retry_in_flight) {
+    DEBUGDPP("budget retry already in flight, skipping", *pg->get_dpp());
+    return;
+  }
+  budget_retry_in_flight = true;
+  std::ignore = do_request_budget_retry();
+}
+
 void PGRecovery::on_pg_clean()
 {
   replica_scan_throttle_releasers.clear();
+  budget_retry_releaser.reset();
+  budget_retry_in_flight = false;
   backfill_state.reset();
 }
 
@@ -775,6 +819,8 @@ void PGRecovery::on_activate_complete()
   LOG_PREFIX(PGRecovery::on_activate_complete);
   DEBUGDPP("backfill_state={}", *pg->get_dpp(), fmt::ptr(backfill_state.get()));
   replica_scan_throttle_releasers.clear();
+  budget_retry_releaser.reset();
+  budget_retry_in_flight = false;
   backfill_state.reset();
 }
 

diff --git a/src/crimson/osd/pg_recovery.h b/src/crimson/osd/pg_recovery.h
index 6d2a9e30ea2b6bf7f8189e023e6ab1bb6fba15b6..c44f14b9e7d792020883f8b6b4958cc5275db597 100644 (file)
--- a/src/crimson/osd/pg_recovery.h
+++ b/src/crimson/osd/pg_recovery.h
@@ -126,13 +126,15 @@ private:
     return backend->recover_object(soid, need);
   }
 
+  interruptible_future<> do_request_budget_retry();
+
   // backfill begin
   std::unique_ptr<crimson::osd::BackfillState> backfill_state;
   std::map<pg_shard_t,
            MURef<MOSDPGBackfillRemove>> backfill_drop_requests;
   std::map<pg_shard_t,
            OperationThrottler::ThrottleReleaser> replica_scan_throttle_releasers;
-
+  std::optional<OperationThrottler::ThrottleReleaser> budget_retry_releaser;
   template <class EventT>
   void start_backfill_recovery(
     const EventT& evt);
@@ -154,6 +156,7 @@ private:
   void update_peers_last_backfill(
     const hobject_t& new_last_backfill) final;
   bool budget_available() const final;
+  void request_budget_retry() final;
 
   template <typename T>
   void start_peering_event_operation_listener(T &&evt, float delay = 0);
@@ -163,6 +166,7 @@ private:
 
   friend crimson::osd::BackfillState::PGFacade;
   friend crimson::osd::PG;
+  bool budget_retry_in_flight = false;
   // backfill end
 };