Skip to content

Commit

Permalink
Merge pull request ceph#56246 from xxhdx1985126/wip-64957
Browse files Browse the repository at this point in the history
crimson/os/seastore/btree: check for reserved ptrs when determining children stability

Reviewed-by: Yingxin Cheng <[email protected]>
Reviewed-by: Chunmei Liu <[email protected]>
  • Loading branch information
cyx1231st authored Mar 22, 2024
2 parents 8282158 + 7dcd74a commit e143d4a
Show file tree
Hide file tree
Showing 6 changed files with 48 additions and 17 deletions.
2 changes: 1 addition & 1 deletion src/crimson/os/seastore/btree/btree_range_pin.cc
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ bool BtreeNodeMapping<key_t, val_t>::is_stable() const
assert(parent->is_valid());
assert(pos != std::numeric_limits<uint16_t>::max());
auto &p = (FixedKVNode<key_t>&)*parent;
return p.is_child_stable(pos);
return p.is_child_stable(ctx, pos);
}

template class BtreeNodeMapping<laddr_t, paddr_t>;
Expand Down
8 changes: 6 additions & 2 deletions src/crimson/os/seastore/btree/fixed_kv_btree.h
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,6 @@
#include "crimson/os/seastore/btree/btree_range_pin.h"
#include "crimson/os/seastore/root_block.h"

#define RESERVATION_PTR reinterpret_cast<ChildableCachedExtent*>(0x1)

namespace crimson::os::seastore::lba_manager::btree {
struct lba_map_val_t;
}
Expand All @@ -25,6 +23,12 @@ namespace crimson::os::seastore {

bool is_valid_child_ptr(ChildableCachedExtent* child);

bool is_reserved_ptr(ChildableCachedExtent* child);

inline ChildableCachedExtent* get_reserved_ptr() {
return (ChildableCachedExtent*)0x1;
}

template <typename T>
phy_tree_root_t& get_phy_tree_root(root_t& r);

Expand Down
6 changes: 5 additions & 1 deletion src/crimson/os/seastore/btree/fixed_kv_node.cc
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,11 @@
namespace crimson::os::seastore {

bool is_valid_child_ptr(ChildableCachedExtent* child) {
return child != nullptr && child != RESERVATION_PTR;
return child != nullptr && child != get_reserved_ptr();
}

bool is_reserved_ptr(ChildableCachedExtent* child) {
return child == get_reserved_ptr();
}

} // namespace crimson::os::seastore
25 changes: 17 additions & 8 deletions src/crimson/os/seastore/btree/fixed_kv_node.h
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ struct FixedKVNode : ChildableCachedExtent {
// copy sources when committing this lba node, because
// we rely on pointers' "nullness" to avoid copying
// pointers for updated values
children[offset] = RESERVATION_PTR;
children[offset] = get_reserved_ptr();
}
}

