From: Samuel Just <sjust@redhat.com>
Date: Wed, 5 Aug 2020 02:50:45 +0000 (-0700)
Subject: crimson/os/seastore/transaction_manager: complete dec_ref
X-Git-Tag: wip-pdonnell-testing-20200918.022351~375^2
X-Git-Url: http://git.apps.os.sepia.ceph.com/?a=commitdiff_plain;h=bb3a7f35152f6f21e5df53349112ee806246f26a;p=ceph-ci.git

crimson/os/seastore/transaction_manager: complete dec_ref

Previously, dec_ref didn't handle actually retiring the
extent from the cache.  dec_ref will now reach into the
cache and mark the extent retired if it exists either
in the cache or in the current transaction.

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 2e7855651ab..7b130c71216 100644
--- a/src/crimson/os/seastore/cache.cc
+++ b/src/crimson/os/seastore/cache.cc
@@ -28,6 +28,24 @@ Cache::~Cache()
   ceph_assert(extents.empty());
 }
 
+Cache::retire_extent_ret Cache::retire_extent_if_cached(
+  Transaction &t, paddr_t addr)
+{
+  if (auto ext = t.write_set.find_offset(addr); ext != t.write_set.end()) {
+    t.add_to_retired_set(CachedExtentRef(&*ext));
+    return retire_extent_ertr::now();
+  } else if (auto iter = extents.find_offset(addr);
+      iter != extents.end()) {
+    auto ret = CachedExtentRef(&*iter);
+    return ret->wait_io().then([&t, ret=std::move(ret)]() mutable {
+      t.add_to_retired_set(ret);
+      return retire_extent_ertr::now();
+    });
+  } else {
+    return retire_extent_ertr::now();
+  }
+}
+
 void Cache::add_extent(CachedExtentRef ref)
 {
   assert(ref->is_valid());
diff --git a/src/crimson/os/seastore/cache.h b/src/crimson/os/seastore/cache.h
index d8859b3e7fb..a31e044e1d6 100644
--- a/src/crimson/os/seastore/cache.h
+++ b/src/crimson/os/seastore/cache.h
@@ -90,6 +90,13 @@ public:
     t.add_to_retired_set(ref);
   }
 
+  /// Declare paddr retired in t, noop if not cached
+  using retire_extent_ertr = crimson::errorator<
+    crimson::ct_error::input_output_error>;
+  using retire_extent_ret = retire_extent_ertr::future<>;
+  retire_extent_ret retire_extent_if_cached(
+    Transaction &t, paddr_t addr);
+
   /**
    * get_root
    *
diff --git a/src/crimson/os/seastore/transaction_manager.cc b/src/crimson/os/seastore/transaction_manager.cc
index 05859a3626a..82a4d3de327 100644
--- a/src/crimson/os/seastore/transaction_manager.cc
+++ b/src/crimson/os/seastore/transaction_manager.cc
@@ -117,21 +117,32 @@ TransactionManager::ref_ret TransactionManager::dec_ref(
   Transaction &t,
   LogicalCachedExtentRef &ref)
 {
-  return dec_ref(t, ref->get_laddr()
-  ).handle_error(
-    ref_ertr::pass_further{},
-    ct_error::all_same_way([](auto e) {
-      ceph_assert(0 == "unhandled error, TODO");
-    }));
+  return lba_manager.decref_extent(t, ref->get_laddr()
+  ).safe_then([this, &t, ref](auto ret) {
+    if (ret.refcount == 0) {
+      cache.retire_extent(t, ref);
+    }
+    return ret.refcount;
+  });
 }
 
 TransactionManager::ref_ret TransactionManager::dec_ref(
   Transaction &t,
   laddr_t offset)
 {
-  // TODO: need to retire the extent (only) if it's live, will need cache call
-  return lba_manager.decref_extent(t, offset).safe_then([](auto ret) {
-    return ret.refcount;
+  return lba_manager.decref_extent(t, offset
+  ).safe_then([this, &t](auto result) -> ref_ret {
+    if (result.refcount == 0) {
+      return cache.retire_extent_if_cached(t, result.addr).safe_then([] {
+	return ref_ret(
+	  ref_ertr::ready_future_marker{},
+	  0);
+      });
+    } else {
+      return ref_ret(
+	ref_ertr::ready_future_marker{},
+	result.refcount);
+    }
   });
 }
 
diff --git a/src/test/crimson/seastore/test_transaction_manager.cc b/src/test/crimson/seastore/test_transaction_manager.cc
index d086c4ca348..076d7ad2dce 100644
--- a/src/test/crimson/seastore/test_transaction_manager.cc
+++ b/src/test/crimson/seastore/test_transaction_manager.cc
@@ -278,6 +278,38 @@ TEST_F(transaction_manager_test_t, mutate)
   });
 }
 
+TEST_F(transaction_manager_test_t, create_remove_same_transaction)
+{
+  constexpr laddr_t SIZE = 4096;
+  run_async([this] {
+    constexpr laddr_t ADDR = 0xFF * SIZE;
+    {
+      auto t = create_transaction();
+      auto extent = alloc_extent(
+	t,
+	ADDR,
+	SIZE,
+	'a');
+      ASSERT_EQ(ADDR, extent->get_laddr());
+      check_mappings(t);
+      dec_ref(t, ADDR);
+      check_mappings(t);
+
+      extent = alloc_extent(
+	t,
+	ADDR,
+	SIZE,
+	'a');
+
+      submit_transaction(std::move(t));
+      check_mappings();
+    }
+    replay();
+    check_mappings();
+  });
+}
+
+
 TEST_F(transaction_manager_test_t, inc_dec_ref)
 {
   constexpr laddr_t SIZE = 4096;