Merge remote-tracking branch 'couchbase/3.0.x' into sherlock 89/65389/2
authorDave Rigby <daver@couchbase.com>
Thu, 7 Jul 2016 09:34:12 +0000 (10:34 +0100)
committerDave Rigby <daver@couchbase.com>
Thu, 7 Jul 2016 09:34:12 +0000 (10:34 +0100)
* couchbase/3.0.x:
  MB-19359: [3] Address lock inversion with vb's state lock and snapshot lock
  MB-19383: [BP] Address possible data race with startuptime
  MB-19380: Address data race observed with vb's pendingBGFetches
  MB-19360: Init mock server in stream module tests
  MB-19382: [BP] Create a variable to get correct locking scope

Change-Id: Ice3e97c12ee1423923ffeda47bc30890332a1770

1  2 
src/dcp-stream.cc
src/ep.cc
src/ep_engine.cc
src/ep_engine.h
src/vbucket.h
tests/module_tests/stream_test.cc

@@@ -143,11 -287,13 +143,12 @@@ ActiveStream::ActiveStream(EventuallyPe
  
      RCPtr<VBucket> vbucket = engine->getVBucket(vb);
      if (vbucket) {
-         ReaderLockHolder rlh(vbucket->getStateLock());
+         // An atomic read of vbucket state without acquiring the
+         // reader lock for state should suffice here.
          if (vbucket->getState() == vbucket_state_replica) {
 -            uint64_t snapshot_start, snapshot_end;
 -            vbucket->getCurrentSnapshot(snapshot_start, snapshot_end);
 -            if (snapshot_end > en_seqno) {
 -                end_seqno_ = snapshot_end;
 +            snapshot_info_t info = vbucket->checkpointManager.getSnapshotInfo();
 +            if (info.range.end > en_seqno) {
 +                end_seqno_ = info.range.end;
              }
          }
      }
diff --cc src/ep.cc
+++ b/src/ep.cc
@@@ -1755,81 -1543,75 +1755,83 @@@ void EventuallyPersistentStore::complet
          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))
-                 || ENGINE_KEY_ENOENT == status) {
-                 /* If ENGINE_KEY_ENOENT is the status from storage and the temp
-                  key is removed from hash table by the time bgfetch returns
-                  (in case multiple bgfetch is scheduled for a key), we still
-                  need to return ENGINE_SUCCESS to the memcached worker thread,
-                  so that the worker thread can visit the ep-engine and figure
-                  out the correct flow */
-                 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)) {
++                if ((v && v->unlocked_restoreMeta(fetchedValue, status, vb->ht))
++                    || ENGINE_KEY_ENOENT == status) {
++                    /* If ENGINE_KEY_ENOENT is the status from storage and the temp
++                     key is removed from hash table by the time bgfetch returns
++                     (in case multiple bgfetch is scheduled for a key), we still
++                     need to return ENGINE_SUCCESS to the memcached worker thread,
++                     so that the worker thread can visit the ep-engine and figure
++                     out the correct flow */
+                     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, NULL);
-                     }
-                 } 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());
++                        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);
++                            queueDirty(vb, v, &blh, NULL);
+                         }
+                     } 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;
Simple merge
diff --cc src/ep_engine.h
Simple merge
diff --cc src/vbucket.h
Simple merge
Simple merge