From 75035edcb9ef50096afdb2dfc2c4e705644424a3 Mon Sep 17 00:00:00 2001
From: Xuehan Xu <xxhdx1985126@gmail.com>
Date: Tue, 31 Aug 2021 11:26:31 +0800
Subject: [PATCH] crimson/os/seastore: add ExtentIndex::clear_and_dispose

When a transaction is interrupted and needs to repeat, its reset_preserve_handle() method
is called to clear various extent set and list. The problem is the clear operation call
ExtentIndex::clear() instead of ExtentIndex::erase(), which would leave CachedExtent::parent_index
still pointing to the write_set/delayed_set while the CachedExtent is no longer linked to
those sets. This would make CachedExtent::~CachedExtent() to try to erase itself from
CachedExtent::parent_index even it's not linked, which would cause failures in the successive
operations

Signed-off-by: Xuehan Xu <xxhdx1985126@gmail.com>
---
 src/crimson/os/seastore/cached_extent.cc |  1 +
 src/crimson/os/seastore/cached_extent.h  | 16 +++++++++++++---
 src/crimson/os/seastore/transaction.h    | 21 ++++++++++-----------
 3 files changed, 24 insertions(+), 14 deletions(-)

diff --git a/src/crimson/os/seastore/cached_extent.cc b/src/crimson/os/seastore/cached_extent.cc
index 4c183589bd516..86d128621da72 100644
--- a/src/crimson/os/seastore/cached_extent.cc
+++ b/src/crimson/os/seastore/cached_extent.cc
@@ -61,6 +61,7 @@ std::ostream &operator<<(std::ostream &out, const CachedExtent &ext)
 CachedExtent::~CachedExtent()
 {
   if (parent_index) {
+    assert(is_linked());
     parent_index->erase(*this);
   }
 }
diff --git a/src/crimson/os/seastore/cached_extent.h b/src/crimson/os/seastore/cached_extent.h
index 8bbaa2a2c00cb..a72d5166339b9 100644
--- a/src/crimson/os/seastore/cached_extent.h
+++ b/src/crimson/os/seastore/cached_extent.h
@@ -354,6 +354,10 @@ private:
   friend class ExtentIndex;
   friend class Transaction;
 
+  bool is_linked() {
+    return extent_index_hook.is_linked();
+  }
+
   /// hook for intrusive ref list (mainly dirty or lru list)
   boost::intrusive::list_member_hook<> primary_ref_list_hook;
   using primary_ref_list_member_options = boost::intrusive::member_hook<
@@ -546,6 +550,12 @@ public:
     );
   }
 
+  template <typename Disposer>
+  void clear_and_dispose(Disposer disposer) {
+    extent_index.clear_and_dispose(disposer);
+    bytes = 0;
+  }
+
   void clear() {
     extent_index.clear();
     bytes = 0;
@@ -567,13 +577,13 @@ public:
 
   void erase(CachedExtent &extent) {
     assert(extent.parent_index);
+    assert(extent.is_linked());
     auto erased = extent_index.erase(
       extent_index.s_iterator_to(extent));
     extent.parent_index = nullptr;
 
-    if (erased) {
-      bytes -= extent.get_length();
-    }
+    assert(erased);
+    bytes -= extent.get_length();
   }
 
   void replace(CachedExtent &to, CachedExtent &from) {
diff --git a/src/crimson/os/seastore/transaction.h b/src/crimson/os/seastore/transaction.h
index 0161a5e90c864..a21beb846400c 100644
--- a/src/crimson/os/seastore/transaction.h
+++ b/src/crimson/os/seastore/transaction.h
@@ -24,6 +24,12 @@ class Transaction;
  * Representation of in-progress mutation. Used exclusively through Cache methods.
  */
 class Transaction {
+  struct cached_extent_disposer {
+    void operator() (CachedExtent* extent) {
+      extent->parent_index = nullptr;
+      extent->state = CachedExtent::extent_state_t::INVALID;
+    }
+  };
 public:
   using Ref = std::unique_ptr<Transaction>;
   using on_destruct_func_t = std::function<void(Transaction&)>;
@@ -224,15 +230,8 @@ public:
 
   ~Transaction() {
     on_destruct(*this);
-    for (auto i = write_set.begin();
-	 i != write_set.end();) {
-      i->state = CachedExtent::extent_state_t::INVALID;
-      write_set.erase(*i++);
-    }
-    for (auto i = delayed_set.begin();
-	 i != delayed_set.end();) {
-      delayed_set.erase(*i++);
-    }
+    write_set.clear_and_dispose(cached_extent_disposer());
+    delayed_set.clear_and_dispose(cached_extent_disposer());
   }
 
   friend class crimson::os::seastore::SeaStore;
@@ -243,8 +242,8 @@ public:
     offset = 0;
     delayed_temp_offset = 0;
     read_set.clear();
-    write_set.clear();
-    delayed_set.clear();
+    write_set.clear_and_dispose(cached_extent_disposer());
+    delayed_set.clear_and_dispose(cached_extent_disposer());
     fresh_block_list.clear();
     mutated_block_list.clear();
     delayed_alloc_list.clear();
-- 
2.39.5