Merge pull request ceph#60185 from xxhdx1985126/wip-68306

crimson/osd/pg_recovery: trigger BackfillState events synchronously

Reviewed-by: Matan Breizman <[email protected]>
Reviewed-by: Radosław Zarzyński <[email protected]>

Author: Matan-B

src/crimson/osd/backfill_state.h

@@ -96,6 +96,9 @@ struct BackfillState {
     BackfillListener& backfill_listener;
     std::unique_ptr<PeeringFacade> peering_state;
     std::unique_ptr<PGFacade> pg;
+    void post_event(const sc::event_base &e) {
+      sc::state_machine<BackfillMachine, Initial>::post_event(e);
+    }
   };
 
 private:
@@ -298,6 +301,10 @@ struct BackfillState {
     const std::vector<pg_shard_t> &peers);
 
 
+  void post_event(boost::intrusive_ptr<const sc::event_base> evt) {
+    backfill_machine.post_event(*std::move(evt));
+  }
+
   bool is_triggered() const {
     return backfill_machine.triggering_event() != nullptr;
   }

src/crimson/osd/osd_operation_external_tracking.h

@@ -342,13 +342,6 @@ struct EventBackendRegistry<osd::RecoverySubRequest> {
   }
 };
 
-template <>
-struct EventBackendRegistry<osd::BackfillRecovery> {
-  static std::tuple<> get_backends() {
-    return {};
-  }
-};
-
 template <>
 struct EventBackendRegistry<osd::PGAdvanceMap> {
   static std::tuple<> get_backends() {

src/crimson/osd/osd_operations/background_recovery.cc

@@ -175,41 +175,7 @@ PglogBasedRecovery::do_recovery()
   });
 }
 
-PGPeeringPipeline &BackfillRecovery::peering_pp(PG &pg)
-{
-  return pg.peering_request_pg_pipeline;
-}
-
-BackfillRecovery::interruptible_future<bool>
-BackfillRecovery::do_recovery()
-{
-  LOG_PREFIX(BackfillRecovery::do_recovery);
-  DEBUGDPPI("{}", *pg, __func__);
-
-  if (pg->has_reset_since(epoch_started)) {
-    DEBUGDPPI("{}: pg got reset since epoch_started={}",
-		*pg, __func__, epoch_started);
-    return seastar::make_ready_future<bool>(false);
-  }
-  // TODO: limits
-  return enter_stage<interruptor>(
-    // process_event() of our boost::statechart machine is non-reentrant.
-    // with the backfill_pipeline we protect it from a second entry from
-    // the implementation of BackfillListener.
-    // additionally, this stage serves to synchronize with PeeringEvent.
-    peering_pp(*pg).process
-  ).then_interruptible([this] {
-    pg->get_recovery_handler()->dispatch_backfill_event(std::move(evt));
-    return handle.complete();
-  }).then_interruptible([] {
-    return seastar::make_ready_future<bool>(false);
-  }).finally([this] {
-    handle.exit();
-  });
-}
-
 template class BackgroundRecoveryT<UrgentRecovery>;
 template class BackgroundRecoveryT<PglogBasedRecovery>;
-template class BackgroundRecoveryT<BackfillRecovery>;
 
 } // namespace crimson::osd

src/crimson/osd/osd_operations/background_recovery.h

