MB-48819: Change Cookie::engine_storage to be atomic

As seen on cluster_run built with TSan, the following race is seen aborting a timed-out SyncWrite: WARNING: ThreadSanitizer: data race (pid=46769) Write of size 8 at 0x7b5400291698 by thread T60 (mutexes: read M542537949550136824, write M360774, read M541974810616805208, write M1133353977207195360): #0 Cookie::setEngineStorage(void*) kv_engine/daemon/cookie.h:432 (memcached+0x66a6e1) #1 EventuallyPersistentEngine::storeEngineSpecific(CookieIface const*, void*) kv_engine/engines/ep/src/ep_engine.cc:1841 (memcached+0x7c4162) #2 operator() kv_engine/engines/ep/src/kv_bucket.cc:2756 (memcached+0xa65268) #3 __invoke_impl<void, KVBucket::makeSyncWriteCompleteCB()::<...> /usr/include/c++/10/bits/invoke.h:60 (memcached+0xa65268) #4 __invoke_r<void, KVBucket::makeSyncWriteCompleteCB()::<...> /usr/include/c++/10/bits/invoke.h:110 (memcached+0xa65268) #5 _M_invoke /usr/include/c++/10/bits/std_function.h:291 (memcached+0xa65268) #6 std::function<void (CookieIface const*, cb::engine_errc)>::operator()(...) const /usr/include/c++/10/bits/std_function.h:622 (memcached+0x9bf920) #7 VBucket::notifyClientOfSyncWriteComplete(CookieIface const*, cb::engine_errc) kv_engine/engines/ep/src/vbucket.cc:1041 (memcached+0x9bf920) ... Previous write of size 8 at 0x7b5400291698 by thread T18 (mutexes: write M3809): #0 Cookie::setEngineStorage(void*) /home/daver/repos/couchbase/server/kv_engine/daemon/cookie.h:432 (memcached+0x66a6e1) #1 EventuallyPersistentEngine::storeEngineSpecific(...) kv_engine/engines/ep/src/ep_engine.cc:1841 (memcached+0x7bb0c2) #2 EventuallyPersistentEngine::storeIfInner(...) kv_engine/engines/ep/src/ep_engine.cc:2539 (memcached+0x7de96a) #3 EventuallyPersistentEngine::store_if(...) kv_engine/engines/ep/src/ep_engine.cc:478 (memcached+0x7debed) #4 bucket_store_if(...) kv_engine/daemon/protocol/mcbp/engine_wrapper.cc:148 (memcached+0x7517bd) #5 MutationCommandContext::storeItem() kv_engine/daemon/protocol/mcbp/mutation_context.cc:288 (memcached+0x733f6d) #6 MutationCommandContext::step() kv_engine/daemon/protocol/mcbp/mutation_context.cc:54 (memcached+0x7385f7) ... When setEngineStorage is called from EventuallyPersistentEngine::store_if (when issueing a SyncWrite) from the frontend thread, the per-frontend thread mutex is held when modifying Cookie:engine_storage. However when the background thread later modifies Cookie:engine_storage the front-end thread mutex is not held. Address this by making Cookie::engine_storage atomic. We could have added a mutex around it, but that would add additional space requirements for each Cookie (of which there are potentially many), so atomic_ptr suffices for the time being. Change-Id: I62e25b6a74d47c2da6b500cb3dc20d7ad2b01e03 Reviewed-on: http://review.couchbase.org/c/kv_engine/+/163278 Tested-by: Build Bot <[email protected]> Reviewed-by: Paolo Cocchi <[email protected]>
couchbase · Oct 11, 2021 · e6f2789 · e6f2789
1 parent c7ca055
commit e6f2789
Show file tree

Hide file tree

Showing 3 changed files with 5 additions and 3 deletions.
diff --git a/daemon/cookie.cc b/daemon/cookie.cc
@@ -68,7 +68,7 @@ nlohmann::json Cookie::toJSON() const {
     ret["ewouldblock"] = ewouldblock;
     ret["aiostat"] = to_string(cb::engine_errc(aiostat));
     ret["refcount"] = uint32_t(refcount);
-    ret["engine_storage"] = cb::to_hex(uint64_t(engine_storage));
+    ret["engine_storage"] = cb::to_hex(uint64_t(engine_storage.load()));
     return ret;
 }
 

diff --git a/daemon/cookie.h b/daemon/cookie.h
@@ -687,8 +687,10 @@ class Cookie : public CookieIface {
      * Pointer to engine-specific data which the engine has requested the server
      * to persist for the life of the connection.
      * See SERVER_COOKIE_API::{get,store}_engine_specific()
+     * Atomic so it can safely be read / updated from engine background threads
+     * in addition to front-end threads.
      */
-    void* engine_storage{nullptr};
+    std::atomic<void*> engine_storage{nullptr};
 
     /**
      * Log a preformatted response text

diff --git a/programs/engine_testapp/mock_cookie.h b/programs/engine_testapp/mock_cookie.h
@@ -144,7 +144,7 @@ class MockCookie : public CookieIface {
 protected:
     static CheckPrivilegeFunction checkPrivilegeFunction;
 
-    void* engine_data{nullptr};
+    std::atomic<void*> engine_data{nullptr};
     uint32_t sfd{};
     bool handle_ewouldblock{true};
     bool handle_mutation_extras{true};