Skip to content

Commit a07305e

Browse files
abhinavdangetichiyoung
authored andcommitted
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 <[email protected]> Reviewed-by: Dave Rigby <[email protected]> Tested-by: buildbot <[email protected]> Reviewed-by: Will Gardner <[email protected]>
1 parent ab15229 commit a07305e

File tree

1 file changed

+14
-10
lines changed

1 file changed

+14
-10
lines changed

src/vbucket.cc

Lines changed: 14 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -183,23 +183,27 @@ void VBucket::fireAllOps(EventuallyPersistentEngine &engine) {
183183

184184
void VBucket::setState(vbucket_state_t to, SERVER_HANDLE_V1 *sapi) {
185185
cb_assert(sapi);
186-
WriterLockHolder wlh(stateLock);
187-
vbucket_state_t oldstate(state);
188186

189-
if (to == vbucket_state_active &&
190-
checkpointManager.getOpenCheckpointId() < 2) {
191-
checkpointManager.setOpenCheckpointId(2);
187+
vbucket_state_t oldstate;
188+
{
189+
WriterLockHolder wlh(stateLock);
190+
oldstate = state;
191+
192+
if (to == vbucket_state_active &&
193+
checkpointManager.getOpenCheckpointId() < 2) {
194+
checkpointManager.setOpenCheckpointId(2);
195+
}
196+
197+
LOG(EXTENSION_LOG_DEBUG, "transitioning vbucket %d from %s to %s",
198+
id, VBucket::toString(oldstate), VBucket::toString(to));
199+
200+
state = to;
192201
}
193202

194203
if (oldstate == vbucket_state_active) {
195204
uint64_t highSeqno = (uint64_t)checkpointManager.getHighSeqno();
196205
setCurrentSnapshot(highSeqno, highSeqno);
197206
}
198-
199-
LOG(EXTENSION_LOG_DEBUG, "transitioning vbucket %d from %s to %s",
200-
id, VBucket::toString(oldstate), VBucket::toString(to));
201-
202-
state = to;
203207
}
204208

205209
void VBucket::doStatsForQueueing(Item& qi, size_t itemBytes)

0 commit comments

Comments
 (0)