From c5a74cb81f5eefa39bb7fbf8aa1f2359d6b03d37 Mon Sep 17 00:00:00 2001
From: Yingxin Cheng <yingxin.cheng@intel.com>
Date: Mon, 6 Jun 2022 10:57:17 +0800
Subject: [PATCH] crimson/os/seastore/cache: fix a potential leak when replace
 placeholder

Signed-off-by: Yingxin Cheng <yingxin.cheng@intel.com>
---
 src/crimson/os/seastore/cache.cc |  8 ++------
 src/crimson/os/seastore/cache.h  | 35 +++++++++-----------------------
 2 files changed, 12 insertions(+), 31 deletions(-)

diff --git a/src/crimson/os/seastore/cache.cc b/src/crimson/os/seastore/cache.cc
index 9183e5cd0bb8b..6803c145cb709 100644
--- 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 f78934defa3c8..40187ca41dc30 100644
--- 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);
     }
-- 
2.39.5