Index write back cache fixes. #207

sanebay · 2023-10-16T20:38:16Z

Add locks for indexbuffer to avoid concurrency issues with cp flush and insert threads.
Create dependency graph.
Add locks for index btree test code shadow map as there are concurrent requests.
Use shared ptr's in wb cache.

Testing. Run the index_btree continuously back to back for several hours.
./../failuntil.sh ./bin/test_index_btree --gtest_filter=BtreeTest/0.* --num_entries 100000

codecov-commenter · 2023-10-19T05:09:59Z

Codecov Report

Attention: 43 lines in your changes are missing coverage. Please review.

Comparison is base (c979057) 73.52% compared to head (5ebfae2) 69.75%.
Report is 1 commits behind head on master.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #207      +/-   ##
==========================================
- Coverage   73.52%   69.75%   -3.77%     
==========================================
  Files         120       97      -23     
  Lines       11926     7941    -3985     
  Branches     1442     1022     -420     
==========================================
- Hits         8768     5539    -3229     
+ Misses       2519     1899     -620     
+ Partials      639      503     -136

Files	Coverage Δ
.../include/homestore/btree/detail/btree_node_mgr.ipp	`71.97% <ø> (ø)`
src/include/homestore/index/wb_cache_base.hpp	`100.00% <ø> (ø)`
src/include/homestore/index_service.hpp	`50.00% <ø> (ø)`
src/lib/checkpoint/cp.hpp	`100.00% <ø> (ø)`
src/lib/index/index_service.cpp	`76.92% <100.00%> (+4.58%)`	⬆️
src/lib/checkpoint/cp_mgr.cpp	`78.89% <75.00%> (+0.55%)`	⬆️
src/lib/index/wb_cache.cpp	`85.89% <96.55%> (+0.86%)`	⬆️
src/include/homestore/index/index_table.hpp	`85.47% <22.22%> (-1.81%)`	⬇️
src/include/homestore/index/index_internal.hpp	`38.46% <28.57%> (-28.21%)`	⬇️
src/lib/index/index_cp.hpp	`36.11% <5.00%> (-37.58%)`	⬇️

... and 39 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

shosseinimotlagh

Please add git comments and explain the issue and the fix in description.

sanebay · 2023-10-25T17:40:07Z

src/include/homestore/btree/detail/btree_node_mgr.ipp

