From aba5a77872f30a86a673f889627bb37f3f080cdd Mon Sep 17 00:00:00 2001 From: Xuehan Xu Date: Mon, 22 Jun 2026 17:48:15 +0800 Subject: [PATCH] crimson/os/seastore/lba: make sure all mappings in the copied range is examined when updating paddrs for the copied mappings Signed-off-by: Xuehan Xu --- .../os/seastore/btree/fixed_kv_btree.h | 61 ++++++++++ .../os/seastore/lba/btree_lba_manager.cc | 110 +++++++++++------- .../os/seastore/lba/btree_lba_manager.h | 1 + src/crimson/os/seastore/linked_tree_node.h | 1 - src/crimson/os/seastore/transaction.h | 17 ++- 5 files changed, 145 insertions(+), 45 deletions(-) diff --git a/src/crimson/os/seastore/btree/fixed_kv_btree.h b/src/crimson/os/seastore/btree/fixed_kv_btree.h index 59b7a150ee02..c9cc522cd54c 100644 --- a/src/crimson/os/seastore/btree/fixed_kv_btree.h +++ b/src/crimson/os/seastore/btree/fixed_kv_btree.h @@ -216,6 +216,21 @@ public: } + // return true if reached the next mapping and false otherwise + bool next_sync(op_context_t c) { +#ifndef NDEBUG + assert_valid(); +#endif + assert(is_full()); + assert(!is_end()); + leaf.pos++; + if (at_boundary()) { + return handle_boundary_sync(c); + } else { + return true; + } + } + /** * Move to the previous entry. If already at position 0 in the current * leaf, walks up the internal stack (ensure_internal_bottom_up) to find @@ -520,6 +535,50 @@ public: }); } + // return true if reached the next mapping and false otherwise + bool handle_boundary_sync(op_context_t c) { + assert(at_boundary()); + assert(is_full()); + depth_t depth = 2; + // find the first level (from the leaf's parent up) with a right sibling + for (; depth <= get_depth(); depth++) { + auto &internal = get_internal(depth); + auto it = internal.node->iter_idx(internal.pos + 1); + if (it != internal.node->end()) { + break; + } + } + if (depth > get_depth()) { + return false; // no next leaf (end) + } + // step to the right sibling at that level, then descend leftmost to the leaf + get_internal(depth).pos++; + while (depth > 2) { + auto &internal = get_internal(depth); + auto it = get_internal(depth).node->iter_idx(internal.pos); + auto child = get_internal(depth).node->template get_child_sync< + internal_node_t>(c.trans, c.cache, internal.pos, it.get_key()); + if (!is_valid_child_ptr(child.get())) { + return false; + } + depth--; + get_internal(depth).node = child; + get_internal(depth).pos = 0; + } + assert(depth == 2); + // position `leaf` at the next leaf + auto &parent = get_internal(depth); + auto it = parent.node->begin(); + auto child = parent.node->template get_child_sync< + leaf_node_t>(c.trans, c.cache, parent.pos, it.get_key()); + if (!is_valid_child_ptr(child.get())) { + return false; + } + leaf.node = child; + leaf.pos = 0; + return true; + } + /** * Called when leaf.pos has advanced past the end of the current leaf * (at_boundary() == true). Walks up the internal stack to find the @@ -763,10 +822,12 @@ public: if (depth > 1) { auto child = entry.node->template get_child_sync( c.trans, c.cache, entry.pos, riter.get_key()); + ceph_assert(is_valid_child_ptr(child.get())); iter.get_internal(depth).node = child; } else { auto child = entry.node->template get_child_sync( c.trans, c.cache, entry.pos, riter.get_key()); + ceph_assert(is_valid_child_ptr(child.get())); iter.leaf.node = child; } } diff --git a/src/crimson/os/seastore/lba/btree_lba_manager.cc b/src/crimson/os/seastore/lba/btree_lba_manager.cc index 10cb09f54907..843861a299b0 100644 --- a/src/crimson/os/seastore/lba/btree_lba_manager.cc +++ b/src/crimson/os/seastore/lba/btree_lba_manager.cc @@ -1496,6 +1496,7 @@ void BtreeLBAManager::update_paddr_sync( Transaction &t, laddr_t laddr, paddr_t paddr, + extent_len_t len, std::optional shadow) { LOG_PREFIX(BtreeLBAManager::update_paddr_sync); @@ -1503,44 +1504,73 @@ void BtreeLBAManager::update_paddr_sync( auto c = get_context(t); auto btree = get_btree_sync(c); auto iter = btree.lower_bound_sync(c, laddr); - assert(iter.get_leaf_node()->is_pending()); - auto child = iter.get_leaf_node()->get_child_sync( - c.trans, c.cache, iter.get_leaf_pos(), iter.get_key()); - ceph_assert(child); - if (child->is_initial_pending()) { - TRACET("{} is initial_pending, skipping", t, *child); - return; - } - ceph_assert(child->is_exist_clean()); - auto cursor = iter.get_cursor(c); - assert(cursor->get_laddr() == laddr); - paddr_t shadow_paddr; - if (shadow) { - // the committing txn changed the shadow - // to *shadow - shadow_paddr = *shadow; - } else if (cursor->has_shadow_paddr()) { - // shadow is preserved by the committer, - // and the copy inherited one, so the source - // was promoted when it's copied - shadow_paddr = cursor->get_shadow_paddr(); - } else { - // shadow is preserved, and nothing is - // inherited: not currently promoted - shadow_paddr = P_ADDR_NULL; + while (iter.get_key() + iter.get_val().len <= laddr + len) { + assert(iter.get_leaf_node()->is_pending()); + if (iter.get_val().pladdr.is_laddr() || + iter.get_val().pladdr.get_paddr().is_zero()) { + TRACET("skipping mapping {}~{}", t, iter.get_key(), iter.get_val()); + if (!iter.next_sync(c)) { + // can't reach the next mapping, which means the next + // mapping mustn't have been touched by the current + // transactions, we don't need to continue, just leave + return; + } + continue; + } + auto child = iter.get_leaf_node()->get_child_sync( + c.trans, c.cache, iter.get_leaf_pos(), iter.get_key()); + ceph_assert(is_valid_child_ptr(child.get())); + if (child->is_initial_pending()) { + TRACET("{} is initial_pending, skipping", t, *child); + if (!iter.next_sync(c)) { + // can't reach the next mapping, which means the next + // mapping mustn't have been touched by the current + // transactions, we don't need to continue, just leave + return; + } + continue; + } + ceph_assert(child->is_exist_clean()); + auto cursor = iter.get_cursor(c); + extent_len_t off = cursor->get_laddr().get_byte_distance< + extent_len_t>(laddr); + paddr_t shadow_paddr; + if (shadow) { + // the committing txn changed the shadow + // to *shadow + shadow_paddr = *shadow; + if (shadow_paddr != P_ADDR_NULL) { + shadow_paddr = shadow_paddr + off; + } + } else if (cursor->has_shadow_paddr()) { + // shadow is preserved by the committer, + // and the copy inherited one, so the source + // was promoted when it's copied + shadow_paddr = cursor->get_shadow_paddr(); + } else { + // shadow is preserved, and nothing is + // inherited: not currently promoted + shadow_paddr = P_ADDR_NULL; + } + iter = btree.update( + c, + std::move(iter), + lba_map_val_t{ + cursor->get_length(), + pladdr_t{paddr + off}, + shadow_paddr, + cursor->get_refcount(), + cursor->get_checksum(), + cursor->get_extent_type()}, + nullptr, + modification_t::TRANS_SYNC); + if (!iter.next_sync(c)) { + // can't reach the next mapping, which means the next + // mapping mustn't have been touched by the current + // transactions, we don't need to continue, just leave + return; + } } - btree.update( - c, - std::move(iter), - lba_map_val_t{ - cursor->get_length(), - pladdr_t{std::move(paddr)}, - shadow_paddr, - cursor->get_refcount(), - cursor->get_checksum(), - cursor->get_extent_type()}, - nullptr, - modification_t::TRANS_SYNC); } // --------------------------------------------------------------------------- @@ -1594,8 +1624,10 @@ BtreeLBAManager::_copy_mapping( c.trans.new_lba_key_copied( ret.src->get_key(), dest_laddr, - [this, c](laddr_t laddr, paddr_t paddr, std::optional shadow) { - update_paddr_sync(c.trans, laddr, paddr, shadow); + ret.src->get_length(), + [this, c](laddr_t laddr, paddr_t paddr, + extent_len_t len, std::optional shadow) { + update_paddr_sync(c.trans, laddr, paddr, len, shadow); }); auto [niter, inserted] = co_await btree.copy( c, diff --git a/src/crimson/os/seastore/lba/btree_lba_manager.h b/src/crimson/os/seastore/lba/btree_lba_manager.h index 1bbd3a07ef27..ae8e88c99043 100644 --- a/src/crimson/os/seastore/lba/btree_lba_manager.h +++ b/src/crimson/os/seastore/lba/btree_lba_manager.h @@ -618,6 +618,7 @@ private: Transaction &t, laddr_t laddr, paddr_t paddr, + extent_len_t len, std::optional shadow); diff --git a/src/crimson/os/seastore/linked_tree_node.h b/src/crimson/os/seastore/linked_tree_node.h index c9cf58902bab..0e500305357f 100644 --- a/src/crimson/os/seastore/linked_tree_node.h +++ b/src/crimson/os/seastore/linked_tree_node.h @@ -350,7 +350,6 @@ public: auto &sparent = me.get_stable_for_key(key); auto spos = sparent.lower_bound(key).get_offset(); child = sparent.children[spos]; - assert(is_valid_child_ptr(child)); auto ret = etvr.get_extent_viewable_by_trans_sync( t, static_cast(child)); return ret->template cast(); diff --git a/src/crimson/os/seastore/transaction.h b/src/crimson/os/seastore/transaction.h index ceab4491e6da..c256e5b8f997 100644 --- a/src/crimson/os/seastore/transaction.h +++ b/src/crimson/os/seastore/transaction.h @@ -747,13 +747,19 @@ public: bool force_rewrite_conflict = false; + struct lmapping_t { + laddr_t dest = L_ADDR_NULL; + extent_len_t len = 0; + }; using update_copied_lba_key_func_t = - std::function)>; + std::function)>; void new_lba_key_copied( laddr_t src, laddr_t dest, + extent_len_t len, update_copied_lba_key_func_t &&func) { - copied_lba_keys.emplace(src, dest); + copied_lba_keys.emplace(src, lmapping_t{dest, len}); if (!update_copied_lba_key) { update_copied_lba_key = std::move(func); } @@ -768,8 +774,9 @@ public: if (it == copied_lba_keys.end()) { return; } - laddr_t key = it->second; - update_copied_lba_key(key, paddr, shadow); + laddr_t key = it->second.dest; + extent_len_t len = it->second.len; + update_copied_lba_key(key, paddr, len, shadow); } RootBlockRef peek_root() { return root; @@ -1002,7 +1009,7 @@ private: cache_hint_t cache_hint = CACHE_HINT_TOUCH; - std::map copied_lba_keys; + std::map copied_lba_keys; update_copied_lba_key_func_t update_copied_lba_key; }; using TransactionRef = Transaction::Ref; -- 2.47.3