Merge pull request ceph#62837 from athanatos/sjust/wip-crimson-stuck-backfilling

crimson: fix several bugs causing stuck backfills

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

Author: athanatos

src/crimson/osd/osd_operation.h

@@ -300,34 +300,6 @@ class OperationThrottler : public BlockerT<OperationThrottler>,
   friend BlockerT<OperationThrottler>;
   static constexpr const char* type_name = "OperationThrottler";
 
-  template <typename OperationT, typename F>
-  auto with_throttle(
-    OperationT* op,
-    crimson::osd::scheduler::params_t params,
-    F &&f) {
-    if (!max_in_progress) return f();
-    return acquire_throttle(params)
-      .then(std::forward<F>(f))
-      .then([this](auto x) {
-	release_throttle();
-	return x;
-      });
-  }
-
-  template <typename OperationT, typename F>
-  seastar::future<> with_throttle_while(
-    OperationT* op,
-    crimson::osd::scheduler::params_t params,
-    F &&f) {
-    return with_throttle(op, params, f).then([this, params, op, f](bool cont) {
-      return cont
-	? seastar::yield().then([params, op, f, this] {
-	  return with_throttle_while(op, params, f); })
-	: seastar::now();
-    });
-  }
-
-
 public:
   OperationThrottler(ConfigProxy &conf);
 
@@ -340,24 +312,28 @@ class OperationThrottler : public BlockerT<OperationThrottler>,
     return !max_in_progress || in_progress < max_in_progress;
   }
 