@@ -52,10 +52,9 @@ class BackgroundRecoveryT : public PhasedOperationT<T> {
 
 /// represent a recovery initiated for serving a client request
 ///
-/// unlike @c PglogBasedRecovery and @c BackfillRecovery,
-/// @c UrgentRecovery is not throttled by the scheduler. and it
-/// utilizes @c RecoveryBackend directly to recover the unreadable
-/// object.
+/// unlike @c PglogBasedRecovery, @c UrgentRecovery is not throttled
+/// by the scheduler. and it utilizes @c RecoveryBackend directly to
+/// recover the unreadable object.
 class UrgentRecovery final : public BackgroundRecoveryT<UrgentRecovery> {
 public:
   UrgentRecovery(
@@ -107,49 +106,9 @@ class PglogBasedRecovery final : public BackgroundRecoveryT<PglogBasedRecovery>
   bool cancelled = false;
 };
 
-class BackfillRecovery final : public BackgroundRecoveryT<BackfillRecovery> {
-public:
-
-  template <class EventT>
-  BackfillRecovery(
-    Ref<PG> pg,
-    ShardServices &ss,
-    epoch_t epoch_started,
-    const EventT& evt);
-
-  PipelineHandle& get_handle() { return handle; }
-
-  std::tuple<
-    OperationThrottler::BlockingEvent,
-    PGPeeringPipeline::Process::BlockingEvent
-  > tracking_events;
-
-private:
-  boost::intrusive_ptr<const boost::statechart::event_base> evt;
-  PipelineHandle handle;
-
-  static PGPeeringPipeline &peering_pp(PG &pg);
-  interruptible_future<bool> do_recovery() override;
-};
-
-template <class EventT>
-BackfillRecovery::BackfillRecovery(
-  Ref<PG> pg,
-  ShardServices &ss,
-  const epoch_t epoch_started,
-  const EventT& evt)
-  : BackgroundRecoveryT(
-      std::move(pg),
-      ss,
-      epoch_started,
-      crimson::osd::scheduler::scheduler_class_t::background_best_effort),
-    evt(evt.intrusive_from_this())
-{}
-
 }
 
 #if FMT_VERSION >= 90000
-template <> struct fmt::formatter<crimson::osd::BackfillRecovery> : fmt::ostream_formatter {};
 template <> struct fmt::formatter<crimson::osd::PglogBasedRecovery> : fmt::ostream_formatter {};
 template <> struct fmt::formatter<crimson::osd::UrgentRecovery> : fmt::ostream_formatter {};
 template <class T> struct fmt::formatter<crimson::osd::BackgroundRecoveryT<T>> : fmt::ostream_formatter {};

src/crimson/osd/osd_operations/peering_event.h

@@ -21,7 +21,6 @@ namespace crimson::osd {
 class OSD;
 class ShardServices;
 class PG;
-class BackfillRecovery;
 
 template <class T>
 class PeeringEvent : public PhasedOperationT<T> {

src/crimson/osd/pg.h

@@ -64,7 +64,6 @@ namespace crimson::os {
 
 namespace crimson::osd {
 class OpsExecuter;
-class BackfillRecovery;
 class SnapTrimEvent;
 class PglogBasedRecovery;
 
@@ -892,7 +891,6 @@ class PG : public boost::intrusive_ref_counter<
   friend class RepRequest;
   friend class LogMissingRequest;
   friend class LogMissingRequestReply;
-  friend class BackfillRecovery;
   friend struct PGFacade;
   friend class InternalClientRequest;
   friend class WatchTimeoutRequest;

src/crimson/osd/pg_recovery.cc

@@ -477,17 +477,6 @@ void PGRecovery::_committed_pushed_object(epoch_t epoch,
   }
 }
 
-template <class EventT>
-void PGRecovery::start_backfill_recovery(const EventT& evt)
-{
-  using BackfillRecovery = crimson::osd::BackfillRecovery;
-  std::ignore = pg->get_shard_services().start_operation<BackfillRecovery>(
-    static_cast<crimson::osd::PG*>(pg),
-    pg->get_shard_services(),
-    pg->get_osdmap_epoch(),
-    evt);
-}
-
 void PGRecovery::request_replica_scan(
   const pg_shard_t& target,
   const hobject_t& begin,
@@ -520,7 +509,8 @@ void PGRecovery::request_primary_scan(
   ).then_interruptible([this] (BackfillInterval bi) {
     logger().debug("request_primary_scan:{}", __func__);
     using BackfillState = crimson::osd::BackfillState;
-    start_backfill_recovery(BackfillState::PrimaryScanned{ std::move(bi) });
+    backfill_state->process_event(
+      BackfillState::PrimaryScanned{ std::move(bi) }.intrusive_from_this());
   });
 }
 
@@ -542,7 +532,13 @@ void PGRecovery::enqueue_push(
   }).then_interruptible([this, obj] {
     logger().debug("enqueue_push:{}", __func__);
     using BackfillState = crimson::osd::BackfillState;
-    start_backfill_recovery(BackfillState::ObjectPushed(std::move(obj)));
+    if (backfill_state->is_triggered()) {
+      backfill_state->post_event(
+       BackfillState::ObjectPushed(std::move(obj)).intrusive_from_this());
+    } else {
+      backfill_state->process_event(
+       BackfillState::ObjectPushed(std::move(obj)).intrusive_from_this());
+    }
   });
 }
 
@@ -643,8 +639,6 @@ void PGRecovery::backfilled()
 
 void PGRecovery::backfill_suspended()
 {
-  // We are not creating a new BackfillRecovery request here, as we
-  // need to cancel the backfill synchronously (before this method returns).
   using BackfillState = crimson::osd::BackfillState;
   backfill_state->process_event(
     BackfillState::SuspendBackfill{}.intrusive_from_this());
@@ -673,13 +667,7 @@ void PGRecovery::on_activate_complete()
 void PGRecovery::on_backfill_reserved()
 {
   logger().debug("{}", __func__);
-  // yes, it's **not** backfilling yet. The PG_STATE_BACKFILLING
-  // will be set after on_backfill_reserved() returns.
-  // Backfill needs to take this into consideration when scheduling
-  // events -- they must be mutually exclusive with PeeringEvent
-  // instances. Otherwise the execution might begin without having
-  // the state updated.
-  ceph_assert(!pg->get_peering_state().is_backfilling());
+  ceph_assert(pg->get_peering_state().is_backfilling());
   // let's be lazy with creating the backfill stuff
   using BackfillState = crimson::osd::BackfillState;
   if (!backfill_state) {
@@ -694,5 +682,6 @@ void PGRecovery::on_backfill_reserved()
   // it may be we either start a completely new backfill (first
   // event since last on_activate_complete()) or to resume already
   // (but stopped one).
-  start_backfill_recovery(BackfillState::Triggered{});
+  backfill_state->process_event(
+    BackfillState::Triggered{}.intrusive_from_this());
 }

src/crimson/osd/recovery_backend.cc

@@ -335,11 +335,10 @@ RecoveryBackend::handle_scan_digest(
     bi.clear_objects();
     ::decode_noclear(bi.objects, p);
   }
-  shard_services.start_operation<crimson::osd::BackfillRecovery>(
-    static_cast<crimson::osd::PG*>(&pg),
-    shard_services,
-    pg.get_osdmap_epoch(),
-    crimson::osd::BackfillState::ReplicaScanned{ m.from, std::move(bi) });
+  auto recovery_handler = pg.get_recovery_handler();
+  recovery_handler->dispatch_backfill_event(
+    crimson::osd::BackfillState::ReplicaScanned{
+      m.from, std::move(bi) }.intrusive_from_this());
   return seastar::now();
 }
 

src/osd/PeeringState.cc

@@ -5079,10 +5079,10 @@ PeeringState::Backfilling::Backfilling(my_context ctx)
 
   DECLARE_LOCALS;
   ps->backfill_reserved = true;
-  pl->on_backfill_reserved();
   ps->state_clear(PG_STATE_BACKFILL_TOOFULL);
   ps->state_clear(PG_STATE_BACKFILL_WAIT);
   ps->state_set(PG_STATE_BACKFILLING);
+  pl->on_backfill_reserved();
   pl->publish_stats_to_osd();
 }
 
@@ -5128,13 +5128,18 @@ PeeringState::Backfilling::react(const DeferBackfill &c)
   ps->state_clear(PG_STATE_BACKFILLING);
   suspend_backfill();
 
-  pl->schedule_event_after(
-    std::make_shared<PGPeeringEvent>(
-      ps->get_osdmap_epoch(),
-      ps->get_osdmap_epoch(),
-      RequestBackfill()),
-    c.delay);
-  return transit<NotBackfilling>();
+  if (ps->needs_backfill()) {
+    pl->schedule_event_after(
+      std::make_shared<PGPeeringEvent>(
+	ps->get_osdmap_epoch(),
+	ps->get_osdmap_epoch(),
+	RequestBackfill()),
+      c.delay);
+    return transit<NotBackfilling>();
+  } else {
+    // raced with MOSDPGBackfill::OP_BACKFILL_FINISH, ignore
+    return discard_event();
+  }
 }
 
 boost::statechart::result