From a07305ed777eb1c484763918a92d3199412c0ff5 Mon Sep 17 00:00:00 2001 From: abhinavdangeti Date: Mon, 25 Apr 2016 10:51:07 -0700 Subject: [PATCH] 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 (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 (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 Reviewed-by: Dave Rigby Tested-by: buildbot Reviewed-by: Will Gardner --- src/vbucket.cc | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/src/vbucket.cc b/src/vbucket.cc index 39aa804..a1a7bc8 100644 --- 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) -- 1.9.1