Skip to content

Commit a847eb3

Browse files
daverigbychiyoung
authored andcommitted
Fix data race in CouchKVStore stats access
As reported by ThreadSanitizer. CouchKVStore maintains three maps of vBucketID to counter - dbFileRevMap, cachedDocCount & cachedDeleteCount. These are read by some of the stats functions (e.g. doDcpVbTakeoverStats) without a lock and hence there is a potential race. Solve this by changing the type of these counters to RelaxedAtomic. WARNING: ThreadSanitizer: data race (pid=14070) Write of size 8 at 0x7d9000002000 by thread T7 (mutexes: write M12364): #0 CouchKVStore::saveDocs(unsigned short, unsigned long, _doc**, _docinfo**, unsigned long, KVStatsCtx&) /home/couchbase/couchbase/ep-engine/src/couch-kvstore/couch-kvstore.cc:1932:9 (ep.so+0x000000146628) couchbase#1 CouchKVStore::commit2couchstore(Callback<KVStatsCtx>*) /home/couchbase/couchbase/ep-engine/src/couch-kvstore/couch-kvstore.cc:1808:34 (ep.so+0x00000013fcb7) couchbase#2 CouchKVStore::commit(Callback<KVStatsCtx>*) /home/couchbase/couchbase/ep-engine/src/couch-kvstore/couch-kvstore.cc:1095:13 (ep.so+0x00000013f941) couchbase#3 EventuallyPersistentStore::commit(unsigned short) /home/couchbase/couchbase/ep-engine/src/ep.cc:3351:13 (ep.so+0x00000008a0f6) couchbase#4 EventuallyPersistentStore::flushVBucket(unsigned short) /home/couchbase/couchbase/ep-engine/src/ep.cc:3298:17 (ep.so+0x0000000891b0) couchbase#5 Flusher::flushVB() /home/couchbase/couchbase/ep-engine/src/flusher.cc:296:13 (ep.so+0x0000000ddd05) #6 Flusher::step(GlobalTask*) /home/couchbase/couchbase/ep-engine/src/flusher.cc:186:9 (ep.so+0x0000000dc6e5) #7 FlusherTask::run() /home/couchbase/couchbase/ep-engine/src/tasks.cc:63:12 (ep.so+0x000000112222) #8 ExecutorThread::run() /home/couchbase/couchbase/ep-engine/src/executorthread.cc:115:26 (ep.so+0x0000000d86ee) #9 launch_executor_thread(void*) /home/couchbase/couchbase/ep-engine/src/executorthread.cc:33:9 (ep.so+0x0000000d8265) #10 platform_thread_wrap /home/couchbase/couchbase/platform/src/cb_pthreads.c:23:5 (libplatform.so.0.1.0+0x000000003c31) Previous read of size 8 at 0x7d9000002000 by main thread (mutexes: write M18926): #0 CouchKVStore::getNumPersistedDeletes(unsigned short) /home/couchbase/couchbase/ep-engine/src/couch-kvstore/couch-kvstore.cc:2239:23 (ep.so+0x000000147e0f) couchbase#1 EventuallyPersistentEngine::doDcpVbTakeoverStats(void const*, void (*)(char const*, unsigned short, char const*, unsigned int, void const*), std::string&, unsigned short) /home/couchbase/couchbase/ep-engine/src/ep_engine.cc:5721:28 (ep.so+0x0000000b196e) couchbase#2 EventuallyPersistentEngine::getStats(void const*, char const*, int, void (*)(char const*, unsigned short, char const*, unsigned int, void const*)) /home/couchbase/couchbase/ep-engine/src/ep_engine.cc:4672:14 (ep.so+0x0000000b069f) couchbase#3 EvpGetStats(engine_interface*, void const*, char const*, int, void (*)(char const*, unsigned short, char const*, unsigned int, void const*)) /home/couchbase/couchbase/ep-engine/src/ep_engine.cc:214:38 (ep.so+0x00000009f26e) couchbase#4 mock_get_stats(engine_interface*, void const*, char const*, int, void (*)(char const*, unsigned short, char const*, unsigned int, void const*)) /home/couchbase/couchbase/memcached/programs/engine_testapp/engine_testapp.cc:239:19 (engine_testapp+0x0000004c553d) couchbase#5 get_int_stat(engine_interface*, engine_interface_v1*, char const*, char const*) /home/couchbase/couchbase/ep-engine/tests/ep_test_apis.cc:990:24 (ep_testsuite.so+0x000000083c04) #6 test_dcp_vbtakeover_no_stream(engine_interface*, engine_interface_v1*) /home/couchbase/couchbase/ep-engine/tests/ep_testsuite.cc:3565:15 (ep_testsuite.so+0x000000055d43) #7 execute_test(test, char const*, char const*) /home/couchbase/couchbase/memcached/programs/engine_testapp/engine_testapp.cc:1090:19 (engine_testapp+0x0000004c4142) #8 main /home/couchbase/couchbase/memcached/programs/engine_testapp/engine_testapp.cc:1439 (engine_testapp+0x0000004c4142) Change-Id: I593b31417bfe47d8ed50dd986f51763604e54645 Reviewed-on: http://review.couchbase.org/55873 Reviewed-by: abhinav dangeti <abhinav@couchbase.com> Reviewed-by: Chiyoung Seo <chiyoung@couchbase.com> Tested-by: Chiyoung Seo <chiyoung@couchbase.com>
1 parent ccdc143 commit a847eb3

