Merge pull request ceph#59368 from xxhdx1985126/wip-crimson-backfill-cancel-part2

crimson/osd: support backfill cancellation ---- part2

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

Author: Matan-B

src/crimson/osd/backfill_state.cc

@@ -101,6 +101,21 @@ BackfillState::Initial::react(const BackfillState::Triggered& evt)
   }
 }
 
+boost::statechart::result
+BackfillState::Cancelled::react(const BackfillState::Triggered& evt)
+{
+  logger().debug("{}: backfill re-triggered", __func__);
+  ceph_assert(peering_state().is_backfilling());
+  if (Enqueuing::all_enqueued(peering_state(),
+                              backfill_state().backfill_info,
+                              backfill_state().peer_backfill_info)) {
+    logger().debug("{}: switching to Done state", __func__);
+    return transit<BackfillState::Done>();
+  } else {
+    logger().debug("{}: switching to Enqueuing state", __func__);
+    return transit<BackfillState::Enqueuing>();
+  }
+}
 
 // -- Enqueuing
 void BackfillState::Enqueuing::maybe_update_range()
@@ -451,6 +466,15 @@ BackfillState::ReplicasScanning::react(ReplicaScanned evt)
   return discard_event();
 }
 
+boost::statechart::result
+BackfillState::ReplicasScanning::react(CancelBackfill evt)
+{
+  logger().debug("{}: cancelled within ReplicasScanning",
+                 __func__);
+  waiting_on_backfill.clear();
+  return transit<Cancelled>();
+}
+
 boost::statechart::result
 BackfillState::ReplicasScanning::react(ObjectPushed evt)
 {
@@ -499,11 +523,10 @@ BackfillState::Crashed::Crashed()
 }
 
 // -- Cancelled
-BackfillState::Cancelled::Cancelled()
+BackfillState::Cancelled::Cancelled(my_context ctx)
+  : my_base(ctx)
 {
-  backfill_state().backfill_info.clear();
-  backfill_state().peer_backfill_info.clear();
-  backfill_state().progress_tracker.reset();
+  ceph_assert(peering_state().get_backfill_targets().size());
 }
 
 // ProgressTracker is an intermediary between the BackfillListener and

src/crimson/osd/backfill_state.h

@@ -135,9 +135,27 @@ struct BackfillState {
     explicit Crashed();
   };
 
