From: Ronen Friedman <rfriedma@redhat.com>
Date: Sun, 12 Nov 2023 11:12:31 +0000 (-0600)
Subject: osd/scrub: fix replica sub-states
X-Git-Tag: v19.0.0~6^2~8
X-Git-Url: http://git-server-git.apps.pok.os.sepia.ceph.com/?a=commitdiff_plain;h=77a4e0b2e1c3aea71ebf5001c877e9f2e885ce22;p=ceph-ci.git

osd/scrub: fix replica sub-states

The replica states hierarchy was not properly specified.
Fixing it here.
Also - removing the (never yet invoked) call to
replica_handling_done() in the replica state machine.
That call must precede the reply to the Primary.

Fixes: https://tracker.ceph.com/issues/63509

Signed-off-by: Ronen Friedman <rfriedma@redhat.com>
---

diff --git a/src/osd/scrubber/scrub_machine.cc b/src/osd/scrubber/scrub_machine.cc
index 40b43b6e077..99286acaa12 100644
--- a/src/osd/scrubber/scrub_machine.cc
+++ b/src/osd/scrubber/scrub_machine.cc
@@ -674,38 +674,42 @@ ReservedReplica::~ReservedReplica()
 
 ReplicaIdle::ReplicaIdle(my_context ctx)
     : my_base(ctx)
-    , NamedSimply(context<ScrubMachine>().m_scrbr, "ReplicaIdle")
-{
-  dout(10) << "-- state -->> ReplicaIdle" << dendl;
-}
-
-ReplicaIdle::~ReplicaIdle()
+    , NamedSimply(
+	  context<ScrubMachine>().m_scrbr,
+	  "ReservedReplica/ReplicaIdle")
 {
+  dout(10) << "-- state -->> ReservedReplica/ReplicaIdle" << dendl;
 }
 
+ReplicaIdle::~ReplicaIdle() = default;
 
 // ----------------------- ReplicaActiveOp --------------------------------
 
 ReplicaActiveOp::ReplicaActiveOp(my_context ctx)
     : my_base(ctx)
-    , NamedSimply(context<ScrubMachine>().m_scrbr, "ReplicaActiveOp")
+    , NamedSimply(
+	  context<ScrubMachine>().m_scrbr,
+	  "ReservedReplica/ReplicaActiveOp")
 {
-  dout(10) << "-- state -->> ReplicaActiveOp" << dendl;
+  dout(10) << "-- state -->> ReservedReplica/ReplicaActiveOp" << dendl;
 }
 
-ReplicaActiveOp::~ReplicaActiveOp()
-{
-  DECLARE_LOCALS;  // 'scrbr' & 'pg_id' aliases
-  scrbr->replica_handling_done();
-}
+/**
+ * \note: here is too late to call replica_handling_done(). See the
+ * comment in build_replica_map_chunk()
+ */
+ReplicaActiveOp::~ReplicaActiveOp() = default;
 
 // ----------------------- ReplicaWaitUpdates --------------------------------
 
 ReplicaWaitUpdates::ReplicaWaitUpdates(my_context ctx)
     : my_base(ctx)
-    , NamedSimply(context<ScrubMachine>().m_scrbr, "ReplicaWaitUpdates")
+    , NamedSimply(
+	  context<ScrubMachine>().m_scrbr,
+	  "ReservedReplica/ReplicaActiveOp/ReplicaWaitUpdates")
 {
-  dout(10) << "-- state -->> ReplicaWaitUpdates" << dendl;
+  dout(10) << "-- state -->> ReservedReplica/ReplicaActiveOp/ReplicaWaitUpdates"
+	   << dendl;
   DECLARE_LOCALS;  // 'scrbr' & 'pg_id' aliases
   scrbr->on_replica_init();
 }
@@ -732,10 +736,13 @@ sc::result ReplicaWaitUpdates::react(const ReplicaPushesUpd&)
 
 ReplicaBuildingMap::ReplicaBuildingMap(my_context ctx)
     : my_base(ctx)
-    , NamedSimply(context<ScrubMachine>().m_scrbr, "ReplicaBuildingMap")
+    , NamedSimply(
+	  context<ScrubMachine>().m_scrbr,
+	  "ReservedReplica/ReplicaActiveOp/ReplicaBuildingMap")
 {
   DECLARE_LOCALS;  // 'scrbr' & 'pg_id' aliases
-  dout(10) << "-- state -->> ReplicaBuildingMap" << dendl;
+  dout(10) << "-- state -->> ReservedReplica/ReplicaActiveOp/ReplicaBuildingMap"
+	   << dendl;
   // and as we might have skipped ReplicaWaitUpdates:
   scrbr->on_replica_init();
   post_event(SchedReplica{});
diff --git a/src/osd/scrubber/scrub_machine.h b/src/osd/scrubber/scrub_machine.h
index cbce07fe183..a67a85b8e11 100644
--- a/src/osd/scrubber/scrub_machine.h
+++ b/src/osd/scrubber/scrub_machine.h
@@ -628,7 +628,7 @@ struct ReplicaIdle : sc::state<ReplicaIdle, ReservedReplica>,
 };
 
 /**
- * ReservedActiveOp
+ * ReplicaActiveOp
  *
  * Lifetime matches handling for a single map request op
  */
@@ -646,7 +646,7 @@ struct ReplicaActiveOp
  * - the details of the Primary's request were internalized by PgScrubber;
  * - 'active' scrubbing is set
  */
-struct ReplicaWaitUpdates : sc::state<ReplicaWaitUpdates, ReservedReplica>,
+struct ReplicaWaitUpdates : sc::state<ReplicaWaitUpdates, ReplicaActiveOp>,
 			    NamedSimply {
   explicit ReplicaWaitUpdates(my_context ctx);
   using reactions = mpl::list<sc::custom_reaction<ReplicaPushesUpd>>;
@@ -655,7 +655,7 @@ struct ReplicaWaitUpdates : sc::state<ReplicaWaitUpdates, ReservedReplica>,
 };
 
 
-struct ReplicaBuildingMap : sc::state<ReplicaBuildingMap, ReservedReplica>
+struct ReplicaBuildingMap : sc::state<ReplicaBuildingMap, ReplicaActiveOp>
 			  , NamedSimply {
   explicit ReplicaBuildingMap(my_context ctx);
   using reactions = mpl::list<sc::custom_reaction<SchedReplica>>;