Merge pull request ceph#58983 from cyx1231st/wip-seastore-cleanup-cache

crimson/os/seastore/cache: cleanups and comments

Reviewed-by: Xuehan Xu <[email protected]>

Author: cyx1231st

src/crimson/os/seastore/async_cleaner.h

@@ -333,12 +333,12 @@ class ExtentCallbackInterface {
     sea_time_point modify_time) = 0;
 
   /**
-   * get_extent_if_live
+   * get_extents_if_live
    *
    * Returns extent at specified location if still referenced by
    * lba_manager and not removed by t.
    *
-   * See TransactionManager::get_extent_if_live and
+   * See TransactionManager::get_extents_if_live and
    * LBAManager::get_physical_extent_if_live.
    */
   using get_extents_if_live_iertr = base_iertr;

src/crimson/os/seastore/btree/fixed_kv_btree.h

@@ -1383,6 +1383,9 @@ class FixedKVBtree {
       "looking up root on {}",
       c.trans,
       *root_block);
+
+    // checking the lba root node must be atomic with creating
+    // and linking the absent root node
     auto [found, fut] = get_root_node(c);
 
     auto on_found_internal =

src/crimson/os/seastore/cache.h

@@ -437,6 +437,11 @@ class Cache {
    * Mostly the same as Cache::get_extent(), with the only difference
    * that get_absent_extent won't search the transaction's context for
    * the specific CachedExtent
+   *
+   * The extent in query is supposed to be absent in Cache.
+   *
+   * User is responsible to call get_extent_viewable_by_trans()
+   * *atomically* prior to call this method.
    */
   template <typename T>
   get_extent_iertr::future<TCachedExtentRef<T>> get_absent_extent(
@@ -471,18 +476,46 @@ class Cache {
     CachedExtentRef extent)
   {
     assert(extent->is_valid());
-    auto p_extent = extent->get_transactional_view(t);
-    if (!p_extent->is_pending_in_trans(t.get_trans_id())) {
-      t.add_to_read_set(p_extent);
-      if (!p_extent->is_mutation_pending()) {
-	touch_extent(*p_extent);
+    CachedExtent* p_extent;
+    if (extent->is_stable()) {
+      p_extent = extent->get_transactional_view(t);
+      if (p_extent != extent.get()) {
+        assert(!extent->is_stable_writting());
+        assert(p_extent->is_pending_in_trans(t.get_trans_id()));
+        assert(!p_extent->is_stable_writting());
+        if (p_extent->is_mutable()) {
+          assert(p_extent->is_fully_loaded());
+          assert(!p_extent->is_pending_io());
+          return get_extent_ertr::make_ready_future<CachedExtentRef>(
+            CachedExtentRef(p_extent));
+        } else {
+          assert(p_extent->is_exist_clean());
+        }
+      } else {
+        // stable from trans-view
+        assert(!p_extent->is_pending_in_trans(t.get_trans_id()));
+        if (t.maybe_add_to_read_set(p_extent)) {
+          touch_extent(*p_extent);
+        }
+      }
+    } else {
+      assert(!extent->is_stable_writting());
+      assert(extent->is_pending_in_trans(t.get_trans_id()));
+      if (extent->is_mutable()) {
+        assert(extent->is_fully_loaded());
+        assert(!extent->is_pending_io());
+        return get_extent_ertr::make_ready_future<CachedExtentRef>(extent);
+      } else {
+        assert(extent->is_exist_clean());
+        p_extent = extent.get();
       }
     }
+
+    assert(p_extent->is_stable() || p_extent->is_exist_clean());
     // user should not see RETIRED_PLACEHOLDER extents
     ceph_assert(p_extent->get_type() != extent_types_t::RETIRED_PLACEHOLDER);
     if (!p_extent->is_fully_loaded()) {
       assert(!p_extent->is_mutable());
-      touch_extent(*p_extent);
       LOG_PREFIX(Cache::get_extent_viewable_by_trans);
       SUBDEBUG(seastore_cache,
         "{} {}~{} is present without been fully loaded, reading ... -- {}",
@@ -784,6 +817,9 @@ class Cache {
    * and read in the extent at location offset~length.
    *
    * The extent in query is supposed to be absent in Cache.
+   *
+   * User is responsible to call get_extent_viewable_by_trans()
+   * *atomically* prior to call this method.
    */
   template <typename Func>
   get_extent_by_type_ret get_absent_extent_by_type(

src/crimson/os/seastore/cached_extent.h

@@ -343,6 +343,8 @@ class CachedExtent
       : "nullopt";
     out << "CachedExtent(addr=" << this
 	<< ", type=" << get_type()
+	<< ", trans=" << pending_for_transaction
+	<< ", pending_io=" << is_pending_io()
 	<< ", version=" << version
 	<< ", dirty_from_or_retired_at=" << dirty_from_or_retired_at
 	<< ", modify_time=" << sea_time_point_printer_t{modify_time}
@@ -440,14 +442,18 @@ class CachedExtent
       state == extent_state_t::DIRTY;
   }
 
+  bool is_stable_writting() const {
+    // MUTATION_PENDING and under-io extents are already stable and visible,
+    // see prepare_record().
+    //
+    // XXX: It might be good to mark this case as DIRTY from the definition,
+    // which probably can make things simpler.
+    return is_mutation_pending() && is_pending_io();
+  }
+
   /// Returns true if extent is stable and shared among transactions
   bool is_stable() const {
-    return is_stable_written() ||
-	   // MUTATION_PENDING and under-io extents are to-be-stable extents,
-	   // for the sake of caveats that checks the correctness of extents
-	   // states, we consider them stable.
-           (is_mutation_pending() &&
-            is_pending_io());
+    return is_stable_written() || is_stable_writting();
   }
 
   bool is_data_stable() const {

src/crimson/os/seastore/transaction.h

@@ -137,14 +137,37 @@ class Transaction {
     }
   }
 
+  // Returns true if added, false if already added or weak
+  bool maybe_add_to_read_set(CachedExtentRef ref) {
+    if (is_weak()) {
+      return false;
+    }
+
+    assert(ref->is_valid());
+
+    auto it = ref->transactions.lower_bound(
+      this, read_set_item_t<Transaction>::trans_cmp_t());
+    if (it != ref->transactions.end() && it->t == this) {
+      return false;
+    }
+
+    auto [iter, inserted] = read_set.emplace(this, ref);
+    ceph_assert(inserted);
+    ref->transactions.insert_before(
+      it, const_cast<read_set_item_t<Transaction>&>(*iter));
+    return true;
+  }
+
   void add_to_read_set(CachedExtentRef ref) {
-    if (is_weak()) return;
+    if (is_weak()) {
+      return;
+    }
 
     assert(ref->is_valid());
 
     auto it = ref->transactions.lower_bound(
       this, read_set_item_t<Transaction>::trans_cmp_t());
-    if (it != ref->transactions.end() && it->t == this) return;
+    assert(it == ref->transactions.end() || it->t != this);
 
     auto [iter, inserted] = read_set.emplace(this, ref);
     ceph_assert(inserted);

src/crimson/os/seastore/transaction_manager.cc

@@ -680,7 +680,7 @@ TransactionManager::get_extents_if_live(
   laddr_t laddr,
   extent_len_t len)
 {
-  LOG_PREFIX(TransactionManager::get_extent_if_live);
+  LOG_PREFIX(TransactionManager::get_extents_if_live);
   TRACET("{} {}~{} {}", t, type, laddr, len, paddr);
 
   // This only works with segments to check if alive,