Fixed race between cp_flush() and reset()

conanfile.py

@@ -9,7 +9,7 @@
 
 class HomestoreConan(ConanFile):
     name = "homestore"
-    version = "8.3.6"
+    version = "8.3.7"
 
     homepage = "https://github.com/eBay/Homestore"
     description = "HomeStore Storage Engine"

src/lib/blkalloc/bitmap_blk_allocator.cpp

@@ -15,6 +15,7 @@
 #include <homestore/homestore.hpp>
 #include <homestore/meta_service.hpp>
 #include <homestore/checkpoint/cp_mgr.hpp>
+#include <iomgr/iomgr_flip.hpp>
 #include "bitmap_blk_allocator.h"
 #include "meta/meta_sb.hpp"
 #include "common/homestore_utils.hpp"
@@ -71,9 +72,22 @@ void BitmapBlkAllocator::on_meta_recovery_completed() {
 void BitmapBlkAllocator::cp_flush(CP*) {
     if (!is_persistent()) { return; }
 
+    // Clean bitmaps skip the lock; anything that dirties one after this check is written by the next CP
+    if (!m_is_disk_bm_dirty.load()) { return; }
+
+    std::lock_guard lg(m_sb_mtx);
+    // A reset allocator has been replaced by a new one for its chunk; its bitmap must not be written anymore
+    if (m_retired) { return; }
+
     if (m_is_disk_bm_dirty.load()) {
         sisl::byte_array bitmap_buf = acquire_underlying_buffer();
         if (m_meta_blk_cookie) {
+#ifdef _PRERELEASE
+            // Flip point: cp_flush is about to update the existing bitmap meta blk
+            if (iomgr_flip::instance()->callback_flip("inside_bitmap_cp_flush", static_cast< uint16_t >(m_chunk_id))) {
+                LOGINFO("Flip triggered: inside_bitmap_cp_flush for chunk_id={}", m_chunk_id);
+            }
+#endif
             meta_service().update_sub_sb(bitmap_buf->cbytes(), bitmap_buf->size(), m_meta_blk_cookie);
         } else {
             meta_service().add_sub_sb(get_name(), bitmap_buf->cbytes(), bitmap_buf->size(), m_meta_blk_cookie);
@@ -201,6 +215,16 @@ sisl::ThreadVector< multi_blk_id >* BitmapBlkAllocator::get_alloc_blk_list() {
 }
 
 void BitmapBlkAllocator::reset() {
+    // The chunk gets a new allocator after this, but a cp_flush may still hold this one (VirtualDev::cp_flush takes a
+    // shared_ptr to it). Flushing it after the meta blk is removed would either update the removed meta blk or write
+    // the bitmap from before the reset as a new one, which the chunk then recovers after a crash. Under m_sb_mtx, such
+    // a cp_flush either finishes before the meta blk is removed or finds this allocator retired and skips it.
+    //
+    // Anything that still reaches this allocator after the reset (a commit, free or alloc) is discarded with it by
+    // design: the chunk's contents are gone, and callers must not allocate on a chunk while it is being reset.
+    std::lock_guard lg(m_sb_mtx);
+    m_retired = true;
+
     // use the reset method to remove the metablk
     if (m_meta_blk_cookie) {
         meta_service().remove_sub_sb(m_meta_blk_cookie);

src/lib/blkalloc/bitmap_blk_allocator.h

@@ -126,6 +126,8 @@ class BitmapBlkAllocator : public BlkAllocator {
     std::unique_ptr< BlkAllocPortion[] > m_blk_portions;
     std::unique_ptr< sisl::Bitset > m_disk_bm{nullptr};
     std::atomic< bool > m_is_disk_bm_dirty{true}; // initially disk_bm treated as dirty
+    std::mutex m_sb_mtx;                          // Serializes cp flush and reset on the bitmap meta blk
+    bool m_retired{false}; // Set by reset(): the chunk has a new allocator, and this one is no longer flushed
     meta_blk* m_meta_blk_cookie{nullptr};
     std::atomic< int64_t > m_alloced_blk_count{0};
 };

src/tests/test_data_service.cpp

@@ -1398,6 +1398,166 @@ TEST_F(BlkDataServiceAppendTest, TestFreeAfterResetBeforeCpFlushSafe) {
 
 #endif // _PRERELEASE
 
+#ifdef _PRERELEASE
+// Race between cp_flush and reset_block_allocator() on a chunk with a bitmap allocator (fixed or varsize).
+// VirtualDev::cp_flush takes a shared_ptr to the chunk's allocator and flushes it while the upper layer may reset the
+// chunk. Neither the bitmap from before the reset nor a crash may come out of that: after a restart without another CP,
+// the chunk must come back empty.
+class BlkDataServiceBitmapResetTest : public testing::TestWithParam< homestore::blk_allocator_type_t > {
+public:
+    blk_data_service& inst() { return homestore::data_service(); }
+
+    virtual void SetUp() override {
+        m_helper.start_homestore("test_data_service_bitmap_reset",
+                                 {{HS_SERVICE::META, {.size_pct = 5.0}},
+                                  {HS_SERVICE::DATA, {.size_pct = 80.0, .blkalloc_type = GetParam()}}});
+    }
+
+    virtual void TearDown() override {
+        auto* fc = iomgr_flip::client_instance();
+        for (auto const& name : m_flips) {
+            fc->remove_flip(name);
+        }
+        m_helper.shutdown_homestore();
+    }
+
+protected:
+    // Waits for flag, so that a missed step fails the test instead of hanging it
+    static bool wait_for(std::atomic< bool > const& flag) {
+        auto const deadline = std::chrono::steady_clock::now() + std::chrono::seconds{30};
+        while (!flag.load()) {
+            if (std::chrono::steady_clock::now() >= deadline) { return false; }
+            std::this_thread::sleep_for(std::chrono::milliseconds{1});
+        }
+        return true;
+    }
+
+    // Runs cb in the thread that hits the flip, the first time it is hit for chunk_id. Later hits, from other chunks or
+    // from CPs that run after the one under test, pass through.
+    void inject_chunk_flip(std::string const& name, chunk_num_t chunk_id, std::function< void() > cb) {
+        auto* fc = iomgr_flip::client_instance();
+        flip::FlipFrequency freq;
+        freq.set_count(1000);
+        freq.set_percent(100);
+        auto fired = std::make_shared< std::atomic< bool > >(false);
+        fc->inject_callback_flip< void, uint16_t >(
+            name, std::array< flip::FlipCondition, 1 >{fc->create_condition("", flip::Operator::DONT_CARE, 0)}, freq,
+            std::function< void(uint16_t) >([chunk_id, fired, cb = std::move(cb)](uint16_t cid) {
+                if ((cid == chunk_id) && !fired->exchange(true)) { cb(); }
+            }));
+        m_flips.push_back(name);
+    }
+
+    // Commits blks on one chunk and flushes them to its persisted bitmap. Also allocates one more blk on that chunk
+    // without committing it, for the test to commit when it wants the bitmap dirty.
+    void prepare_chunk(homestore::Chunk*& chunk, multi_blk_id& pending) {
+        auto const blk_size = inst().get_blk_size();
+        auto bids = inst().alloc_blks(blk_size, blk_alloc_hints{});
+        ASSERT_TRUE(bids.has_value());
+        ASSERT_TRUE(inst().commit_blk(bids.value()).has_value());
+        ASSERT_TRUE(sisl::async::sync_get(hs()->cp_mgr().trigger_cp_flush(true /* force */)));
+
+        auto const chunk_id = bids.value().chunk_num();
+        blk_alloc_hints hints;
+        hints.chunk_id_hint = chunk_id;
+        auto more = inst().alloc_blks(blk_size, hints);
+        ASSERT_TRUE(more.has_value());
+        ASSERT_EQ(more.value().chunk_num(), chunk_id);
+        pending = more.value();
+
+        chunk = hs()->device_mgr()->get_chunk_mutable(chunk_id);
+        ASSERT_NE(chunk, nullptr);
+        ASSERT_LT(chunk->blk_allocator()->available_blks(), chunk->blk_allocator()->get_total_blks());
+    }
+
+    // Restarts without another CP and verifies the reset chunk came back empty
+    void crash_and_verify_chunk_empty(chunk_num_t chunk_id) {
+        hs()->crash_simulator().crash();
+        m_helper.wait_for_crash_recovery();
+
+        auto* chunk = hs()->device_mgr()->get_chunk_mutable(chunk_id);
+        ASSERT_NE(chunk, nullptr);
+        EXPECT_EQ(chunk->blk_allocator()->available_blks(), chunk->blk_allocator()->get_total_blks())
+            << "The bitmap from before the reset came back";
+    }
+
+    test_common::HSTestHelper m_helper;
+    std::vector< std::string > m_flips;
+};
+
+// Order: cp_flush gets the chunk's allocator -> reset_block_allocator() completes -> cp_flush flushes that allocator
+TEST_P(BlkDataServiceBitmapResetTest, ResetWhileCpFlushHoldsAllocator) {
+    homestore::Chunk* chunk{nullptr};
+    multi_blk_id pending;
+    ASSERT_NO_FATAL_FAILURE(prepare_chunk(chunk, pending));
+    auto const chunk_id = chunk->chunk_id();
+    ASSERT_TRUE(inst().commit_blk(pending).has_value());
+
+    std::atomic< bool > flush_got_allocator{false};
+    std::atomic< bool > reset_completed{false};
+    inject_chunk_flip("after_get_allocator_shared", chunk_id, [&]() {
+        flush_got_allocator.store(true);
+        EXPECT_TRUE(wait_for(reset_completed)) << "reset_block_allocator should complete before cp_flush continues";
+    });
+    inject_chunk_flip("before_allocator_reset", chunk_id, [&]() {
+        EXPECT_TRUE(wait_for(flush_got_allocator)) << "cp_flush should get the allocator first";
+    });
+
+    std::thread reset_thread([&]() {
+        chunk->reset_block_allocator();
+        reset_completed.store(true);
+    });
+    EXPECT_TRUE(sisl::async::sync_get(hs()->cp_mgr().trigger_cp_flush(true /* force */)));
+    reset_thread.join();
+
+    crash_and_verify_chunk_empty(chunk_id);
+}
+
+// Order: cp_flush holds the bitmap lock and is about to write the bitmap -> reset_block_allocator() starts -> the reset
+// waits for that write to finish before it clears the bitmap
+TEST_P(BlkDataServiceBitmapResetTest, ResetWhileCpFlushWritesBitmap) {
+    homestore::Chunk* chunk{nullptr};
+    multi_blk_id pending;
+    ASSERT_NO_FATAL_FAILURE(prepare_chunk(chunk, pending));
+    auto const chunk_id = chunk->chunk_id();
+
+    std::atomic< bool > flush_inside{false};
+    std::atomic< bool > reset_started{false};
+    std::atomic< bool > reset_completed{false};
+    // Commit as the CP gets to the chunk, so the bitmap is dirty in whichever CP that is
+    inject_chunk_flip("after_get_allocator_shared", chunk_id,
+                      [&]() { EXPECT_TRUE(inst().commit_blk(pending).has_value()); });
+    inject_chunk_flip("inside_bitmap_cp_flush", chunk_id, [&]() {
+        flush_inside.store(true);
+        EXPECT_TRUE(wait_for(reset_started)) << "reset_block_allocator should start while cp_flush writes the bitmap";
+        // The reset needs the bitmap lock this cp_flush holds, so it can't get anywhere until the write is done
+        std::this_thread::sleep_for(std::chrono::milliseconds{200});
+        EXPECT_FALSE(reset_completed.load()) << "reset_block_allocator completed while cp_flush was writing the bitmap";
+    });
+    inject_chunk_flip("before_allocator_reset", chunk_id, [&]() {
+        EXPECT_TRUE(wait_for(flush_inside)) << "cp_flush should be about to write the bitmap first";
+        reset_started.store(true);
+    });
+
+    std::thread reset_thread([&]() {
+        chunk->reset_block_allocator();
+        reset_completed.store(true);
+    });
+    EXPECT_TRUE(sisl::async::sync_get(hs()->cp_mgr().trigger_cp_flush(true /* force */)));
+    reset_thread.join();
+
+    crash_and_verify_chunk_empty(chunk_id);
+}
+
+INSTANTIATE_TEST_SUITE_P(BitmapAllocators, BlkDataServiceBitmapResetTest,
+                         testing::Values(homestore::blk_allocator_type_t::varsize,
+                                         homestore::blk_allocator_type_t::fixed),
+                         [](testing::TestParamInfo< homestore::blk_allocator_type_t > const& info) {
+                             return std::string{(info.param == homestore::blk_allocator_type_t::fixed) ? "Fixed"
+                                                                                                       : "Varsize"};
+                         });
+#endif // _PRERELEASE
+
 // Stream related test
 
 SISL_OPTION_GROUP(test_data_service,