SDSTOR-22729: index/wb_cache: Fix recovery corruption after root-split crash

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"

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;

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<int>(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

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,17 @@ 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) {
+            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);
+            }
+        }
+
         process_txn_record(rec, buf_map);
         cur_ptr += rec->size();
         LOGTRACEMOD(wbcache, "Recovered txn record: {}: {}", t, rec->to_string());
@@ -264,24 +299,11 @@ 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 = [](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 +312,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 +355,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 +381,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;
@@ -367,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
 
@@ -391,9 +416,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) {

src/lib/index/index_cp.hpp

@@ -15,6 +15,8 @@
  *********************************************************************************/
 #pragma once
 #include <atomic>
+#include <set>
+#include <unordered_set>
 #include <sisl/fds/concurrent_insert_vector.hpp>
 #include <homestore/blk.h>
 #include <homestore/index/index_internal.hpp>
@@ -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;

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 <iomgr/iomgr_flip.hpp>
+#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 */);