Volume destroy data race

conanfile.py

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

src/lib/replication/service/generic_repl_svc.cpp

@@ -79,7 +79,7 @@ hs_stats GenericReplService::get_cap_stats() const {
 
 ///////////////////// SoloReplService specializations and CP Callbacks /////////////////////////////
 SoloReplService::SoloReplService(cshared< ReplApplication >& repl_app) : GenericReplService{repl_app} {}
-SoloReplService::~SoloReplService() {};
+SoloReplService::~SoloReplService(){};
 
 void SoloReplService::start() {
     for (auto const& [buf, mblk] : m_sb_bufs) {
@@ -151,23 +151,19 @@ folly::SemiFuture< ReplServiceError > SoloReplService::remove_repl_dev(group_id_
 
     auto rdev_ptr = rdev.value();
 
-    // 1. Firstly stop the repl dev which waits for any outstanding requests to finish
+    // 1. Stop: wait for outstanding requests
     rdev_ptr->stop();
 
-    // 2. Destroy the repl dev which will remove the logstore and free the memory;
-    dp_cast< SoloReplDev >(rdev_ptr)->destroy();
-
-    // 3. detaches both ways:
-    // detach rdev from its listener and listener from rdev;
-    rdev_ptr->detach_listener();
+    // 2. Remove from rd map first so CP flush/cleanup can no longer see this rdev.
+    // Taking the unique lock also waits out any in-flight iterate_repl_devs().
     {
-        // 4. remove from rd map which finally call SoloReplDev's destructor because this is the last one holding ref to
-        // this instance;
         std::unique_lock lg(m_rd_map_mtx);
         m_rd_map.erase(group_id);
     }
 
-    // 5. now destroy the upper layer's listener instance;
+    // 3. Now it is safe to destroy logstore/logdev and the superblk
+    dp_cast< SoloReplDev >(rdev_ptr)->destroy();
+    rdev_ptr->detach_listener();
     m_repl_app->destroy_repl_dev_listener(group_id);
 
     return folly::makeSemiFuture(ReplServiceError::OK);

src/lib/replication/service/raft_repl_service.h

@@ -138,7 +138,7 @@ class ReplSvcCPContext : public CPContext {
     std::map< ReplDev*, cshared< ReplDevCPContext > > m_cp_ctx_map;
 
 public:
-    ReplSvcCPContext(CP* cp) : CPContext(cp) {};
+    ReplSvcCPContext(CP* cp) : CPContext(cp){};
     virtual ~ReplSvcCPContext() = default;
     int add_repl_dev_ctx(ReplDev* dev, cshared< ReplDevCPContext > dev_ctx);
     cshared< ReplDevCPContext > get_repl_dev_ctx(ReplDev* dev);

src/tests/test_raft_repl_dev.cpp

@@ -752,13 +752,13 @@ TEST_F(RaftReplDevTest, RaftLogTruncationTest) {
 
     auto pre_raft_logstore_reserve_threshold = 0;
     auto pre_raft_logstore_truncation_reserve_count = 0;
-    HS_SETTINGS_FACTORY().modifiable_settings([&pre_raft_logstore_reserve_threshold,
-                                               &pre_raft_logstore_truncation_reserve_count](auto& s) {
-        pre_raft_logstore_reserve_threshold = s.resource_limits.raft_logstore_reserve_threshold;
-        pre_raft_logstore_truncation_reserve_count = s.resource_limits.raft_logstore_truncation_reserve_count;
-        s.resource_limits.raft_logstore_reserve_threshold = 200;
-        s.resource_limits.raft_logstore_truncation_reserve_count = 1;
-    });
+    HS_SETTINGS_FACTORY().modifiable_settings(
+        [&pre_raft_logstore_reserve_threshold, &pre_raft_logstore_truncation_reserve_count](auto& s) {
+            pre_raft_logstore_reserve_threshold = s.resource_limits.raft_logstore_reserve_threshold;
+            pre_raft_logstore_truncation_reserve_count = s.resource_limits.raft_logstore_truncation_reserve_count;
+            s.resource_limits.raft_logstore_reserve_threshold = 200;
+            s.resource_limits.raft_logstore_truncation_reserve_count = 1;
+        });
     HS_SETTINGS_FACTORY().save();
 
     uint64_t entries_per_attempt = 100;
@@ -872,11 +872,11 @@ TEST_F(RaftReplDevTest, RaftLogTruncationTest) {
     // set the settings back and save.
     LOGINFO("Set raft logstore truncation settings back to previous values, reserve_threshold={}, reserve_count={}",
             pre_raft_logstore_reserve_threshold, pre_raft_logstore_truncation_reserve_count);
-    HS_SETTINGS_FACTORY().modifiable_settings([pre_raft_logstore_reserve_threshold,
-                                               pre_raft_logstore_truncation_reserve_count](auto& s) {
-        s.resource_limits.raft_logstore_reserve_threshold = pre_raft_logstore_reserve_threshold;
-        s.resource_limits.raft_logstore_truncation_reserve_count = pre_raft_logstore_truncation_reserve_count;
-    });
+    HS_SETTINGS_FACTORY().modifiable_settings(
+        [pre_raft_logstore_reserve_threshold, pre_raft_logstore_truncation_reserve_count](auto& s) {
+            s.resource_limits.raft_logstore_reserve_threshold = pre_raft_logstore_reserve_threshold;
+            s.resource_limits.raft_logstore_truncation_reserve_count = pre_raft_logstore_truncation_reserve_count;
+        });
     HS_SETTINGS_FACTORY().save();
 
     g_helper->sync_for_cleanup_start();