Merge pull request ceph#62633 from cyx1231st/wip-seastore-le-map-cleanup

crimson/os/seastore: cleanups related to lba and cache

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

Author: cyx1231st

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

@@ -190,7 +190,8 @@ class BtreeNodeMapping : public PhysicalNodeMapping<key_t, val_t> {
       auto &trans = extent.retired_transactions;
       unviewable = (trans.find(trans_id, trans_spec_view_t::cmp_t()) !=
 		 trans.end());
-      assert(unviewable == t.is_retired(extent.get_paddr(), extent.get_length()));
+      assert(unviewable ==
+             t.is_stable_extent_retired(extent.get_paddr(), extent.get_length()));
     }
     return unviewable;
   }

src/crimson/os/seastore/cache.cc

@@ -51,70 +51,68 @@ Cache::~Cache()
 
 // TODO: this method can probably be removed in the future
 Cache::retire_extent_ret Cache::retire_extent_addr(
-  Transaction &t, paddr_t addr, extent_len_t length)
+  Transaction &t, paddr_t paddr, extent_len_t length)
 {
   LOG_PREFIX(Cache::retire_extent_addr);
-  TRACET("retire {}~0x{:x}", t, addr, length);
+  TRACET("retire {}~0x{:x}", t, paddr, length);
 
-  assert(addr.is_real() && !addr.is_block_relative());
+  assert(paddr.is_real() && !paddr.is_block_relative());
 
   CachedExtentRef ext;
-  auto result = t.get_extent(addr, &ext);
+  auto result = t.get_extent(paddr, &ext);
   if (result == Transaction::get_extent_ret::PRESENT) {
-    DEBUGT("retire {}~0x{:x} on t -- {}", t, addr, length, *ext);
-    t.add_present_to_retired_set(CachedExtentRef(&*ext));
+    DEBUGT("retire {}~0x{:x} on t -- {}",
+           t, paddr, length, *ext);
+    t.add_present_to_retired_set(ext);
     return retire_extent_iertr::now();
   } else if (result == Transaction::get_extent_ret::RETIRED) {
-    ERRORT("retire {}~0x{:x} failed, already retired -- {}", t, addr, length, *ext);
+    ERRORT("retire {}~0x{:x} failed, already retired -- {}",
+           t, paddr, length, *ext);
     ceph_abort();
   }
 
-  // any relative addr must have been on the transaction
-  assert(!addr.is_relative());
+  // any relative paddr must have been on the transaction
+  assert(!paddr.is_relative());
 
   // absent from transaction
   // retiring is not included by the cache hit metrics
-  ext = query_cache(addr);
+  ext = query_cache(paddr);
   if (ext) {
-    DEBUGT("retire {}~0x{:x} in cache -- {}", t, addr, length, *ext);
+    DEBUGT("retire {}~0x{:x} in cache -- {}", t, paddr, length, *ext);
   } else {
     // add a new placeholder to Cache
     ext = CachedExtent::make_cached_extent_ref<
       RetiredExtentPlaceholder>(length);
-    ext->init(CachedExtent::extent_state_t::CLEAN,
-              addr,
-              PLACEMENT_HINT_NULL,
-              NULL_GENERATION,
-	      TRANS_ID_NULL);
+    ext->init(
+      CachedExtent::extent_state_t::CLEAN, paddr,
+      PLACEMENT_HINT_NULL, NULL_GENERATION, TRANS_ID_NULL);
     DEBUGT("retire {}~0x{:x} as placeholder, add extent -- {}",
-           t, addr, length, *ext);
+           t, paddr, length, *ext);
     add_extent(ext);
   }
   t.add_absent_to_retired_set(ext);
   return retire_extent_iertr::now();
 }
 
 void Cache::retire_absent_extent_addr(
-  Transaction &t, paddr_t addr, extent_len_t length)
+  Transaction &t, paddr_t paddr, extent_len_t length)
 {
   CachedExtentRef ext;
 #ifndef NDEBUG
-  auto result = t.get_extent(addr, &ext);
+  auto result = t.get_extent(paddr, &ext);
   assert(result != Transaction::get_extent_ret::PRESENT
     && result != Transaction::get_extent_ret::RETIRED);
-  assert(!query_cache(addr));
+  assert(!query_cache(paddr));
 #endif
   LOG_PREFIX(Cache::retire_absent_extent_addr);
   // add a new placeholder to Cache
   ext = CachedExtent::make_cached_extent_ref<
     RetiredExtentPlaceholder>(length);
-  ext->init(CachedExtent::extent_state_t::CLEAN,
-	    addr,
-	    PLACEMENT_HINT_NULL,
-	    NULL_GENERATION,
-	    TRANS_ID_NULL);
+  ext->init(
+    CachedExtent::extent_state_t::CLEAN, paddr,
+    PLACEMENT_HINT_NULL, NULL_GENERATION, TRANS_ID_NULL);
   DEBUGT("retire {}~0x{:x} as placeholder, add extent -- {}",
-	 t, addr, length, *ext);
+	 t, paddr, length, *ext);
   add_extent(ext);
   t.add_absent_to_retired_set(ext);
 }
