Merge pull request ceph#62425 from cyx1231st/wip-seastore-cleanups

crimson/os/seastore: misc cleanups

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

Author: cyx1231st

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

@@ -182,7 +182,7 @@ class BtreeNodeMapping : public PhysicalNodeMapping<key_t, val_t> {
       assert(extent.is_pending_in_trans(t.get_trans_id()));
       return false;
     }
-    auto &pendings = extent.mutation_pendings;
+    auto &pendings = extent.mutation_pending_extents;
     auto trans_id = t.get_trans_id();
     bool unviewable = (pendings.find(trans_id, trans_spec_view_t::cmp_t()) !=
 		       pendings.end());

src/crimson/os/seastore/cache.cc

@@ -62,7 +62,7 @@ Cache::retire_extent_ret Cache::retire_extent_addr(
   auto result = t.get_extent(addr, &ext);
   if (result == Transaction::get_extent_ret::PRESENT) {
     DEBUGT("retire {}~0x{:x} on t -- {}", t, addr, length, *ext);
-    t.add_to_retired_set(CachedExtentRef(&*ext));
+    t.add_present_to_retired_set(CachedExtentRef(&*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);
@@ -90,8 +90,7 @@ Cache::retire_extent_ret Cache::retire_extent_addr(
            t, addr, length, *ext);
     add_extent(ext);
   }
-  t.add_to_read_set(ext);
-  t.add_to_retired_set(ext);
+  t.add_absent_to_retired_set(ext);
   return retire_extent_iertr::now();
 }
 
@@ -117,8 +116,7 @@ void Cache::retire_absent_extent_addr(
   DEBUGT("retire {}~0x{:x} as placeholder, add extent -- {}",
 	 t, addr, length, *ext);
   add_extent(ext);
-  t.add_to_read_set(ext);
-  t.add_to_retired_set(ext);
+  t.add_absent_to_retired_set(ext);
 }
 
 void Cache::dump_contents()
@@ -919,14 +917,14 @@ void Cache::invalidate_extent(
     CachedExtent& extent)
 {
   if (!extent.may_conflict()) {
-    assert(extent.transactions.empty());
+    assert(extent.read_transactions.empty());
     extent.set_invalid(t);
     return;
   }
 
   LOG_PREFIX(Cache::invalidate_extent);
   bool do_conflict_log = true;
-  for (auto &&i: extent.transactions) {
+  for (auto &&i: extent.read_transactions) {
     if (!i.t->conflicted) {
       if (do_conflict_log) {
         SUBDEBUGT(seastore_t, "conflict begin -- {}", t, extent);
@@ -1155,10 +1153,13 @@ CachedExtentRef Cache::duplicate_for_write(
   LOG_PREFIX(Cache::duplicate_for_write);
   assert(i->is_fully_loaded());
 
-  if (i->is_mutable())
+  if (i->is_mutable()) {
     return i;
+  }
 
   if (i->is_exist_clean()) {
+    assert(i->is_logical());
+    assert(static_cast<LogicalCachedExtent&>(*i).has_laddr());
     i->version++;
     i->state = CachedExtent::extent_state_t::EXIST_MUTATION_PENDING;
     i->last_committed_crc = i->calc_crc32c();
@@ -1178,7 +1179,7 @@ CachedExtentRef Cache::duplicate_for_write(
   ret->prior_instance = i;
   // duplicate_for_write won't occur after ool write finished
   assert(!i->prior_poffset);
-  auto [iter, inserted] = i->mutation_pendings.insert(*ret);
+  auto [iter, inserted] = i->mutation_pending_extents.insert(*ret);
   ceph_assert(inserted);
   t.add_mutated_extent(ret);
   if (is_root_type(ret->get_type())) {
@@ -1189,6 +1190,12 @@ CachedExtentRef Cache::duplicate_for_write(
 
   ret->version++;
   ret->state = CachedExtent::extent_state_t::MUTATION_PENDING;
+  if (i->is_logical()) {
+    auto& lextent = static_cast<LogicalCachedExtent&>(*i);
+    assert(lextent.has_laddr());
+    assert(ret->is_logical());
+    static_cast<LogicalCachedExtent&>(*ret).set_laddr(lextent.get_laddr());
+  }
   DEBUGT("{} -> {}", t, *i, *ret);
   return ret;
 }

src/crimson/os/seastore/cache.h

@@ -153,7 +153,7 @@ class Cache : public ExtentTransViewRetriever {
   void retire_extent(Transaction &t, CachedExtentRef ref) {
     LOG_PREFIX(Cache::retire_extent);
     SUBDEBUGT(seastore_cache, "retire extent -- {}", t, *ref);
-    t.add_to_retired_set(ref);
+    t.add_present_to_retired_set(ref);
   }
 
   /// Declare paddr retired in t
@@ -665,9 +665,10 @@ class Cache : public ExtentTransViewRetriever {
           "{} {}~0x{:x} is absent, add extent and reading range 0x{:x}~0x{:x} ... -- {}",
           T::TYPE, offset, length, partial_off, partial_len, *ret);
       add_extent(ret);
-      // touch_extent() should be included in on_cache
-      on_cache(*ret);
       extent_init_func(*ret);
+      // touch_extent() should be included in on_cache,
+      // required by add_extent()
+      on_cache(*ret);
       return read_extent<T>(
 	std::move(ret), partial_off, partial_len, p_src);
     }
@@ -685,16 +686,16 @@ class Cache : public ExtentTransViewRetriever {
           "{} {}~0x{:x} is absent(placeholder), add extent and reading range 0x{:x}~0x{:x} ... -- {}",
           T::TYPE, offset, length, partial_off, partial_len, *ret);
       extents_index.replace(*ret, *cached);
-      on_cache(*ret);
 
       // replace placeholder in transactions
-      while (!cached->transactions.empty()) {
-        auto t = cached->transactions.begin()->t;
+      while (!cached->read_transactions.empty()) {
+        auto t = cached->read_transactions.begin()->t;
         t->replace_placeholder(*cached, *ret);
       }
 
       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);
     }

src/crimson/os/seastore/cached_extent.cc

@@ -81,8 +81,8 @@ CachedExtent* CachedExtent::get_transactional_view(Transaction &t) {
 }
 
 CachedExtent* CachedExtent::get_transactional_view(transaction_id_t tid) {
-  auto it = mutation_pendings.find(tid, trans_spec_view_t::cmp_t());
-  if (it != mutation_pendings.end()) {
+  auto it = mutation_pending_extents.find(tid, trans_spec_view_t::cmp_t());
+  if (it != mutation_pending_extents.end()) {
     return (CachedExtent*)&(*it);
   } else {
     return this;

src/crimson/os/seastore/cached_extent.h

@@ -74,7 +74,7 @@ class read_set_item_t {
     &read_set_item_t::trans_hook>;
 
 public:
-  struct cmp_t {
+  struct extent_cmp_t {
     using is_transparent = paddr_t;
     bool operator()(const read_set_item_t<T> &lhs, const read_set_item_t &rhs) const;
     bool operator()(const paddr_t &lhs, const read_set_item_t<T> &rhs) const;
@@ -113,10 +113,14 @@ class read_set_item_t {
   read_set_item_t(read_set_item_t &&) = default;
   ~read_set_item_t() = default;
 };
+
 template <typename T>
-using read_set_t = std::set<
+using read_extent_set_t = std::set<
   read_set_item_t<T>,
-  typename read_set_item_t<T>::cmp_t>;
+  typename read_set_item_t<T>::extent_cmp_t>;
+
+template <typename T>
+using read_trans_set_t = typename read_set_item_t<T>::trans_set_t;
 
 struct trans_spec_view_t {
   // if the extent is pending, contains the id of the owning transaction;
@@ -403,6 +407,8 @@ class CachedExtent
   virtual extent_types_t get_type() const = 0;
 
   virtual bool is_logical() const {
+    assert(!is_logical_type(get_type()));
+    assert(is_physical_type(get_type()));
     return false;
   }
 
@@ -872,7 +878,7 @@ class CachedExtent
   CachedExtent* get_transactional_view(Transaction &t);
   CachedExtent* get_transactional_view(transaction_id_t tid);
 
-  read_set_item_t<Transaction>::trans_set_t transactions;
+  read_trans_set_t<Transaction> read_transactions;
 
   placement_hint_t user_hint = PLACEMENT_HINT_NULL;
 
@@ -881,7 +887,7 @@ class CachedExtent
   rewrite_gen_t rewrite_generation = NULL_GENERATION;
 
 protected:
-  trans_view_set_t mutation_pendings;
+  trans_view_set_t mutation_pending_extents;
   trans_view_set_t retired_transactions;
 
   CachedExtent(CachedExtent &&other) = delete;
@@ -1167,7 +1173,7 @@ template <typename T, typename C, typename Cmp>
 class addr_extent_set_base_t
   : public std::set<C, Cmp> {};
 
-using pextent_set_t = addr_extent_set_base_t<
+using retired_extent_set_t = addr_extent_set_base_t<
   paddr_t,
   trans_retired_extent_link_t,
   ref_paddr_cmp
@@ -1349,10 +1355,6 @@ class RetiredExtentPlaceholder : public CachedExtent {
     ceph_assert(0 == "Should never happen for a placeholder");
   }
 
-  bool is_logical() const final {
-    return false;
-  }
-
   void on_rewrite(Transaction &, CachedExtent&, extent_len_t) final {}
 
   std::ostream &print_detail(std::ostream &out) const final {
@@ -1406,6 +1408,8 @@ class LogicalCachedExtent : public CachedExtent {
   }
 
   bool is_logical() const final {
+    assert(is_logical_type(get_type()));
+    assert(!is_physical_type(get_type()));
     return true;
   }
 
@@ -1468,17 +1472,17 @@ read_set_item_t<T>::read_set_item_t(T *t, CachedExtentRef ref)
 {}
 
 template <typename T>
-inline bool read_set_item_t<T>::cmp_t::operator()(
+inline bool read_set_item_t<T>::extent_cmp_t::operator()(
   const read_set_item_t<T> &lhs, const read_set_item_t<T> &rhs) const {
   return lhs.ref->poffset < rhs.ref->poffset;
 }
 template <typename T>
-inline bool read_set_item_t<T>::cmp_t::operator()(
+inline bool read_set_item_t<T>::extent_cmp_t::operator()(
   const paddr_t &lhs, const read_set_item_t<T> &rhs) const {
   return lhs < rhs.ref->poffset;
 }
 template <typename T>
-inline bool read_set_item_t<T>::cmp_t::operator()(
+inline bool read_set_item_t<T>::extent_cmp_t::operator()(
   const read_set_item_t<T> &lhs, const paddr_t &rhs) const {
   return lhs.ref->poffset < rhs;
 }

src/crimson/os/seastore/lba_manager.h

@@ -50,17 +50,6 @@ class LBAManager {
     Transaction &t,
     laddr_t offset, extent_len_t length) = 0;
 
-  /**
-   * Fetches mappings for a list of laddr_t in range [offset, offset + len)
-   *
-   * Future will not resolve until all pins have resolved (set_paddr called)
-   * For indirect lba mappings, get_mappings will always retrieve the original
-   * lba value.
-   */
-  virtual get_mappings_ret get_mappings(
-    Transaction &t,
-    laddr_list_t &&extent_lisk) = 0;
-
   /**
    * Fetches the mapping for laddr_t
    *
@@ -120,20 +109,11 @@ class LBAManager {
   using ref_ret = ref_iertr::future<ref_update_result_t>;
 
   /**
-   * Decrements ref count on extent
-   *
-   * @return returns resulting refcount
-   */
-  virtual ref_ret decref_extent(
-    Transaction &t,
-    laddr_t addr) = 0;
-
-  /**
-   * Increments ref count on extent
+   * Removes a mapping and deal with indirection
    *
    * @return returns resulting refcount
    */
-  virtual ref_ret incref_extent(
+  virtual ref_ret remove_mapping(
     Transaction &t,
     laddr_t addr) = 0;
 

src/crimson/os/seastore/lba_manager/btree/btree_lba_manager.cc

@@ -242,31 +242,6 @@ BtreeLBAManager::_get_original_mappings(
   });
 }
 
-
-BtreeLBAManager::get_mappings_ret
-BtreeLBAManager::get_mappings(
-  Transaction &t,
-  laddr_list_t &&list)
-{
-  LOG_PREFIX(BtreeLBAManager::get_mappings);
-  TRACET("{}", t, list);
-  auto l = std::make_unique<laddr_list_t>(std::move(list));
-  auto retptr = std::make_unique<lba_pin_list_t>();
-  auto &ret = *retptr;
-  return trans_intr::do_for_each(
-    l->begin(),
-    l->end(),
-    [this, &t, &ret](const auto &p) {
-      return this->get_mappings(t, p.first, p.second).si_then(
-	[&ret](auto res) {
-	  ret.splice(ret.end(), res, res.begin(), res.end());
-	  return get_mappings_iertr::now();
-	});
-    }).si_then([l=std::move(l), retptr=std::move(retptr)]() mutable {
-      return std::move(*retptr);
-    });
-}
-
 BtreeLBAManager::get_mapping_ret
 BtreeLBAManager::get_mapping(
   Transaction &t,

src/crimson/os/seastore/lba_manager/btree/btree_lba_manager.h

@@ -269,15 +269,10 @@ class BtreeLBAManager : public LBAManager {
     Transaction &t,
     laddr_t offset, extent_len_t length) final;
 
-  get_mappings_ret get_mappings(
-    Transaction &t,
-    laddr_list_t &&list) final;
-
   get_mapping_ret get_mapping(
     Transaction &t,
     laddr_t offset) final;
 
-
   struct alloc_mapping_info_t {
     laddr_t key = L_ADDR_NULL; // once assigned, the allocation to
 			       // key must be exact and successful
@@ -430,7 +425,7 @@ class BtreeLBAManager : public LBAManager {
     });
   }
 
-  ref_ret decref_extent(
+  ref_ret remove_mapping(
     Transaction &t,
     laddr_t addr) final {
     return update_refcount(t, addr, -1, true
@@ -439,15 +434,6 @@ class BtreeLBAManager : public LBAManager {
     });
   }
 
-  ref_ret incref_extent(
-    Transaction &t,
-    laddr_t addr) final {
-    return update_refcount(t, addr, 1, false
-    ).si_then([](auto res) {
-      return std::move(res.ref_update_res);
-    });
-  }
-
   remap_ret remap_mappings(
     Transaction &t,
     LBAMappingRef orig_mapping,

src/crimson/os/seastore/linked_tree_node.h

@@ -302,9 +302,9 @@ class ParentNode {
   TCachedExtentRef<T> find_pending_version(Transaction &t, node_key_t key) {
     auto &me = down_cast();
     assert(me.is_stable());
-    auto mut_iter = me.mutation_pendings.find(
+    auto mut_iter = me.mutation_pending_extents.find(
       t.get_trans_id(), trans_spec_view_t::cmp_t());
-    if (mut_iter != me.mutation_pendings.end()) {
+    if (mut_iter != me.mutation_pending_extents.end()) {
       assert(copy_dests_by_trans.find(t.get_trans_id()) ==
 	copy_dests_by_trans.end());
       return static_cast<T*>(&(*mut_iter));