From fe067217a31c05c406c056cf2ea0f1fc52a54c42 Mon Sep 17 00:00:00 2001 From: Jie Yao Date: Wed, 29 Jul 2026 11:10:01 +0800 Subject: [PATCH 1/2] 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), which updates the in-memory superblock (SB) and 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 written at the very end of CP flush, after all node buffers complete. A SIGKILL landing after step 2 but before the SB write exposed three latent bugs that combined to corrupt the tree. **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. This caused old_root_buf to link directly to meta_buf, making new_root_buf and old_root_buf siblings 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 crash left old_root on disk in split state and new_root durable but the SB unwritten, the recovery loop could not reliably identify new_root as the intended root. Existing logic either discarded the committed new_root candidate or had no mechanism to promote it, leaving the persisted SB still pointing to old_root. **Bug C — repair_root_node applied an unsafe edge repair** With old_root as the recovered tree root (per the stale SB), 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. **Fix 1 — Restore correct flush-DAG ordering (Bug A)** Changed link_buf Condition 1 to bypass up_buf only when BOTH up_buf AND down_buf were created in the current CP: 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()) When down_buf is an older node (e.g. old_root), the dependency chain through up_buf (new_root) is preserved. The key guarantee this establishes is: if new_root is committed (on disk), then old_root and child_node2 are also committed, making Fix 2's root promotion safe. The mirror fix is applied in index_cp.cpp process_txn_record so that journal recovery rebuilds an identical DAG. The sanity check is tightened accordingly: only a buffer that was itself created in the current CP must not point to another same-CP-new up_buffer. **Fix 2 — Pre-flush new-root nodes and promote on recovery (Bug B)** Added a pre-flush phase in async_cp_flush: before the normal DAG flush starts, all newly-created nodes belonging to ordinals that had a root change are written to disk via async_write. Only after these writes complete does the normal DAG flush begin. This ensures new_root (and child_node2) are always durable before old_root can be written in its transient split state. During recovery, the journal is parsed to identify the final intended new-root BlkId per ordinal (m_recovered_root_ids). A candidate is promoted when: - The journal identifies it as the final root for its ordinal - was_node_committed() confirms it is durable on disk - persisted_root_was_committed() confirms the old (persisted SB) root was also written in this CP, meaning the tree is in post-split state This check is stronger than simply testing whether SB root is non-empty: if old_root was not yet written in split state, the tree is still in a pre-split consistent state and the new_root candidate is discarded. set_root_from_committed_buf() updates the in-memory SB (root_node, root_link_version, btree_depth) and the btree's root pointer before the forced recovery CP writes the corrected SB to disk. The first-CP path (SB root still empty) is excluded so that recovery_completed() can build a fresh root as before. **Fix 3 — Harden repair_root_node (Bug C)** Added guards before applying the edge repair: a. Buffer validity: skip if raw_buffer is null, node magic is invalid, or node_id does not match blkid — the buffer was never written. b. next_bnode empty: skip if next_bnode is already empty_bnodeid, meaning new_root was already promoted and nothing needs repairing. c. Level invariant: read the candidate next_bnode from disk; if its level >= old_root's level (partial root-split state), skip the repair rather than corrupting the edge. Also changed the raw pointer `n` to BtreeNodePtr `bn` to prevent a memory leak on the early-return paths added by this fix. The same validity guard is applied to repair_node to protect against same-CP new nodes that were never flushed (all-zero on disk). **Additional hardening** - skip load_buf for meta_buf in recover(): MetaIndexBuffer has blkid={0,0,0,0}; calling load_buf on it reads garbage from vdev and corrupts m_dirtied_cp_id. - recover_buf() returns early for meta_buf: meta durability is tracked via the new-root candidate path, not via was_node_committed. - read_buf() validates node magic and node_id before init_node to catch corrupt or unwritten blocks early. - to_string() replaces unescaped inner braces {{{}}} with [{}] to avoid fmt::format_error when HS_*_ASSERT_CMP re-parses the message string. Added two regression tests to IndexCrashTest: CrashAtMetaBufOnSecondRootSplit: Establishes a durable level-1 tree, then sets crash_flush_on_meta and inserts enough keys to force a second root split (level-1 → level-2). The crash fires after new_root and old_root are both durable but before meta_buf writes. Fix 2 detects new_root and promotes it; Fix 3 would catch the level invariant violation if Fix 2 had not applied. Tree integrity is verified via reapply_after_crash and get_all. CrashAfterOldRootFlushOnSecondRootSplit: Same setup, but uses crash_flush_on_root to crash specifically when old_root is flushed (after the pre-flush has already made new_root durable). Also sets skip_cp_after_index_root_recovery to keep the original journal current, then performs a second restart to verify that replaying an already-promoted candidate is idempotent. --- conanfile.py | 2 +- .../homestore/index/index_internal.hpp | 2 + src/include/homestore/index/index_table.hpp | 155 +++++++++++++---- src/lib/index/index_cp.cpp | 58 ++++++- src/lib/index/index_cp.hpp | 6 + src/lib/index/index_service.cpp | 9 + src/lib/index/wb_cache.cpp | 159 +++++++++++++++--- src/lib/index/wb_cache.hpp | 1 + src/tests/test_index_crash_recovery.cpp | 159 ++++++++++++++++++ 9 files changed, 483 insertions(+), 68 deletions(-) diff --git a/conanfile.py b/conanfile.py index 545d174c0..9fecfaa6f 100644 --- a/conanfile.py +++ b/conanfile.py @@ -9,7 +9,7 @@ class HomestoreConan(ConanFile): name = "homestore" - version = "7.5.16" + version = "7.5.17" homepage = "https://github.com/eBay/Homestore" description = "HomeStore Storage Engine" diff --git a/src/include/homestore/index/index_internal.hpp b/src/include/homestore/index/index_internal.hpp index 7dff5172f..2a79a5f74 100644 --- a/src/include/homestore/index/index_internal.hpp +++ b/src/include/homestore/index/index_internal.hpp @@ -95,6 +95,8 @@ class IndexTableBase { virtual void repair_node(IndexBufferPtr const& buf) = 0; virtual void repair_root_node(IndexBufferPtr const& buf) = 0; virtual void delete_stale_children(IndexBufferPtr const& buf) = 0; + virtual bnodeid_t persisted_root_node_id() const = 0; + virtual bool set_root_from_committed_buf(IndexBufferPtr const& buf) = 0; virtual void audit_tree() const = 0; virtual void update_sb() = 0; virtual void load_metrics(uint64_t interior, uint64_t leaf, uint8_t depth) = 0; diff --git a/src/include/homestore/index/index_table.hpp b/src/include/homestore/index/index_table.hpp index b79381962..e573bce65 100644 --- a/src/include/homestore/index/index_table.hpp +++ b/src/include/homestore/index/index_table.hpp @@ -208,37 +208,117 @@ class IndexTable : public IndexTableBase, public Btree< K, V > { void repair_root_node(IndexBufferPtr const& idx_buf) override { LOGTRACEMOD(wbcache, "check if this was the previous root node {} for buf {} ", m_sb->root_node, idx_buf->to_string()); - 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()); - 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(); - - if (n->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()); - hs()->fc_service().trigger_fc(FaultContainmentEvent::ENTER, static_cast< void* >(&(m_sb->parent_uuid)), - reason); - return; - } else { - BT_REL_ASSERT(!n->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()); - } - 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); - auto cpg = cp_mgr().cp_guard(); - write_node_impl(n, (void*)cpg.context(cp_consumer_t::INDEX_SVC)); - - } else { + if (m_sb->root_node != idx_buf->blkid().to_integer()) { LOGTRACEMOD(wbcache, "This is not the root node, so we can ignore this repair call for buf {}", idx_buf->to_string()); + return; + } + + LOGTRACEMOD(wbcache, "{} is old root so we need to update the meta node ", idx_buf->to_string()); + auto* const raw_buf = idx_buf->raw_buffer(); + if (raw_buf == nullptr || !BtreeNode::is_valid_node(sisl::blob{raw_buf, this->m_bt_cfg.node_size()})) { + LOGERROR("repair_root_node: skip invalid/unwritten buf {}", idx_buf->to_string()); + return; + } + + auto const* phdr = r_cast< persistent_hdr_t const* >(raw_buf); + if (phdr->node_id != idx_buf->blkid().to_integer()) { + LOGERROR("repair_root_node: skip invalid/unwritten buf {}", idx_buf->to_string()); + return; + } + 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(raw_buf, idx_buf->blkid().to_integer(), false /* init_buf */, + BtreeNode::identify_leaf_node(raw_buf)); + static_cast< IndexBtreeNode* >(n)->attach_buf(idx_buf); + BtreeNodePtr root{n}; + auto const edge_id = root->next_bnode(); + + BtreeNodePtr edge_node; + auto const ret = read_node_impl(edge_id, edge_node); + if (ret != btree_status_t::success || edge_node->level() >= root->level()) { + LOGERROR("repair_root_node: skip unsafe edge repair for buf={} next_bnode={} ret={} " + "candidate_level={} root_level={}", + idx_buf->to_string(), edge_id, enum_name(ret), + edge_node ? static_cast< int >(edge_node->level()) : -1, root->level()); + return; } + + if (root->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", + root->to_string(), root->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(!root->has_valid_edge(), + "root {} already has a valid edge {}, so we should have found the new root node", + root->to_string(), root->get_edge_value().bnode_id()); + } + root->set_next_bnode(empty_bnodeid); + root->set_edge_value(BtreeLinkInfo{edge_id, 0}); + LOGTRACEMOD(wbcache, "change root node {}: edge updated to {} and invalidate the next node! ", root->node_id(), + edge_id); + auto cpg = cp_mgr().cp_guard(); + write_node_impl(root, (void*)cpg.context(cp_consumer_t::INDEX_SVC)); + } + + bnodeid_t persisted_root_node_id() const override { return m_sb->root_node; } + + bool set_root_from_committed_buf(IndexBufferPtr const& idx_buf) override { + auto* const raw_buf = idx_buf->raw_buffer(); + if (m_sb->root_node == empty_bnodeid || raw_buf == nullptr || + !BtreeNode::is_valid_node(sisl::blob{raw_buf, this->m_bt_cfg.node_size()})) { + LOGERROR("set_root_from_committed_buf: reject invalid candidate {}", idx_buf->to_string()); + return false; + } + + auto const* candidate_hdr = r_cast< persistent_hdr_t const* >(raw_buf); + if (candidate_hdr->node_id != idx_buf->blkid().to_integer()) { + LOGERROR("set_root_from_committed_buf: reject invalid candidate {}", idx_buf->to_string()); + return false; + } + + try { + this->validate_node(idx_buf->blkid().to_integer()); + } catch (std::exception const& e) { + LOGERROR("set_root_from_committed_buf: candidate={} failed validation: {}", idx_buf->to_string(), e.what()); + return false; + } + + auto const candidate_level = candidate_hdr->level; + if (m_sb->root_node == idx_buf->blkid().to_integer()) { + if (candidate_level != m_sb->btree_depth) { + LOGERROR("set_root_from_committed_buf: persisted root {} has level={} but SB depth={}", + idx_buf->blkid().to_integer(), candidate_level, m_sb->btree_depth); + return false; + } + this->m_btree_depth = candidate_level; + this->set_root_node_info(BtreeLinkInfo{m_sb->root_node, m_sb->root_link_version}); + return true; + } + + BtreeNode* n = this->init_node(raw_buf, idx_buf->blkid().to_integer(), false /* init_buf */, + BtreeNode::identify_leaf_node(raw_buf)); + static_cast< IndexBtreeNode* >(n)->attach_buf(idx_buf); + BtreeNodePtr root{n}; + + LOGINFOMOD(wbcache, "Recovery promotes committed root {} -> {} at level {}", m_sb->root_node, root->node_id(), + root->level()); + m_sb->root_node = root->node_id(); + m_sb->root_link_version = root->link_version(); + m_sb->btree_depth = root->level(); + this->m_btree_depth = root->level(); + this->set_root_node_info(BtreeLinkInfo{root->node_id(), root->link_version()}); + + // Recovery promotion must survive another crash even when no index buffer is dirty in the forced CP. + m_sb.write(); + return true; } void delete_stale_children(IndexBufferPtr const& idx_buf) override { @@ -266,8 +346,21 @@ class IndexTable : public IndexTableBase, public Btree< K, V > { this->root_node_id()); 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())); + + auto* const raw_buf = idx_buf->raw_buffer(); + if (raw_buf == nullptr || !BtreeNode::is_valid_node(sisl::blob{raw_buf, this->m_bt_cfg.node_size()})) { + LOGERROR("repair_node: skip invalid/unwritten buf {}", idx_buf->to_string()); + return; + } + auto const* phdr = r_cast< persistent_hdr_t const* >(raw_buf); + if (phdr->node_id != idx_buf->blkid().to_integer()) { + LOGERROR("repair_node: skip buf {} whose persisted node_id={} does not match blkid={}", + idx_buf->to_string(), phdr->node_id, idx_buf->blkid().to_integer()); + return; + } + + BtreeNode* n = this->init_node(raw_buf, idx_buf->blkid().to_integer(), false /* init_buf */, + BtreeNode::identify_leaf_node(raw_buf)); static_cast< IndexBtreeNode* >(n)->attach_buf(idx_buf); auto cpg = cp_mgr().cp_guard(); @@ -307,7 +400,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/lib/index/index_cp.cpp b/src/lib/index/index_cp.cpp index 6f4ef044c..4c080fb05 100644 --- a/src/lib/index/index_cp.cpp +++ b/src/lib/index/index_cp.cpp @@ -31,6 +31,9 @@ void IndexCPContext::add_to_txn_journal(uint32_t index_ordinal, const IndexBuffe auto record_size = txn_record::size_for_num_ids(created_bufs.size() + freed_bufs.size() + (left_child_buf ? 1 : 0) + (parent_buf ? 1 : 0)); std::unique_lock< iomgr::FiberManagerLib::mutex > lg{m_txn_journal_mtx}; + if (parent_buf && parent_buf->is_meta_buf() && !left_child_buf && !created_bufs.empty()) { + m_root_changed_ordinals.insert(index_ordinal); + } if (m_txn_journal_buf.bytes() == nullptr) { m_txn_journal_buf = std::move(sisl::io_blob_safe{std::max(sizeof(txn_journal), 512ul), 512, sisl::buftag::metablk}); @@ -64,6 +67,27 @@ void IndexCPContext::add_to_txn_journal(uint32_t index_ordinal, const IndexBuffe } } +IndexBufferPtrList IndexCPContext::root_change_preflush_bufs() { + IndexBufferPtrList bufs; + std::set< BlkId > selected_blkids; + std::unique_lock< iomgr::FiberManagerLib::mutex > lg{m_txn_journal_mtx}; + if (m_root_changed_ordinals.empty()) { return bufs; } + + m_dirty_buf_list.foreach_entry([this, &bufs, &selected_blkids](IndexBufferPtr const& buf) { + if (buf->is_meta_buf() || buf->m_node_freed || buf->m_created_cp_id != id() || + !m_root_changed_ordinals.contains(buf->m_index_ordinal)) { + return; + } + if (selected_blkids.insert(buf->blkid()).second) { bufs.push_back(buf); } + }); + return bufs; +} + +BlkId IndexCPContext::recovered_root_id(uint32_t ordinal) const { + auto const it = m_recovered_root_ids.find(ordinal); + return it == m_recovered_root_ids.end() ? BlkId{} : it->second; +} + void IndexCPContext::add_to_dirty_list(const IndexBufferPtr& buf) { m_dirty_buf_list.push_back(buf); buf->set_state(index_buf_state_t::DIRTY); @@ -246,6 +270,18 @@ std::map< BlkId, IndexBufferPtr > IndexCPContext::recover(sisl::byte_view sb) { txn_record const* rec = r_cast< txn_record const* >(cur_ptr); HS_DBG_ASSERT_GT(rec->total_ids(), 0, "Invalid txn_record, has no ids in it"); + // Root-change records contain no split/merge side effects. Retaining the last record per ordinal identifies + // the final intended root even if the same CP grows and then collapses the tree. + if (rec->is_parent_meta && rec->num_freed_ids == 0) { + if (!rec->has_inplace_child && rec->num_new_ids == 1) { + auto const new_root_idx = rec->has_inplace_parent ? 1 : 0; + m_recovered_root_ids[rec->index_ordinal] = rec->blk_id(new_root_idx); + } else if (rec->has_inplace_child && rec->num_new_ids == 0) { + auto const child_idx = rec->has_inplace_parent ? 1 : 0; + m_recovered_root_ids[rec->index_ordinal] = rec->blk_id(child_idx); + } + } + process_txn_record(rec, buf_map); cur_ptr += rec->size(); LOGTRACEMOD(wbcache, "Recovered txn record: {}: {}", t, rec->to_string()); @@ -281,7 +317,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 +326,11 @@ 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); + 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 {} created in the same CP ({})", + bufferPtr->to_string(), up_buffer->to_string(), 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 +369,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,9 +395,11 @@ void IndexCPContext::process_txn_record(txn_record const* rec, std::map< BlkId, buf = it->second; } + 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()) { + if (up_buf->m_created_cp_id == cpg->id() && buf->m_created_cp_id == cpg->id()) { real_up_buf = up_buf->m_up_buffer; } else if (up_buf->m_node_freed) { real_up_buf = up_buf->m_up_buffer; @@ -391,9 +432,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_cp.hpp b/src/lib/index/index_cp.hpp index b15cba892..724b14971 100644 --- a/src/lib/index/index_cp.hpp +++ b/src/lib/index/index_cp.hpp @@ -15,6 +15,8 @@ *********************************************************************************/ #pragma once #include +#include +#include #include #include #include @@ -144,6 +146,8 @@ struct IndexCPContext : public VDevCPContext { iomgr::FiberManagerLib::mutex m_txn_journal_mtx; sisl::io_blob_safe m_txn_journal_buf; + std::unordered_set< uint32_t > m_root_changed_ordinals; + std::map< uint32_t, BlkId > m_recovered_root_ids; public: IndexCPContext(CP* cp); @@ -156,6 +160,8 @@ struct IndexCPContext : public VDevCPContext { std::map< BlkId, IndexBufferPtr > recover(sisl::byte_view sb); sisl::io_blob_safe const& journal_buf() const { return m_txn_journal_buf; } + IndexBufferPtrList root_change_preflush_bufs(); + BlkId recovered_root_id(uint32_t ordinal) const; void add_to_dirty_list(const IndexBufferPtr& buf); bool any_dirty_buffers() const; diff --git a/src/lib/index/index_service.cpp b/src/lib/index/index_service.cpp index a4c4cd71c..54bd9ac6d 100644 --- a/src/lib/index/index_service.cpp +++ b/src/lib/index/index_service.cpp @@ -23,6 +23,10 @@ #include "common/homestore_assert.hpp" #include "device/virtual_dev.hpp" #include "device/physical_dev.hpp" + +#ifdef _PRERELEASE +#include +#endif #include "device/chunk.h" namespace homestore { @@ -118,6 +122,11 @@ void IndexService::start() { tbl->audit_tree(); #endif } +#ifdef _PRERELEASE + // Tests can keep the recovered journal current, then perform a second crash sequentially to verify replay + // idempotence without racing nested HomeStore restarts. + if (iomgr_flip::instance()->test_flip("skip_cp_after_index_root_recovery")) { return; } +#endif // Force taking cp after recovery done. This makes sure that the index table is in consistent state and dirty // buffer after recovery can be added to dirty list for flushing in the new cp hs()->cp_mgr().trigger_cp_flush(true /* force */); diff --git a/src/lib/index/wb_cache.cpp b/src/lib/index/wb_cache.cpp index 7660b5db3..c23a8c2ae 100644 --- a/src/lib/index/wb_cache.cpp +++ b/src/lib/index/wb_cache.cpp @@ -366,11 +366,9 @@ 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: Flatten only new-to-new links. An existing node modified by a root split must retain the new root + // as its up-buffer so recovery reconstructs the same root transition deterministically. + 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 +403,11 @@ 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()); + 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 @@ -611,6 +611,28 @@ void IndexWBCache::recover(sisl::byte_view sb) { std::vector< IndexBufferPtr > pruned_bufs_to_repair; std::set< IndexBufferPtr > bufs_to_skip_sanity_check; + std::map< uint32_t, IndexBufferPtr > new_root_candidates; + std::map< uint32_t, bool > persisted_root_commit_status; + + auto persisted_root_was_committed = [&](uint32_t ordinal) { + if (auto const it = persisted_root_commit_status.find(ordinal); it != persisted_root_commit_status.end()) { + return it->second; + } + + auto const table = index_service().get_index_table(ordinal); + bool committed{false}; + if (table && table->persisted_root_node_id() != empty_bnodeid) { + auto const old_root_it = bufs.find(BlkId{table->persisted_root_node_id()}); + committed = old_root_it != bufs.end() && was_node_committed(old_root_it->second); + } + persisted_root_commit_status.emplace(ordinal, committed); + return committed; + }; + auto commit_recovered_blk = [this](IndexBufferPtr const& buf) { + auto const status = m_vdev->commit_blk(buf->m_blkid); + HS_REL_ASSERT(status == BlkAllocStatus::SUCCESS, "Failed to commit recovered index block {}", + buf->m_blkid.to_string()); + }; auto prune_from_up_buffer = [&](IndexBufferPtr const& buf) { if (!buf->m_up_buffer) { return; } @@ -623,7 +645,16 @@ 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 journal placeholders rather than vdev blocks. + if (!buf->is_meta_buf()) { load_buf(buf); } + + auto const recovered_root_id = icp_ctx->recovered_root_id(buf->m_index_ordinal); + auto const is_final_root = + !buf->is_meta_buf() && recovered_root_id.is_valid() && recovered_root_id == buf->blkid(); + if (is_final_root && buf->m_created_cp_id != icp_ctx->id() && was_node_committed(buf) && + persisted_root_was_committed(buf->m_index_ordinal)) { + new_root_candidates[buf->m_index_ordinal] = buf; + } if (buf->m_node_freed) { LOGTRACEMOD(wbcache, "recovering free buf {}", buf->to_string()); @@ -657,33 +688,52 @@ void IndexWBCache::recover(sisl::byte_view sb) { was_node_committed(buf->m_up_buffer)); buf->m_node_freed = false; r_cast< persistent_hdr_t* >(buf->m_bytes)->node_deleted = false; - auto alloc_status = m_vdev->commit_blk(buf->m_blkid); - HS_REL_ASSERT_EQ(alloc_status, BlkAllocStatus::SUCCESS, "Unsuccessful commit_blk() in recover()"); + commit_recovered_blk(buf); if (buf->m_node_level) { potential_parent_recovered_bufs.insert(buf); } prune_from_up_buffer(buf); } } else if (buf->m_created_cp_id == icp_ctx->id()) { LOGTRACEMOD(wbcache, "recovering new buf {}", buf->to_string()); - // New node - if (was_node_committed(buf) && was_node_committed(buf->m_up_buffer)) { + auto const buf_was_committed = was_node_committed(buf); + if (buf_was_committed && buf->m_up_buffer && buf->m_up_buffer->is_meta_buf()) { + // Every root-transition node under meta remains live when the old root was written. Only the final + // journal-selected node is published, but intermediate roots must also remain allocator-owned. + if (persisted_root_was_committed(buf->m_index_ordinal)) { + commit_recovered_blk(buf); + if (is_final_root) { + new_root_candidates[buf->m_index_ordinal] = buf; + LOGINFOMOD(wbcache, "Recovery found final committed root candidate {}", buf->to_string()); + } + } else { + LOGTRACEMOD(wbcache, "Discard new-root candidate {} because persisted old root was not written", + buf->to_string()); + prune_from_up_buffer(buf); + bufs_to_skip_sanity_check.insert(buf); + } + } else if (buf_was_committed && was_node_committed(buf->m_up_buffer)) { // Both current and up buffer is committed, we can safely commit the current block LOGTRACEMOD(wbcache, "New buffer {} and the up buffer {} are committed", buf->to_string(), buf->m_up_buffer->to_string()); - auto alloc_status = m_vdev->commit_blk(buf->m_blkid); - HS_REL_ASSERT_EQ(alloc_status, BlkAllocStatus::SUCCESS, "Unsuccessful commit_blk() in recover()"); + commit_recovered_blk(buf); pending_bufs.push_back(buf->m_up_buffer); } 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); + prune_from_up_buffer(buf); // Skip the sanity check on this buf as we do not keep it bufs_to_skip_sanity_check.insert(buf); - // buf->m_up_buffer = nullptr; } } } + + for (auto const& [ordinal, candidate] : new_root_candidates) { + auto const table = index_service().get_index_table(ordinal); + HS_REL_ASSERT(table && table->set_root_from_committed_buf(candidate), + "Unable to publish committed root candidate {} for index ordinal {}", candidate->to_string(), + ordinal); + } + 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={}", @@ -823,6 +873,7 @@ void IndexWBCache::recover_buf(IndexBufferPtr const& buf) { bool IndexWBCache::was_node_committed(IndexBufferPtr const& buf) { if (buf == nullptr) { return false; } + 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) { @@ -837,6 +888,22 @@ bool IndexWBCache::was_node_committed(IndexBufferPtr const& buf) { } //////////////////// CP Related API section ///////////////////////////////// +void IndexWBCache::start_buffer_flush(IndexCPContext* cp_ctx) { + cp_ctx->prepare_flush_iteration(); + m_updated_ordinals.clear(); + for (auto& fiber : m_cp_flush_fibers) { + iomanager.run_on_forget(fiber, [this, cp_ctx]() { + IndexBufferPtrList buf_list; + get_next_bufs(cp_ctx, resource_mgr().get_dirty_buf_qd(), buf_list); + + for (auto& buf : buf_list) { + do_flush_one_buf(cp_ctx, buf, true); + } + m_vdev->submit_batch(); + }); + } +} + folly::Future< bool > IndexWBCache::async_cp_flush(IndexCPContext* cp_ctx) { LOGINFOMOD(wbcache, "Starting Index CP Flush with cp {}, dirty_buf_count={}, nodes_added={}, nodes_removed={}", cp_ctx->id(), cp_ctx->m_dirty_buf_count.get(), cp_ctx->m_num_nodes_added.load(), @@ -853,7 +920,7 @@ folly::Future< bool > IndexWBCache::async_cp_flush(IndexCPContext* cp_ctx) { // Always try to flush, will be a no-op when not needed m_vdev->cp_flush(cp_ctx); - cp_ctx->complete(true); + cp_ctx->complete(true); return folly::makeFuture< bool >(true); // nothing to flush } @@ -877,17 +944,55 @@ folly::Future< bool > IndexWBCache::async_cp_flush(IndexCPContext* cp_ctx) { } } - cp_ctx->prepare_flush_iteration(); - m_updated_ordinals.clear(); - for (auto& fiber : m_cp_flush_fibers) { - iomanager.run_on_forget(fiber, [this, cp_ctx]() { - IndexBufferPtrList buf_list; - get_next_bufs(cp_ctx, resource_mgr().get_dirty_buf_qd(), buf_list); + auto preflush_bufs = cp_ctx->root_change_preflush_bufs(); + if (preflush_bufs.empty()) { + start_buffer_flush(cp_ctx); + } else { + // A root split modifies the persisted root in place. Preflush its new nodes before the normal DAG can write + // that root. The DAG writes them again so its completion and recovery semantics remain unchanged. + // PhysicalDev batching is reactor-local, so submission must run on an I/O fiber with a drive channel. + iomanager.run_on_forget(m_cp_flush_fibers.front(), [this, cp_ctx, preflush_bufs = std::move(preflush_bufs)]() { +#ifdef _PRERELEASE + if (iomgr_flip::instance()->test_flip("crash_during_root_preflush")) { + auto const& buf = preflush_bufs.front(); + LOGINFO("Simulating crash after partially preflushing root-transition node {}", buf->to_string()); + m_vdev + ->async_write(r_cast< const char* >(buf->raw_buffer()), m_node_size, buf->m_blkid, + true /* part_of_batch */) + .thenTry([cp_ctx](auto&& result) { + HS_REL_ASSERT(!result.hasException(), + "Partial root-transition preflush failed with an exception"); + auto const error = result.value(); + HS_REL_ASSERT(!error, "Partial root-transition preflush failed with error={} ({})", + error.value(), error.message()); + hs()->crash_simulator().crash(); + cp_ctx->complete(true); + }); + m_vdev->submit_batch(); + return; + } +#endif - for (auto& buf : buf_list) { - do_flush_one_buf(cp_ctx, buf, true); + std::vector< folly::Future< std::error_code > > preflush_futures; + preflush_futures.reserve(preflush_bufs.size()); + for (auto const& buf : preflush_bufs) { + LOGTRACEMOD(wbcache, "Preflushing root-transition node {}", buf->to_string()); + preflush_futures.emplace_back( + m_vdev->async_write(r_cast< const char* >(buf->raw_buffer()), m_node_size, buf->m_blkid, true)); } m_vdev->submit_batch(); + folly::collectAllUnsafe(preflush_futures).thenTry([this, cp_ctx](auto&& preflush_results) { + HS_REL_ASSERT(!preflush_results.hasException(), "Root-transition preflush failed with an exception"); + + for (auto const& result : preflush_results.value()) { + HS_REL_ASSERT(!result.hasException(), "Root-transition preflush failed with an exception"); + auto const error = result.value(); + HS_REL_ASSERT(!error, "Root-transition preflush failed with error={} ({})", error.value(), + error.message()); + } + + start_buffer_flush(cp_ctx); + }); }); } return cp_ctx->get_future(); diff --git a/src/lib/index/wb_cache.hpp b/src/lib/index/wb_cache.hpp index bf04dbc67..5d5f9525a 100644 --- a/src/lib/index/wb_cache.hpp +++ b/src/lib/index/wb_cache.hpp @@ -72,6 +72,7 @@ class IndexWBCache : public IndexWBCacheBase { private: void start_flush_threads(); + void start_buffer_flush(IndexCPContext* cp_ctx); void recover_new_nodes(sisl::byte_view sb); void process_write_completion(IndexCPContext* cp_ctx, IndexBufferPtr const& pbuf); void do_flush_one_buf(IndexCPContext* cp_ctx, IndexBufferPtr const& buf, bool part_of_batch); diff --git a/src/tests/test_index_crash_recovery.cpp b/src/tests/test_index_crash_recovery.cpp index e8572ea34..25e68f496 100644 --- a/src/tests/test_index_crash_recovery.cpp +++ b/src/tests/test_index_crash_recovery.cpp @@ -867,6 +867,165 @@ TYPED_TEST(IndexCrashTest, SplitCrash1) { } } +// Cover the first root split after a leaf root has already been made durable. +TYPED_TEST(IndexCrashTest, CrashAtMetaBufOnFirstRootSplit) { + const uint32_t max_keys = SISL_OPTIONS["max_keys_in_node"].as< uint32_t >(); + const uint32_t durable_key_count = max_keys / 2; + + for (uint32_t k = 0; k < durable_key_count; ++k) { + this->put(k, btree_put_type::INSERT, true /* expect_success */); + } + test_common::HSTestHelper::trigger_cp(true); + this->m_shadow_map.save(this->m_shadow_filename); + auto const durable_root = this->m_bt->root_node_id(); + ASSERT_EQ(this->m_bt->get_btree_depth(), 0); + + this->set_basic_flip("crash_flush_on_meta"); + uint32_t next_key = durable_key_count; + while (this->m_bt->get_btree_depth() == 0) { + this->put(next_key++, btree_put_type::INSERT, true /* expect_success */); + } + ASSERT_NE(this->m_bt->root_node_id(), durable_root); + ASSERT_EQ(this->m_bt->get_btree_depth(), 1); + ASSERT_TRUE(hs()->crash_simulator().will_crash()); + + test_common::HSTestHelper::trigger_cp(false); + this->wait_for_crash_recovery(true); + + ASSERT_EQ(this->m_bt->get_btree_depth(), 1); + this->reapply_after_crash(); + this->get_all(); +} + +// Cover the first root split window after the modified leaf root is durable but before its new root is published. +TYPED_TEST(IndexCrashTest, CrashAfterOldRootFlushOnFirstRootSplit) { + const uint32_t max_keys = SISL_OPTIONS["max_keys_in_node"].as< uint32_t >(); + const uint32_t durable_key_count = max_keys / 2; + + for (uint32_t k = 0; k < durable_key_count; ++k) { + this->put(k, btree_put_type::INSERT, true /* expect_success */); + } + test_common::HSTestHelper::trigger_cp(true); + this->m_shadow_map.save(this->m_shadow_filename); + auto const durable_root = this->m_bt->root_node_id(); + ASSERT_EQ(this->m_bt->get_btree_depth(), 0); + + this->set_basic_flip("crash_flush_on_root"); + uint32_t next_key = durable_key_count; + while (this->m_bt->get_btree_depth() == 0) { + this->put(next_key++, btree_put_type::INSERT, true /* expect_success */); + } + ASSERT_NE(this->m_bt->root_node_id(), durable_root); + ASSERT_EQ(this->m_bt->get_btree_depth(), 1); + ASSERT_TRUE(hs()->crash_simulator().will_crash()); + + test_common::HSTestHelper::trigger_cp(false); + this->wait_for_crash_recovery(true); + + ASSERT_EQ(this->m_bt->get_btree_depth(), 1); + this->reapply_after_crash(); + this->get_all(); +} + +// A partial preflush must leave the durable old root authoritative and safely discard the incomplete transition. +TYPED_TEST(IndexCrashTest, CrashDuringRootPreflushOnFirstRootSplit) { + const uint32_t max_keys = SISL_OPTIONS["max_keys_in_node"].as< uint32_t >(); + const uint32_t durable_key_count = max_keys / 2; + + for (uint32_t k = 0; k < durable_key_count; ++k) { + this->put(k, btree_put_type::INSERT, true /* expect_success */); + } + test_common::HSTestHelper::trigger_cp(true); + this->m_shadow_map.save(this->m_shadow_filename); + auto const durable_root = this->m_bt->root_node_id(); + ASSERT_EQ(this->m_bt->get_btree_depth(), 0); + + this->set_basic_flip("crash_during_root_preflush"); + hs()->crash_simulator().set_will_crash(true); + uint32_t next_key = durable_key_count; + while (this->m_bt->get_btree_depth() == 0) { + this->put(next_key++, btree_put_type::INSERT, true /* expect_success */); + } + ASSERT_NE(this->m_bt->root_node_id(), durable_root); + ASSERT_EQ(this->m_bt->get_btree_depth(), 1); + ASSERT_TRUE(hs()->crash_simulator().will_crash()); + + test_common::HSTestHelper::trigger_cp(false); + this->wait_for_crash_recovery(true); + + ASSERT_EQ(this->m_bt->root_node_id(), durable_root); + ASSERT_EQ(this->m_bt->get_btree_depth(), 0); + this->reapply_after_crash(); + this->get_all(); +} + +// Recovery must publish the durable new root if the table superblock still names the modified old root. +TYPED_TEST(IndexCrashTest, CrashAtMetaBufOnSecondRootSplit) { + const uint32_t max_keys = SISL_OPTIONS["max_keys_in_node"].as< uint32_t >(); + + // Establish a durable level-1 root before triggering the crash-sensitive level-1 -> level-2 split. + for (uint32_t k = 0; k <= max_keys; ++k) { + this->put(k, btree_put_type::INSERT, true /* expect_success */); + } + test_common::HSTestHelper::trigger_cp(true); + this->m_shadow_map.save(this->m_shadow_filename); + auto const persisted_root = this->m_bt->root_node_id(); + auto const persisted_depth = this->m_bt->get_btree_depth(); + + this->set_basic_flip("crash_flush_on_meta"); + const uint32_t phase2_count = max_keys * max_keys; + for (uint32_t k = max_keys + 1; k <= max_keys + phase2_count; ++k) { + this->put(k, btree_put_type::INSERT, true /* expect_success */); + } + ASSERT_NE(this->m_bt->root_node_id(), persisted_root); + ASSERT_GT(this->m_bt->get_btree_depth(), persisted_depth); + ASSERT_TRUE(hs()->crash_simulator().will_crash()); + + test_common::HSTestHelper::trigger_cp(false); + this->wait_for_crash_recovery(true); + + ASSERT_GT(this->m_bt->get_btree_depth(), persisted_depth); + this->reapply_after_crash(); + this->get_all(); +} + +// Cover the window where the old root is durable but the table superblock is stale, then restart again to verify that +// recovery persisted the promoted root. +TYPED_TEST(IndexCrashTest, CrashAfterOldRootFlushOnSecondRootSplit) { + const uint32_t max_keys = SISL_OPTIONS["max_keys_in_node"].as< uint32_t >(); + + for (uint32_t k = 0; k <= max_keys; ++k) { + this->put(k, btree_put_type::INSERT, true /* expect_success */); + } + test_common::HSTestHelper::trigger_cp(true); + this->m_shadow_map.save(this->m_shadow_filename); + auto const persisted_root = this->m_bt->root_node_id(); + auto const persisted_depth = this->m_bt->get_btree_depth(); + + this->set_basic_flip("crash_flush_on_root"); + this->set_basic_flip("skip_cp_after_index_root_recovery"); + const uint32_t phase2_count = max_keys * max_keys; + for (uint32_t k = max_keys + 1; k <= max_keys + phase2_count; ++k) { + this->put(k, btree_put_type::INSERT, true /* expect_success */); + } + ASSERT_NE(this->m_bt->root_node_id(), persisted_root); + ASSERT_GT(this->m_bt->get_btree_depth(), persisted_depth); + ASSERT_TRUE(hs()->crash_simulator().will_crash()); + + test_common::HSTestHelper::trigger_cp(false); + this->wait_for_crash_recovery(true); + ASSERT_GT(this->m_bt->get_btree_depth(), persisted_depth); + + // The recovery CP was deliberately skipped, so the original journal is still current. Crash and wait sequentially + // to prove replaying the already-published candidate is idempotent. + hs()->crash_simulator().set_will_crash(true); + hs()->crash_simulator().crash(); + this->wait_for_crash_recovery(true); + ASSERT_GT(this->m_bt->get_btree_depth(), persisted_depth); + this->reapply_after_crash(); + this->get_all(); +} + TYPED_TEST(IndexCrashTest, long_running_put_crash) { long_running_crash_options crash_test_options{ .put_freq = 100, From 9042e6713130011b198a0caa59b640ee47b56f5c Mon Sep 17 00:00:00 2001 From: Jie Yao Date: Fri, 7 Aug 2026 14:46:05 +0800 Subject: [PATCH 2/2] add detailed comments and simplify code --- src/lib/index/index_cp.cpp | 26 +----- src/lib/index/wb_cache.cpp | 117 ++++++++++++++++-------- src/tests/test_index_crash_recovery.cpp | 90 ++++++++++++++++-- 3 files changed, 167 insertions(+), 66 deletions(-) diff --git a/src/lib/index/index_cp.cpp b/src/lib/index/index_cp.cpp index 4c080fb05..4afc1a6f5 100644 --- a/src/lib/index/index_cp.cpp +++ b/src/lib/index/index_cp.cpp @@ -273,12 +273,11 @@ std::map< BlkId, IndexBufferPtr > IndexCPContext::recover(sisl::byte_view sb) { // Root-change records contain no split/merge side effects. Retaining the last record per ordinal identifies // the final intended root even if the same CP grows and then collapses the tree. if (rec->is_parent_meta && rec->num_freed_ids == 0) { - if (!rec->has_inplace_child && rec->num_new_ids == 1) { - auto const new_root_idx = rec->has_inplace_parent ? 1 : 0; - m_recovered_root_ids[rec->index_ordinal] = rec->blk_id(new_root_idx); - } else if (rec->has_inplace_child && rec->num_new_ids == 0) { - auto const child_idx = rec->has_inplace_parent ? 1 : 0; - m_recovered_root_ids[rec->index_ordinal] = rec->blk_id(child_idx); + bool const is_root_split = !rec->has_inplace_child && rec->num_new_ids == 1; + bool const is_root_collapse = rec->has_inplace_child && rec->num_new_ids == 0; + if (is_root_split || is_root_collapse) { + auto const root_idx = rec->has_inplace_parent ? 1 : 0; + m_recovered_root_ids[rec->index_ordinal] = rec->blk_id(root_idx); } } @@ -300,22 +299,9 @@ std::map< BlkId, IndexBufferPtr > IndexCPContext::recover(sisl::byte_view sb) { buffer->m_up_buffer->to_string()); } }; -#if 0 - auto dag_print = [](const std::map< BlkId, IndexBufferPtr >& dags, std::string delimiter) { - int index = 1; - for (const auto& [blkid, bufferPtr] : dags) { - LOGTRACEMOD(wbcache, "{}{} - blkid {} buffer {} ", delimiter, index++, blkid.to_integer(), - bufferPtr->to_string()); - } - }; - LOGTRACEMOD(wbcache,"Before modify : \n "); - dag_print(buf_map, "Before: "); -#endif for (auto& [blkid, bufferPtr] : buf_map) { modifyBuffer(bufferPtr); } - // LOGTRACEMOD(wbcache,"\n\n\nAFTER modify : \n "); - // dag_print(buf_map, "After: "); auto sanityCheck = [cp_id = id()](const std::map< BlkId, IndexBufferPtr >& dags) { for (const auto& [blkid, bufferPtr] : dags) { @@ -408,8 +394,6 @@ void IndexCPContext::process_txn_record(txn_record const* rec, std::map< BlkId, } #ifndef NDEBUG - // if (!is_sibling_link || (buf->m_up_buffer == real_up_buf)) { return buf;} - // Already linked with same buf or its not a sibling link to override if (real_up_buf->is_in_down_buffers(buf)) { return buf; } #endif diff --git a/src/lib/index/wb_cache.cpp b/src/lib/index/wb_cache.cpp index c23a8c2ae..4d0fb8b2b 100644 --- a/src/lib/index/wb_cache.cpp +++ b/src/lib/index/wb_cache.cpp @@ -612,22 +612,14 @@ void IndexWBCache::recover(sisl::byte_view sb) { std::vector< IndexBufferPtr > pruned_bufs_to_repair; std::set< IndexBufferPtr > bufs_to_skip_sanity_check; std::map< uint32_t, IndexBufferPtr > new_root_candidates; - std::map< uint32_t, bool > persisted_root_commit_status; auto persisted_root_was_committed = [&](uint32_t ordinal) { - if (auto const it = persisted_root_commit_status.find(ordinal); it != persisted_root_commit_status.end()) { - return it->second; - } - auto const table = index_service().get_index_table(ordinal); - bool committed{false}; - if (table && table->persisted_root_node_id() != empty_bnodeid) { - auto const old_root_it = bufs.find(BlkId{table->persisted_root_node_id()}); - committed = old_root_it != bufs.end() && was_node_committed(old_root_it->second); - } - persisted_root_commit_status.emplace(ordinal, committed); - return committed; + if (!table || table->persisted_root_node_id() == empty_bnodeid) { return false; } + auto const it = bufs.find(BlkId{table->persisted_root_node_id()}); + return it != bufs.end() && was_node_committed(it->second); }; + auto commit_recovered_blk = [this](IndexBufferPtr const& buf) { auto const status = m_vdev->commit_blk(buf->m_blkid); HS_REL_ASSERT(status == BlkAllocStatus::SUCCESS, "Failed to commit recovered index block {}", @@ -648,12 +640,28 @@ void IndexWBCache::recover(sisl::byte_view sb) { // Meta buffers are journal placeholders rather than vdev blocks. if (!buf->is_meta_buf()) { load_buf(buf); } - auto const recovered_root_id = icp_ctx->recovered_root_id(buf->m_index_ordinal); - auto const is_final_root = - !buf->is_meta_buf() && recovered_root_id.is_valid() && recovered_root_id == buf->blkid(); - if (is_final_root && buf->m_created_cp_id != icp_ctx->id() && was_node_committed(buf) && - persisted_root_was_committed(buf->m_index_ordinal)) { + // Root-collapse recovery: the surviving child (C0) was promoted to root and + // written to disk, but the crash happened before the SB could record the new + // root pointer. + // + // During a root collapse the flush DAG is: + // meta_buf(SB) <-- C0(new root) <-- R(old root, freed) + // R is always flushed before C0, so was_node_committed(C0) implies R was also + // written. The tree is therefore in mid-collapse state on disk: R carries + // node_deleted=true, C0 is durable as the new root, but the SB still points + // to R. C0 must be promoted so the recovery CP can write the corrected SB. + // + // New-CP root candidates produced by a root split (where new_root_buf is + // created inside the crashed CP) do not reach this branch because their + // m_created_cp_id == icp_ctx->id(); they are handled in the is_meta_buf + // branch below. + auto const is_final_root = icp_ctx->recovered_root_id(buf->m_index_ordinal) == buf->blkid(); + if (is_final_root // journal's last root-change record for this ordinal points to C0 + && buf->m_created_cp_id < icp_ctx->id() // C0 predates the crashed CP, confirming root-collapse (not split) + && was_node_committed(buf)) // C0 is durable; because R precedes C0 in the DAG, R is also on disk + { new_root_candidates[buf->m_index_ordinal] = buf; + continue; // C0 is neither freed nor new-CP; no further processing applies } if (buf->m_node_freed) { @@ -695,9 +703,44 @@ void IndexWBCache::recover(sisl::byte_view sb) { } else if (buf->m_created_cp_id == icp_ctx->id()) { LOGTRACEMOD(wbcache, "recovering new buf {}", buf->to_string()); auto const buf_was_committed = was_node_committed(buf); - if (buf_was_committed && buf->m_up_buffer && buf->m_up_buffer->is_meta_buf()) { - // Every root-transition node under meta remains live when the old root was written. Only the final - // journal-selected node is published, but intermediate roots must also remain allocator-owned. + if (!buf_was_committed) { + // This node was never written in the crashed CP; discard it. + prune_from_up_buffer(buf); + bufs_to_skip_sanity_check.insert(buf); + } else if (buf->m_up_buffer && buf->m_up_buffer->is_meta_buf()) { + // Root-split recovery: buf is a new root node (new_root_buf) created in the + // crashed CP, sitting directly under the SB in the flush DAG: + // meta_buf(SB) <-- new_root_buf(new-CP) <-- old_root_buf(old-CP, modified) + // + // There are two sub-cases depending on how far the flush progressed: + // + // Case A — SB still points to old_root (crash before meta_buf was written): + // persisted_root_was_committed() looks up old_root_buf in bufs and calls + // was_node_committed(old_root_buf). Returns true only if old_root was + // flushed to disk, confirming the tree is in a consistent mid-split state + // (both old_root and new_root are on disk, only the SB pointer is stale). + // If old_root was NOT flushed, the split is half-done and new_root_buf must + // be discarded. + // + // Case B — SB already points to new_root (crash after meta_buf was written): + // persisted_root_was_committed() resolves the SB root to new_root_buf's own + // blkid, finds buf itself in bufs, and calls was_node_committed(buf). Since + // buf_was_committed is already true (we're in this branch), the function + // always returns true. The blkid is re-committed to the allocator to restore + // any in-memory bitmap state lost during the crash. set_root_from_committed_buf + // later detects that the SB root already matches new_root and only refreshes + // the in-memory root pointer without rewriting the SB. + // + // is_final_root is true when the journal's last root-change record for this + // ordinal points to buf, and false for intermediate new roots created by earlier + // splits in the same CP. When the same CP contains two or more root splits, + // link_buf's Condition 1 (flatten new-to-new links) keeps every intermediate new + // root directly under meta_buf rather than under the next new root, so all of + // them reach this branch. m_recovered_root_ids[ordinal] retains only the blkid + // from the last root-change journal entry (the final root), so intermediate roots + // have is_final_root == false. They are still committed in the allocator to + // prevent blkid reuse, because they were written to disk; but they are reachable + // from the final new root as ordinary interior nodes and must not be re-promoted. if (persisted_root_was_committed(buf->m_index_ordinal)) { commit_recovered_blk(buf); if (is_final_root) { @@ -710,19 +753,22 @@ void IndexWBCache::recover(sisl::byte_view sb) { prune_from_up_buffer(buf); bufs_to_skip_sanity_check.insert(buf); } - } else if (buf_was_committed && was_node_committed(buf->m_up_buffer)) { - // Both current and up buffer is committed, we can safely commit the current block - LOGTRACEMOD(wbcache, "New buffer {} and the up buffer {} are committed", buf->to_string(), - buf->m_up_buffer->to_string()); - commit_recovered_blk(buf); - pending_bufs.push_back(buf->m_up_buffer); } 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()); - prune_from_up_buffer(buf); - // Skip the sanity check on this buf as we do not keep it - bufs_to_skip_sanity_check.insert(buf); + // Non-root-split new node: every new-CP node must have an up_buffer in the DAG. + HS_DBG_ASSERT(buf->m_up_buffer, "New-CP buf {} has no up_buffer", buf->to_string()); + if (was_node_committed(buf->m_up_buffer)) { + // Both this node and its parent are on disk; safe to commit. + LOGTRACEMOD(wbcache, "New buffer {} and the up buffer {} are committed", buf->to_string(), + buf->m_up_buffer->to_string()); + commit_recovered_blk(buf); + pending_bufs.push_back(buf->m_up_buffer); + } else { + // Parent is not yet on disk; discard this node and let the parent be repaired. + LOGTRACEMOD(wbcache, "The up buffer {} is not committed for the new buffer {}", + buf->m_up_buffer->to_string(), buf->to_string()); + prune_from_up_buffer(buf); + bufs_to_skip_sanity_check.insert(buf); + } } } } @@ -881,7 +927,6 @@ bool IndexWBCache::was_node_committed(IndexBufferPtr const& buf) { return was_node_committed(buf->m_up_buffer); } - // All down_buf has indicated that they have seen this up buffer, now its time to repair them. load_buf(buf); if (!BtreeNode::is_valid_node(sisl::blob{buf->m_bytes, m_node_size})) { return false; } return (buf->m_dirtied_cp_id == cp_mgr().cp_guard()->id()); @@ -909,12 +954,6 @@ folly::Future< bool > IndexWBCache::async_cp_flush(IndexCPContext* cp_ctx) { cp_ctx->id(), cp_ctx->m_dirty_buf_count.get(), cp_ctx->m_num_nodes_added.load(), cp_ctx->m_num_nodes_removed.load()); LOGTRACEMOD(wbcache, "Index CP Flush with cp {}, \ndag={}", cp_ctx->id(), cp_ctx->to_string_with_dags()); - // #ifdef _PRERELEASE - // static int id = 0; - // auto filename = "cp_" + std::to_string(id++) + "_" + std::to_string(rand() % 100) + ".dot"; - // LOGTRACEMOD(wbcache, "Transact cp storing in file {}\n\n\n", filename); - // cp_ctx->to_string_dot(filename); - // #endif if (!cp_ctx->any_dirty_buffers()) { LOGINFO("Flush the vdev to ensure all cp information is created"); // Always try to flush, will be a no-op when not needed diff --git a/src/tests/test_index_crash_recovery.cpp b/src/tests/test_index_crash_recovery.cpp index 25e68f496..40381e44e 100644 --- a/src/tests/test_index_crash_recovery.cpp +++ b/src/tests/test_index_crash_recovery.cpp @@ -867,7 +867,20 @@ TYPED_TEST(IndexCrashTest, SplitCrash1) { } } -// Cover the first root split after a leaf root has already been made durable. +// Scenario: first root split (depth 0 → 1), crash while writing the SB (meta_buf). +// +// Setup: insert max_keys/2 keys and checkpoint to establish a durable leaf root (depth=0). +// Then insert more keys until the first root split fires (depth becomes 1), with +// "crash_flush_on_meta" armed so that the crash fires the moment the SB write begins. +// +// Disk state at crash: +// - new_root_buf is durable (Fix 2 pre-flush wrote it before the normal DAG flush). +// - old_root (the modified leaf) is durable in split state. +// - SB still names the old_root as root. +// +// Expected recovery (Fix 2): the journal identifies new_root_buf as the intended root, +// persisted_root_was_committed() confirms old_root was written, so new_root_buf is +// promoted. After recovery depth == 1 and all keys are intact. TYPED_TEST(IndexCrashTest, CrashAtMetaBufOnFirstRootSplit) { const uint32_t max_keys = SISL_OPTIONS["max_keys_in_node"].as< uint32_t >(); const uint32_t durable_key_count = max_keys / 2; @@ -897,7 +910,21 @@ TYPED_TEST(IndexCrashTest, CrashAtMetaBufOnFirstRootSplit) { this->get_all(); } -// Cover the first root split window after the modified leaf root is durable but before its new root is published. +// Scenario: first root split (depth 0 → 1), crash after old_root is flushed but before +// new_root_buf is written. +// +// Setup: same as CrashAtMetaBufOnFirstRootSplit, but "crash_flush_on_root" fires when +// the new_root_buf write begins, so old_root reaches disk in its split state while +// new_root_buf has not yet been written. +// +// Disk state at crash: +// - old_root is durable with edge_info=EMPTY and next_bnode=child_node2 (split state). +// - new_root_buf has NOT been written (Fix 2 pre-flush was interrupted). +// - SB still names old_root. +// +// Expected recovery: Fix 2 pre-flush guarantees new_root_buf is written before old_root +// reaches disk (Fix 2 barrier), so new_root_buf must be durable. Recovery identifies +// it via the journal and promotes it. After recovery depth == 1 and all keys are intact. TYPED_TEST(IndexCrashTest, CrashAfterOldRootFlushOnFirstRootSplit) { const uint32_t max_keys = SISL_OPTIONS["max_keys_in_node"].as< uint32_t >(); const uint32_t durable_key_count = max_keys / 2; @@ -927,7 +954,23 @@ TYPED_TEST(IndexCrashTest, CrashAfterOldRootFlushOnFirstRootSplit) { this->get_all(); } -// A partial preflush must leave the durable old root authoritative and safely discard the incomplete transition. +// Scenario: first root split (depth 0 → 1), crash during Fix 2's pre-flush barrier +// before any node of the split reaches disk. +// +// Setup: same initial state (durable leaf root at depth=0), but "crash_during_root_preflush" +// fires inside the async pre-flush writes, before the normal DAG flush starts. +// crash_simulator.set_will_crash(true) is called explicitly because this flip fires +// before the CP engine's own crash point. +// +// Disk state at crash: +// - Neither new_root_buf nor old_root has been written in the crashed CP. +// - The tree on disk is still in the pre-split consistent state (depth=0, old leaf root intact). +// - SB still names the original durable leaf root. +// +// Expected recovery: because old_root was never written in split state, +// persisted_root_was_committed() returns false, new_root_buf is discarded, and the tree +// reverts to its last fully consistent checkpoint. After recovery root_node_id equals +// durable_root and depth == 0. reapply_after_crash re-inserts all post-CP keys. TYPED_TEST(IndexCrashTest, CrashDuringRootPreflushOnFirstRootSplit) { const uint32_t max_keys = SISL_OPTIONS["max_keys_in_node"].as< uint32_t >(); const uint32_t durable_key_count = max_keys / 2; @@ -959,7 +1002,20 @@ TYPED_TEST(IndexCrashTest, CrashDuringRootPreflushOnFirstRootSplit) { this->get_all(); } -// Recovery must publish the durable new root if the table superblock still names the modified old root. +// Scenario: second (or higher) root split (depth N → N+1), crash while writing the SB. +// +// Setup: insert max_keys+1 keys and checkpoint to establish a durable level-1 root. +// Then insert max_keys*max_keys more keys to trigger one or more additional root splits, +// with "crash_flush_on_meta" armed so the crash fires when the SB write begins. +// +// Disk state at crash: +// - new_root_buf (and any intermediate new roots) are durable via Fix 2 pre-flush. +// - old_root (level-1 internal node) is durable in split state. +// - SB still names the level-1 old_root. +// +// Expected recovery: same Fix 2 path as the first-split cases, but exercised on a +// multi-level tree to confirm that the journal-based root promotion works regardless of +// tree height. After recovery depth > persisted_depth and all keys are intact. TYPED_TEST(IndexCrashTest, CrashAtMetaBufOnSecondRootSplit) { const uint32_t max_keys = SISL_OPTIONS["max_keys_in_node"].as< uint32_t >(); @@ -989,8 +1045,30 @@ TYPED_TEST(IndexCrashTest, CrashAtMetaBufOnSecondRootSplit) { this->get_all(); } -// Cover the window where the old root is durable but the table superblock is stale, then restart again to verify that -// recovery persisted the promoted root. +// Scenario: second (or higher) root split, crash after old_root is flushed but before +// new_root_buf is written; then crash a second time without a recovery CP to prove +// that replaying the same journal and re-promoting the same root is idempotent. +// +// Setup: establish a durable level-1 root, then arm both "crash_flush_on_root" and +// "skip_cp_after_index_root_recovery". The first flip causes the crash after old_root +// hits disk; the second flip suppresses the forced recovery CP so the original journal +// remains on disk unchanged after the first recovery. +// +// Disk state at first crash: +// - old_root is durable in split state; new_root_buf is durable (Fix 2 pre-flush). +// - SB still names old_root. +// +// First recovery: Fix 2 promotes new_root_buf; depth > persisted_depth. +// +// Second crash (immediate, no recovery CP written): +// - The journal on disk still records the same root-change. +// - new_root_buf is already the in-memory root, and its blkid is already in the SB +// (written by set_root_from_committed_buf during the first recovery). +// +// Second recovery: the journal candidate is re-evaluated; set_root_from_committed_buf +// detects the SB already names new_root_buf and is a no-op. This verifies that +// promoting an already-promoted root does not corrupt the tree. +// After both recoveries depth > persisted_depth and all keys are intact. TYPED_TEST(IndexCrashTest, CrashAfterOldRootFlushOnSecondRootSplit) { const uint32_t max_keys = SISL_OPTIONS["max_keys_in_node"].as< uint32_t >();