MB-16500 [BP]: MB-16496 Fix the race on vbucket state between persistVBState() and... 68/56068/5
authorChiyoung Seo <chiyoung.seo@gmail.com>
Fri, 9 Oct 2015 18:39:10 +0000 (11:39 -0700)
committerChiyoung Seo <chiyoung@couchbase.com>
Tue, 13 Oct 2015 23:11:54 +0000 (23:11 +0000)
The following data race is reported by thread sanitizer:

WARNING: ThreadSanitizer: data race (pid=29921)
  Write of size 8 at 0x7d680001f580 by thread T5 (mutexes: write M12734):
    #0 VBucket::setPurgeSeqno() ep-engine/src/vbucket.h:215:9 (ep.so+0x000000086204)
    #1 EventuallyPersistentStore::compactVBucket() ep-engine/src/ep.cc:1584 (ep.so+0x000000086204)
    #2 CompactVBucketTask::run() ep-engine/src/tasks.cc:94:12 (ep.so+0x00000012971e)
    #3 ExecutorThread::run() ep-engine/src/executorthread.cc:115:26 (ep.so+0x0000000ea41d)
    #4 launch_executor_thread() ep-engine/src/executorthread.cc:33:9 (ep.so+0x0000000e9fe5)
    #5 platform_thread_wrap platform/src/cb_pthreads.c:23:5 (libplatform.so.0.1.0+0x000000004161)

  Previous read of size 8 at 0x7d680001f580 by thread T7:
    #0 VBucket::getPurgeSeqno() ep-engine/src/vbucket.h:211:16 (ep.so+0x0000000821d3)
    #1 EventuallyPersistentStore::persistVBState() ep-engine/src/ep.cc:1217 (ep.so+0x0000000821d3)
    #2 VBStatePersistTask::run() ep-engine/src/tasks.cc:86:12 (ep.so+0x000000129636)
    #3 ExecutorThread::run() ep-engine/src/executorthread.cc:115:26 (ep.so+0x0000000ea41d)
    #4 launch_executor_thread() ep-engine/src/executorthread.cc:33:9 (ep.so+0x0000000e9fe5)
    #5 platform_thread_wrap platform/src/cb_pthreads.c:23:5 (libplatform.so.0.1.0+0x000000004161)

  Location is heap block of size 1392 at 0x7d680001f200 allocated by main thread:
    #0 operator new() <null> (engine_testapp+0x00000045cded)
    #1 EventuallyPersistentStore::setVBucketState() ep-engine/src/ep.cc:1300:30 (ep.so+0x000000082b1a)
    #2 EventuallyPersistentEngine::setVBucketState() ep-engine/src/ep_engine.h:718:16 (ep.so+0x0000000ca308)
    #3 setVBucket()) ep-engine/src/ep_engine.cc:884 (ep.so+0x0000000ca308)
    #4 processUnknownCommand()) ep-engine/src/ep_engine.cc:1178 (ep.so+0x0000000ca308)
    #5 EvpUnknownCommand()) ep-engine/src/ep_engine.cc:1389:38 (ep.so+0x0000000aafc8)
    #6 mock_unknown_command()) memcached/programs/engine_testapp/engine_testapp.cc:380:19 (engine_testapp+0x0000004c56b9)
    #7 set_vbucket_state() ep-engine/tests/ep_test_apis.cc:607:9 (ep_testsuite.so+0x0000000a3a4b)
    #8 test_setup() ep-engine/tests/ep_testsuite_common.cc:146:28 (ep_testsuite.so+0x00000009cdda)
    #9 execute_test() memcached/programs/engine_testapp/engine_testapp.cc:1085:47 (engine_testapp+0x0000004c4103)
    #10 main memcached/programs/engine_testapp/engine_testapp.cc:1439 (engine_testapp+0x0000004c4103)

To address the above issue, vbucket states should be read after grabbing
the vbucket writer lock in EPStore::persistVBState().

Change-Id: I5a42b3e15a1cf5c941d399897bc68d6f35746ff3
Reviewed-on: http://review.couchbase.org/55972
Tested-by: buildbot <build@couchbase.com>
Reviewed-by: abhinav dangeti <abhinav@couchbase.com>
Reviewed-on: http://review.couchbase.org/56068
Reviewed-by: Chiyoung Seo <chiyoung@couchbase.com>
src/ep.cc

index 8ce6f36..3d78374 100644 (file)
--- a/src/ep.cc
+++ b/src/ep.cc
@@ -979,16 +979,6 @@ bool EventuallyPersistentStore::persistVBState(const Priority &priority,
         return false;
     }
 
-    KVStatsCallback kvcb(this);
-    uint64_t chkId = vbMap.getPersistenceCheckpointId(vbid);
-    std::string failovers = vb->failovers->toJSON();
-    uint64_t snapStart = 0;
-    uint64_t snapEnd = 0;
-
-    vb->getCurrentSnapshot(snapStart, snapEnd);
-    vbucket_state vb_state(vb->getState(), chkId, 0, vb->getHighSeqno(),
-                           vb->getPurgeSeqno(), snapStart, snapEnd, failovers);
-
     bool inverse = false;
     LockHolder lh(vb_mutexes[vbid], true /*tryLock*/);
     if (!lh.islocked()) {
@@ -1000,6 +990,16 @@ bool EventuallyPersistentStore::persistVBState(const Priority &priority,
         }
     }
 
+    KVStatsCallback kvcb(this);
+    uint64_t chkId = vbMap.getPersistenceCheckpointId(vbid);
+    std::string failovers = vb->failovers->toJSON();
+    uint64_t snapStart = 0;
+    uint64_t snapEnd = 0;
+
+    vb->getCurrentSnapshot(snapStart, snapEnd);
+    vbucket_state vb_state(vb->getState(), chkId, 0, vb->getHighSeqno(),
+                           vb->getPurgeSeqno(), snapStart, snapEnd, failovers);
+
     KVStore *rwUnderlying = getRWUnderlying(vbid);
     if (rwUnderlying->snapshotVBucket(vbid, vb_state, &kvcb)) {
         if (vbMap.setBucketCreation(vbid, false)) {