MB-19359: [1] Address lock inversion with vb's state lock and snapshot lock

+ [Not a backport, this code was altered/removed in master]
+ This scenario should not occur in real operation.
+ Most DCP unit tests would however flag this as an issue because
  of how we do things in the tests --> [setting vbucket's state to
  replica at the very beginning (by the main thread)].
+ Suppressing this lock inversion, by moving the function call to
  update the vbucket's snapshot range to outside the state lock
  context in setState(), as it isn't necessary to acquire the state
  lock to update the snapshot range.

WARNING: ThreadSanitizer: lock-order-inversion (potential deadlock) (pid=39750)
  Cycle in lock order graph: M43306 (0x7d640000fcf0) => M43309 (0x7d640000fe18) => M43306

  Mutex M43309 acquired here while holding mutex M43306 in main thread:
    #0 pthread_mutex_lock <null> (engine_testapp+0x000000474420)
    #1 cb_mutex_enter /home/daver/repos/couchbase/server/platform/src/cb_pthreads.c:85 (libplatform.so.0.1.0+0x0000000034a0)
    #2 Mutex::acquire() /home/daver/repos/couchbase/server/ep-engine/src/mutex.cc:31 (ep.so+0x0000001c611e)
    #3 LockHolder::lock() /home/daver/repos/couchbase/server/ep-engine/src/locks.h:71 (ep.so+0x00000006a4e3)
    #4 LockHolder /home/daver/repos/couchbase/server/ep-engine/src/locks.h:48 (ep.so+0x00000006a172)
    #5 VBucket::setCurrentSnapshot(unsigned long, unsigned long) /home/daver/repos/couchbase/server/ep-engine/src/vbucket.h:217 (ep.so+0x0000000e5ee5)
    #6 VBucket::setState(vbucket_state_t, server_handle_v1_t*) /home/daver/repos/couchbase/server/ep-engine/src/vbucket.cc:196 (ep.so+0x0000002932e9)
    #7 EventuallyPersistentStore::setVBucketState(unsigned short, vbucket_state_t, bool, bool) /home/daver/repos/couchbase/server/ep-engine/src/ep.cc:1060 (ep.so+0x0000000c0b61)
    #8 EventuallyPersistentEngine::setVBucketState(unsigned short, vbucket_state_t, bool) /home/daver/repos/couchbase/server/ep-engine/src/ep_engine.h:628 (ep.so+0x000000188a12)
    #9 setVBucket(EventuallyPersistentEngine*, void const*, protocol_binary_request_header*, bool (*)(void const*, unsigned short, void const*, unsigned char, void const*, unsigned int, unsigned char, unsigned short, unsigned long, void const*)) /home/daver/repos/couchbase/server/ep-engine/src/ep_engine.cc:824 (ep.so+0x00000014aaaa)
    #10 processUnknownCommand(EventuallyPersistentEngine*, void const*, protocol_binary_request_header*, bool (*)(void const*, unsigned short, void const*, unsigned char, void const*, unsigned int, unsigned char, unsigned short, unsigned long, void const*)) /home/daver/repos/couchbase/server/ep-engine/src/ep_engine.cc:1118 (ep.so+0x000000147707)
    #11 EvpUnknownCommand(engine_interface*, void const*, protocol_binary_request_header*, bool (*)(void const*, unsigned short, void const*, unsigned char, void const*, unsigned int, unsigned char, unsigned short, unsigned long, void const*)) /home/daver/repos/couchbase/server/ep-engine/src/ep_engine.cc:1312 (ep.so+0x00000011a055)
    #12 mock_unknown_command /home/daver/repos/couchbase/server/memcached/programs/engine_testapp/engine_testapp.c:335 (engine_testapp+0x0000004be97a)
    #13 set_vbucket_state(engine_interface*, engine_interface_v1*, unsigned short, vbucket_state_t) /home/daver/repos/couchbase/server/ep-engine/tests/ep_test_apis.cc:484 (ep_testsuite.so+0x0000000e1562)
    #14 test_dcp_replica_stream_backfill(engine_interface*, engine_interface_v1*) /home/daver/repos/couchbase/server/ep-engine/tests/ep_testsuite.cc:5278 (ep_testsuite.so+0x00000008c84a)
    #15 execute_test /home/daver/repos/couchbase/server/memcached/programs/engine_testapp/engine_testapp.c:1042 (engine_testapp+0x0000004ba8ff)
    #16 main /home/daver/repos/couchbase/server/memcached/programs/engine_testapp/engine_testapp.c:1296 (engine_testapp+0x0000004b8861)

  Mutex M43306 acquired here while holding mutex M43309 in thread T17:
    #0 pthread_rwlock_rdlock <null> (engine_testapp+0x000000457ca0)
    #1 cb_rw_reader_enter /home/daver/repos/couchbase/server/platform/src/cb_pthreads.c:264 (libplatform.so.0.1.0+0x0000000042e0)
    #2 RWLock::readerLock() /home/daver/repos/couchbase/server/ep-engine/src/rwlock.h:38 (ep.so+0x000000115cf0)
    #3 ReaderLockHolder /home/daver/repos/couchbase/server/ep-engine/src/locks.h:167 (ep.so+0x0000000dbbe7)
    #4 EventuallyPersistentStore::addTAPBackfillItem(Item const&, unsigned char, bool) /home/daver/repos/couchbase/server/ep-engine/src/ep.cc:851 (ep.so+0x0000000be35d)
    #5 PassiveStream::commitMutation(MutationResponse*, bool) /home/daver/repos/couchbase/server/ep-engine/src/dcp-stream.cc:1370 (ep.so+0x00000027e8cc)
    #6 PassiveStream::processMutation(MutationResponse*) /home/daver/repos/couchbase/server/ep-engine/src/dcp-stream.cc:1346 (ep.so+0x00000027d680)
    #7 PassiveStream::processBufferedMessages(unsigned int&) /home/daver/repos/couchbase/server/ep-engine/src/dcp-stream.cc:1286 (ep.so+0x00000027cfbc)
    #8 DcpConsumer::processBufferedItems() /home/daver/repos/couchbase/server/ep-engine/src/dcp-consumer.cc:599 (ep.so+0x0000002454cc)
    #9 Processer::run() /home/daver/repos/couchbase/server/ep-engine/src/dcp-consumer.cc:48 (ep.so+0x0000002450ef)
    #10 ExecutorThread::run() /home/daver/repos/couchbase/server/ep-engine/src/executorthread.cc:109 (ep.so+0x0000001c76d9)
    #11 launch_executor_thread(void*) /home/daver/repos/couchbase/server/ep-engine/src/executorthread.cc:34 (ep.so+0x0000001c6caa)
    #12 platform_thread_wrap /home/daver/repos/couchbase/server/platform/src/cb_pthreads.c:19 (libplatform.so.0.1.0+0x00000000325c)

