crimson: fix mismatch between backfill's enqueue_push() and recovery.

The unittesting discovered an issue between `BackfillState` and
the low-layer recovery code: `RecoveryBackend::recover_object()`.
`BackfillState::enqueue_push()` was assuming it controlls also
to which backfill target the push is sent while `recover_object()`
calculated the set on its own.
Fixing this is the reason why `enqueue_push()` loses the `const
pg_shard_t& target` parameter.

Signed-off-by: Radoslaw Zarzynski <rzarzyns@redhat.com>

diff --git a/src/crimson/osd/backfill_state.cc b/src/crimson/osd/backfill_state.cc
index c5ad1cd8e160655a99579d3eeb7c897460e6176b..f92a6b50cc35f272c6e9c8d112d03ff5e337f8dd 100644 (file)
--- a/src/crimson/osd/backfill_state.cc
+++ b/src/crimson/osd/backfill_state.cc
@@ -258,20 +258,20 @@ BackfillState::Enqueuing::update_on_peers(const hobject_t& check)
     // Find all check peers that have the wrong version
     if (const eversion_t& obj_v = primary_bi.objects.begin()->second;
         check == primary_bi.begin && check == peer_bi.begin) {
-      if(peer_bi.objects.begin()->second != obj_v) {
-        backfill_state().progress_tracker->enqueue_push(primary_bi.begin);
-        backfill_listener().enqueue_push(bt, primary_bi.begin, obj_v);
+      if(peer_bi.objects.begin()->second != obj_v &&
+          backfill_state().progress_tracker->enqueue_push(primary_bi.begin)) {
+        backfill_listener().enqueue_push(primary_bi.begin, obj_v);
       } else {
-        // it's fine, keep it!
+        // it's fine, keep it! OR already recovering
       }
       result.pbi_targets.insert(bt);
     } else {
       // Only include peers that we've caught up to their backfill line
       // otherwise, they only appear to be missing this object
       // because their peer_bi.begin > backfill_info.begin.
-      if (primary_bi.begin > peering_state().get_peer_last_backfill(bt)) {
-        backfill_state().progress_tracker->enqueue_push(primary_bi.begin);
-        backfill_listener().enqueue_push(bt, primary_bi.begin, obj_v);
+      if (primary_bi.begin > peering_state().get_peer_last_backfill(bt) &&
+          backfill_state().progress_tracker->enqueue_push(primary_bi.begin)) {
+        backfill_listener().enqueue_push(primary_bi.begin, obj_v);
       }
     }
   }
@@ -496,16 +496,17 @@ bool BackfillState::ProgressTracker::tracked_objects_completed() const
   return registry.empty();
 }
 
-void BackfillState::ProgressTracker::enqueue_push(const hobject_t& obj)
+bool BackfillState::ProgressTracker::enqueue_push(const hobject_t& obj)
 {
-  ceph_assert(registry.count(obj) == 0);
-  registry[obj] = registry_item_t{ op_stage_t::enqueued_push, std::nullopt };
+  [[maybe_unused]] const auto [it, first_seen] = registry.try_emplace(
+    obj, registry_item_t{op_stage_t::enqueued_push, std::nullopt});
+  return first_seen;
 }
 
 void BackfillState::ProgressTracker::enqueue_drop(const hobject_t& obj)
 {
-  ceph_assert(registry.count(obj) == 0);
-  registry[obj] = registry_item_t{ op_stage_t::enqueued_drop, pg_stat_t{} };
+  registry.try_emplace(
+    obj, registry_item_t{op_stage_t::enqueued_drop, pg_stat_t{}});
 }
 
 void BackfillState::ProgressTracker::complete_to(

diff --git a/src/crimson/osd/backfill_state.h b/src/crimson/osd/backfill_state.h
index 62ce036bdc14f747f1fad9716b5024c1296ef0fb..f2995883e6775c46fa779125576850dce52a5b00 100644 (file)
--- a/src/crimson/osd/backfill_state.h
+++ b/src/crimson/osd/backfill_state.h
@@ -287,7 +287,6 @@ struct BackfillState::BackfillListener {
     const hobject_t& begin) = 0;
 
   virtual void enqueue_push(
-    const pg_shard_t& target,
     const hobject_t& obj,
     const eversion_t& v) = 0;
 
@@ -339,7 +338,7 @@ public:
 
   bool tracked_objects_completed() const;
 
-  void enqueue_push(const hobject_t&);
+  bool enqueue_push(const hobject_t&);
   void enqueue_drop(const hobject_t&);
   void complete_to(const hobject_t&, const pg_stat_t&);
 };

diff --git a/src/crimson/osd/pg_recovery.cc b/src/crimson/osd/pg_recovery.cc
index 14fc62565a42d071f5724e40bc6e8319497eb29b..cdf26274533bae71efbd27017f9fc2e1887f2c95 100644 (file)
--- a/src/crimson/osd/pg_recovery.cc
+++ b/src/crimson/osd/pg_recovery.cc
@@ -430,12 +430,11 @@ void PGRecovery::request_primary_scan(
 }
 
 void PGRecovery::enqueue_push(
-  const pg_shard_t& target,
   const hobject_t& obj,
   const eversion_t& v)
 {
-  logger().debug("{}: target={} obj={} v={}",
-                 __func__, target, obj, v);
+  logger().debug("{}: obj={} v={}",
+                 __func__, obj, v);
   pg->get_recovery_backend()->add_recovering(obj);
   std::ignore = pg->get_recovery_backend()->recover_object(obj, v).\
   handle_exception([] (auto) {

diff --git a/src/crimson/osd/pg_recovery.h b/src/crimson/osd/pg_recovery.h
index e55547c95b59a90c59b3dc7ef163848b10816547..0f65f389a77ce750dbf825d9d283603a2bd4b154 100644 (file)
--- a/src/crimson/osd/pg_recovery.h
+++ b/src/crimson/osd/pg_recovery.h
@@ -93,7 +93,6 @@ private:
   void request_primary_scan(
     const hobject_t& begin) final;
   void enqueue_push(
-    const pg_shard_t& target,
     const hobject_t& obj,
     const eversion_t& v) final;
   void enqueue_drop(

diff --git a/src/test/crimson/test_backfill.cc b/src/test/crimson/test_backfill.cc
index 6caa18fd9988c62a967b381b1a4db3e8f7ccb113..64dc1aea538d6b0fba56b46910fda87f70b0c0b2 100644 (file)
--- a/src/test/crimson/test_backfill.cc
+++ b/src/test/crimson/test_backfill.cc
@@ -124,7 +124,6 @@ class BackfillFixture : public crimson::osd::BackfillState::BackfillListener {
     const hobject_t& begin) override;
 
   void enqueue_push(
-    const pg_shard_t& target,
     const hobject_t& obj,
     const eversion_t& v) override;
 
@@ -281,11 +280,12 @@ void BackfillFixture::request_primary_scan(
 }
 
 void BackfillFixture::enqueue_push(
-  const pg_shard_t& target,
   const hobject_t& obj,
   const eversion_t& v)
 {
-  backfill_targets.at(target).store.push(obj, v);
+  for (auto& [ _, bt ] : backfill_targets) {
+    bt.store.push(obj, v);
+  }
   schedule_event(crimson::osd::BackfillState::ObjectPushed{ obj });
 }