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 <[email protected]>
Tested-by: buildbot <[email protected]>
Reviewed-by: Will Gardner <[email protected]>
Reviewed-by: Chiyoung Seo <[email protected]>

Author: abhinavdangeti

src/ep.cc

@@ -534,7 +534,7 @@ StoredValue *EventuallyPersistentStore::fetchValidValue(RCPtr<VBucket> &vb,
             if (vb->getState() != vbucket_state_active) {
                 return wantDeleted ? v : NULL;
             }
-            ReaderLockHolder(vb->getStateLock());
+
             // queueDirty only allowed on active VB
             if (queueExpired && vb->getState() == vbucket_state_active) {
                 incExpirationStat(vb, false);
@@ -632,7 +632,7 @@ ENGINE_ERROR_CODE EventuallyPersistentStore::set(const Item &itm,
 
     // Obtain read-lock on VB state to ensure VB state changes are interlocked
     // with this set
-    ReaderLockHolder(vb->getStateLock());
+    ReaderLockHolder rlh(vb->getStateLock());
     if (vb->getState() == vbucket_state_dead) {
         ++stats.numNotMyVBuckets;
         return ENGINE_NOT_MY_VBUCKET;
@@ -711,7 +711,7 @@ ENGINE_ERROR_CODE EventuallyPersistentStore::add(const Item &itm,
 
     // Obtain read-lock on VB state to ensure VB state changes are interlocked
     // with this add
-    ReaderLockHolder(vb->getStateLock());
+    ReaderLockHolder rlh(vb->getStateLock());
     if (vb->getState() == vbucket_state_dead ||
         vb->getState() == vbucket_state_replica) {
         ++stats.numNotMyVBuckets;
@@ -764,7 +764,7 @@ ENGINE_ERROR_CODE EventuallyPersistentStore::replace(const Item &itm,
 
     // Obtain read-lock on VB state to ensure VB state changes are interlocked
     // with this replace
-    ReaderLockHolder(vb->getStateLock());
+    ReaderLockHolder rlh(vb->getStateLock());
     if (vb->getState() == vbucket_state_dead ||
         vb->getState() == vbucket_state_replica) {
         ++stats.numNotMyVBuckets;
@@ -848,7 +848,7 @@ ENGINE_ERROR_CODE EventuallyPersistentStore::addTAPBackfillItem(const Item &itm,
 
     // Obtain read-lock on VB state to ensure VB state changes are interlocked
     // with this add-tapbackfill
-    ReaderLockHolder(vb->getStateLock());
+    ReaderLockHolder rlh(vb->getStateLock());
     if (vb->getState() == vbucket_state_dead ||
         vb->getState() == vbucket_state_active) {
         ++stats.numNotMyVBuckets;
@@ -1437,6 +1437,7 @@ void EventuallyPersistentStore::completeBGFetch(const std::string &key,
 
     RCPtr<VBucket> vb = getVBucket(vbucket);
     if (vb) {
+        ReaderLockHolder rlh(vb->getStateLock());
         int bucket_num(0);
         LockHolder hlh = vb->ht.getLockedBucket(key, &bucket_num);
         StoredValue *v = fetchValidValue(vb, key, bucket_num, true);
@@ -1473,7 +1474,6 @@ void EventuallyPersistentStore::completeBGFetch(const std::string &key,
                 if (gcb.val.getStatus() == ENGINE_SUCCESS) {
                     v->unlocked_restoreValue(gcb.val.getValue(), vb->ht);
                     cb_assert(v->isResident());
-                    ReaderLockHolder(vb->getStateLock());
                     if (vb->getState() == vbucket_state_active &&
                         v->getExptime() != gcb.val.getValue()->getExptime() &&
                         v->getCas() == gcb.val.getValue()->getCas()) {
@@ -1543,74 +1543,75 @@ void EventuallyPersistentStore::completeBGFetchMulti(uint16_t vbId,
         ENGINE_ERROR_CODE status = bgitem->value.getStatus();
         Item *fetchedValue = bgitem->value.getValue();
         const std::string &key = (*itemItr).first;
-
-        int bucket = 0;
-        LockHolder blh = vb->ht.getLockedBucket(key, &bucket);
-        StoredValue *v = fetchValidValue(vb, key, bucket, true);
-        if (bgitem->metaDataOnly) {
-            if (v && v->unlocked_restoreMeta(fetchedValue, status, vb->ht)) {
-                status = ENGINE_SUCCESS;
-            }
-        } else {
-            bool restore = false;
-            if (v && v->isResident()) {
-                status = ENGINE_SUCCESS;
+        {   // locking scope
+            ReaderLockHolder rlh(vb->getStateLock());
+
+            int bucket = 0;
+            LockHolder blh = vb->ht.getLockedBucket(key, &bucket);
+            StoredValue *v = fetchValidValue(vb, key, bucket, true);
+            if (bgitem->metaDataOnly) {
+                if (v && v->unlocked_restoreMeta(fetchedValue, status, vb->ht)) {
+                    status = ENGINE_SUCCESS;
+                }
             } else {
-                switch (eviction_policy) {
-                    case VALUE_ONLY:
-                        if (v && !v->isResident() && !v->isDeleted()) {
-                            restore = true;
-                        }
-                        break;
-                    case FULL_EVICTION:
-                        if (v) {
-                            if (v->isTempInitialItem() ||
-                                (!v->isResident() && !v->isDeleted())) {
+                bool restore = false;
+                if (v && v->isResident()) {
+                    status = ENGINE_SUCCESS;
+                } else {
+                    switch (eviction_policy) {
+                        case VALUE_ONLY:
+                            if (v && !v->isResident() && !v->isDeleted()) {
                                 restore = true;
                             }
-                        }
-                        break;
-                    default:
-                        throw std::logic_error("Unknown eviction policy");
+                            break;
+                        case FULL_EVICTION:
+                            if (v) {
+                                if (v->isTempInitialItem() ||
+                                    (!v->isResident() && !v->isDeleted())) {
+                                    restore = true;
+                                }
+                            }
+                            break;
+                        default:
+                            throw std::logic_error("Unknown eviction policy");
+                    }
                 }
-            }
 
-            if (restore) {
-                if (status == ENGINE_SUCCESS) {
-                    v->unlocked_restoreValue(fetchedValue, vb->ht);
-                    cb_assert(v->isResident());
-                    ReaderLockHolder(vb->getStateLock());
-                    if (vb->getState() == vbucket_state_active &&
-                        v->getExptime() != fetchedValue->getExptime() &&
-                        v->getCas() == fetchedValue->getCas()) {
-                        // MB-9306: It is possible that by the time
-                        // bgfetcher returns, the item may have been
-                        // updated and queued
-                        // Hence test the CAS value to be the same first.
-                        // exptime mutated, schedule it into new checkpoint
-                        queueDirty(vb, v, &blh);
-                    }
-                } else if (status == ENGINE_KEY_ENOENT) {
-                    v->setStoredValueState(StoredValue::state_non_existent_key);
-                    if (eviction_policy == FULL_EVICTION) {
-                        // For the full eviction, we should notify
-                        // ENGINE_SUCCESS to the memcached worker thread,
-                        // so that the worker thread can visit the
-                        // ep-engine and figure out the correct error
-                        // code.
-                        status = ENGINE_SUCCESS;
+                if (restore) {
+                    if (status == ENGINE_SUCCESS) {
+                        v->unlocked_restoreValue(fetchedValue, vb->ht);
+                        cb_assert(v->isResident());
+                        if (vb->getState() == vbucket_state_active &&
+                            v->getExptime() != fetchedValue->getExptime() &&
+                            v->getCas() == fetchedValue->getCas()) {
+                            // MB-9306: It is possible that by the time
+                            // bgfetcher returns, the item may have been
+                            // updated and queued
+                            // Hence test the CAS value to be the same first.
+                            // exptime mutated, schedule it into new checkpoint
+                            queueDirty(vb, v, &blh);
+                        }
+                    } else if (status == ENGINE_KEY_ENOENT) {
+                        v->setStoredValueState(StoredValue::state_non_existent_key);
+                        if (eviction_policy == FULL_EVICTION) {
+                            // For the full eviction, we should notify
+                            // ENGINE_SUCCESS to the memcached worker thread,
+                            // so that the worker thread can visit the
+                            // ep-engine and figure out the correct error
+                            // code.
+                            status = ENGINE_SUCCESS;
+                        }
+                    } else {
+                        // underlying kvstore couldn't fetch requested data
+                        // log returned error and notify TMPFAIL to client
+                        LOG(EXTENSION_LOG_WARNING,
+                            "Warning: failed background fetch for vb=%d "
+                            "key=%s", vbId, key.c_str());
+                        status = ENGINE_TMPFAIL;
                     }
-                } else {
-                    // underlying kvstore couldn't fetch requested data
-                    // log returned error and notify TMPFAIL to client
-                    LOG(EXTENSION_LOG_WARNING,
-                        "Warning: failed background fetch for vb=%d "
-                        "key=%s", vbId, key.c_str());
-                    status = ENGINE_TMPFAIL;
                 }
             }
-        }
-        blh.unlock();
+        } // locking scope ends
 
         if (bgitem->metaDataOnly) {
             ++stats.bg_meta_fetched;
@@ -1680,7 +1681,10 @@ GetValue EventuallyPersistentStore::getInternal(const std::string &key,
     if (!vb) {
         ++stats.numNotMyVBuckets;
         return GetValue(NULL, ENGINE_NOT_MY_VBUCKET);
-    } else if (honorStates && vb->getState() == vbucket_state_dead) {
+    }
+
+    ReaderLockHolder rlh(vb->getStateLock());
+    if (honorStates && vb->getState() == vbucket_state_dead) {
         ++stats.numNotMyVBuckets;
         return GetValue(NULL, ENGINE_NOT_MY_VBUCKET);
     } else if (honorStates && vb->getState() == disallowedState) {
@@ -1777,7 +1781,13 @@ ENGINE_ERROR_CODE EventuallyPersistentStore::getMetaData(
 {
     (void) cookie;
     RCPtr<VBucket> vb = getVBucket(vbucket);
-    if (!vb || vb->getState() == vbucket_state_dead ||
+    if (!vb) {
+        ++stats.numNotMyVBuckets;
+        return ENGINE_NOT_MY_VBUCKET;
+    }
+
+    ReaderLockHolder rlh(vb->getStateLock());
+    if (vb->getState() == vbucket_state_dead ||
         vb->getState() == vbucket_state_replica) {
         ++stats.numNotMyVBuckets;
         return ENGINE_NOT_MY_VBUCKET;
@@ -1839,7 +1849,7 @@ ENGINE_ERROR_CODE EventuallyPersistentStore::setWithMeta(const Item &itm,
         return ENGINE_NOT_MY_VBUCKET;
     }
 
-    ReaderLockHolder(vb->getStateLock());
+    ReaderLockHolder rlh(vb->getStateLock());
     if (vb->getState() == vbucket_state_dead) {
         ++stats.numNotMyVBuckets;
         return ENGINE_NOT_MY_VBUCKET;
@@ -1925,7 +1935,10 @@ GetValue EventuallyPersistentStore::getAndUpdateTtl(const std::string &key,
     if (!vb) {
         ++stats.numNotMyVBuckets;
         return GetValue(NULL, ENGINE_NOT_MY_VBUCKET);
-    } else if (vb->getState() == vbucket_state_dead) {
+    }
+
+    ReaderLockHolder rlh(vb->getStateLock());
+    if (vb->getState() == vbucket_state_dead) {
         ++stats.numNotMyVBuckets;
         return GetValue(NULL, ENGINE_NOT_MY_VBUCKET);
     } else if (vb->getState() == vbucket_state_replica) {
@@ -1967,7 +1980,6 @@ GetValue EventuallyPersistentStore::getAndUpdateTtl(const std::string &key,
                     ENGINE_SUCCESS, v->getBySeqno());
 
         if (exptime_mutated) {
-            ReaderLockHolder(vb->getStateLock());
             if (vb->getState() == vbucket_state_active) {
                 // persist the item in the underlying storage for
                 // mutated exptime but only if VB is active.