-  struct Cancelled : sc::simple_state<Cancelled, BackfillMachine>,
-		     StateHelper<Cancelled> {
-    explicit Cancelled();
+  struct Cancelled : sc::state<Cancelled, BackfillMachine>,
+                     StateHelper<Cancelled> {
+    using reactions = boost::mpl::list<
+      sc::custom_reaction<Triggered>,
+      sc::custom_reaction<PrimaryScanned>,
+      sc::custom_reaction<ReplicaScanned>,
+      sc::custom_reaction<ObjectPushed>,
+      sc::transition<sc::event_base, Crashed>>;
+    explicit Cancelled(my_context);
+    // resume after triggering backfill by on_activate_complete().
+    // transit to Enqueuing.
+    sc::result react(const Triggered&);
+    sc::result react(const PrimaryScanned&) {
+      return discard_event();
+    }
+    sc::result react(const ReplicaScanned&) {
+      return discard_event();
+    }
+    sc::result react(const ObjectPushed&) {
+      return discard_event();
+    }
   };
 
   struct Initial : sc::state<Initial, BackfillMachine>,
@@ -159,6 +177,8 @@ struct BackfillState {
       sc::transition<RequestPrimaryScanning, PrimaryScanning>,
       sc::transition<RequestReplicasScanning, ReplicasScanning>,
       sc::transition<RequestWaiting, Waiting>,
+      sc::transition<RequestDone, Done>,
+      sc::transition<CancelBackfill, Cancelled>,
       sc::transition<sc::event_base, Crashed>>;
     explicit Enqueuing(my_context);
 
@@ -229,14 +249,15 @@ struct BackfillState {
     using reactions = boost::mpl::list<
       sc::custom_reaction<ObjectPushed>,
       sc::custom_reaction<ReplicaScanned>,
+      sc::custom_reaction<CancelBackfill>,
       sc::transition<RequestDone, Done>,
-      sc::transition<CancelBackfill, Cancelled>,
       sc::transition<sc::event_base, Crashed>>;
     explicit ReplicasScanning(my_context);
     // collect scanning result; if all results are collected, transition
     // to Enqueuing will happen.
     sc::result react(ObjectPushed);
     sc::result react(ReplicaScanned);
+    sc::result react(CancelBackfill);
 
     // indicate whether a particular peer should be scanned to retrieve
     // BackfillInterval for new range of hobject_t namespace.

src/crimson/osd/pg.cc

@@ -425,6 +425,7 @@ void PG::on_activate_complete()
       PeeringState::AllReplicasRecovered{});
   }
   publish_stats_to_osd();
+  recovery_handler->on_activate_complete();
 }
 
 void PG::prepare_write(pg_info_t &info,

src/crimson/osd/pg_recovery.cc

@@ -623,42 +623,51 @@ void PGRecovery::backfill_cancelled()
   using BackfillState = crimson::osd::BackfillState;
   backfill_state->process_event(
     BackfillState::CancelBackfill{}.intrusive_from_this());
-  backfill_state.reset();
 }
 
 void PGRecovery::dispatch_backfill_event(
   boost::intrusive_ptr<const boost::statechart::event_base> evt)
 {
   logger().debug("{}", __func__);
-  if (backfill_state) {
-    backfill_state->process_event(evt);
-  } else {
-    // TODO: Do we need to worry about cases in which the pg has
-    // 	     been through both backfill cancellations and backfill
-    // 	     restarts between the sendings and replies of
-    // 	     ReplicaScan/ObjectPush requests? Seems classic OSDs
-    // 	     doesn't handle these cases.
-    logger().debug("{}, backfill cancelled, dropping evt");
-  }
+  assert(backfill_state);
+  backfill_state->process_event(evt);
+  // TODO: Do we need to worry about cases in which the pg has
+  //       been through both backfill cancellations and backfill
+  //       restarts between the sendings and replies of
+  //       ReplicaScan/ObjectPush requests? Seems classic OSDs
+  //       doesn't handle these cases.
+}
+
+void PGRecovery::on_activate_complete()
+{
+  logger().debug("{} backfill_state={}",
+                 __func__, fmt::ptr(backfill_state.get()));
+  backfill_state.reset();
 }
 
 void PGRecovery::on_backfill_reserved()
 {
   logger().debug("{}", __func__);
-  // PIMP and depedency injection for the sake unittestability.
-  // I'm not afraid about the performance here.
-  using BackfillState = crimson::osd::BackfillState;
-  backfill_state = std::make_unique<BackfillState>(
-    *this,
-    std::make_unique<crimson::osd::PeeringFacade>(pg->get_peering_state()),
-    std::make_unique<crimson::osd::PGFacade>(
-      *static_cast<crimson::osd::PG*>(pg)));
   // 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());
+  // let's be lazy with creating the backfill stuff
+  using BackfillState = crimson::osd::BackfillState;
+  if (!backfill_state) {
+    // PIMP and depedency injection for the sake of unittestability.
+    // I'm not afraid about the performance here.
+    backfill_state = std::make_unique<BackfillState>(
+      *this,
+      std::make_unique<crimson::osd::PeeringFacade>(pg->get_peering_state()),
+      std::make_unique<crimson::osd::PGFacade>(
+        *static_cast<crimson::osd::PG*>(pg)));
+  }
+  // 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{});
 }

src/crimson/osd/pg_recovery.h

@@ -33,6 +33,7 @@ class PGRecovery : public crimson::osd::BackfillState::BackfillListener {
   interruptible_future<bool> start_recovery_ops(
     RecoveryBackend::RecoveryBlockingEvent::TriggerI&,
     size_t max_to_start);
+  void on_activate_complete();
   void on_backfill_reserved();
   void dispatch_backfill_event(
     boost::intrusive_ptr<const boost::statechart::event_base> evt);

src/test/crimson/test_backfill.cc

@@ -154,6 +154,18 @@ class BackfillFixture : public crimson::osd::BackfillState::BackfillListener {
     }
   }
 