@@ -307,7 +307,7 @@ BtreeNode* Btree< K, V >::init_node(uint8_t* node_buf, uint32_t node_ctx_size, b
 /* Note:- This function assumes that access of this node is thread safe. */
 template < typename K, typename V >
 void Btree< K, V >::free_node(const BtreeNodePtr& node, locktype_t cur_lock, void* context) {
-    BT_NODE_LOG(DEBUG, node, "Freeing node");
+    BT_NODE_LOG(TRACE, node, "Freeing node");


Too much trace in logs during shutdown.

sanebay · 2023-10-25T17:40:19Z

src/include/homestore/btree/detail/prefix_node.hpp

@@ -687,7 +687,7 @@ class FixedPrefixNode : public VariantNode< K, V > {
            --phdr->used_slots;
            prefix_bitset_.reset_bit(slot_num);
            if ((slot_num != 0) && (slot_num == phdr->tail_slot)) {
-                uint16_t prev_slot = prefix_bitset_.get_prev_set_bit(slot_num);
+                uint16_t prev_slot = prefix_bitset_.get_next_set_bit(slot_num);


will remove once rebased to latest.

sanebay · 2023-10-25T17:40:45Z

src/include/homestore/index/index_internal.hpp

+    bool is_clean() const {
+        RELEASE_ASSERT(m_node_buf, "Node buffer null");
+        return (m_node_buf->m_state.load() == index_buf_state_t::CLEAN);
+    }


added some wrapper functions and to_string cleaner

sanebay · 2023-10-25T17:41:37Z

src/include/homestore/index/index_table.hpp

+            str = fmt::format("cp {} left {} parent {}", cp_ctx->id(), left_child_buf->to_string(),
+                              parent_buf->to_string());
+            for (const auto& right_child_node : new_nodes) {
+                auto right_child = IndexBtreeNode::convert(right_child_node.get());


Add these trace logs only if its enabled. hopefully trace_index_bufs only called when logtrace enabled.

sanebay · 2023-10-25T17:42:00Z

src/include/homestore/index/index_table.hpp

        // buffer prior to this copy, would have been written and already added into the dirty buffer list.
-        idx_node->m_idx_buf = wb_cache().copy_buffer(idx_node->m_idx_buf);
+        idx_node->m_idx_buf = wb_cache().copy_buffer(idx_node->m_idx_buf, cp_ctx);


Creating per cp IndexBuffer for writes.

sanebay · 2023-10-25T17:42:48Z

src/lib/index/index_cp.hpp

@@ -47,11 +47,6 @@ struct IndexCPContext : public CPContext {
            CPContext(cp_id), m_dirty_buf_list{dirty_list}, m_free_node_blkid_list{free_blkid_list} {}

    virtual ~IndexCPContext() {
-        auto it = m_dirty_buf_list->begin(true /* latest */);
-        IndexBufferPtr *tmp = nullptr;
-        while((tmp = m_dirty_buf_list->next(it)) != nullptr) {


m_dirty_buf_list->clear() will cleanup

sanebay · 2023-10-25T17:43:24Z

src/lib/index/index_cp.hpp

+        // Mapping from a node to all its parents in the graph.
+        // Display all buffers and its dependencies and state.
+        std::unordered_map< IndexBuffer*, std::vector< IndexBuffer* > > parents;
+        IndexBufferPtr* buf;


show a buf and dependencies
ex:
IndexBuffer 0x60b000055a80 blkid=562954248388608 state=1 node_buf=0x6210000a3e00 next_buffer=0x60b000055870 wait_for=2
Depends: 0x60b00004bdc0(1) 0x60b000055b30(1)

sanebay · 2023-10-25T17:43:42Z

src/lib/index/index_cp.hpp

+    void check_cycle() {
+        // Use dfs to find if the graph is cycle
+        IndexBufferPtr* buf;
+        auto it = m_dirty_buf_list->begin(true /* latest */);


Utility functions to find cycle and validate leader count.

sanebay · 2023-10-25T17:44:09Z

src/lib/index/wb_cache.cpp

+
+    // When we copy the buffer we check if the node buffer is clean or not. If its clean
+    // we could reuse it otherwise create a copy.
+    if (cur_buf->is_clean()) {


Comments added.

sanebay · 2023-10-25T17:44:25Z

src/lib/index/wb_cache.cpp

+    third->m_wait_for_leaders.increment(1);
+    if (chain != second) {
+        // We want buffers to be append to the end of the chain which are related.
+        // If we split a node multiple times in same or different CP's, each dirty buffer will be


Details in the comments

sanebay · 2023-10-25T17:44:41Z

src/lib/index/wb_cache.cpp

-    // TODO the index buffer are added to end of the chain, instead add to the dependency.
-    auto& last_in_chain = r_cast< IndexCPContext* >(cp_ctx)->m_last_in_chain;
-    if (last_in_chain) {
-        // Add this to the end of the chain.


This last_in_chain was not used in the past. Removed it.

sanebay · 2023-10-25T17:45:22Z

src/lib/index/wb_cache.cpp

-    buf->m_buf_state = index_buf_state_t::FLUSHING;
+void IndexWBCache::do_flush_one_buf(IndexCPContext* cp_ctx, IndexBufferPtr buf, bool part_of_batch) {
+    LOGTRACEMOD(wbcache, "cp {} buf {}", cp_ctx->id(), buf->to_string());
+    { buf->set_state(index_buf_state_t::FLUSHING); }
    m_vdev->async_write(r_cast< const char* >(buf->raw_buffer()), m_node_size, buf->m_blkid, part_of_batch)


Instead of using IndexBuffer* , using IndexBufferPtr for safety and consistency.

sanebay · 2023-10-25T17:46:02Z

src/tests/btree_helpers/btree_test_helper.hpp

@@ -70,6 +70,7 @@ struct BtreeTestHelper : public testing::Test {
 protected:
    std::shared_ptr< typename T::BtreeType > m_bt;
    ShadowMap< K, V > m_shadow_map;
+    std::mutex m_shadow_map_mutex;


Lock needed for the map as below api's are called multi threaded.

sanebay · 2023-10-25T17:46:41Z

src/tests/btree_helpers/btree_test_helper.hpp

+        std::ifstream b(before, std::ifstream::ate);
+        std::ifstream a(after, std::ifstream::ate);
+        if (a.fail() || b.fail()) {
+            LOGINFO("Failed to open file");


Too much memory and overhead comparing large files earlier. So read in chunks and compare. Optimize to compare size first.

sanebay · 2023-10-25T17:47:23Z

src/tests/test_index_btree.cpp

-        while (!stop_cp_flush) {
-            LOGINFO("Trigger checkpoint flush wait=false.");
-            test_common::HSTestHelper::trigger_cp(false /* wait */);
+    auto remove_io_thread = std::thread([this, &stop, num_entries, &last_index] {


Run insert, remove and flush cp threads in parallel.

src/tests/test_index_btree.cpp

src/include/homestore/index/index_internal.hpp

hkadayam · 2023-10-27T15:29:07Z

src/include/homestore/index/index_internal.hpp

@@ -64,29 +64,70 @@ enum class index_buf_state_t : uint8_t {
 };

 ///////////////////////// Btree Node and Buffer Portion //////////////////////////
+


General comment, it would be very useful if we put a README under wb_cache directory to illustrate the hierarchy of btree_node -> IndexBuffer and NodeBuffer relationship and its clean/dirty copy behavior.

And it will be super helpful if we can explain first, second and third meaning while creating the chain with an example, so that after several month we can read the code easier. :)

src/lib/index/wb_cache.cpp

src/include/homestore/index/index_internal.hpp

src/lib/index/wb_cache.cpp

src/lib/index/index_cp.hpp

src/lib/index/wb_cache.cpp

src/include/homestore/index/index_internal.hpp

src/lib/index/wb_cache.cpp

src/lib/index/wb_cache.hpp

src/lib/index/wb_cache.cpp

src/include/homestore/index/wb_cache_base.hpp

src/tests/btree_helpers/shadow_map.hpp

hkadayam

Looks good to me

shosseinimotlagh

LGTM

Add atomic for state for indexbuffer to avoid concurrency issues with cp flush and insert threads. Create per CP indexbuffer. Create NodeBuffer which points to actual data buffer. Several indexbuffer an can point to same node buffer. Add locks in test for index btree shadow map as there are concurrent requests. Use shared ptr's in wb cache.

sanebay requested review from hkadayam, yamingk and shosseinimotlagh October 16, 2023 20:38

sanebay force-pushed the index_wb_fixes branch from a308448 to 6971f4c Compare October 19, 2023 04:41

shosseinimotlagh reviewed Oct 24, 2023

View reviewed changes

sanebay force-pushed the index_wb_fixes branch 2 times, most recently from 9a9af75 to 8d92ff4 Compare October 25, 2023 17:39

sanebay commented Oct 25, 2023

View reviewed changes

shosseinimotlagh reviewed Oct 25, 2023

View reviewed changes

src/tests/test_index_btree.cpp Show resolved Hide resolved