From 6f8ff6e9b4cc2d0f77d9e83e546cd679bc89f675 Mon Sep 17 00:00:00 2001 From: Yingxin Cheng Date: Thu, 7 Nov 2024 14:03:59 +0800 Subject: [PATCH] crimson/os/seastore: delay setting bptr for all types of extent in case of read Signed-off-by: Yingxin Cheng --- src/crimson/common/fixed_kv_node_layout.h | 9 +++- src/crimson/os/seastore/btree/fixed_kv_node.h | 43 ++++++++++++++----- src/crimson/os/seastore/cached_extent.h | 18 +++++++- .../collection_manager/collection_flat_node.h | 2 + .../omap_manager/btree/omap_btree_node.h | 3 +- .../omap_manager/btree/omap_btree_node_impl.h | 40 +++++++++++++---- .../btree/string_kv_node_layout.h | 18 ++++++-- .../node_extent_manager/seastore.h | 4 +- src/test/crimson/seastore/test_block.h | 10 +++-- src/test/crimson/test_fixed_kv_node_layout.cc | 6 ++- 10 files changed, 119 insertions(+), 34 deletions(-) diff --git a/src/crimson/common/fixed_kv_node_layout.h b/src/crimson/common/fixed_kv_node_layout.h index 2a91ac3954006..db62a2df32dd6 100644 --- a/src/crimson/common/fixed_kv_node_layout.h +++ b/src/crimson/common/fixed_kv_node_layout.h @@ -360,11 +360,16 @@ public: } - FixedKVNodeLayout(char *buf) : - buf(buf) {} + FixedKVNodeLayout() : buf(nullptr) {} virtual ~FixedKVNodeLayout() = default; + void set_layout_buf(char *_buf) { + assert(buf == nullptr); + assert(_buf != nullptr); + buf = _buf; + } + const_iterator begin() const { return const_iterator( this, diff --git a/src/crimson/os/seastore/btree/fixed_kv_node.h b/src/crimson/os/seastore/btree/fixed_kv_node.h index 1f1b37c3d81ea..63e2ca38c427d 100644 --- a/src/crimson/os/seastore/btree/fixed_kv_node.h +++ b/src/crimson/os/seastore/btree/fixed_kv_node.h @@ -165,6 +165,11 @@ struct FixedKVNode : ChildableCachedExtent { : ChildableCachedExtent(std::move(ptr)), children(capacity, nullptr), capacity(capacity) {} + // Must be identical with FixedKVNode(capacity, ptr) after on_fully_loaded() + explicit FixedKVNode(uint16_t capacity, extent_len_t length) + : ChildableCachedExtent(length), + children(capacity, nullptr), + capacity(capacity) {} FixedKVNode(const FixedKVNode &rhs) : ChildableCachedExtent(rhs), range(rhs.range), @@ -708,12 +713,17 @@ struct FixedKVInternalNode node_size, node_type_t>; - FixedKVInternalNode(ceph::bufferptr &&ptr) - : FixedKVNode(CAPACITY, std::move(ptr)), - node_layout_t(this->get_bptr().c_str()) {} + explicit FixedKVInternalNode(ceph::bufferptr &&ptr) + : FixedKVNode(CAPACITY, std::move(ptr)) { + this->set_layout_buf(this->get_bptr().c_str()); + } + // Must be identical with FixedKVInternalNode(ptr) after on_fully_loaded() + explicit FixedKVInternalNode(extent_len_t length) + : FixedKVNode(CAPACITY, length) {} FixedKVInternalNode(const FixedKVInternalNode &rhs) - : FixedKVNode(rhs), - node_layout_t(this->get_bptr().c_str()) {} + : FixedKVNode(rhs) { + this->set_layout_buf(this->get_bptr().c_str()); + } bool have_children() const final { return true; @@ -985,6 +995,10 @@ struct FixedKVInternalNode pivot); } + void on_fully_loaded() final { + this->set_layout_buf(this->get_bptr().c_str()); + } + /** * Internal relative addresses on read or in memory prior to commit * are either record or block relative depending on whether this @@ -1121,13 +1135,18 @@ struct FixedKVLeafNode node_type_t, has_children>; using base_t = FixedKVNode; - FixedKVLeafNode(ceph::bufferptr &&ptr) - : FixedKVNode(has_children ? CAPACITY : 0, std::move(ptr)), - node_layout_t(this->get_bptr().c_str()) {} + explicit FixedKVLeafNode(ceph::bufferptr &&ptr) + : FixedKVNode(has_children ? CAPACITY : 0, std::move(ptr)) { + this->set_layout_buf(this->get_bptr().c_str()); + } + // Must be identical with FixedKVLeafNode(ptr) after on_fully_loaded() + explicit FixedKVLeafNode(extent_len_t length) + : FixedKVNode(has_children ? CAPACITY : 0, length) {} FixedKVLeafNode(const FixedKVLeafNode &rhs) : FixedKVNode(rhs), - node_layout_t(this->get_bptr().c_str()), - modifications(rhs.modifications) {} + modifications(rhs.modifications) { + this->set_layout_buf(this->get_bptr().c_str()); + } static constexpr bool do_has_children = has_children; // for the stable extent, modifications is always 0; @@ -1234,6 +1253,10 @@ struct FixedKVLeafNode } } + void on_fully_loaded() final { + this->set_layout_buf(this->get_bptr().c_str()); + } + void prepare_commit() final { if constexpr (has_children) { if (this->is_initial_pending()) { diff --git a/src/crimson/os/seastore/cached_extent.h b/src/crimson/os/seastore/cached_extent.h index 07db3bd488e11..0df66e37d1169 100644 --- a/src/crimson/os/seastore/cached_extent.h +++ b/src/crimson/os/seastore/cached_extent.h @@ -348,6 +348,17 @@ public: */ virtual void on_initial_write() {} + /** + * on_fully_loaded + * + * Called when ptr is ready. Normally this should be used to initiate + * the extent to be identical to CachedExtent(ptr). + * + * Note this doesn't mean the content is fully read, use on_clean_read for + * this purpose. + */ + virtual void on_fully_loaded() {} + /** * on_clean_read * @@ -880,7 +891,7 @@ protected: CachedExtent(CachedExtent &&other) = delete; /// construct a fully loaded CachedExtent - CachedExtent(ceph::bufferptr &&_ptr) + explicit CachedExtent(ceph::bufferptr &&_ptr) : length(_ptr.length()), loaded_length(_ptr.length()) { ptr = std::move(_ptr); @@ -892,7 +903,8 @@ protected: } /// construct a partially loaded CachedExtent - CachedExtent(extent_len_t _length) + /// must be identical with CachedExtent(ptr) after on_fully_loaded() + explicit CachedExtent(extent_len_t _length) : length(_length), loaded_length(0), buffer_space(std::in_place) { @@ -1048,6 +1060,7 @@ protected: loaded_length = _length; buffer_space.reset(); assert(is_fully_loaded()); + on_fully_loaded(); load_ranges_t ret; ret.push_back(offset, *ptr); return ret; @@ -1061,6 +1074,7 @@ protected: ptr = buffer_space->to_full_ptr(length); buffer_space.reset(); assert(is_fully_loaded()); + on_fully_loaded(); // adjust ret since the ptr has been rebuild for (load_range_t& range : ret.ranges) { auto range_length = range.ptr.length(); diff --git a/src/crimson/os/seastore/collection_manager/collection_flat_node.h b/src/crimson/os/seastore/collection_manager/collection_flat_node.h index aa1e713561308..1f4de652bba36 100644 --- a/src/crimson/os/seastore/collection_manager/collection_flat_node.h +++ b/src/crimson/os/seastore/collection_manager/collection_flat_node.h @@ -96,6 +96,8 @@ struct CollectionNode explicit CollectionNode(ceph::bufferptr &&ptr) : LogicalCachedExtent(std::move(ptr)) {} + explicit CollectionNode(extent_len_t length) + : LogicalCachedExtent(length) {} explicit CollectionNode(const CollectionNode &other) : LogicalCachedExtent(other), decoded(other.decoded) {} diff --git a/src/crimson/os/seastore/omap_manager/btree/omap_btree_node.h b/src/crimson/os/seastore/omap_manager/btree/omap_btree_node.h index 795daeddb11c5..7c2392731c0e8 100644 --- a/src/crimson/os/seastore/omap_manager/btree/omap_btree_node.h +++ b/src/crimson/os/seastore/omap_manager/btree/omap_btree_node.h @@ -48,7 +48,8 @@ struct OMapNode : LogicalCachedExtent { need_merge(n_merge) {} }; - OMapNode(ceph::bufferptr &&ptr) : LogicalCachedExtent(std::move(ptr)) {} + explicit OMapNode(ceph::bufferptr &&ptr) : LogicalCachedExtent(std::move(ptr)) {} + explicit OMapNode(extent_len_t length) : LogicalCachedExtent(length) {} OMapNode(const OMapNode &other) : LogicalCachedExtent(other) {} diff --git a/src/crimson/os/seastore/omap_manager/btree/omap_btree_node_impl.h b/src/crimson/os/seastore/omap_manager/btree/omap_btree_node_impl.h index a2b51bbb0e135..2267942f0355a 100644 --- a/src/crimson/os/seastore/omap_manager/btree/omap_btree_node_impl.h +++ b/src/crimson/os/seastore/omap_manager/btree/omap_btree_node_impl.h @@ -31,10 +31,18 @@ struct OMapInnerNode StringKVInnerNodeLayout { using OMapInnerNodeRef = TCachedExtentRef; using internal_iterator_t = const_iterator; - template - OMapInnerNode(T&&... t) : - OMapNode(std::forward(t)...), - StringKVInnerNodeLayout(get_bptr().c_str()) {} + + explicit OMapInnerNode(ceph::bufferptr &&ptr) + : OMapNode(std::move(ptr)) { + this->set_layout_buf(this->get_bptr().c_str()); + } + // Must be identical with OMapInnerNode(ptr) after on_fully_loaded() + explicit OMapInnerNode(extent_len_t length) + : OMapNode(length) {} + OMapInnerNode(const OMapInnerNode &rhs) + : OMapNode(rhs) { + this->set_layout_buf(this->get_bptr().c_str()); + } omap_node_meta_t get_node_meta() const final { return get_meta(); } bool extent_will_overflow(size_t ksize, std::optional vsize) const { @@ -46,6 +54,10 @@ struct OMapInnerNode bool extent_is_below_min() const { return below_min(); } uint32_t get_node_size() { return get_size(); } + void on_fully_loaded() final { + this->set_layout_buf(this->get_bptr().c_str()); + } + CachedExtentRef duplicate_for_write(Transaction&) final { assert(delta_buffer.empty()); return CachedExtentRef(new OMapInnerNode(*this)); @@ -148,10 +160,18 @@ struct OMapLeafNode using OMapLeafNodeRef = TCachedExtentRef; using internal_iterator_t = const_iterator; - template - OMapLeafNode(T&&... t) : - OMapNode(std::forward(t)...), - StringKVLeafNodeLayout(get_bptr().c_str()) {} + + explicit OMapLeafNode(ceph::bufferptr &&ptr) + : OMapNode(std::move(ptr)) { + this->set_layout_buf(this->get_bptr().c_str()); + } + // Must be identical with OMapLeafNode(ptr) after on_fully_loaded() + explicit OMapLeafNode(extent_len_t length) + : OMapNode(length) {} + OMapLeafNode(const OMapLeafNode &rhs) + : OMapNode(rhs) { + this->set_layout_buf(this->get_bptr().c_str()); + } omap_node_meta_t get_node_meta() const final { return get_meta(); } bool extent_will_overflow( @@ -164,6 +184,10 @@ struct OMapLeafNode bool extent_is_below_min() const { return below_min(); } uint32_t get_node_size() { return get_size(); } + void on_fully_loaded() final { + this->set_layout_buf(this->get_bptr().c_str()); + } + CachedExtentRef duplicate_for_write(Transaction&) final { assert(delta_buffer.empty()); return CachedExtentRef(new OMapLeafNode(*this)); diff --git a/src/crimson/os/seastore/omap_manager/btree/string_kv_node_layout.h b/src/crimson/os/seastore/omap_manager/btree/string_kv_node_layout.h index 72b13fedfb12b..3825ebef14553 100644 --- a/src/crimson/os/seastore/omap_manager/btree/string_kv_node_layout.h +++ b/src/crimson/os/seastore/omap_manager/btree/string_kv_node_layout.h @@ -504,8 +504,13 @@ public: inner_remove(iter); } - StringKVInnerNodeLayout(char *buf) : - buf(buf) {} + StringKVInnerNodeLayout() : buf(nullptr) {} + + void set_layout_buf(char *_buf) { + assert(buf == nullptr); + assert(_buf != nullptr); + buf = _buf; + } uint32_t get_size() const { ceph_le32 &size = *layout.template Pointer<0>(buf); @@ -1120,8 +1125,13 @@ public: leaf_remove(iter); } - StringKVLeafNodeLayout(char *buf) : - buf(buf) {} + StringKVLeafNodeLayout() : buf(nullptr) {} + + void set_layout_buf(char *_buf) { + assert(buf == nullptr); + assert(_buf != nullptr); + buf = _buf; + } const_iterator iter_begin() const { return const_iterator( diff --git a/src/crimson/os/seastore/onode_manager/staged-fltree/node_extent_manager/seastore.h b/src/crimson/os/seastore/onode_manager/staged-fltree/node_extent_manager/seastore.h index b5d674ffcaaaa..04b959f767de9 100644 --- a/src/crimson/os/seastore/onode_manager/staged-fltree/node_extent_manager/seastore.h +++ b/src/crimson/os/seastore/onode_manager/staged-fltree/node_extent_manager/seastore.h @@ -41,8 +41,10 @@ class SeastoreSuper final: public Super { class SeastoreNodeExtent final: public NodeExtent { public: - SeastoreNodeExtent(ceph::bufferptr &&ptr) + explicit SeastoreNodeExtent(ceph::bufferptr &&ptr) : NodeExtent(std::move(ptr)) {} + explicit SeastoreNodeExtent(extent_len_t length) + : NodeExtent(length) {} SeastoreNodeExtent(const SeastoreNodeExtent& other) : NodeExtent(other) {} ~SeastoreNodeExtent() override = default; diff --git a/src/test/crimson/seastore/test_block.h b/src/test/crimson/seastore/test_block.h index fde6ad99c4142..e1fe8e06f8a46 100644 --- a/src/test/crimson/seastore/test_block.h +++ b/src/test/crimson/seastore/test_block.h @@ -51,12 +51,12 @@ struct TestBlock : crimson::os::seastore::LogicalCachedExtent { interval_set modified_region; - TestBlock(ceph::bufferptr &&ptr) + explicit TestBlock(ceph::bufferptr &&ptr) : LogicalCachedExtent(std::move(ptr)) {} + explicit TestBlock(extent_len_t length) + : LogicalCachedExtent(length) {} TestBlock(const TestBlock &other) : LogicalCachedExtent(other), modified_region(other.modified_region) {} - TestBlock(extent_len_t length) - : LogicalCachedExtent(length) {} CachedExtentRef duplicate_for_write(Transaction&) final { return CachedExtentRef(new TestBlock(*this)); @@ -113,8 +113,10 @@ struct TestBlockPhysical : crimson::os::seastore::CachedExtent{ void on_rewrite(Transaction&, CachedExtent&, extent_len_t) final {} - TestBlockPhysical(ceph::bufferptr &&ptr) + explicit TestBlockPhysical(ceph::bufferptr &&ptr) : CachedExtent(std::move(ptr)) {} + explicit TestBlockPhysical(extent_len_t length) + : CachedExtent(length) {} TestBlockPhysical(const TestBlockPhysical &other) : CachedExtent(other) {} diff --git a/src/test/crimson/test_fixed_kv_node_layout.cc b/src/test/crimson/test_fixed_kv_node_layout.cc index e6377ec14e3cf..9b6d19661ac1e 100644 --- a/src/test/crimson/test_fixed_kv_node_layout.cc +++ b/src/test/crimson/test_fixed_kv_node_layout.cc @@ -88,12 +88,14 @@ struct TestNode : FixedKVNodeLayout< uint32_t, ceph_le32, test_val_t, test_val_le_t> { char buf[4096]; - TestNode() : FixedKVNodeLayout(buf) { + TestNode() : FixedKVNodeLayout() { + set_layout_buf(buf); memset(buf, 0, sizeof(buf)); set_meta({0, std::numeric_limits::max()}); } TestNode(const TestNode &rhs) - : FixedKVNodeLayout(buf) { + : FixedKVNodeLayout() { + set_layout_buf(buf); ::memcpy(buf, rhs.buf, sizeof(buf)); } -- 2.39.5