crimson/os/seastore/cache: fix a potential leak when replace placeholder

Signed-off-by: Yingxin Cheng <yingxin.cheng@intel.com>

diff --git a/src/crimson/os/seastore/cache.cc b/src/crimson/os/seastore/cache.cc
index 9183e5cd0bb8b22e090c8332271b1a91bb552653..6803c145cb709fcac8fc4b10799fed980c2f5ee9 100644 (file)
--- a/src/crimson/os/seastore/cache.cc
+++ b/src/crimson/os/seastore/cache.cc
@@ -77,12 +77,8 @@ Cache::retire_extent_ret Cache::retire_extent_addr(
     DEBUGT("retire {}~{} in cache -- {}", t, addr, length, *ext);
     if (ext->get_type() != extent_types_t::RETIRED_PLACEHOLDER) {
       t.add_to_read_set(ext);
-      return trans_intr::make_interruptible(
-        ext->wait_io()
-      ).then_interruptible([&t, ext=std::move(ext)]() mutable {
-        t.add_to_retired_set(ext);
-        return retire_extent_iertr::now();
-      });
+      t.add_to_retired_set(ext);
+      return retire_extent_iertr::now();
     }
     // the retired-placeholder exists
   } else {

diff --git a/src/crimson/os/seastore/cache.h b/src/crimson/os/seastore/cache.h
index f78934defa3c81e65b9181ae1583f60d367197c4..40187ca41dc3016bacc344db098f05863241376b 100644 (file)
--- a/src/crimson/os/seastore/cache.h
+++ b/src/crimson/os/seastore/cache.h
@@ -299,6 +299,7 @@ public:
           "{} {}~{} is absent(placeholder), reading ... -- {}",
           T::TYPE, offset, length, *ret);
       extents.replace(*ret, *cached);
+      on_cache(*ret);
 
       // replace placeholder in transactions
       while (!cached->transactions.empty()) {
@@ -425,20 +426,16 @@ public:
     } else {
       SUBTRACET(seastore_cache, "{} {}~{} is absent on t, query cache ...",
                 t, T::TYPE, offset, length);
-      auto f = [&t](CachedExtent &ext) {
+      auto f = [&t, this](CachedExtent &ext) {
        t.add_to_read_set(CachedExtentRef(&ext));
+       touch_extent(ext);
       };
       auto metric_key = std::make_pair(t.get_src(), T::TYPE);
       return trans_intr::make_interruptible(
        get_extent<T>(
          offset, length, &metric_key,
          std::forward<Func>(extent_init_func), std::move(f))
-      ).si_then([this](auto ref) {
-       (void)this; // silence incorrect clang warning about captur
-       touch_extent(*ref);
-       return get_extent_iertr::make_ready_future<TCachedExtentRef<T>>(
-         std::move(ref));
-      });
+      );
     }
   }
   template <typename T>
@@ -513,19 +510,16 @@ private:
     } else {
       SUBTRACET(seastore_cache, "{} {}~{} {} is absent on t, query cache ...",
                 t, type, offset, length, laddr);
-      auto f = [&t](CachedExtent &ext) {
+      auto f = [&t, this](CachedExtent &ext) {
        t.add_to_read_set(CachedExtentRef(&ext));
+       touch_extent(ext);
       };
       auto src = t.get_src();
       return trans_intr::make_interruptible(
        _get_extent_by_type(
          type, offset, laddr, length, &src,
          std::move(extent_init_func), std::move(f))
-      ).si_then([this](CachedExtentRef ret) {
-       touch_extent(*ret);
-       return get_extent_ertr::make_ready_future<CachedExtentRef>(
-         std::move(ret));
-      });
+      );
     }
   }
 
@@ -1017,10 +1011,7 @@ private:
     }
 
     void add_to_lru(CachedExtent &extent) {
-      assert(
-       extent.is_clean() &&
-       !extent.is_pending() &&
-       !extent.is_placeholder());
+      assert(extent.is_clean() && !extent.is_placeholder());
       
       if (!extent.primary_ref_list_hook.is_linked()) {
        contents += extent.get_length();
@@ -1046,9 +1037,7 @@ private:
     }
 
     void remove_from_lru(CachedExtent &extent) {
-      assert(extent.is_clean());
-      assert(!extent.is_pending());
-      assert(!extent.is_placeholder());
+      assert(extent.is_clean() && !extent.is_placeholder());
 
       if (extent.primary_ref_list_hook.is_linked()) {
        lru.erase(lru.s_iterator_to(extent));
@@ -1059,10 +1048,7 @@ private:
     }
 
     void move_to_top(CachedExtent &extent) {
-      assert(
-       extent.is_clean() &&
-       !extent.is_pending() &&
-       !extent.is_placeholder());
+      assert(extent.is_clean() && !extent.is_placeholder());
 
       if (extent.primary_ref_list_hook.is_linked()) {
        lru.erase(lru.s_iterator_to(extent));
@@ -1232,7 +1218,6 @@ private:
 
   /// Update lru for access to ref
   void touch_extent(CachedExtent &ext) {
-    assert(!ext.is_pending());
     if (ext.is_clean() && !ext.is_placeholder()) {
       lru.move_to_top(ext);
     }