Merge pull request ceph#57966 from xxhdx1985126/wip-crimson-concurrent-recover-missing

crimson/osd/osd_operations: make the "recover_missing" phase concurrent

Reviewed-by: Yingxin Cheng <[email protected]>
Reviewed-by: Matan Breizman <[email protected]>

Author: Matan-B

src/crimson/osd/osd_operation_external_tracking.h

@@ -33,10 +33,9 @@ struct LttngBackend
     PGActivationBlocker::BlockingEvent::Backend,
     scrub::PGScrubber::BlockingEvent::Backend,
     ClientRequest::PGPipeline::RecoverMissing::BlockingEvent::Backend,
-    ClientRequest::PGPipeline::RecoverMissingLockOBC::BlockingEvent::Backend,
-    ClientRequest::PGPipeline::RecoverMissingLockOBC::
+    ClientRequest::PGPipeline::RecoverMissing::
       BlockingEvent::ExitBarrierEvent::Backend,
-    ClientRequest::PGPipeline::RecoverMissingSnaps::BlockingEvent::Backend,
+    ClientRequest::PGPipeline::CheckAlreadyCompleteGetObc::BlockingEvent::Backend,
     ClientRequest::PGPipeline::GetOBC::BlockingEvent::Backend,
     ClientRequest::PGPipeline::LockOBC::BlockingEvent::Backend,
     ClientRequest::PGPipeline::LockOBC::BlockingEvent::ExitBarrierEvent::Backend,
@@ -109,19 +108,13 @@ struct LttngBackend
               const ClientRequest::PGPipeline::RecoverMissing& blocker) override {
   }
 