Change-Id: I2f3cf88e6aebbf6078f533fb1ed87bd9fe618616
Reviewed-on: http://review.couchbase.org/63319
Well-Formed: buildbot <build@couchbase.com>
Reviewed-by: Dave Rigby <daver@couchbase.com>
Tested-by: buildbot <build@couchbase.com>
Reviewed-by: Will Gardner <will.gardner@couchbase.com>

diff --git a/src/vbucket.cc b/src/vbucket.cc
index 39aa804..a1a7bc8 100644 (file)
--- a/src/vbucket.cc
+++ b/src/vbucket.cc
@@ -183,23 +183,27 @@ void VBucket::fireAllOps(EventuallyPersistentEngine &engine) {
 
 void VBucket::setState(vbucket_state_t to, SERVER_HANDLE_V1 *sapi) {
     cb_assert(sapi);
-    WriterLockHolder wlh(stateLock);
-    vbucket_state_t oldstate(state);
 
-    if (to == vbucket_state_active &&
-        checkpointManager.getOpenCheckpointId() < 2) {
-        checkpointManager.setOpenCheckpointId(2);
+    vbucket_state_t oldstate;
+    {
+        WriterLockHolder wlh(stateLock);
+        oldstate = state;
+
+        if (to == vbucket_state_active &&
+            checkpointManager.getOpenCheckpointId() < 2) {
+            checkpointManager.setOpenCheckpointId(2);
+        }
+
+        LOG(EXTENSION_LOG_DEBUG, "transitioning vbucket %d from %s to %s",
+            id, VBucket::toString(oldstate), VBucket::toString(to));
+
+        state = to;
     }
 
     if (oldstate == vbucket_state_active) {
         uint64_t highSeqno = (uint64_t)checkpointManager.getHighSeqno();
         setCurrentSnapshot(highSeqno, highSeqno);
     }
-
-    LOG(EXTENSION_LOG_DEBUG, "transitioning vbucket %d from %s to %s",
-        id, VBucket::toString(oldstate), VBucket::toString(to));
-
-    state = to;
 }
 
 void VBucket::doStatsForQueueing(Item& qi, size_t itemBytes)