@@ -1062,15 +1060,15 @@ void Cache::on_transaction_destruct(Transaction& t)
   }
 }
 
-CachedExtentRef Cache::alloc_new_extent_by_type(
+CachedExtentRef Cache::alloc_new_non_data_extent_by_type(
   Transaction &t,        ///< [in, out] current transaction
   extent_types_t type,   ///< [in] type tag
   extent_len_t length,   ///< [in] length
   placement_hint_t hint, ///< [in] user hint
   rewrite_gen_t gen      ///< [in] rewrite generation
 )
 {
-  LOG_PREFIX(Cache::alloc_new_extent_by_type);
+  LOG_PREFIX(Cache::alloc_new_non_data_extent_by_type);
   SUBDEBUGT(seastore_cache, "allocate {} 0x{:x}B, hint={}, gen={}",
             t, type, length, hint, rewrite_gen_printer_t{gen});
   ceph_assert(get_extent_category(type) == data_category_t::METADATA);
@@ -1181,7 +1179,6 @@ CachedExtentRef Cache::duplicate_for_write(
   assert(!i->prior_poffset);
   auto [iter, inserted] = i->mutation_pending_extents.insert(*ret);
   ceph_assert(inserted);
-  t.add_mutated_extent(ret);
   if (is_root_type(ret->get_type())) {
     t.root = ret->cast<RootBlock>();
   } else {
@@ -1196,6 +1193,8 @@ CachedExtentRef Cache::duplicate_for_write(
     assert(ret->is_logical());
     static_cast<LogicalCachedExtent&>(*ret).set_laddr(lextent.get_laddr());
   }
+
+  t.add_mutated_extent(ret);
   DEBUGT("{} -> {}", t, *i, *ret);
   return ret;
 }

src/crimson/os/seastore/cache.h

@@ -201,20 +201,21 @@ class Cache : public ExtentTransViewRetriever {
     get_extent_if_cached_iertr::future<CachedExtentRef>;
   get_extent_if_cached_ret get_extent_if_cached(
     Transaction &t,
-    paddr_t offset,
+    paddr_t paddr,
+    extent_len_t len,
     extent_types_t type) {
-    CachedExtentRef ret;
     LOG_PREFIX(Cache::get_extent_if_cached);
-    auto result = t.get_extent(offset, &ret);
     const auto t_src = t.get_src();
+    CachedExtentRef ret;
+    auto result = t.get_extent(paddr, &ret);
     extent_access_stats_t& access_stats = get_by_ext(
       get_by_src(stats.access_by_src_ext, t_src),
       type);
     if (result == Transaction::get_extent_ret::RETIRED) {
-      SUBDEBUGT(seastore_cache, "{} {} is retired on t -- {}",
-                t, type, offset, *ret);
-      return get_extent_if_cached_iertr::make_ready_future<
-        CachedExtentRef>(ret);
+      SUBDEBUGT(seastore_cache,
+        "{} {}~0x{:x} is retired on t",
+        t, type, paddr, len);
+      return get_extent_if_cached_iertr::make_ready_future<CachedExtentRef>();
     } else if (result == Transaction::get_extent_ret::PRESENT) {
       if (ret->is_stable()) {
         if (ret->is_dirty()) {
@@ -229,32 +230,46 @@ class Cache : public ExtentTransViewRetriever {
         ++stats.access.s.trans_pending;
       }
 
-      if (ret->is_fully_loaded()) {
-        SUBTRACET(seastore_cache, "{} {} is present on t -- {}",
-                  t, type, offset, *ret);
-        return ret->wait_io().then([ret] {
-	  return get_extent_if_cached_iertr::make_ready_future<
-	    CachedExtentRef>(ret);
-        });
-      } else {
+      if (ret->get_length() != len) {
         SUBDEBUGT(seastore_cache,
-            "{} {} is present on t -- {} without fully loaded",
-            t, type, offset, *ret);
-        return get_extent_if_cached_iertr::make_ready_future<
-          CachedExtentRef>();
+          "{} {}~0x{:x} is present on t with inconsistent length 0x{:x} -- {}",
+          t, type, paddr, len, ret->get_length(), *ret);
+        return get_extent_if_cached_iertr::make_ready_future<CachedExtentRef>();
       }
+
+      ceph_assert(ret->get_type() == type);
+      if (!ret->is_fully_loaded()) {
+        SUBDEBUGT(seastore_cache,
+          "{} {}~0x{:x} is present on t without fully loaded -- {}",
+          t, type, paddr, len, *ret);
+        return trans_intr::make_interruptible(
+          do_read_extent_maybe_partial<CachedExtent>(
+            ret->cast<CachedExtent>(), 0, ret->get_length(), &t_src));
+      }
+
+      SUBTRACET(seastore_cache,
+        "{} {}~0x{:x} is present on t -- {}",
+        t, type, paddr, len, *ret);
+      return ret->wait_io().then([ret] {
+        return get_extent_if_cached_iertr::make_ready_future<CachedExtentRef>(ret);
+      });
     }
 
     // get_extent_ret::ABSENT from transaction
-    ret = query_cache(offset);
+    ret = query_cache(paddr);
     if (!ret) {
-      SUBDEBUGT(seastore_cache, "{} {} is absent", t, type, offset);
+      SUBDEBUGT(seastore_cache,
+        "{} {}~0x{:x} is absent in cache",
+        t, type, paddr, len);
       account_absent_access(t_src);
       return get_extent_if_cached_iertr::make_ready_future<CachedExtentRef>();
-    } else if (is_retired_placeholder_type(ret->get_type())) {
+    }
+
+    if (is_retired_placeholder_type(ret->get_type())) {
       // retired_placeholder is not really cached yet
-      SUBDEBUGT(seastore_cache, "{} {} is absent(placeholder)",
-                t, type, offset);
+      SUBDEBUGT(seastore_cache,
+        "{} {}~0x{:x} ~0x{:x} is absent(placeholder) in cache",
+        t, type, paddr, len, ret->get_length());
       account_absent_access(t_src);
       return get_extent_if_cached_iertr::make_ready_future<CachedExtentRef>();
     }
@@ -267,18 +282,29 @@ class Cache : public ExtentTransViewRetriever {
       ++stats.access.s.cache_lru;
     }
 
-    if (!ret->is_fully_loaded()) {
-      // ignore non-full extent
+    if (ret->get_length() != len) {
       SUBDEBUGT(seastore_cache,
-          "{} {} is present without fully loaded", t, type, offset);
+        "{} {}~0x{:x} is present in cache with inconsistent length 0x{:x} -- {}",
+        t, type, paddr, len, ret->get_length(), *ret);
       return get_extent_if_cached_iertr::make_ready_future<CachedExtentRef>();
     }
 
-    // present in cache(fully loaded) and is not a retired_placeholder
-    SUBDEBUGT(seastore_cache, "{} {} is present in cache -- {}",
-              t, type, offset, *ret);
+    ceph_assert(ret->get_type() == type);
     t.add_to_read_set(ret);
     touch_extent(*ret, &t_src, t.get_cache_hint());
+    if (!ret->is_fully_loaded()) {
+      SUBDEBUGT(seastore_cache,
+        "{} {}~0x{:x} is present without fully loaded in cache -- {}",
+        t, type, paddr, len, *ret);
+      return trans_intr::make_interruptible(
+        do_read_extent_maybe_partial<CachedExtent>(
+          ret->cast<CachedExtent>(), 0, ret->get_length(), &t_src));
+    }
+
+    // present in cache(fully loaded) and is not a retired_placeholder
+    SUBDEBUGT(seastore_cache,
+      "{} {}~0x{:x} is present in cache -- {}",
+      t, type, paddr, len, *ret);
     return ret->wait_io().then([ret] {
       return get_extent_if_cached_iertr::make_ready_future<
         CachedExtentRef>(ret);
@@ -685,6 +711,8 @@ class Cache : public ExtentTransViewRetriever {
       SUBDEBUG(seastore_cache,
           "{} {}~0x{:x} is absent(placeholder), add extent and reading range 0x{:x}~0x{:x} ... -- {}",
           T::TYPE, offset, length, partial_off, partial_len, *ret);
+      extent_init_func(*ret);
+      on_cache(*ret);
       extents_index.replace(*ret, *cached);
 
       // replace placeholder in transactions
@@ -694,8 +722,6 @@ class Cache : public ExtentTransViewRetriever {
       }
 
       cached->state = CachedExtent::extent_state_t::INVALID;
-      extent_init_func(*ret);
-      on_cache(*ret);
       return read_extent<T>(
 	std::move(ret), partial_off, partial_len, p_src);
     }
@@ -1101,11 +1127,11 @@ class Cache : public ExtentTransViewRetriever {
   }
 
   /**
-   * alloc_new_extent
+   * alloc_new_non_data_extent_by_type
    *
-   * Allocates a fresh extent.  addr will be relative until commit.
+   * Allocates a fresh non data extent.  addr will be relative until commit.
    */
-  CachedExtentRef alloc_new_extent_by_type(
+  CachedExtentRef alloc_new_non_data_extent_by_type(
     Transaction &t,        ///< [in, out] current transaction
     extent_types_t type,   ///< [in] type tag
     extent_len_t length,   ///< [in] length
@@ -1114,9 +1140,9 @@ class Cache : public ExtentTransViewRetriever {
     );
 
   /**
-   * alloc_new_extent
+   * alloc_new_data_extents_by_type
    *
-   * Allocates a fresh extent.  addr will be relative until commit.
+   * Allocates fresh data extents.  addr will be relative until commit.
    */
   std::vector<CachedExtentRef> alloc_new_data_extents_by_type(
     Transaction &t,        ///< [in, out] current transaction

src/crimson/os/seastore/cached_extent.h

@@ -1258,6 +1258,14 @@ class ExtentIndex {
     return extent_index.find(offset, paddr_cmp());
   }
 
+  bool exists(CachedExtent& extent) const {
+    auto iter = extent_index.find(extent.get_paddr(), paddr_cmp());
+    if (iter == extent_index.end()) {
+      return false;
+    }
+    return (&*iter == &extent);
+  }
+
   auto begin() {
     return extent_index.begin();
   }
@@ -1336,12 +1344,12 @@ class RetiredExtentPlaceholder : public CachedExtent {
     : CachedExtent(CachedExtent::retired_placeholder_construct_t{}, length) {}
 
   CachedExtentRef duplicate_for_write(Transaction&) final {
-    ceph_assert(0 == "Should never happen for a placeholder");
+    ceph_abort("Should never happen for a placeholder");
     return CachedExtentRef();
   }
 
   ceph::bufferlist get_delta() final {
-    ceph_assert(0 == "Should never happen for a placeholder");
+    ceph_abort("Should never happen for a placeholder");
     return ceph::bufferlist();
   }
 
@@ -1352,7 +1360,7 @@ class RetiredExtentPlaceholder : public CachedExtent {
 
   void apply_delta_and_adjust_crc(
     paddr_t base, const ceph::bufferlist &bl) final {
-    ceph_assert(0 == "Should never happen for a placeholder");
+    ceph_abort("Should never happen for a placeholder");
   }
 
   void on_rewrite(Transaction &, CachedExtent&, extent_len_t) final {}
@@ -1362,7 +1370,7 @@ class RetiredExtentPlaceholder : public CachedExtent {
   }
 
   void on_delta_write(paddr_t record_block_offset) final {
-    ceph_assert(0 == "Should never happen for a placeholder");
+    ceph_abort("Should never happen for a placeholder");
   }
 };
 
@@ -1420,10 +1428,13 @@ class LogicalCachedExtent : public CachedExtent {
     extent_len_t len;
   };
   virtual std::optional<modified_region_t> get_modified_region() {
+    ceph_abort("Unsupported");
     return std::nullopt;
   }
 
-  virtual void clear_modified_region() {}
+  virtual void clear_modified_region() {
+    ceph_abort("Unsupported");
+  }
 
   virtual ~LogicalCachedExtent() {}
 
@@ -1497,7 +1508,7 @@ template <typename T>
 using lextent_list_t = addr_extent_list_base_t<
   laddr_t, TCachedExtentRef<T>>;
 
-}
+} // namespace crimson::os::seastore
 
 #if FMT_VERSION >= 90000
 template <> struct fmt::formatter<crimson::os::seastore::CachedExtent> : fmt::ostream_formatter {};