crimson/os/seastore: initialize logical pins before exposing to cache

Otherwise, another task may get a reference to the extent before
we've set the pin.

Fixes: https://tracker.ceph.com/issues/53267
Signed-off-by: Samuel Just <sjust@redhat.com>

diff --git a/src/crimson/os/seastore/cache.cc b/src/crimson/os/seastore/cache.cc
index 22c8af25d739d66a04303a732f9a7d480043ae70..b5ce1cf2371cc352b0fdc9dbc1a4c95b3e792852 100644 (file)
--- a/src/crimson/os/seastore/cache.cc
+++ b/src/crimson/os/seastore/cache.cc
@@ -1161,12 +1161,13 @@ Cache::replay_delta(
     };
     auto extent_fut = (delta.pversion == 0 ?
       // replay is not included by the cache hit metrics
-      get_extent_by_type(
+      _get_extent_by_type(
         delta.type,
         delta.paddr,
         delta.laddr,
         delta.length,
-        nullptr) :
+        nullptr,
+        [](CachedExtent &) {}) :
       _get_extent_if_cached(
        delta.paddr)
     ).handle_error(
@@ -1298,14 +1299,15 @@ Cache::get_root_ret Cache::get_root(Transaction &t)
   }
 }
 
-Cache::get_extent_ertr::future<CachedExtentRef> Cache::get_extent_by_type(
+Cache::get_extent_ertr::future<CachedExtentRef> Cache::_get_extent_by_type(
   extent_types_t type,
   paddr_t offset,
   laddr_t laddr,
   segment_off_t length,
-  const Transaction::src_t* p_src)
+  const Transaction::src_t* p_src,
+  extent_init_func_t &&extent_init_func)
 {
-  return [=] {
+  return [=, extent_init_func=std::move(extent_init_func)]() mutable {
     src_ext_t* p_metric_key = nullptr;
     src_ext_t metric_key;
     if (p_src) {
@@ -1319,43 +1321,43 @@ Cache::get_extent_ertr::future<CachedExtentRef> Cache::get_extent_by_type(
       return get_extent_ertr::make_ready_future<CachedExtentRef>();
     case extent_types_t::LADDR_INTERNAL:
       return get_extent<lba_manager::btree::LBAInternalNode>(
-          offset, length, p_metric_key
+       offset, length, p_metric_key, std::move(extent_init_func)
       ).safe_then([](auto extent) {
        return CachedExtentRef(extent.detach(), false /* add_ref */);
       });
     case extent_types_t::LADDR_LEAF:
       return get_extent<lba_manager::btree::LBALeafNode>(
-          offset, length, p_metric_key
+       offset, length, p_metric_key, std::move(extent_init_func)
       ).safe_then([](auto extent) {
        return CachedExtentRef(extent.detach(), false /* add_ref */);
       });
     case extent_types_t::OMAP_INNER:
       return get_extent<omap_manager::OMapInnerNode>(
-          offset, length, p_metric_key
+          offset, length, p_metric_key, std::move(extent_init_func)
       ).safe_then([](auto extent) {
         return CachedExtentRef(extent.detach(), false /* add_ref */);
       });
     case extent_types_t::OMAP_LEAF:
       return get_extent<omap_manager::OMapLeafNode>(
-          offset, length, p_metric_key
+          offset, length, p_metric_key, std::move(extent_init_func)
       ).safe_then([](auto extent) {
         return CachedExtentRef(extent.detach(), false /* add_ref */);
       });
     case extent_types_t::COLL_BLOCK:
       return get_extent<collection_manager::CollectionNode>(
-          offset, length, p_metric_key
+          offset, length, p_metric_key, std::move(extent_init_func)
       ).safe_then([](auto extent) {
         return CachedExtentRef(extent.detach(), false /* add_ref */);
       });
     case extent_types_t::ONODE_BLOCK_STAGED:
       return get_extent<onode::SeastoreNodeExtent>(
-          offset, length, p_metric_key
+          offset, length, p_metric_key, std::move(extent_init_func)
       ).safe_then([](auto extent) {
        return CachedExtentRef(extent.detach(), false /* add_ref */);
       });
     case extent_types_t::OBJECT_DATA_BLOCK:
       return get_extent<ObjectDataBlock>(
-          offset, length, p_metric_key
+          offset, length, p_metric_key, std::move(extent_init_func)
       ).safe_then([](auto extent) {
        return CachedExtentRef(extent.detach(), false /* add_ref */);
       });
@@ -1364,13 +1366,13 @@ Cache::get_extent_ertr::future<CachedExtentRef> Cache::get_extent_by_type(
       return get_extent_ertr::make_ready_future<CachedExtentRef>();
     case extent_types_t::TEST_BLOCK:
       return get_extent<TestBlock>(
-          offset, length, p_metric_key
+          offset, length, p_metric_key, std::move(extent_init_func)
       ).safe_then([](auto extent) {
        return CachedExtentRef(extent.detach(), false /* add_ref */);
       });
     case extent_types_t::TEST_BLOCK_PHYSICAL:
       return get_extent<TestBlockPhysical>(
-          offset, length, p_metric_key
+          offset, length, p_metric_key, std::move(extent_init_func)
       ).safe_then([](auto extent) {
        return CachedExtentRef(extent.detach(), false /* add_ref */);
       });

diff --git a/src/crimson/os/seastore/cache.h b/src/crimson/os/seastore/cache.h
index ffd7cacc50d008d7aca603d77543eed4cca4c15f..6ebfef1c95df40b9d70693dab4dd24df5fde3b5e 100644 (file)
--- a/src/crimson/os/seastore/cache.h
+++ b/src/crimson/os/seastore/cache.h
@@ -195,11 +195,12 @@ public:
   using get_extent_ertr = base_ertr;
   template <typename T>
   using get_extent_ret = get_extent_ertr::future<TCachedExtentRef<T>>;
-  template <typename T>
+  template <typename T, typename Func>
   get_extent_ret<T> get_extent(
     paddr_t offset,                ///< [in] starting addr
     segment_off_t length,          ///< [in] length
-    const src_ext_t* p_metric_key  ///< [in] cache query metric key
+    const src_ext_t* p_metric_key, ///< [in] cache query metric key
+    Func &&extent_init_func        ///< [in] init func for extent
   ) {
     auto cached = query_cache(offset, p_metric_key);
     if (!cached) {
@@ -208,7 +209,8 @@ public:
       ret->set_paddr(offset);
       ret->state = CachedExtent::extent_state_t::CLEAN;
       add_extent(ret);
-      return read_extent<T>(std::move(ret));
+      return read_extent<T>(
+       std::move(ret), std::forward<Func>(extent_init_func));
     }
 
     // extent PRESENT in cache
@@ -226,11 +228,14 @@ public:
       }
 
       cached->state = CachedExtent::extent_state_t::INVALID;
-      return read_extent<T>(std::move(ret));
+      return read_extent<T>(
+       std::move(ret), std::forward<Func>(extent_init_func));
     } else {
       auto ret = TCachedExtentRef<T>(static_cast<T*>(cached.get()));
       return ret->wait_io(
-      ).then([ret=std::move(ret)]() mutable -> get_extent_ret<T> {
+      ).then([ret=std::move(ret),
+             extent_init_func=std::forward<Func>(extent_init_func)]() mutable
+            -> get_extent_ret<T> {
         // ret may be invalid, caller must check
         return get_extent_ret<T>(
           get_extent_ertr::ready_future_marker{},
@@ -238,6 +243,16 @@ public:
       });
     }
   }
+  template <typename T>
+  get_extent_ret<T> get_extent(
+    paddr_t offset,                ///< [in] starting addr
+    segment_off_t length,          ///< [in] length
+    const src_ext_t* p_metric_key  ///< [in] cache query metric key
+  ) {
+    return get_extent<T>(
+      offset, length, p_metric_key,
+      [](T &){});
+  }
 
   /**
    * get_extent_if_cached
@@ -298,11 +313,12 @@ public:
    * t *must not* have retired offset
    */
   using get_extent_iertr = base_iertr;
-  template <typename T>
+  template <typename T, typename Func>
   get_extent_iertr::future<TCachedExtentRef<T>> get_extent(
     Transaction &t,
     paddr_t offset,
-    segment_off_t length) {
+    segment_off_t length,
+    Func &&extent_init_func) {
     CachedExtentRef ret;
     LOG_PREFIX(Cache::get_extent);
     auto result = t.get_extent(offset, &ret);
@@ -316,7 +332,9 @@ public:
     } else {
       auto metric_key = std::make_pair(t.get_src(), T::TYPE);
       return trans_intr::make_interruptible(
-       get_extent<T>(offset, length, &metric_key)
+       get_extent<T>(
+         offset, length, &metric_key,
+         std::forward<Func>(extent_init_func))
       ).si_then([this, FNAME, offset, &t](auto ref) {
        (void)this; // silence incorrect clang warning about capture
        if (!ref->is_valid()) {
@@ -334,6 +352,13 @@ public:
       });
     }
   }
+  template <typename T>
+  get_extent_iertr::future<TCachedExtentRef<T>> get_extent(
+    Transaction &t,
+    paddr_t offset,
+    segment_off_t length) {
+    return get_extent<T>(t, offset, length, [](T &){});
+  }
 
 
   /**
@@ -342,23 +367,52 @@ public:
    * Based on type, instantiate the correct concrete type
    * and read in the extent at location offset~length.
    */
-  get_extent_ertr::future<CachedExtentRef> get_extent_by_type(
-    extent_types_t type,             ///< [in] type tag
-    paddr_t offset,                  ///< [in] starting addr
-    laddr_t laddr,                   ///< [in] logical address if logical
-    segment_off_t length,            ///< [in] length
-    const Transaction::src_t* p_src  ///< [in] src tag for cache query metric
+private:
+  // This is a workaround std::move_only_function not being available,
+  // not really worth generalizing at this time.
+  class extent_init_func_t {
+    struct callable_i {
+      virtual void operator()(CachedExtent &extent) = 0;
+      virtual ~callable_i() = default;
+    };
+    template <typename Func>
+    struct callable_wrapper final : callable_i {
+      Func func;
+      callable_wrapper(Func &&func) : func(std::forward<Func>(func)) {}
+      void operator()(CachedExtent &extent) final {
+       return func(extent);
+      }
+      ~callable_wrapper() final = default;
+    };
+  public:
+    std::unique_ptr<callable_i> wrapped;
+    template <typename Func>
+    extent_init_func_t(Func &&func) : wrapped(
+      std::make_unique<callable_wrapper<Func>>(std::forward<Func>(func)))
+    {}
+    void operator()(CachedExtent &extent) {
+      return (*wrapped)(extent);
+    }
+  };
+  get_extent_ertr::future<CachedExtentRef> _get_extent_by_type(
+    extent_types_t type,
+    paddr_t offset,
+    laddr_t laddr,
+    segment_off_t length,
+    const Transaction::src_t* p_src,
+    extent_init_func_t &&extent_init_func
   );
 
   using get_extent_by_type_iertr = get_extent_iertr;
   using get_extent_by_type_ret = get_extent_by_type_iertr::future<
     CachedExtentRef>;
-  get_extent_by_type_ret get_extent_by_type(
+  get_extent_by_type_ret _get_extent_by_type(
     Transaction &t,
     extent_types_t type,
     paddr_t offset,
     laddr_t laddr,
-    segment_off_t length) {
+    segment_off_t length,
+    extent_init_func_t &&extent_init_func) {
     CachedExtentRef ret;
     auto status = t.get_extent(offset, &ret);
     if (status == Transaction::get_extent_ret::RETIRED) {
@@ -368,7 +422,9 @@ public:
     } else {
       auto src = t.get_src();
       return trans_intr::make_interruptible(
-       get_extent_by_type(type, offset, laddr, length, &src)
+       _get_extent_by_type(
+         type, offset, laddr, length, &src,
+         std::move(extent_init_func))
       ).si_then([=, &t](CachedExtentRef ret) {
         if (!ret->is_valid()) {
           LOG_PREFIX(Cache::get_extent_by_type);
@@ -384,6 +440,36 @@ public:
     }
   }
 
+public:
+  template <typename Func>
+  get_extent_by_type_ret get_extent_by_type(
+    Transaction &t,         ///< [in] transaction
+    extent_types_t type,    ///< [in] type tag
+    paddr_t offset,         ///< [in] starting addr
+    laddr_t laddr,          ///< [in] logical address if logical
+    segment_off_t length,   ///< [in] length
+    Func &&extent_init_func ///< [in] extent init func
+  ) {
+    return _get_extent_by_type(
+      t,
+      type,
+      offset,
+      laddr,
+      length,
+      extent_init_func_t(std::forward<Func>(extent_init_func)));
+  }
+  get_extent_by_type_ret get_extent_by_type(
+    Transaction &t,
+    extent_types_t type,
+    paddr_t offset,
+    laddr_t laddr,
+    segment_off_t length
+  ) {
+    return get_extent_by_type(
+      t, type, offset, laddr, length, [](CachedExtent &) {});
+  }
+
+
   /**
    * alloc_new_extent
    *
@@ -786,9 +872,10 @@ private:
   /// Introspect transaction when it is being destructed
   void on_transaction_destruct(Transaction& t);
 
-  template <typename T>
+  template <typename T, typename Func>
   get_extent_ret<T> read_extent(
-    TCachedExtentRef<T>&& extent
+    TCachedExtentRef<T>&& extent,
+    Func &&func
   ) {
     extent->set_io_wait();
     return reader.read(
@@ -796,11 +883,12 @@ private:
       extent->get_length(),
       extent->get_bptr()
     ).safe_then(
-      [extent=std::move(extent)]() mutable {
+      [extent=std::move(extent), func=std::forward<Func>(func)]() mutable {
         /* TODO: crc should be checked against LBA manager */
         extent->last_committed_crc = extent->get_crc32c();
 
         extent->on_clean_read();
+       func(*extent);
         extent->complete_io();
         return get_extent_ertr::make_ready_future<TCachedExtentRef<T>>(
           std::move(extent));

diff --git a/src/crimson/os/seastore/transaction_manager.cc b/src/crimson/os/seastore/transaction_manager.cc
index 643becf009b1d1c44c0d79fd9ecdd13d2c360e39..08527ff8ec3bb6c4182bb481232b0542f4bbe73e 100644 (file)
--- a/src/crimson/os/seastore/transaction_manager.cc
+++ b/src/crimson/os/seastore/transaction_manager.cc
@@ -437,19 +437,16 @@ TransactionManager::get_extent_if_live_ret TransactionManager::get_extent_if_liv
              type,
              addr,
              laddr,
-             len).si_then(
-               [this, pin=std::move(pin)](CachedExtentRef ret) mutable {
-                 auto lref = ret->cast<LogicalCachedExtent>();
-                 if (!lref->has_pin()) {
-                   assert(!(pin->has_been_invalidated() ||
-                            lref->has_been_invalidated()));
-                   lref->set_pin(std::move(pin));
-                   lba_manager->add_pin(lref->get_pin());
-                 }
-                 return inner_ret(
-                   interruptible::ready_future_marker{},
-                   ret);
-               });
+             len,
+             [this, pin=std::move(pin)](CachedExtent &extent) mutable {
+               auto lref = extent.cast<LogicalCachedExtent>();
+               if (!lref->has_pin()) {
+                 assert(!(pin->has_been_invalidated() ||
+                          lref->has_been_invalidated()));
+                 lref->set_pin(std::move(pin));
+                 lba_manager->add_pin(lref->get_pin());
+               }
+             });
          } else {
            return inner_ret(
              interruptible::ready_future_marker{},

diff --git a/src/crimson/os/seastore/transaction_manager.h b/src/crimson/os/seastore/transaction_manager.h
index 50c5c1c12065b8ead0b28455a1ccd0bc426f3db4..eede86737a62934ddfc0feeea34adbadfd5f0073 100644 (file)
--- a/src/crimson/os/seastore/transaction_manager.h
+++ b/src/crimson/os/seastore/transaction_manager.h
@@ -148,16 +148,20 @@ public:
     LOG_PREFIX(TransactionManager::pin_to_extent);
     using ret = pin_to_extent_ret<T>;
     DEBUGT("getting extent {}", t, *pin);
+    auto &pref = *pin;
     return cache->get_extent<T>(
       t,
-      pin->get_paddr(),
-      pin->get_length()
-    ).si_then([this, FNAME, &t, pin=std::move(pin)](auto ref) mutable -> ret {
-      if (!ref->has_pin()) {
-       assert(!(pin->has_been_invalidated() || ref->has_been_invalidated()));
-       ref->set_pin(std::move(pin));
-       lba_manager->add_pin(ref->get_pin());
+      pref.get_paddr(),
+      pref.get_length(),
+      [this, pin=std::move(pin)](T &extent) mutable {
+       if (!extent.has_pin()) {
+         assert(!(extent.has_been_invalidated() ||
+                  pin->has_been_invalidated()));
+         extent.set_pin(std::move(pin));
+         lba_manager->add_pin(extent.get_pin());
+       }
       }
+    ).si_then([FNAME, &t](auto ref) mutable -> ret {
       DEBUGT("got extent {}", t, *ref);
       return pin_to_extent_ret<T>(
        interruptible::ready_future_marker{},