-  void handle(ClientRequest::PGPipeline::RecoverMissingLockOBC::BlockingEvent& ev,
-              const Operation& op,
-              const ClientRequest::PGPipeline::RecoverMissingLockOBC& blocker) override {
-  }
-
-  void handle(ClientRequest::PGPipeline::RecoverMissingLockOBC::
-                BlockingEvent::ExitBarrierEvent& ev,
-              const Operation& op) override {
+  void handle(ClientRequest::PGPipeline::RecoverMissing::BlockingEvent::ExitBarrierEvent& ev,
+	      const Operation& op) override {
   }
 
-  void handle(ClientRequest::PGPipeline::RecoverMissingSnaps::BlockingEvent& ev,
+  void handle(ClientRequest::PGPipeline::CheckAlreadyCompleteGetObc::BlockingEvent& ev,
               const Operation& op,
-              const ClientRequest::PGPipeline::RecoverMissingSnaps& blocker) override {
+              const ClientRequest::PGPipeline::CheckAlreadyCompleteGetObc& blocker) override {
   }
 
   void handle(ClientRequest::PGPipeline::GetOBC::BlockingEvent& ev,
@@ -175,10 +168,9 @@ struct HistoricBackend
     PGActivationBlocker::BlockingEvent::Backend,
     scrub::PGScrubber::BlockingEvent::Backend,
     ClientRequest::PGPipeline::RecoverMissing::BlockingEvent::Backend,
-    ClientRequest::PGPipeline::RecoverMissingLockOBC::BlockingEvent::Backend,
-    ClientRequest::PGPipeline::RecoverMissingLockOBC::
+    ClientRequest::PGPipeline::RecoverMissing::
       BlockingEvent::ExitBarrierEvent::Backend,
-    ClientRequest::PGPipeline::RecoverMissingSnaps::BlockingEvent::Backend,
+    ClientRequest::PGPipeline::CheckAlreadyCompleteGetObc::BlockingEvent::Backend,
     ClientRequest::PGPipeline::GetOBC::BlockingEvent::Backend,
     ClientRequest::PGPipeline::LockOBC::BlockingEvent::Backend,
     ClientRequest::PGPipeline::LockOBC::BlockingEvent::ExitBarrierEvent::Backend,
@@ -251,19 +243,13 @@ struct HistoricBackend
               const ClientRequest::PGPipeline::RecoverMissing& blocker) override {
   }
 
-  void handle(ClientRequest::PGPipeline::RecoverMissingLockOBC::BlockingEvent& ev,
-              const Operation& op,
-              const ClientRequest::PGPipeline::RecoverMissingLockOBC& blocker) override {
-  }
-
-  void handle(ClientRequest::PGPipeline::RecoverMissingLockOBC::
-                BlockingEvent::ExitBarrierEvent& ev,
+  void handle(ClientRequest::PGPipeline::RecoverMissing::BlockingEvent::ExitBarrierEvent& ev,
               const Operation& op) override {
   }
 
-  void handle(ClientRequest::PGPipeline::RecoverMissingSnaps::BlockingEvent& ev,
+  void handle(ClientRequest::PGPipeline::CheckAlreadyCompleteGetObc::BlockingEvent& ev,
               const Operation& op,
-              const ClientRequest::PGPipeline::RecoverMissingSnaps& blocker) override {
+              const ClientRequest::PGPipeline::CheckAlreadyCompleteGetObc& blocker) override {
   }
 
   void handle(ClientRequest::PGPipeline::GetOBC::BlockingEvent& ev,

src/crimson/osd/osd_operations/client_request.cc

@@ -294,9 +294,7 @@ ClientRequest::recover_missing_snaps(
   ObjectContextRef head,
   std::set<snapid_t> &snaps)
 {
-  LOG_PREFIX(ClientRequest::process_op);
-  co_await ihref.enter_stage<interruptor>(
-    client_pp(*pg).recover_missing_snaps, *this);
+  LOG_PREFIX(ClientRequest::recover_missing_snaps);
   for (auto &snap : snaps) {
     auto coid = head->obs.oi.soid;
     coid.snap = snap;
@@ -323,9 +321,7 @@ ClientRequest::process_op(
   instance_handle_t &ihref, Ref<PG> pg, unsigned this_instance_id)
 {
   LOG_PREFIX(ClientRequest::process_op);
-  co_await ihref.enter_stage<interruptor>(
-    client_pp(*pg).recover_missing, *this
-  );
+  ihref.enter_stage_sync(client_pp(*pg).recover_missing, *this);
   if (!pg->is_primary()) {
     DEBUGDPP(
       "Skipping recover_missings on non primary pg for soid {}",
@@ -341,9 +337,6 @@ ClientRequest::process_op(
 
     std::set<snapid_t> snaps = snaps_need_to_recover();
     if (!snaps.empty()) {
-      // call with_obc() in order, but wait concurrently for loading.
-      ihref.enter_stage_sync(
-          client_pp(*pg).recover_missing_lock_obc, *this);
       auto with_obc = pg->obc_loader.with_obc<RWState::RWREAD>(
         m->get_hobj().get_head(),
         [&snaps, &ihref, pg, this](auto head, auto) {
@@ -356,6 +349,16 @@ ClientRequest::process_op(
     }
   }
 
+  /**
+   * The previous stage of recover_missing is a concurrent phase.
+   * Checking for already_complete requests must done exclusively.
+   * Since get_obc is also an exclusive stage, we can merge both stages into
+   * a single stage and avoid stage switching overhead.
+   */
+  DEBUGDPP("{}.{}: entering check_already_complete_get_obc",
+	   *pg, *this, this_instance_id);
+  co_await ihref.enter_stage<interruptor>(
+    client_pp(*pg).check_already_complete_get_obc, *this);
   DEBUGDPP("{}.{}: checking already_complete",
 	   *pg, *this, this_instance_id);
   auto completed = co_await pg->already_complete(m->get_reqid());
@@ -374,11 +377,7 @@ ClientRequest::process_op(
     co_return;
   }
 
-  DEBUGDPP("{}.{}: not completed, entering get_obc stage",
-	   *pg, *this, this_instance_id);
-  co_await ihref.enter_stage<interruptor>(client_pp(*pg).get_obc, *this);
-
-  DEBUGDPP("{}.{}: entered get_obc stage, about to wait_scrub",
+  DEBUGDPP("{}.{}: not completed, about to wait_scrub",
 	   *pg, *this, this_instance_id);
   co_await ihref.enter_blocker(
     *this, pg->scrubber, &decltype(pg->scrubber)::wait_scrub,

src/crimson/osd/osd_operations/client_request.h

@@ -44,12 +44,6 @@ class ClientRequest final : public PhasedOperationT<ClientRequest>,
 public:
   class PGPipeline : public CommonPGPipeline {
     public:
-    struct RecoverMissingLockOBC : OrderedConcurrentPhaseT<RecoverMissingLockOBC> {
-      static constexpr auto type_name = "ClientRequest::PGPipeline::recover_missing_lock_obc";
-    } recover_missing_lock_obc;
-    struct RecoverMissingSnaps : OrderedExclusivePhaseT<RecoverMissingSnaps> {
-      static constexpr auto type_name = "ClientRequest::PGPipeline::recover_missing_snaps";
-    } recover_missing_snaps;
     struct AwaitMap : OrderedExclusivePhaseT<AwaitMap> {
       static constexpr auto type_name = "ClientRequest::PGPipeline::await_map";
     } await_map;
@@ -108,9 +102,8 @@ class ClientRequest final : public PhasedOperationT<ClientRequest>,
       PGPipeline::WaitForActive::BlockingEvent,
       PGActivationBlocker::BlockingEvent,
       PGPipeline::RecoverMissing::BlockingEvent,
-      PGPipeline::RecoverMissingLockOBC::BlockingEvent,
-      PGPipeline::RecoverMissingSnaps::BlockingEvent,
       scrub::PGScrubber::BlockingEvent,
+      PGPipeline::CheckAlreadyCompleteGetObc::BlockingEvent,
       PGPipeline::GetOBC::BlockingEvent,
       PGPipeline::LockOBC::BlockingEvent,
       PGPipeline::Process::BlockingEvent,

src/crimson/osd/osd_operations/common/pg_pipeline.h

@@ -17,9 +17,12 @@ class CommonPGPipeline {
   struct WaitForActive : OrderedExclusivePhaseT<WaitForActive> {
     static constexpr auto type_name = "CommonPGPipeline:::wait_for_active";
   } wait_for_active;
-  struct RecoverMissing : OrderedExclusivePhaseT<RecoverMissing> {
+  struct RecoverMissing : OrderedConcurrentPhaseT<RecoverMissing> {
     static constexpr auto type_name = "CommonPGPipeline::recover_missing";
   } recover_missing;
+  struct CheckAlreadyCompleteGetObc : OrderedExclusivePhaseT<CheckAlreadyCompleteGetObc> {
+    static constexpr auto type_name = "CommonPGPipeline::check_already_complete_get_obc";
+  } check_already_complete_get_obc;
   struct GetOBC : OrderedExclusivePhaseT<GetOBC> {
     static constexpr auto type_name = "CommonPGPipeline::get_obc";
   } get_obc;