-  template <typename F>
-  auto with_throttle(
-    crimson::osd::scheduler::params_t params,
-    F &&f) {
-    if (!max_in_progress) return f();
-    return acquire_throttle(params)
-      .then(std::forward<F>(f))
-      .finally([this] {
-	release_throttle();
-      });
-  }
+  class ThrottleReleaser {
+    OperationThrottler *parent = nullptr;
+  public:
+    ThrottleReleaser(OperationThrottler *parent) : parent(parent) {}
+    ThrottleReleaser(const ThrottleReleaser &) = delete;
+    ThrottleReleaser(ThrottleReleaser &&rhs) noexcept {
+      std::swap(parent, rhs.parent);
+    }
 
-  template <class OpT, class... Args>
-  seastar::future<> with_throttle_while(
-    BlockingEvent::Trigger<OpT>&& trigger,
-    Args&&... args) {
-    return trigger.maybe_record_blocking(
-      with_throttle_while(std::forward<Args>(args)...), *this);
+    ~ThrottleReleaser() {
+      if (parent) {
+	parent->release_throttle();
+      }
+    }
+  };
+
+  auto get_throttle(crimson::osd::scheduler::params_t params) {
+    return acquire_throttle(
+      params
+    ).then([this] {
+      return ThrottleReleaser{this};
+    });
   }
 
 private:

src/crimson/osd/pg.h

@@ -734,6 +734,9 @@ class PG : public boost::intrusive_ref_counter<
   ShardServices& get_shard_services() final {
     return shard_services;
   }
+  DoutPrefixProvider& get_dpp() final {
+    return *this;
+  }
   seastar::future<> stop();
 private:
   class C_PG_FinishRecovery : public Context {

src/crimson/osd/pg_recovery.cc

@@ -5,6 +5,8 @@
 #include <fmt/ostream.h>
 #include <fmt/ranges.h>
 
+#include "crimson/common/coroutine.h"
+#include "crimson/common/log.h"
 #include "crimson/common/type_helpers.h"
 #include "crimson/osd/backfill_facades.h"
 #include "crimson/osd/osd_operations/background_recovery.h"
@@ -16,6 +18,8 @@
 #include "osd/osd_types.h"
 #include "osd/PeeringState.h"
 
+SET_SUBSYS(osd);
+
 namespace {
   seastar::logger& logger() {
     return crimson::get_logger(ceph_subsys_osd);
@@ -522,21 +526,19 @@ void PGRecovery::request_primary_scan(
 
 PGRecovery::interruptible_future<>
 PGRecovery::recover_object_with_throttle(
-  const hobject_t &soid,
+  hobject_t soid,
   eversion_t need)
 {
-  crimson::osd::scheduler::params_t params =
-    {1, 0, crimson::osd::scheduler::scheduler_class_t::background_best_effort};
-  auto &ss = pg->get_shard_services();
-  logger().debug("{} {}", soid, need);
-  return ss.with_throttle(
-    std::move(params),
-    [this, soid, need] {
-    logger().debug("got throttle: {} {}", soid, need);
-    auto backend = pg->get_recovery_backend();
-    assert(backend);
-    return backend->recover_object(soid, need);
-  });
+  LOG_PREFIX(PGRecovery::recover_object_with_throttle);
+  DEBUGDPP("{} {}", pg->get_dpp(), soid, need);
+  auto releaser = co_await interruptor::make_interruptible(
+    pg->get_shard_services().get_throttle(
+      crimson::osd::scheduler::params_t{
+	1, 0, crimson::osd::scheduler::scheduler_class_t::background_best_effort
+      }));
+  DEBUGDPP("got throttle: {} {}", pg->get_dpp(), soid, need);
+  co_await pg->get_recovery_backend()->recover_object(soid, need);
+  co_return;
 }
 
 void PGRecovery::enqueue_push(

src/crimson/osd/pg_recovery.h

@@ -25,6 +25,9 @@ class PGBackend;
 
 class PGRecovery : public crimson::osd::BackfillState::BackfillListener {
 public:
+  using interruptor =
+    ::crimson::interruptible::interruptor<
+      ::crimson::osd::IOInterruptCondition>;
   template <typename T = void>
   using interruptible_future = RecoveryBackend::interruptible_future<T>;
   PGRecovery(PGRecoveryListener* pg) : pg(pg) {}
@@ -100,7 +103,7 @@ class PGRecovery : public crimson::osd::BackfillState::BackfillListener {
   friend class crimson::osd::UrgentRecovery;
 
   interruptible_future<> recover_object_with_throttle(
-    const hobject_t &soid,
+    hobject_t soid,
     eversion_t need);
 
   interruptible_future<> recover_object(

src/crimson/osd/pg_recovery_listener.h

@@ -20,6 +20,7 @@ class PGRecovery;
 class PGRecoveryListener {
 public:
   virtual crimson::osd::ShardServices& get_shard_services() = 0;
+  virtual DoutPrefixProvider& get_dpp() = 0;
   virtual PGRecovery* get_recovery_handler() = 0;
   virtual epoch_t get_osdmap_epoch() const = 0;
   virtual bool is_primary() const = 0;

src/crimson/osd/recovery_backend.cc

@@ -235,12 +235,25 @@ RecoveryBackend::scan_for_backfill(
   co_await interruptor::parallel_for_each(objects, seastar::coroutine::lambda([FNAME, this, version_map]
     (const hobject_t& object) -> interruptible_future<> {
     DEBUGDPP("querying obj:{}", pg, object);
-    auto obc_manager = pg.obc_loader.get_obc_manager(object);
-    co_await pg.obc_loader.load_and_lock(
+    auto obc_manager = pg.obc_loader.get_obc_manager(
+      object, /* resolve_clone = */ false);
+
+    auto found = co_await pg.obc_loader.load_and_lock(
       obc_manager, RWState::RWREAD
-    ).handle_error_interruptible(
+    ).si_then([] {
+      return true;
+    }).handle_error_interruptible(
+      crimson::ct_error::enoent::handle([](auto) {
+	return false;
+      }),
       crimson::ct_error::assert_all("unexpected error")
     );
+    if (!found) {
+      // if the object does not exist here, it must have been removed
+      // between the collection_list_partial and here.  This can happen
+      // for the first item in the range, which is usually last_backfill.
+      co_return;
+    }
 
     if (obc_manager.get_obc()->obs.exists) {
       auto version = obc_manager.get_obc()->obs.oi.version;

src/crimson/osd/shard_services.h

@@ -593,7 +593,7 @@ class ShardServices : public OSDMapService {
   }
 
   FORWARD_TO_OSD_SINGLETON(get_pool_info)
-  FORWARD(with_throttle, with_throttle, local_state.throttler)
+  FORWARD(get_throttle, get_throttle, local_state.throttler)
 
   FORWARD_TO_OSD_SINGLETON(build_incremental_map_msg)
   FORWARD_TO_OSD_SINGLETON(send_incremental_map)