+  template <class EventT>
+  void next_round2() {
+    ceph_assert(events_to_dispatch.size());
+    // workaround for Clang's `-Wpotentially-evaluated-expression`. See:
+    //   * https://gitlab.cern.ch/gaudi/Gaudi/-/merge_requests/970
+    //   * https://stackoverflow.com/q/46494928
+    const auto& front_event = *events_to_dispatch.front();
+    ceph_assert(typeid(front_event) == typeid(EventT));
+    backfill_state.process_event(std::move(events_to_dispatch.front()));
+    events_to_dispatch.pop_front();
+  }
+
   void next_till_done() {
     while (!events_to_dispatch.empty()) {
       next_round();
@@ -171,6 +183,15 @@ class BackfillFixture : public crimson::osd::BackfillState::BackfillListener {
 
   struct PeeringFacade;
   struct PGFacade;
+
+  void cancel() {
+    events_to_dispatch.clear();
+    schedule_event(crimson::osd::BackfillState::CancelBackfill{});
+  }
+
+  void resume() {
+    schedule_event(crimson::osd::BackfillState::Triggered{});
+  }
 };
 
 struct BackfillFixture::PeeringFacade
@@ -411,6 +432,68 @@ TEST(backfill, two_empty_replicas)
   EXPECT_TRUE(cluster_fixture.all_stores_look_like(reference_store));
 }
 
+TEST(backfill, cancel_resume)
+{
+  const auto reference_store = FakeStore{ {
+    { "1:00058bcc:::rbd_data.1018ac3e755.00000000000000d5:head", {10, 234} },
+    { "1:00ed7f8e:::rbd_data.1018ac3e755.00000000000000af:head", {10, 196} },
+    { "1:01483aea:::rbd_data.1018ac3e755.0000000000000095:head", {10, 169} },
+  }};
+  auto cluster_fixture = BackfillFixtureBuilder::add_source(
+    reference_store.objs
+  ).add_target(
+    { /* nothing 1 */ }
+  ).add_target(
+    { /* nothing 2 */ }
+  ).get_result();
+
+  EXPECT_CALL(cluster_fixture, backfilled);
+  cluster_fixture.next_round2<crimson::osd::BackfillState::PrimaryScanned>();
+  cluster_fixture.cancel();
+  cluster_fixture.next_round2<crimson::osd::BackfillState::CancelBackfill>();
+  cluster_fixture.resume();
+  cluster_fixture.next_round2<crimson::osd::BackfillState::Triggered>();
+  cluster_fixture.next_round2<crimson::osd::BackfillState::ReplicaScanned>();
+  cluster_fixture.next_round2<crimson::osd::BackfillState::ReplicaScanned>();
+  cluster_fixture.next_round2<crimson::osd::BackfillState::ObjectPushed>();
+  cluster_fixture.next_round2<crimson::osd::BackfillState::ObjectPushed>();
+  cluster_fixture.next_round2<crimson::osd::BackfillState::ObjectPushed>();
+  cluster_fixture.next_till_done();
+
+  EXPECT_TRUE(cluster_fixture.all_stores_look_like(reference_store));
+}
+
+TEST(backfill, cancel_resume_middle_of_scan)
+{
+  const auto reference_store = FakeStore{ {
+    { "1:00058bcc:::rbd_data.1018ac3e755.00000000000000d5:head", {10, 234} },
+    { "1:00ed7f8e:::rbd_data.1018ac3e755.00000000000000af:head", {10, 196} },
+    { "1:01483aea:::rbd_data.1018ac3e755.0000000000000095:head", {10, 169} },
+  }};
+  auto cluster_fixture = BackfillFixtureBuilder::add_source(
+    reference_store.objs
+  ).add_target(
+    { /* nothing 1 */ }
+  ).add_target(
+    { /* nothing 2 */ }
+  ).get_result();
+
+  EXPECT_CALL(cluster_fixture, backfilled);
+  cluster_fixture.next_round2<crimson::osd::BackfillState::PrimaryScanned>();
+  cluster_fixture.next_round2<crimson::osd::BackfillState::ReplicaScanned>();
+  cluster_fixture.cancel();
+  cluster_fixture.next_round2<crimson::osd::BackfillState::CancelBackfill>();
+  cluster_fixture.resume();
+  cluster_fixture.next_round2<crimson::osd::BackfillState::Triggered>();
+  cluster_fixture.next_round2<crimson::osd::BackfillState::ReplicaScanned>();
+  cluster_fixture.next_round2<crimson::osd::BackfillState::ObjectPushed>();
+  cluster_fixture.next_round2<crimson::osd::BackfillState::ObjectPushed>();
+  cluster_fixture.next_round2<crimson::osd::BackfillState::ObjectPushed>();
+  cluster_fixture.next_till_done();
+
+  EXPECT_TRUE(cluster_fixture.all_stores_look_like(reference_store));
+}
+
 namespace StoreRandomizer {
   // FIXME: copied & pasted from test/test_snap_mapper.cc. We need to
   // find a way to avoid code duplication in test. A static library?