From ea3515ded72e65603c9db72f27c824735d03faa3 Mon Sep 17 00:00:00 2001 From: Radoslaw Zarzynski Date: Thu, 19 Nov 2020 17:12:38 +0100 Subject: [PATCH] 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 --- src/crimson/osd/backfill_state.cc | 25 +++++++++++++------------ src/crimson/osd/backfill_state.h | 3 +-- src/crimson/osd/pg_recovery.cc | 5 ++--- src/crimson/osd/pg_recovery.h | 1 - src/test/crimson/test_backfill.cc | 6 +++--- 5 files changed, 19 insertions(+), 21 deletions(-) diff --git a/src/crimson/osd/backfill_state.cc b/src/crimson/osd/backfill_state.cc index c5ad1cd8e1606..f92a6b50cc35f 100644 --- 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 62ce036bdc14f..f2995883e6775 100644 --- 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 14fc62565a42d..cdf26274533ba 100644 --- 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 e55547c95b59a..0f65f389a77ce 100644 --- 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 6caa18fd9988c..64dc1aea538d6 100644 --- 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 }); } -- 2.39.5