Expand Down Expand Up @@ -229,13 +229,14 @@ struct FixedKVNode : ChildableCachedExtent {
virtual get_child_ret_t<LogicalCachedExtent>
get_logical_child(op_context_t<node_key_t> c, uint16_t pos) = 0;

virtual bool is_child_stable(uint16_t pos) const = 0;
virtual bool is_child_stable(op_context_t<node_key_t>, uint16_t pos) const = 0;

template <typename T, typename iter_t>
get_child_ret_t<T> get_child(op_context_t<node_key_t> c, iter_t iter) {
auto pos = iter.get_offset();
assert(children.capacity());
auto child = children[pos];
ceph_assert(!is_reserved_ptr(child));
if (is_valid_child_ptr(child)) {
ceph_assert(child->get_type() == T::TYPE);
return c.cache.template get_extent_viewable_by_trans<T>(c.trans, (T*)child);
Expand Down Expand Up @@ -431,7 +432,7 @@ struct FixedKVNode : ChildableCachedExtent {
if (!child) {
child = source.children[foreign_it.get_offset()];
// child can be either valid if present, nullptr if absent,
// or RESERVATION_PTR.
// or reserved ptr.
}
foreign_it++;
local_it++;
Expand Down Expand Up @@ -596,7 +597,7 @@ struct FixedKVInternalNode
return get_child_ret_t<LogicalCachedExtent>(child_pos_t(nullptr, 0));
}

bool is_child_stable(uint16_t pos) const final {
bool is_child_stable(op_context_t<NODE_KEY>, uint16_t pos) const final {
ceph_abort("impossible");
return false;
}
Expand Down Expand Up @@ -972,6 +973,7 @@ struct FixedKVLeafNode
get_child_ret_t<LogicalCachedExtent>
get_logical_child(op_context_t<NODE_KEY> c, uint16_t pos) final {
auto child = this->children[pos];
ceph_assert(!is_reserved_ptr(child));
if (is_valid_child_ptr(child)) {
ceph_assert(child->is_logical());
return c.cache.template get_extent_viewable_by_trans<
Expand All @@ -996,19 +998,26 @@ struct FixedKVLeafNode
// children are considered stable if any of the following case is true:
// 1. The child extent is absent in cache
// 2. The child extent is stable
bool is_child_stable(uint16_t pos) const final {
//
// For reserved mappings, the return values are undefined.
bool is_child_stable(op_context_t<NODE_KEY> c, uint16_t pos) const final {
auto child = this->children[pos];
if (is_valid_child_ptr(child)) {
if (is_reserved_ptr(child)) {
return true;
} else if (is_valid_child_ptr(child)) {
ceph_assert(child->is_logical());
return child->is_stable();
ceph_assert(
child->is_pending_in_trans(c.trans.get_trans_id())
|| this->is_stable_written());
return c.cache.is_viewable_extent_stable(c.trans, child);
} else if (this->is_pending()) {
auto key = this->iter_idx(pos).get_key();
auto &sparent = this->get_stable_for_key(key);
auto spos = sparent.child_pos_for_key(key);
auto child = sparent.children[spos];
if (is_valid_child_ptr(child)) {
ceph_assert(child->is_logical());
return child->is_stable();
return c.cache.is_viewable_extent_stable(c.trans, child);
} else {
return true;
}
Expand Down
9 changes: 9 additions & 0 deletions src/crimson/os/seastore/cache.h
Original file line number Diff line number Diff line change
Expand Up @@ -443,6 +443,15 @@ class Cache {
return get_absent_extent<T>(t, offset, length, [](T &){});
}

bool is_viewable_extent_stable(
Transaction &t,
CachedExtentRef extent)
{
assert(extent);
auto view = extent->get_transactional_view(t);
return view->is_stable();
}

using get_extent_ertr = base_ertr;
get_extent_ertr::future<CachedExtentRef>
get_extent_viewable_by_trans(
Expand Down
15 changes: 10 additions & 5 deletions src/crimson/os/seastore/cached_extent.h
Original file line number Diff line number Diff line change
Expand Up @@ -426,6 +426,9 @@ class CachedExtent
/// 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());
}
Expand Down Expand Up @@ -615,6 +618,11 @@ class CachedExtent
return last_committed_crc;
}

/// Returns true if the extent part of the open transaction
bool is_pending_in_trans(transaction_id_t id) const {
return is_pending() && pending_for_transaction == id;
}

private:
template <typename T>
friend class read_set_item_t;
Expand Down Expand Up @@ -645,11 +653,6 @@ class CachedExtent
ptr = nptr;
}

/// Returns true if the extent part of the open transaction
bool is_pending_in_trans(transaction_id_t id) const {
return is_pending() && pending_for_transaction == id;
}

/// 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<
Expand Down Expand Up @@ -1062,6 +1065,8 @@ class PhysicalNodeMapping {
child_pos->link_child(c);
}

// For reserved mappings, the return values are
// undefined although it won't crash
virtual bool is_stable() const = 0;
virtual bool is_clone() const = 0;
bool is_zero_reserved() const {
Expand Down

0 comments on commit e143d4a

Please sign in to comment.