2 files changed

Lines changed: 16 additions & 8 deletions

File tree

src/couch-kvstore/couch-kvstore.cc

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -281,9 +281,9 @@ CouchKVStore::CouchKVStore(KVStoreConfig &config, bool read_only) :
281281

282282
// pre-allocate lookup maps (vectors) given we have a relatively
283283
// small, fixed number of vBuckets.
284-
dbFileRevMap.assign(numDbFiles, 1);
285-
cachedDocCount.assign(numDbFiles, -1);
286-
cachedDeleteCount.assign(numDbFiles, -1);
284+
dbFileRevMap.assign(numDbFiles, Couchbase::RelaxedAtomic<uint64_t>(1));
285+
cachedDocCount.assign(numDbFiles, Couchbase::RelaxedAtomic<size_t>(-1));
286+
cachedDeleteCount.assign(numDbFiles, Couchbase::RelaxedAtomic<size_t>(-1));
287287
cachedVBStates.assign(numDbFiles, nullptr);
288288

289289
initialize();

src/couch-kvstore/couch-kvstore.h

Lines changed: 13 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020

2121
#include "config.h"
2222
#include "libcouchstore/couch_db.h"
23+
#include <relaxed_atomic.h>
2324

2425
#include <map>
2526
#include <string>
@@ -527,19 +528,26 @@ class CouchKVStore : public KVStore
527528
void removeCompactFile(const std::string &filename);
528529

529530
const std::string dbname;
530-
std::vector<uint64_t>dbFileRevMap;
531+
532+
// Map of the fileRev for each vBucket. Using RelaxedAtomic so
533+
// stats gathering (doDcpVbTakeoverStats) can get a snapshot
534+
// without having to lock.
535+
std::vector<Couchbase::RelaxedAtomic<uint64_t>> dbFileRevMap;
536+
531537
uint16_t numDbFiles;
532538
std::vector<CouchRequest *> pendingReqsQ;
533539
bool intransaction;
534540

535541
/* all stats */
536542
CouchKVStoreStats st;
537543
couch_file_ops statCollectingFileOps;
538-
/* deleted docs in each file, indexed by vBucket */
539-
std::vector<size_t> cachedDeleteCount;
544+
/* deleted docs in each file, indexed by vBucket. RelaxedAtomic
545+
to allow stats access witout lock */
546+
std::vector<Couchbase::RelaxedAtomic<size_t>> cachedDeleteCount;
540547

541-
/* non-deleted docs in each file, indexed by vBucket */
542-
std::vector<size_t> cachedDocCount;
548+
/* non-deleted docs in each file, indexed by vBucket.
549+
RelaxedAtomic to allow stats access witout lock. */
550+
std::vector<Couchbase::RelaxedAtomic<size_t>> cachedDocCount;
543551

544552
/* pending file deletions */
545553
AtomicQueue<std::string> pendingFileDeletions;

0 commit comments

Comments
 (0)