Skip to content

Commit 0b6248b

Browse files
abhinavdangetichiyoung
abhinavdangeti
authored and
chiyoung
committed
MB-19382: [BP] Create a variable to get correct locking scope
A mistake in 495e00a means that no variable is created for the ReaderLockHolder, the compiler either optimises away the lock constructor/destructor or the lock scope is wrong. Either way we need to create a variable. Includes some lock ordering changes as per ThreadSantitiser warnings. (Reviewed-on: http://review.couchbase.org/56978) This will address the following lock inversion: 11:56:19 WARNING: ThreadSanitizer: lock-order-inversion (potential deadlock) (pid=51509) 11:56:19 Cycle in lock order graph: M21441 (0x7d780000f450) => M21477 (0x7d640005edf0) => M21441 11:56:19 11:56:19 Mutex M21477 acquired here while holding mutex M21441 in main thread: 11:56:19 #0 pthread_rwlock_rdlock <null> (engine_testapp+0x000000462260) 11:56:19 #1 cb_rw_reader_enter <null> (libplatform.so.0.1.0+0x000000004800) 11:56:19 #2 RWLock::readerLock() /home/couchbase/jenkins/workspace/ep-engine-threadsanitizer-3.0.x/ep-engine/src/rwlock.h:38 (ep.so+0x000000132360) 11:56:19 #3 ReaderLockHolder::ReaderLockHolder(RWLock&) /home/couchbase/jenkins/workspace/ep-engine-threadsanitizer-3.0.x/ep-engine/src/locks.h:167 (ep.so+0x0000000f8087) 11:56:19 #4 EventuallyPersistentStore::getAndUpdateTtl(std::string const&, unsigned short, void const*, long) /home/couchbase/jenkins/workspace/ep-engine-threadsanitizer-3.0.x/ep-engine/src/ep.cc:1970 (ep.so+0x0000000e6b45) 11:56:19 #5 EventuallyPersistentEngine::touch(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/couchbase/jenkins/workspace/ep-engine-threadsanitizer-3.0.x/ep-engine/src/ep_engine.cc:4619 (ep.so+0x000000155fe8) 11:56:19 #6 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/couchbase/jenkins/workspace/ep-engine-threadsanitizer-3.0.x/ep-engine/src/ep_engine.cc:1126 (ep.so+0x000000163a9b) 11:56:19 #7 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/couchbase/jenkins/workspace/ep-engine-threadsanitizer-3.0.x/ep-engine/src/ep_engine.cc:1312 (ep.so+0x000000137365) 11:56:19 #8 mock_unknown_command /home/couchbase/jenkins/workspace/ep-engine-threadsanitizer-3.0.x/memcached/programs/engine_testapp/engine_testapp.c (engine_testapp+0x0000004c8f1a) 11:56:19 #9 gat(engine_interface*, engine_interface_v1*, char const*, unsigned short, unsigned int, bool) /home/couchbase/jenkins/workspace/ep-engine-threadsanitizer-3.0.x/ep-engine/tests/ep_test_apis.cc:348 (ep_testsuite.so+0x0000000e2d6b) 11:56:19 #10 test_expired_item_with_item_eviction(engine_interface*, engine_interface_v1*) /home/couchbase/jenkins/workspace/ep-engine-threadsanitizer-3.0.x/ep-engine/tests/ep_testsuite.cc:11401 (ep_testsuite.so+0x0000000acbd4) 11:56:19 #11 execute_test /home/couchbase/jenkins/workspace/ep-engine-threadsanitizer-3.0.x/memcached/programs/engine_testapp/engine_testapp.c (engine_testapp+0x0000004c4e9f) 11:56:19 #12 main crtstuff.c (engine_testapp+0x0000004c2e01) 11:56:19 11:56:19 Mutex M21441 acquired here while holding mutex M21477 in thread T8: 11:56:19 #0 pthread_mutex_lock <null> (engine_testapp+0x00000047e9e0) 11:56:19 #1 cb_mutex_enter <null> (libplatform.so.0.1.0+0x0000000039c0) 11:56:19 #2 Mutex::acquire() /home/couchbase/jenkins/workspace/ep-engine-threadsanitizer-3.0.x/ep-engine/src/mutex.cc:31 (ep.so+0x0000001e241e) 11:56:19 #3 LockHolder::lock() /home/couchbase/jenkins/workspace/ep-engine-threadsanitizer-3.0.x/ep-engine/src/locks.h:71 (ep.so+0x000000080bc3) 11:56:19 #4 LockHolder::LockHolder(Mutex&, bool) /home/couchbase/jenkins/workspace/ep-engine-threadsanitizer-3.0.x/ep-engine/src/locks.h:48 (ep.so+0x000000080832) 11:56:19 #5 HashTable::getLockedBucket(int, int*) /home/couchbase/jenkins/workspace/ep-engine-threadsanitizer-3.0.x/ep-engine/src/stored-value.h:1266 (ep.so+0x00000008280a) 11:56:19 #6 HashTable::getLockedBucket(std::string const&, int*) /home/couchbase/jenkins/workspace/ep-engine-threadsanitizer-3.0.x/ep-engine/src/stored-value.h:1295 (ep.so+0x00000007c61b) 11:56:19 #7 EventuallyPersistentStore::deleteExpiredItem(unsigned short, std::string&, long, unsigned long) /home/couchbase/jenkins/workspace/ep-engine-threadsanitizer-3.0.x/ep-engine/src/ep.cc:481 (ep.so+0x0000000d4d80) 11:56:19 #8 ExpiredItemsCallback::callback(compaction_ctx&) /home/couchbase/jenkins/workspace/ep-engine-threadsanitizer-3.0.x/ep-engine/src/ep.cc:1258 (ep.so+0x000000123ecb) 11:56:19 #9 CouchKVStore::compactVBucket(unsigned short, compaction_ctx*, Callback<compaction_ctx>&, Callback<KVStatsCtx>&) /home/couchbase/jenkins/workspace/ep-engine-threadsanitizer-3.0.x/ep-engine/src/couch-kvstore/couch-kvstore.cc:862 (ep.so+0x0000003159f7) 11:56:19 #10 EventuallyPersistentStore::compactVBucket(unsigned short, compaction_ctx*, void const*) /home/couchbase/jenkins/workspace/ep-engine-threadsanitizer-3.0.x/ep-engine/src/ep.cc:1326 (ep.so+0x0000000df2ec) 11:56:19 #11 CompactVBucketTask::run() /home/couchbase/jenkins/workspace/ep-engine-threadsanitizer-3.0.x/ep-engine/src/tasks.cc:76 (ep.so+0x000000251ed1) 11:56:19 #12 ExecutorThread::run() /home/couchbase/jenkins/workspace/ep-engine-threadsanitizer-3.0.x/ep-engine/src/executorthread.cc:109 (ep.so+0x0000001e38f1) 11:56:19 #13 launch_executor_thread(void*) /home/couchbase/jenkins/workspace/ep-engine-threadsanitizer-3.0.x/ep-engine/src/executorthread.cc:34 (ep.so+0x0000001e2f1a) 11:56:19 #14 platform_thread_wrap /home/couchbase/jenkins/workspace/ep-engine-threadsanitizer-3.0.x/platform/src/cb_pthreads.c (libplatform.so.0.1.0+0x00000000377c) Change-Id: I5d5ca33fdd3c17df2be9d2b2d6acc8c254f1cb2d Reviewed-on: http://review.couchbase.org/63418 Well-Formed: buildbot <build@couchbase.com> Tested-by: buildbot <build@couchbase.com> Reviewed-by: Will Gardner <will.gardner@couchbase.com> Reviewed-by: Chiyoung Seo <chiyoung@couchbase.com>
1 parent 4b25964 commit 0b6248b

1 file changed

Lines changed: 83 additions & 71 deletions

File tree

‎src/ep.cc‎

Lines changed: 83 additions & 71 deletions
Original file line numberDiff line numberDiff line change
@@ -534,7 +534,7 @@ StoredValue *EventuallyPersistentStore::fetchValidValue(RCPtr<VBucket> &vb,
534534
if (vb->getState() != vbucket_state_active) {
535535
return wantDeleted ? v : NULL;
536536
}
537-
ReaderLockHolder(vb->getStateLock());
537+
538538
// queueDirty only allowed on active VB
539539
if (queueExpired && vb->getState() == vbucket_state_active) {
540540
incExpirationStat(vb, false);
@@ -632,7 +632,7 @@ ENGINE_ERROR_CODE EventuallyPersistentStore::set(const Item &itm,
632632

633633
// Obtain read-lock on VB state to ensure VB state changes are interlocked
634634
// with this set
635-
ReaderLockHolder(vb->getStateLock());
635+
ReaderLockHolder rlh(vb->getStateLock());
636636
if (vb->getState() == vbucket_state_dead) {
637637
++stats.numNotMyVBuckets;
638638
return ENGINE_NOT_MY_VBUCKET;
@@ -711,7 +711,7 @@ ENGINE_ERROR_CODE EventuallyPersistentStore::add(const Item &itm,
711711

712712
// Obtain read-lock on VB state to ensure VB state changes are interlocked
713713
// with this add
714-
ReaderLockHolder(vb->getStateLock());
714+
ReaderLockHolder rlh(vb->getStateLock());
715715
if (vb->getState() == vbucket_state_dead ||
716716
vb->getState() == vbucket_state_replica) {
717717
++stats.numNotMyVBuckets;
@@ -764,7 +764,7 @@ ENGINE_ERROR_CODE EventuallyPersistentStore::replace(const Item &itm,
764764

765765
// Obtain read-lock on VB state to ensure VB state changes are interlocked
766766
// with this replace
767-
ReaderLockHolder(vb->getStateLock());
767+
ReaderLockHolder rlh(vb->getStateLock());
768768
if (vb->getState() == vbucket_state_dead ||
769769
vb->getState() == vbucket_state_replica) {
770770
++stats.numNotMyVBuckets;
@@ -848,7 +848,7 @@ ENGINE_ERROR_CODE EventuallyPersistentStore::addTAPBackfillItem(const Item &itm,
848848

849849
// Obtain read-lock on VB state to ensure VB state changes are interlocked
850850
// with this add-tapbackfill
851-
ReaderLockHolder(vb->getStateLock());
851+
ReaderLockHolder rlh(vb->getStateLock());
852852
if (vb->getState() == vbucket_state_dead ||
853853
vb->getState() == vbucket_state_active) {
854854
++stats.numNotMyVBuckets;
@@ -1437,6 +1437,7 @@ void EventuallyPersistentStore::completeBGFetch(const std::string &key,
14371437

14381438
RCPtr<VBucket> vb = getVBucket(vbucket);
14391439
if (vb) {
1440+
ReaderLockHolder rlh(vb->getStateLock());
14401441
int bucket_num(0);
14411442
LockHolder hlh = vb->ht.getLockedBucket(key, &bucket_num);
14421443
StoredValue *v = fetchValidValue(vb, key, bucket_num, true);
@@ -1473,7 +1474,6 @@ void EventuallyPersistentStore::completeBGFetch(const std::string &key,
14731474
if (gcb.val.getStatus() == ENGINE_SUCCESS) {
14741475
v->unlocked_restoreValue(gcb.val.getValue(), vb->ht);
14751476
cb_assert(v->isResident());
1476-
ReaderLockHolder(vb->getStateLock());
14771477
if (vb->getState() == vbucket_state_active &&
14781478
v->getExptime() != gcb.val.getValue()->getExptime() &&
14791479
v->getCas() == gcb.val.getValue()->getCas()) {
@@ -1543,74 +1543,75 @@ void EventuallyPersistentStore::completeBGFetchMulti(uint16_t vbId,
15431543
ENGINE_ERROR_CODE status = bgitem->value.getStatus();
15441544
Item *fetchedValue = bgitem->value.getValue();
15451545
const std::string &key = (*itemItr).first;
1546-
1547-
int bucket = 0;
1548-
LockHolder blh = vb->ht.getLockedBucket(key, &bucket);
1549-
StoredValue *v = fetchValidValue(vb, key, bucket, true);
1550-
if (bgitem->metaDataOnly) {
1551-
if (v && v->unlocked_restoreMeta(fetchedValue, status, vb->ht)) {
1552-
status = ENGINE_SUCCESS;
1553-
}
1554-
} else {
1555-
bool restore = false;
1556-
if (v && v->isResident()) {
1557-
status = ENGINE_SUCCESS;
1546+
{ // locking scope
1547+
ReaderLockHolder rlh(vb->getStateLock());
1548+
1549+
int bucket = 0;
1550+
LockHolder blh = vb->ht.getLockedBucket(key, &bucket);
1551+
StoredValue *v = fetchValidValue(vb, key, bucket, true);
1552+
if (bgitem->metaDataOnly) {
1553+
if (v && v->unlocked_restoreMeta(fetchedValue, status, vb->ht)) {
1554+
status = ENGINE_SUCCESS;
1555+
}
15581556
} else {
1559-
switch (eviction_policy) {
1560-
case VALUE_ONLY:
1561-
if (v && !v->isResident() && !v->isDeleted()) {
1562-
restore = true;
1563-
}
1564-
break;
1565-
case FULL_EVICTION:
1566-
if (v) {
1567-
if (v->isTempInitialItem() ||
1568-
(!v->isResident() && !v->isDeleted())) {
1557+
bool restore = false;
1558+
if (v && v->isResident()) {
1559+
status = ENGINE_SUCCESS;
1560+
} else {
1561+
switch (eviction_policy) {
1562+
case VALUE_ONLY:
1563+
if (v && !v->isResident() && !v->isDeleted()) {
15691564
restore = true;
15701565
}
1571-
}
1572-
break;
1573-
default:
1574-
throw std::logic_error("Unknown eviction policy");
1566+
break;
1567+
case FULL_EVICTION:
1568+
if (v) {
1569+
if (v->isTempInitialItem() ||
1570+
(!v->isResident() && !v->isDeleted())) {
1571+
restore = true;
1572+
}
1573+
}
1574+
break;
1575+
default:
1576+
throw std::logic_error("Unknown eviction policy");
1577+
}
15751578
}
1576-
}
15771579

1578-
if (restore) {
1579-
if (status == ENGINE_SUCCESS) {
1580-
v->unlocked_restoreValue(fetchedValue, vb->ht);
1581-
cb_assert(v->isResident());
1582-
ReaderLockHolder(vb->getStateLock());
1583-
if (vb->getState() == vbucket_state_active &&
1584-
v->getExptime() != fetchedValue->getExptime() &&
1585-
v->getCas() == fetchedValue->getCas()) {
1586-
// MB-9306: It is possible that by the time
1587-
// bgfetcher returns, the item may have been
1588-
// updated and queued
1589-
// Hence test the CAS value to be the same first.
1590-
// exptime mutated, schedule it into new checkpoint
1591-
queueDirty(vb, v, &blh);
1592-
}
1593-
} else if (status == ENGINE_KEY_ENOENT) {
1594-
v->setStoredValueState(StoredValue::state_non_existent_key);
1595-
if (eviction_policy == FULL_EVICTION) {
1596-
// For the full eviction, we should notify
1597-
// ENGINE_SUCCESS to the memcached worker thread,
1598-
// so that the worker thread can visit the
1599-
// ep-engine and figure out the correct error
1600-
// code.
1601-
status = ENGINE_SUCCESS;
1580+
if (restore) {
1581+
if (status == ENGINE_SUCCESS) {
1582+
v->unlocked_restoreValue(fetchedValue, vb->ht);
1583+
cb_assert(v->isResident());
1584+
if (vb->getState() == vbucket_state_active &&
1585+
v->getExptime() != fetchedValue->getExptime() &&
1586+
v->getCas() == fetchedValue->getCas()) {
1587+
// MB-9306: It is possible that by the time
1588+
// bgfetcher returns, the item may have been
1589+
// updated and queued
1590+
// Hence test the CAS value to be the same first.
1591+
// exptime mutated, schedule it into new checkpoint
1592+
queueDirty(vb, v, &blh);
1593+
}
1594+
} else if (status == ENGINE_KEY_ENOENT) {
1595+
v->setStoredValueState(StoredValue::state_non_existent_key);
1596+
if (eviction_policy == FULL_EVICTION) {
1597+
// For the full eviction, we should notify
1598+
// ENGINE_SUCCESS to the memcached worker thread,
1599+
// so that the worker thread can visit the
1600+
// ep-engine and figure out the correct error
1601+
// code.
1602+
status = ENGINE_SUCCESS;
1603+
}
1604+
} else {
1605+
// underlying kvstore couldn't fetch requested data
1606+
// log returned error and notify TMPFAIL to client
1607+
LOG(EXTENSION_LOG_WARNING,
1608+
"Warning: failed background fetch for vb=%d "
1609+
"key=%s", vbId, key.c_str());
1610+
status = ENGINE_TMPFAIL;
16021611
}
1603-
} else {
1604-
// underlying kvstore couldn't fetch requested data
1605-
// log returned error and notify TMPFAIL to client
1606-
LOG(EXTENSION_LOG_WARNING,
1607-
"Warning: failed background fetch for vb=%d "
1608-
"key=%s", vbId, key.c_str());
1609-
status = ENGINE_TMPFAIL;
16101612
}
16111613
}
1612-
}
1613-
blh.unlock();
1614+
} // locking scope ends
16141615

16151616
if (bgitem->metaDataOnly) {
16161617
++stats.bg_meta_fetched;
@@ -1680,7 +1681,10 @@ GetValue EventuallyPersistentStore::getInternal(const std::string &key,
16801681
if (!vb) {
16811682
++stats.numNotMyVBuckets;
16821683
return GetValue(NULL, ENGINE_NOT_MY_VBUCKET);
1683-
} else if (honorStates && vb->getState() == vbucket_state_dead) {
1684+
}
1685+
1686+
ReaderLockHolder rlh(vb->getStateLock());
1687+
if (honorStates && vb->getState() == vbucket_state_dead) {
16841688
++stats.numNotMyVBuckets;
16851689
return GetValue(NULL, ENGINE_NOT_MY_VBUCKET);
16861690
} else if (honorStates && vb->getState() == disallowedState) {
@@ -1777,7 +1781,13 @@ ENGINE_ERROR_CODE EventuallyPersistentStore::getMetaData(
17771781
{
17781782
(void) cookie;
17791783
RCPtr<VBucket> vb = getVBucket(vbucket);
1780-
if (!vb || vb->getState() == vbucket_state_dead ||
1784+
if (!vb) {
1785+
++stats.numNotMyVBuckets;
1786+
return ENGINE_NOT_MY_VBUCKET;
1787+
}
1788+
1789+
ReaderLockHolder rlh(vb->getStateLock());
1790+
if (vb->getState() == vbucket_state_dead ||
17811791
vb->getState() == vbucket_state_replica) {
17821792
++stats.numNotMyVBuckets;
17831793
return ENGINE_NOT_MY_VBUCKET;
@@ -1839,7 +1849,7 @@ ENGINE_ERROR_CODE EventuallyPersistentStore::setWithMeta(const Item &itm,
18391849
return ENGINE_NOT_MY_VBUCKET;
18401850
}
18411851

1842-
ReaderLockHolder(vb->getStateLock());
1852+
ReaderLockHolder rlh(vb->getStateLock());
18431853
if (vb->getState() == vbucket_state_dead) {
18441854
++stats.numNotMyVBuckets;
18451855
return ENGINE_NOT_MY_VBUCKET;
@@ -1925,7 +1935,10 @@ GetValue EventuallyPersistentStore::getAndUpdateTtl(const std::string &key,
19251935
if (!vb) {
19261936
++stats.numNotMyVBuckets;
19271937
return GetValue(NULL, ENGINE_NOT_MY_VBUCKET);
1928-
} else if (vb->getState() == vbucket_state_dead) {
1938+
}
1939+
1940+
ReaderLockHolder rlh(vb->getStateLock());
1941+
if (vb->getState() == vbucket_state_dead) {
19291942
++stats.numNotMyVBuckets;
19301943
return GetValue(NULL, ENGINE_NOT_MY_VBUCKET);
19311944
} else if (vb->getState() == vbucket_state_replica) {
@@ -1967,7 +1980,6 @@ GetValue EventuallyPersistentStore::getAndUpdateTtl(const std::string &key,
19671980
ENGINE_SUCCESS, v->getBySeqno());
19681981

19691982
if (exptime_mutated) {
1970-
ReaderLockHolder(vb->getStateLock());
19711983
if (vb->getState() == vbucket_state_active) {
19721984
// persist the item in the underlying storage for
19731985
// mutated exptime but only if VB is active.

0 commit comments

Comments
 (0)