From 96f4aade8777c4e7e62289b3e1f8655176e838f6 Mon Sep 17 00:00:00 2001 From: Jie Yao Date: Fri, 24 Jul 2026 14:05:44 +0800 Subject: [PATCH] SDSTOR-22729: index/wb_cache: Fix recovery corruption after root-split crash MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A B-tree root split (tree height N → N+1) proceeds in three steps: 1. Allocate new_root and call on_root_changed(new_root) — updates the in-memory superblock (SB) and calls transact_bufs(meta_buf, new_root_buf), which links meta_buf → new_root_buf in the CP flush DAG. 2. split_node(new_root, old_root) modifies old_root in memory (edge_info = EMPTY, next_bnode = child_node2) and calls transact_nodes({child_node2}, {}, old_root, new_root), which invokes link_buf(new_root_buf, old_root_buf). 3. The on-disk SB is only written at the very end of CP flush, after all node buffers complete. Three bugs combined to corrupt the tree when a SIGKILL landed after step 2 but before the SB write: Bug A — link_buf Condition 1 created a flat flush DAG Condition 1 bypassed new_root_buf whenever it was newly created in the current CP, regardless of whether old_root_buf was new or old. As a result, old_root_buf linked directly to meta_buf instead of through new_root_buf, making new_root_buf and old_root_buf siblings under meta_buf with no ordering between them. old_root could therefore reach disk in its transient split state (edge_info=EMPTY, next_bnode=child_node2) before new_root, widening the crash window. Bug B — Recovery discarded a committed new_root When the SB write did not complete, the recovery loop saw new_root_buf's parent (meta_buf) as uncommitted and threw new_root_buf away. The persisted SB still pointed to old_root. Bug C — repair_root_node applied an unsafe edge repair With old_root as the recovered tree root, repair_root_node read old_root.next_bnode (= child_node2, level N) and set it as old_root's edge child (also level N). This violated the B-tree invariant child.level == parent.level − 1, causing validate_node to abort with "Child node level mismatch" on the next B-tree access. The same bug sequence applies to the first root split (leaf → level=1) as well as any subsequent split. Fix 1 — Restore correct flush-DAG ordering (covers Bug A) Changed link_buf Condition 1 in wb_cache.cpp to bypass up_buf only when BOTH up_buf AND down_buf were created in the current CP. When down_buf is an older node (e.g. old_root), the dependency chain through up_buf (new_root) is preserved, ensuring old_root cannot be flushed before new_root: Before: if (up_buf->m_created_cp_id == icp_ctx->id()) After: if (up_buf->m_created_cp_id == icp_ctx->id() && down_buf->m_created_cp_id == icp_ctx->id()) The same fix is mirrored in index_cp.cpp process_txn_record so that journal recovery rebuilds an identical DAG. The sanity check is tightened: only a buffer that was itself created in the current CP must not point to another same-CP-new up_buffer; older downs pointing to new ups are now valid. Fix 2 — Detect and promote committed new roots during recovery (covers Bug B) After the main recovery loop, newly introduced new_root_candidates tracking collects every committed node whose direct parent is an unwritten meta_buf, provided the index table already has a persisted SB root (has_persisted_root() == true). For each ordinal, the highest-level candidate is selected and set_root_from_committed_buf() is called to update the in-memory SB (root_node, root_link_version, btree_depth) and the btree's own root pointer before the next forced CP writes the corrected SB to disk. The first-CP path (SB root still empty) is intentionally excluded so that recovery_completed() can build a fresh root as before. Fix 3 — Harden repair_root_node fallback (covers Bug C) Added two guards before applying the edge repair: a. Buffer validity: skip if raw_buffer is null, the node magic is invalid, or node_id does not match blkid — the buffer was never written to disk. b. Level invariant: read the candidate next_bnode from disk; if its level is ≥ old_root's level (partial root-split state), skip the repair rather than corrupting the edge. A third early return handles the case where next_bnode is already empty, meaning new_root was already promoted and there is nothing to repair. Fix 4 — Complete node_id validation in set_root_from_committed_buf repair_root_node and repair_node both perform a three-way check (raw_buffer != nullptr, is_valid_node, node_id == blkid). The initial implementation of set_root_from_committed_buf checked only the first two. Added the node_id != blkid guard to match, preventing a disk block whose node magic is valid but whose node_id disagrees with its blkid from being promoted as the new tree root. - prune_up_buffers: guard against cascading into committed ancestors; only schedule durable (valid magic + matching node_id) buffers for repair. - update_up_buffer_counters: skip detaching committed, non-freed buffers from their parent so they remain available for new-root promotion. - recover_buf: return early for meta buffers (tracked separately via new_root_candidates; loading them as vdev nodes corrupts m_dirtied_cp_id). - read_buf: reject invalid or mismatched blocks before init_node to surface disk corruption early with a clear error instead of a debug assertion. - IndexBuffer::to_string: replace bare {...} delimiters with [...] to prevent fmt from misinterpreting them as format placeholders when the string is later re-parsed by HS_*_ASSERT_CMP macros. - btree_common.ipp: include next_bnode value in "Broken child linkage" message; use enum_name(ret) for node-read failure reasons. - Version bump: 7.5.14 → 7.5.15. Added two regression tests to IndexCrashTest: CrashAtMetaBufOnFirstRootSplit Phase 1: insert max_keys/2 entries and flush cleanly so the tree is at a stable leaf-only state (btree_depth=0, SB root = old_leaf). Phase 2: set crash_flush_on_meta, insert max_keys+1 more entries to force the first root split (leaf → level=1), trigger CP. The flip fires after new_root and old_leaf are both durable but before meta_buf writes, leaving the SB stale. Recovery: Fix 2 detects new_root (cand_level=1 > btree_depth=0) and promotes it; Fix 3 would catch candidate.level(0) >= old_leaf.level(0) if Fix 2 had not applied. Tree integrity verified via reapply_after_crash and get_all. CrashAtMetaBufOnSecondRootSplit Phase 1: insert max_keys+1 entries to trigger the first root split cleanly, then flush (tree now at level=1, SB up to date). Phase 2: set crash_flush_on_meta, insert max_keys² more entries to force the second root split (level=1 → level=2), trigger CP. Same crash mechanics as above. Recovery: same verification. --- conanfile.py | 2 +- .../homestore/btree/detail/btree_common.ipp | 12 +- .../homestore/index/index_internal.hpp | 4 + src/include/homestore/index/index_table.hpp | 121 +++++++++++++-- src/include/homestore/index_service.hpp | 2 + src/lib/index/index_cp.cpp | 32 ++-- src/lib/index/index_service.cpp | 17 +- src/lib/index/wb_cache.cpp | 146 +++++++++++++++--- src/tests/test_index_crash_recovery.cpp | 116 ++++++++++++++ 9 files changed, 405 insertions(+), 47 deletions(-) diff --git a/conanfile.py b/conanfile.py index 17eec5915..545d174c0 100644 --- a/conanfile.py +++ b/conanfile.py @@ -9,7 +9,7 @@ class HomestoreConan(ConanFile): name = "homestore" - version = "7.5.15" + version = "7.5.16" homepage = "https://github.com/eBay/Homestore" description = "HomeStore Storage Engine" diff --git a/src/include/homestore/btree/detail/btree_common.ipp b/src/include/homestore/btree/detail/btree_common.ipp index b4e730b67..aa9196f39 100644 --- a/src/include/homestore/btree/detail/btree_common.ipp +++ b/src/include/homestore/btree/detail/btree_common.ipp @@ -292,7 +292,10 @@ void Btree::validate_node_child_relation(BtreeNodePtr node, BtreeNodePtr& if(ind > 0){ if (previous_child->next_bnode()!= child_node->node_id()) { - throw std::runtime_error(fmt::format("Broken child linkage: {}-th Child node [{}] node id is not equal to previous child node [{}] next node",ind, child_node->to_string(), child_node->node_id(), previous_child->to_string())); + throw std::runtime_error(fmt::format( + "Broken child linkage: {}-th Child node [{}] node id {} is not equal to previous child node [{}] next node {}", + ind, child_node->to_string(), child_node->node_id(), previous_child->to_string(), + previous_child->next_bnode())); } K last_parent_key = node->get_nth_key< K >(ind-1, false /* copy */); K previous_child_last_key = previous_child->get_last_key< K >(); @@ -370,7 +373,8 @@ template void Btree::validate_node(const bnodeid_t& bnodeid) const { BtreeNodePtr node; if (auto ret = read_node_impl(bnodeid, node); ret != btree_status_t::success) { - throw std::runtime_error(fmt::format("node read failed for bnodeid: {} reason: {}", bnodeid, ret)); + throw std::runtime_error( + fmt::format("node read failed for bnodeid: {} reason: {}", bnodeid, enum_name(ret))); } else { try { if (node->is_node_deleted()) { return; } @@ -402,7 +406,9 @@ void Btree::validate_node(const bnodeid_t& bnodeid) const { if (neighbor_id != empty_bnodeid) { BtreeNodePtr neighbor_node; if (auto ret = read_node_impl(neighbor_id, neighbor_node); ret != btree_status_t::success) { - throw std::runtime_error(fmt::format("reading neighbor node of [{}] failed for bnodeid: {} reason : {}", node->to_string(), neighbor_id, ret)); + throw std::runtime_error(fmt::format( + "reading neighbor node of [{}] failed for bnodeid: {} reason : {}", node->to_string(), + neighbor_id, enum_name(ret))); } validate_next_node_relation(node, neighbor_node, last_child_node); } diff --git a/src/include/homestore/index/index_internal.hpp b/src/include/homestore/index/index_internal.hpp index 7dff5172f..7594da8e5 100644 --- a/src/include/homestore/index/index_internal.hpp +++ b/src/include/homestore/index/index_internal.hpp @@ -94,6 +94,10 @@ class IndexTableBase { virtual void stop() = 0; virtual void repair_node(IndexBufferPtr const& buf) = 0; virtual void repair_root_node(IndexBufferPtr const& buf) = 0; + // Returns true if SB root was updated to the committed buffer. + virtual bool set_root_from_committed_buf(IndexBufferPtr const& buf) = 0; + // True when on-disk SB already has a non-empty root (not first-CP create crash). + virtual bool has_persisted_root() const = 0; virtual void delete_stale_children(IndexBufferPtr const& buf) = 0; virtual void audit_tree() const = 0; virtual void update_sb() = 0; diff --git a/src/include/homestore/index/index_table.hpp b/src/include/homestore/index/index_table.hpp index b79381962..9af88c171 100644 --- a/src/include/homestore/index/index_table.hpp +++ b/src/include/homestore/index/index_table.hpp @@ -211,29 +211,72 @@ class IndexTable : public IndexTableBase, public Btree< K, V > { if (m_sb->root_node == idx_buf->blkid().to_integer()) { // This is the root node, we need to update the root node in superblk LOGTRACEMOD(wbcache, "{} is old root so we need to update the meta node ", idx_buf->to_string()); + if (idx_buf->raw_buffer() == nullptr || + !BtreeNode::is_valid_node(sisl::blob{idx_buf->raw_buffer(), this->m_bt_cfg.node_size()}) || + r_cast< persistent_hdr_t const* >(idx_buf->raw_buffer())->node_id != idx_buf->blkid().to_integer()) { + LOGERROR("repair_root_node: skip invalid/unwritten buf {}", idx_buf->to_string()); + return; + } + + // Already a proper root (e.g. new_root just promoted by set_root_from_committed_buf): + // valid edge, empty next. recover_buf may still call update_root because this buf sits + // under meta after Condition-1 linking — nothing to convert. Check header before + // allocating a BtreeNode to avoid an unnecessary (and previously leaked) object. + auto const* phdr = r_cast< persistent_hdr_t const* >(idx_buf->raw_buffer()); + if (phdr->next_node == empty_bnodeid) { + LOGTRACEMOD(wbcache, "repair_root_node: buf={} already has empty next_bnode; nothing to repair", + idx_buf->to_string()); + return; + } + BtreeNode* n = this->init_node(idx_buf->raw_buffer(), idx_buf->blkid().to_integer(), false /* init_buf */, BtreeNode::identify_leaf_node(idx_buf->raw_buffer())); static_cast< IndexBtreeNode* >(n)->attach_buf(idx_buf); - auto edge_id = n->next_bnode(); + // Own via intrusive_ptr so every early-return path releases the node (and its + // attached IndexBuffer) — otherwise LSan reports the node plus the recovery DAG. + BtreeNodePtr bn{n}; + auto edge_id = bn->next_bnode(); + + // In a partial root-split crash old_root's next_bnode points to child_node2 which is + // at the same level as old_root (not one level below as a valid edge child must be). Applying + // the edge repair would violate the level invariant. next_bnode may also be unwritten + // (flat flush DAG); read_node_impl fails safely in that case — skip repair rather than + // corrupting the node. + { + BtreeNodePtr candidate; + auto const ret = read_node_impl(edge_id, candidate); + if (ret != btree_status_t::success) { + LOGERROR("repair_root_node: SKIP buf={} next_bnode={} unreadable (ret={}); " + "partial root-split / unflushed sibling, not repairing", + idx_buf->to_string(), edge_id, enum_name(ret)); + return; + } + if (candidate->level() >= bn->level()) { + LOGERROR("repair_root_node: SKIP buf={} next_bnode={} level={} >= parent level={}; " + "partial root-split state, not repairing", + idx_buf->to_string(), edge_id, candidate->level(), bn->level()); + return; + } + } - if (n->has_valid_edge() && hs()->has_fc_service()) { + if (bn->has_valid_edge() && hs()->has_fc_service()) { auto const reason = fmt::format("root {} already has a valid edge {}, so we should have found the new root node", - n->to_string(), n->get_edge_value().bnode_id()); + bn->to_string(), bn->get_edge_value().bnode_id()); hs()->fc_service().trigger_fc(FaultContainmentEvent::ENTER, static_cast< void* >(&(m_sb->parent_uuid)), reason); return; } else { - BT_REL_ASSERT(!n->has_valid_edge(), + BT_REL_ASSERT(!bn->has_valid_edge(), "root {} already has a valid edge {}, so we should have found the new root node", - n->to_string(), n->get_edge_value().bnode_id()); + bn->to_string(), bn->get_edge_value().bnode_id()); } - n->set_next_bnode(empty_bnodeid); - n->set_edge_value(BtreeLinkInfo{edge_id, 0}); - LOGTRACEMOD(wbcache, "change root node {}: edge updated to {} and invalidate the next node! ", n->node_id(), - edge_id); + bn->set_next_bnode(empty_bnodeid); + bn->set_edge_value(BtreeLinkInfo{edge_id, 0}); + LOGTRACEMOD(wbcache, "change root node {}: edge updated to {} and invalidate the next node! ", + bn->node_id(), edge_id); auto cpg = cp_mgr().cp_guard(); - write_node_impl(n, (void*)cpg.context(cp_consumer_t::INDEX_SVC)); + write_node_impl(bn, (void*)cpg.context(cp_consumer_t::INDEX_SVC)); } else { LOGTRACEMOD(wbcache, "This is not the root node, so we can ignore this repair call for buf {}", @@ -241,6 +284,55 @@ class IndexTable : public IndexTableBase, public Btree< K, V > { } } + bool has_persisted_root() const override { return m_sb->root_node != empty_bnodeid; } + + // companion: called during recovery when new_root was committed but the SB write crashed. + // Updates in-memory SB and btree root info so the next forced CP persists the correct root. + // Returns true only when the SB root was actually updated. + bool set_root_from_committed_buf(IndexBufferPtr const& idx_buf) override { + if (idx_buf->raw_buffer() == nullptr || + !BtreeNode::is_valid_node(sisl::blob{idx_buf->raw_buffer(), this->m_bt_cfg.node_size()}) || + r_cast< persistent_hdr_t const* >(idx_buf->raw_buffer())->node_id != idx_buf->blkid().to_integer()) { + LOGERROR("set_root_from_committed_buf: skip invalid/unwritten buf {}", idx_buf->to_string()); + return false; + } + + auto const cand_level = r_cast< persistent_hdr_t const* >(idx_buf->raw_buffer())->level; + + // First-CP crash before any SB root was persisted: keep root empty so + // recovery_completed() creates a fresh root (CrashBeforeFirstCp). + if (m_sb->root_node == empty_bnodeid) { + LOGINFOMOD(wbcache, + "set_root_from_committed_buf: SB root empty, skip promote of {} level={} (first-CP path)", + idx_buf->blkid().to_integer(), cand_level); + return false; + } + + // Only adopt a height-increasing new root (level > persisted depth). Same-or-lower + // level nodes under meta are split siblings / old roots from Condition-1 flattening, + // not the new root. + if (cand_level <= m_sb->btree_depth) { + LOGINFOMOD(wbcache, "set_root_from_committed_buf: skip {} level={} <= sb depth={}", + idx_buf->blkid().to_integer(), cand_level, m_sb->btree_depth); + return false; + } + + BtreeNode* n = this->init_node(idx_buf->raw_buffer(), idx_buf->blkid().to_integer(), false /* init_buf */, + BtreeNode::identify_leaf_node(idx_buf->raw_buffer())); + static_cast< IndexBtreeNode* >(n)->attach_buf(idx_buf); + BtreeNodePtr bn{n}; + + LOGINFOMOD(wbcache, "set_root_from_committed_buf: updating SB root {} -> {} level={}", m_sb->root_node, + idx_buf->blkid().to_integer(), bn->level()); + + m_sb->root_node = idx_buf->blkid().to_integer(); + m_sb->root_link_version = bn->link_version(); + m_sb->btree_depth = bn->level(); + this->m_btree_depth = bn->level(); + this->set_root_node_info(BtreeLinkInfo{m_sb->root_node, m_sb->root_link_version}); + return true; + } + void delete_stale_children(IndexBufferPtr const& idx_buf) override { if (!idx_buf->is_meta_buf() && idx_buf->m_created_cp_id == -1) { BtreeNode* n = this->init_node(idx_buf->raw_buffer(), idx_buf->blkid().to_integer(), false /* init_buf */, @@ -266,6 +358,13 @@ class IndexTable : public IndexTableBase, public Btree< K, V > { this->root_node_id()); return; } + // Same-CP new nodes that never flushed are zero-filled; init_node(init_buf=false) would assert. + if (idx_buf->raw_buffer() == nullptr || + !BtreeNode::is_valid_node(sisl::blob{idx_buf->raw_buffer(), this->m_bt_cfg.node_size()}) || + r_cast< persistent_hdr_t const* >(idx_buf->raw_buffer())->node_id != idx_buf->blkid().to_integer()) { + LOGERROR("repair_node: skip invalid/unwritten buf {}", idx_buf->to_string()); + return; + } BtreeNode* n = this->init_node(idx_buf->raw_buffer(), idx_buf->blkid().to_integer(), false /* init_buf */, BtreeNode::identify_leaf_node(idx_buf->raw_buffer())); static_cast< IndexBtreeNode* >(n)->attach_buf(idx_buf); @@ -307,7 +406,7 @@ class IndexTable : public IndexTableBase, public Btree< K, V > { node->set_checksum(); auto prev_state = idx_node->m_idx_buf->m_state.exchange(index_buf_state_t::DIRTY); LOGTRACEMOD(wbcache, "write_node_impl: node_id={} cp_id={} prev_state={} -> DIRTY", node->node_id(), - cp_ctx->id(), static_cast(prev_state)); + cp_ctx->id(), static_cast< int >(prev_state)); idx_node->m_idx_buf->m_node_level = node->level(); if (prev_state == index_buf_state_t::CLEAN) { // It was clean before, dirtying it first time, add it to the wb_cache list to flush diff --git a/src/include/homestore/index_service.hpp b/src/include/homestore/index_service.hpp index 0483a9e55..4eeeb7728 100644 --- a/src/include/homestore/index_service.hpp +++ b/src/include/homestore/index_service.hpp @@ -98,6 +98,8 @@ class IndexService { void repair_index_node(uint32_t ordinal, IndexBufferPtr const& node_buf); void parent_recover(uint32_t ordinal, IndexBufferPtr const& node_buf); void update_root(uint32_t ordinal, IndexBufferPtr const& node_buf); + // Returns true if SB root was updated to the committed buffer. + bool set_root_from_committed_buf(uint32_t ordinal, IndexBufferPtr const& node_buf); IndexWBCacheBase& wb_cache() { if (!m_wb_cache) { throw std::runtime_error("Attempted to access a null pointer wb_cache"); } diff --git a/src/lib/index/index_cp.cpp b/src/lib/index/index_cp.cpp index 6f4ef044c..f482ea561 100644 --- a/src/lib/index/index_cp.cpp +++ b/src/lib/index/index_cp.cpp @@ -281,7 +281,7 @@ std::map< BlkId, IndexBufferPtr > IndexCPContext::recover(sisl::byte_view sb) { // LOGTRACEMOD(wbcache,"\n\n\nAFTER modify : \n "); // dag_print(buf_map, "After: "); - auto sanityCheck = [](const std::map< BlkId, IndexBufferPtr >& dags) { + auto sanityCheck = [cp_id = id()](const std::map< BlkId, IndexBufferPtr >& dags) { for (const auto& [blkid, bufferPtr] : dags) { auto up_buffer = bufferPtr->m_up_buffer; if (up_buffer) { @@ -290,9 +290,16 @@ std::map< BlkId, IndexBufferPtr > IndexCPContext::recover(sisl::byte_view sb) { "Sanity check failed: Buffer {} blkdid {} has an up_buffer {} blkid that is marked as freed.", bufferPtr->to_string(), blkid.to_integer(), up_buffer->to_string(), up_buffer->blkid().to_integer()); - HS_REL_ASSERT(up_buffer->m_created_cp_id == -1, - "Sanity check failed: Buffer {} has an up_buffer {} that just created (created_cp_id={})", - bufferPtr->to_string(), up_buffer->to_string(), up_buffer->m_created_cp_id); + // link_buf Condition 1 bypasses new→new links, so a buffer created in this CP must not + // still point at another same-CP-new up. Older downs may depend on a newly created up + // (e.g. old_root → new_root during root split). During journal recovery, buffers are + // fresh objects: -1 means not marked created-in-this-CP (equivalent to != cp_id). + if (bufferPtr->m_created_cp_id == cp_id) { + HS_REL_ASSERT( + up_buffer->m_created_cp_id != cp_id, + "Sanity check failed: new Buffer {} has an up_buffer {} that just created (created_cp_id={})", + bufferPtr->to_string(), up_buffer->to_string(), up_buffer->m_created_cp_id); + } HS_REL_ASSERT(up_buffer->m_index_ordinal == bufferPtr->m_index_ordinal, "Sanity check failed: Buffer {} has an up_buffer {} with different index_ordinal " "(up_ordinal={}, buf_ordinal={})", @@ -331,7 +338,8 @@ void IndexCPContext::process_txn_record(txn_record const* rec, std::map< BlkId, auto cpg = cp_mgr().cp_guard(); auto const rec_to_buf = [&buf_map, &cpg](txn_record const* rec, bool is_meta, BlkId const& bid, - IndexBufferPtr const& up_buf) -> IndexBufferPtr { + IndexBufferPtr const& up_buf, + bool mark_created_in_cp = false) -> IndexBufferPtr { IndexBufferPtr buf; // MetaIndexBuffer always has blkid={0,0,0,0} regardless of which BTree table it belongs to. // When multiple tables have a root split in the same CP, all their MetaBufs share the same blkid @@ -356,10 +364,17 @@ void IndexCPContext::process_txn_record(txn_record const* rec, std::map< BlkId, buf = it->second; } + // Mark before linking so Condition 1 matches IndexWBCache::link_buf (both ends new). + if (mark_created_in_cp) { buf->m_created_cp_id = cpg->id(); } + if (up_buf) { auto real_up_buf = up_buf; - if (up_buf->m_created_cp_id == cpg->id()) { + // Mirror link_buf Condition 1: bypass up_buf only when BOTH up and down were created + // in this CP. If down is an older node, keep the dependency through up_buf. + if (up_buf->m_created_cp_id == cpg->id() && buf->m_created_cp_id == cpg->id()) { real_up_buf = up_buf->m_up_buffer; + HS_DBG_ASSERT(real_up_buf, + "Up buffer is newly created in this cp, but it doesn't have its own up_buffer"); } else if (up_buf->m_node_freed) { real_up_buf = up_buf->m_up_buffer; LOGTRACEMOD(wbcache, "\n\n change upbuffer from {} to {}\n\n", up_buf->to_string(), @@ -391,9 +406,8 @@ void IndexCPContext::process_txn_record(txn_record const* rec, std::map< BlkId, } for (uint8_t idx{0}; idx < rec->num_new_ids; ++idx) { - auto new_buf = rec_to_buf(rec, false /* is_meta */, rec->blk_id(cur_idx++), - inplace_child_buf ? inplace_child_buf : parent_buf); - new_buf->m_created_cp_id = cpg->id(); + rec_to_buf(rec, false /* is_meta */, rec->blk_id(cur_idx++), + inplace_child_buf ? inplace_child_buf : parent_buf, true /* mark_created_in_cp */); } for (uint8_t idx{0}; idx < rec->num_freed_ids; ++idx) { diff --git a/src/lib/index/index_service.cpp b/src/lib/index/index_service.cpp index a4c4cd71c..230537d98 100644 --- a/src/lib/index/index_service.cpp +++ b/src/lib/index/index_service.cpp @@ -230,6 +230,16 @@ void IndexService::update_root(uint32_t ordinal, IndexBufferPtr const& node_buf) } } +bool IndexService::set_root_from_committed_buf(uint32_t ordinal, IndexBufferPtr const& node_buf) { + auto tbl = get_index_table(ordinal); + if (tbl) { + return tbl->set_root_from_committed_buf(node_buf); + } else { + HS_DBG_ASSERT(false, "Index corresponding to ordinal={} has not been loaded yet, unexpected", ordinal); + return false; + } +} + uint32_t IndexService::node_size() const { return m_vdev->atomic_page_size(); } uint64_t IndexService::used_size() const { @@ -274,15 +284,18 @@ std::string IndexBuffer::to_string() const { } #endif + // Avoid embedding bare `{...}` in the result: HS_*_ASSERT_CMP pre-formats the message via + // format_log_msg and then re-parses it in _cmp_assert_with_msg; unescaped braces there throw + // fmt::format_error("invalid format string"). if (m_is_meta_buf) { - return fmt::format("[Meta] Buf={} index={} state={} create/dirty_cp={}/{} down_wait#={}{} down={{{}}}", + return fmt::format("[Meta] Buf={} index={} state={} create/dirty_cp={}/{} down_wait#={}{} down=[{}]", voidptr_cast(const_cast< IndexBuffer* >(this)), m_index_ordinal, state_str[int_cast(state())], m_created_cp_id, m_dirtied_cp_id, m_wait_for_down_buffers.get(), m_node_freed ? " Freed" : "", down_bufs); } else { return fmt::format( - "Buf={} index={} state={} create/dirty_cp={}/{} down_wait#={}{} up={} node=[{}] down={{{}}}", + "Buf={} index={} state={} create/dirty_cp={}/{} down_wait#={}{} up={} node=[{}] down=[{}]", voidptr_cast(const_cast< IndexBuffer* >(this)), m_index_ordinal, state_str[int_cast(state())], m_created_cp_id, m_dirtied_cp_id, m_wait_for_down_buffers.get(), m_node_freed ? " Freed" : "", voidptr_cast(const_cast< IndexBuffer* >(m_up_buffer.get())), diff --git a/src/lib/index/wb_cache.cpp b/src/lib/index/wb_cache.cpp index afa1cc958..d0ab601b4 100644 --- a/src/lib/index/wb_cache.cpp +++ b/src/lib/index/wb_cache.cpp @@ -153,6 +153,12 @@ void IndexWBCache::read_buf(bnodeid_t id, BtreeNodePtr& node, node_initializer_t auto idx_buf = std::make_shared< IndexBuffer >(blkid, m_node_size, m_vdev->align_size()); m_vdev->sync_read(r_cast< char* >(idx_buf->raw_buffer()), m_node_size, blkid); + // Reject unwritten/corrupt blocks before init_node (which DEBUG_ASSERTs node_id match). + auto const* phdr = r_cast< persistent_hdr_t const* >(idx_buf->raw_buffer()); + if (!BtreeNode::is_valid_node(sisl::blob{idx_buf->raw_buffer(), m_node_size}) || phdr->node_id != id) { + throw std::runtime_error(fmt::format("Read invalid index node id={} disk_node_id={}", id, phdr->node_id)); + } + // Create the btree node out of buffer node = node_initializer(idx_buf); @@ -366,11 +372,16 @@ void IndexWBCache::link_buf(IndexBufferPtr const& up_buf, IndexBufferPtr const& IndexBufferPtr real_up_buf = up_buf; IndexCPContext* icp_ctx = r_cast< IndexCPContext* >(cp_ctx); - // Condition 1: If the down buffer and up buffer are both created by the current cp_id, unconditionally we need - // to link it with up_buffer's up_buffer. In other words, there should never a link between down and up buffers - // created in current generation (cp). In real terms, it means all new buffers can be flushed independently to - // each other and dependency is needed only for the buffers created in previous cps. - if (up_buf->m_created_cp_id == icp_ctx->id()) { + // Condition 1: If BOTH up_buf and down_buf are newly created in the current CP, bypass up_buf and link + // down_buf directly to up_buf's own parent. This allows all same-CP new nodes to flush independently + // without ordering constraints between each other — they only need to wait for the pre-existing + // ancestor (e.g. meta_buf or an older interior node). + // + // Critically, we only bypass when down_buf is ALSO new in the current CP. If down_buf is an older + // node (e.g. old_root during a root split), it must retain the dependency chain through up_buf + // (new_root) so that old_root cannot flush before new_root, preventing a transient split state from + // reaching disk without the new root also being durable. + if (up_buf->m_created_cp_id == icp_ctx->id() && down_buf->m_created_cp_id == icp_ctx->id()) { real_up_buf = up_buf->m_up_buffer; HS_DBG_ASSERT(real_up_buf, "Up buffer is newly created in this cp, but it doesn't have its own up_buffer, its not expected"); @@ -405,9 +416,14 @@ void IndexWBCache::link_buf(IndexBufferPtr const& up_buf, IndexBufferPtr const& // This link is acheived by unconditionally changing the link in case of is_sibling=true to passed up_buf, but // conditionally do it in case of parent link where it already has a link don't override it. if (down_buf->m_up_buffer != nullptr) { - HS_DBG_ASSERT_LT(down_buf->m_up_buffer->m_created_cp_id, icp_ctx->id(), - "down_buf=[{}] up_buffer=[{}] should never have been created on same cp", - down_buf->to_string(), down_buf->m_up_buffer->to_string()); + // Condition 1 only flattens new→new. An older down may legitimately depend on a same-CP-new + // up (e.g. old_root → new_root). Forbid only residual new→new up links that should have been + // bypassed. + if (down_buf->m_created_cp_id == icp_ctx->id()) { + HS_DBG_ASSERT_LT(down_buf->m_up_buffer->m_created_cp_id, icp_ctx->id(), + "down_buf=[{}] up_buffer=[{}] should never have been created on same cp", + down_buf->to_string(), down_buf->m_up_buffer->to_string()); + } if (!is_sibling_link || (down_buf->m_up_buffer == real_up_buf)) { // Already linked with same buf or its not a sibling link to override, nothing to do other than asserts @@ -533,21 +549,45 @@ std::string IndexWBCache::to_string_dag_bufs(DagMap& dags, cp_id_t cp_id) { void IndexWBCache::prune_up_buffers(IndexBufferPtr const& buf, std::vector< IndexBufferPtr >& pruned_bufs_to_repair) { auto up_buf = buf->m_up_buffer; - auto grand_up_buf = up_buf->m_up_buffer; if (!up_buf || !up_buf->m_wait_for_down_buffers.testz()) { return; } + // Committed ancestors must remain linked for recover_buf / new-root promotion. Only abandon + // uncommitted (or freed) ups when their last down dependency was pruned. + if (!up_buf->m_node_freed && was_node_committed(up_buf)) { + LOGTRACEMOD(wbcache, "Skip prune-cascade for committed up_buffer {}", up_buf->to_string()); + return; + } + + auto grand_up_buf = up_buf->m_up_buffer; // if up buffer has up buffer, then we need to decrement its wait_for_down_buffers LOGINFOMOD(wbcache, "\n\npruning up_buffer due to zero dependency of child\n up buffer {}\n buffer {}", up_buf->to_string(), buf->to_string()); update_up_buffer_counters(up_buf); - pruned_bufs_to_repair.push_back(up_buf); + // Same-CP new nodes that never flushed are all-zero on disk. repair_node → init_node(init_buf=false) + // would DEBUG_ASSERT node_id match. Only schedule durable buffers for repair. + auto const is_durable = [this](IndexBufferPtr const& b) { + if (!b || b->is_meta_buf() || b->raw_buffer() == nullptr) { return false; } + auto const* phdr = r_cast< persistent_hdr_t const* >(b->raw_buffer()); + return BtreeNode::is_valid_node(sisl::blob{b->raw_buffer(), m_node_size}) && + phdr->node_id == b->blkid().to_integer(); + }; + + if (is_durable(up_buf)) { + pruned_bufs_to_repair.push_back(up_buf); + } else { + LOGINFOMOD(wbcache, "Skip repair for unwritten pruned up_buffer {}", up_buf->to_string()); + } if (grand_up_buf && !grand_up_buf->is_meta_buf() && grand_up_buf->m_wait_for_down_buffers.testz()) { - LOGTRACEMOD( - wbcache, - "\nadding grand_buffer to repair list due to zero dependency of child\n grand buffer {}\n buffer {}", - grand_up_buf->to_string(), buf->to_string()); - pruned_bufs_to_repair.push_back(grand_up_buf); + if (is_durable(grand_up_buf)) { + LOGTRACEMOD( + wbcache, + "\nadding grand_buffer to repair list due to zero dependency of child\n grand buffer {}\n buffer {}", + grand_up_buf->to_string(), buf->to_string()); + pruned_bufs_to_repair.push_back(grand_up_buf); + } else { + LOGINFOMOD(wbcache, "Skip repair for unwritten pruned grand_buffer {}", grand_up_buf->to_string()); + } } } @@ -611,6 +651,9 @@ void IndexWBCache::recover(sisl::byte_view sb) { std::vector< IndexBufferPtr > pruned_bufs_to_repair; std::set< IndexBufferPtr > bufs_to_skip_sanity_check; + // Track newly-created nodes that were committed but whose meta_buf was not written. + // These are new-root candidates; we pick the highest-level one per ordinal after the loop. + std::map< uint32_t, std::vector< IndexBufferPtr > > new_root_candidates; auto prune_from_up_buffer = [&](IndexBufferPtr const& buf) { if (!buf->m_up_buffer) { return; } @@ -623,7 +666,8 @@ void IndexWBCache::recover(sisl::byte_view sb) { LOGTRACEMOD(wbcache, "\n\n\nRecovery processing begins\n\n\n"); for (auto const& [_, buf] : bufs) { - load_buf(buf); + // Meta buffers are not vdev blocks; loading them corrupts m_dirtied_cp_id from garbage reads. + if (!buf->is_meta_buf()) { load_buf(buf); } if (buf->m_node_freed) { LOGTRACEMOD(wbcache, "recovering free buf {}", buf->to_string()); @@ -670,18 +714,68 @@ void IndexWBCache::recover(sisl::byte_view sb) { buf->m_up_buffer->to_string()); m_vdev->commit_blk(buf->m_blkid); pending_bufs.push_back(buf->m_up_buffer); + } else if (was_node_committed(buf) && buf->m_up_buffer && buf->m_up_buffer->is_meta_buf()) { + // Committed new node under unwritten meta. Only keep as new-root candidate when SB + // already has a persisted root (prior CP completed). First-CP crashes leave SB root + // empty — prune like the generic uncommitted-up path so recovery_completed() can + // create a fresh root and sanity check is not run on partial-split debris. + auto tbl = index_service().get_index_table(buf->m_index_ordinal); + if (tbl && tbl->has_persisted_root()) { + LOGINFOMOD(wbcache, "Recovery: new buf {} committed but SB not written; new root candidate", + buf->to_string()); + m_vdev->commit_blk(buf->m_blkid); + new_root_candidates[buf->m_index_ordinal].push_back(buf); + } else { + LOGINFOMOD(wbcache, + "Recovery: new buf {} committed under meta but SB root empty; pruning " + "(first-CP path)", + buf->to_string()); + prune_from_up_buffer(buf); + bufs_to_skip_sanity_check.insert(buf); + } } else { // Up buffer is not committed, we need to repair it first LOGTRACEMOD(wbcache, "The up buffer {} is not committed for the new buffer {}", - buf->m_up_buffer->to_string(), buf->to_string()); - buf->m_up_buffer->remove_down_buffer(buf); - prune_up_buffers(buf, pruned_bufs_to_repair); - // Skip the sanity check on this buf as we do not keep it + buf->m_up_buffer ? buf->m_up_buffer->to_string() : std::string{"null"}, buf->to_string()); + // Must clear m_up_buffer after unlink. Condition 1 keeps longer chains + // (e.g. meta ← new_root ← old_root ← sibling); pruning a descendant later + // cascades through update_up_buffer_counters and would double-remove otherwise. + prune_from_up_buffer(buf); bufs_to_skip_sanity_check.insert(buf); - // buf->m_up_buffer = nullptr; } } } + + // Among new-root candidates per ordinal, promote the highest-level node when it is a true + // height increase. Use on-disk header level — m_node_level is unset for journal-recovered nodes. + // Non-selected / non-promoted candidates are skipped in sanity check (split debris under meta). + auto node_level_of = [](IndexBufferPtr const& b) -> uint32_t { + return r_cast< persistent_hdr_t const* >(b->raw_buffer())->level; + }; + for (auto const& [ordinal, candidates] : new_root_candidates) { + auto it = std::max_element(candidates.begin(), candidates.end(), + [&node_level_of](IndexBufferPtr const& a, IndexBufferPtr const& b) { + return node_level_of(a) < node_level_of(b); + }); + LOGINFOMOD(wbcache, "Recovery: considering buf {} (level={}) as new root for ordinal {}", (*it)->to_string(), + node_level_of(*it), ordinal); + if (index_service().set_root_from_committed_buf(ordinal, *it)) { + if ((*it)->m_up_buffer) { pending_bufs.push_back((*it)->m_up_buffer); } // meta + for (auto const& c : candidates) { + if (c != *it) { bufs_to_skip_sanity_check.insert(c); } + } + } else { + // No height-increasing root to adopt (e.g. crash_flush_on_root before new_root durable). + // Prune candidates like the pre-Fix-1 path so partial-split nodes are not sanity-checked. + // Cascade from earlier prunes may already have unlinked some candidates — prune_from_up_buffer + // is a no-op when m_up_buffer is already null. + for (auto const& c : candidates) { + prune_from_up_buffer(c); + bufs_to_skip_sanity_check.insert(c); + } + } + } + LOGTRACEMOD(wbcache, "\n\n\nRecovery processing Ends\n\n\n"); #ifdef _PRERELEASE LOGINFOMOD(wbcache, "Index Recovery detected {} nodes out of {} as new/freed nodes to be recovered in prev cp={}", @@ -783,6 +877,13 @@ void IndexWBCache::update_up_buffer_counters(IndexBufferPtr const& buf) { LOGINFOMOD(wbcache, "Finish decrementing wait_for_down_buffers\n"); return; } + // Condition 1 keeps dependency chains such as meta ← new_root ← old_root. Pruning an + // uncommitted descendant can drive new_root's wait to zero; do not detach a committed node + // from its parent — it still needs new-root promotion / recover_buf. + if (!buf->m_node_freed && was_node_committed(buf)) { + LOGTRACEMOD(wbcache, "Skip detach of committed buf {} from up during prune cascade", buf->to_string()); + return; + } auto grand_buf = buf->m_up_buffer; LOGINFOMOD(wbcache, "Decrementing wait_for_down_buffers due to zero dependency of child for grand_buffer {} up_buffer {}, " @@ -822,6 +923,9 @@ void IndexWBCache::recover_buf(IndexBufferPtr const& buf) { bool IndexWBCache::was_node_committed(IndexBufferPtr const& buf) { if (buf == nullptr) { return false; } + // Meta/SB durability is tracked separately (new_root_candidates). Meta is never a vdev node. + if (buf->is_meta_buf()) { return false; } + // If the node is freed, then it can be considered committed as long as its up buffer was committed if (buf->m_node_freed) { HS_DBG_ASSERT(buf->m_up_buffer, "Buf {} was marked deleted, but doesn't have an up_buffer", buf->to_string()); diff --git a/src/tests/test_index_crash_recovery.cpp b/src/tests/test_index_crash_recovery.cpp index e8572ea34..5b0fe22a5 100644 --- a/src/tests/test_index_crash_recovery.cpp +++ b/src/tests/test_index_crash_recovery.cpp @@ -32,6 +32,7 @@ using namespace homestore; SISL_LOGGING_INIT(HOMESTORE_LOG_MODS) +SISL_LOGGING_DEF(HOMESTORE_LOG_MODS) SISL_OPTIONS_ENABLE(logging, test_index_crash_recovery, iomgr, test_common_setup) SISL_LOGGING_DECL(test_index_crash_recovery) @@ -1748,6 +1749,121 @@ TEST_F(IndexCrashCreatedFreedReuseTest, CreatedAndFreedBlkReusedByRecoveryRepair } } +// Regression test for first-root-split crash (leaf→level=1): +// Crash during the very first root split after all node buffers (new_root, +// old_leaf, child_node2) are written to disk but before meta_buf (SB) writes. +// +// Disk state after crash: +// - SB: root_node=old_leaf (level=0), btree_depth=0 [meta_buf not written] +// - old_leaf on disk: edge_info=EMPTY, next_bnode=child_node2 (level=0) +// - new_root (level=1) on disk [written before meta_buf] +// +// Fix 1: has_persisted_root()=true (SB root=old_leaf, non-empty); new_root +// (cand_level=1 > btree_depth=0) is promoted via set_root_from_committed_buf(). +// repair_root_node is never called on old_leaf. +// Fix 3 (fallback guard): would catch candidate.level(0) >= old_leaf.level(0) and +// skip the unsafe edge repair if Fix 1 somehow did not apply. +TYPED_TEST(IndexCrashTest, CrashAtMetaBufOnFirstRootSplit) { + const uint32_t max_keys = SISL_OPTIONS["max_keys_in_node"].as< uint32_t >(); + + // Phase 1: establish a stable leaf-only tree (btree_depth=0). Insert fewer than + // max_keys entries so the leaf never splits before the crash CP. + const uint32_t stable_count = max_keys / 2; + LOGINFO("Phase 1: Insert {} entries into leaf without triggering root split", stable_count); + for (uint32_t k = 0; k < stable_count; ++k) { + this->put(k, btree_put_type::INSERT, true /* expect_success */); + } + LOGINFO("Phase 1: Flush cleanly — tree is level=0 (leaf only), SB root=old_leaf"); + test_common::HSTestHelper::trigger_cp(true); + this->m_shadow_map.save(this->m_shadow_filename); + + // Phase 2: crash during the first root split (leaf→level=1). + // crash_flush_on_meta fires when meta_buf is ready to write — after new_root and + // old_leaf are both durable — leaving the SB stale on disk. + LOGINFO("Phase 2: Set crash_flush_on_meta flip"); + this->set_basic_flip("crash_flush_on_meta"); + + // Insert enough to push total past max_keys, forcing the first root split. + // stable_count + split_count = max_keys/2 + max_keys + 1 >> max_keys. + const uint32_t split_count = max_keys + 1; + LOGINFO("Phase 2: Insert {} entries to trigger first root split (leaf→level=1)", split_count); + for (uint32_t k = stable_count; k < stable_count + split_count; ++k) { + this->put(k, btree_put_type::INSERT, true /* expect_success */); + } + + LOGINFO("Phase 2: Trigger CP (crash expected at meta_buf flush)"); + test_common::HSTestHelper::trigger_cp(false); + LOGINFO("Phase 2: Waiting for crash and recovery"); + this->wait_for_crash_recovery(true); + + LOGINFO("Recovery complete — reapplying and verifying tree integrity"); + this->reapply_after_crash(); + this->get_all(); +} + +// Regression test for the root-split crash bug (SDSTOR-XXXXX): +// During a root split (tree height N→N+1), crash_flush_on_meta fires after +// all node buffers (new_root, old_root, child_node2) are written to disk but +// before meta_buf (SB) writes. On recovery, repair_root_node reads +// old_root.next_bnode (= child_node2.blkid, a level-N interior node) and +// writes it into old_root.edge_info, violating the invariant +// child.level == parent.level - 1. +// +// Two-phase design: +// Phase 1: trigger the 1st root split (leaf → level=1) cleanly so the tree +// reaches a stable level=1 state with an interior old_root. +// Phase 2: set crash_flush_on_meta, insert enough entries to trigger the 2nd +// root split (level=1 → level=2), then crash at meta_buf flush. +// +// Expected outcome after fix: recovery succeeds and the tree is consistent. +// Without fix: repair_root_node writes child_node2 (level=1) as edge of +// old_root (level=1) → validate_node asserts "child level mismatch". +TYPED_TEST(IndexCrashTest, CrashAtMetaBufOnSecondRootSplit) { + const uint32_t max_keys = SISL_OPTIONS["max_keys_in_node"].as< uint32_t >(); + + // Phase 1: insert enough entries to trigger the first root split (leaf → level=1) + // and flush cleanly so the tree is at a stable level=1 before the crash CP. + LOGINFO("Phase 1: Insert {} entries to trigger first root split (leaf→level=1)", max_keys + 1); + for (uint32_t k = 0; k <= max_keys; ++k) { + this->put(k, btree_put_type::INSERT, true /* expect_success */); + } + LOGINFO("Phase 1: Flush cleanly — tree is now at level=1, SB is up to date"); + test_common::HSTestHelper::trigger_cp(true); + this->m_shadow_map.save(this->m_shadow_filename); + + // Phase 2: set crash_flush_on_meta before inserts so it fires when the 2nd + // root split calls transact_bufs(meta_buf, new_root_buf). The flip marks + // meta_buf with crash_flag; the actual crash fires only when meta_buf is + // ready to write (after all node bufs complete), leaving the SB stale. + LOGINFO("Phase 2: Set crash_flush_on_meta flip"); + this->set_basic_flip("crash_flush_on_meta"); + + // max_keys^2 entries is sufficient to fill the level=1 root (which requires + // ~max_keys child splits, each needing ~max_keys inserts to fill a leaf). + const uint32_t phase2_count = max_keys * max_keys; + LOGINFO("Phase 2: Insert {} entries to trigger second root split (level=1→2)", phase2_count); + for (uint32_t k = max_keys + 1; k <= max_keys + phase2_count; ++k) { + this->put(k, btree_put_type::INSERT, true /* expect_success */); + } + + LOGINFO("Phase 2: Trigger CP (crash expected at meta_buf flush)"); + test_common::HSTestHelper::trigger_cp(false); + LOGINFO("Phase 2: Waiting for crash and recovery"); + this->wait_for_crash_recovery(true); + + // Disk state after crash (verified against gdb evidence from the production incident): + // - SB: root_node=old_root (level=1), btree_depth=1 [meta_buf not written] + // - old_root on disk: edge_info=EMPTY, next_bnode=child_node2.blkid + // - new_root (level=2) on disk [written before meta_buf] + // + // Without fix: repair_root_node sets old_root.edge_info=child_node2 (level=1) + // → validate_node aborts: "child level: 1, expected: 0" + // With fix: recovery finds new_root correctly; tree is consistent. + LOGINFO("Recovery complete — reapplying and verifying tree integrity"); + this->reapply_after_crash(); + this->get_all(); +} + #endif int main(int argc, char* argv[]) {