SERVER-16431 Move the acquisition of the flush lock to a separate method

Also adds assertions and new tests. No functional changes yet - this is in
preparation for removal of the parallel batch writer lock.

4 files changed, 102 insertions, 70 deletions

diff --git a/src/mongo/db/concurrency/lock_state.cpp b/src/mongo/db/concurrency/lock_state.cpp
index 435ed15bb2f..12eed73724e 100644
--- a/src/mongo/db/concurrency/lock_state.cpp
+++ b/src/mongo/db/concurrency/lock_state.cpp
@@ -268,56 +268,21 @@ namespace {
 
     template<bool IsForMMAPV1>
     LockResult LockerImpl<IsForMMAPV1>::lockGlobal(LockMode mode, unsigned timeoutMs) {
-        LockResult globalLockResult = lockGlobalBegin(mode);
-        if (globalLockResult != LOCK_OK) {
-            // Could only be LOCK_WAITING (checked by lockGlobalComplete)
-            globalLockResult = lockGlobalComplete(timeoutMs);
-
-            // If waiting for the lock failed, no point in asking for the flush lock
-            if (globalLockResult != LOCK_OK) {
-                return globalLockResult;
-            }
-        }
-
-        // We would have returned above if global lock acquisition failed for any reason
-        invariant(globalLockResult == LOCK_OK);
-
-        // We are done if this is not MMAP V1
-        if (!IsForMMAPV1) {
-            return LOCK_OK;
-        }
-
-        // Special-handling for MMAP V1 commit concurrency control. We will not obey the timeout
-        // request to simplify the logic here, since the only places which acquire global lock with
-        // a timeout is the shutdown code.
-
-        // The flush lock always has a reference count of 1, because it is dropped at the end of
-        // each write unit of work in order to allow the flush thread to run. See the comments in
-        // the header for information on how the MMAP V1 journaling system works.
-        const LockRequest* globalLockRequest = _requests.find(resourceIdGlobal).objAddr();
-        if (globalLockRequest->recursiveCount > 1){
-            return LOCK_OK;
+        LockResult result = lockGlobalBegin(mode);
+        if (result == LOCK_WAITING) {
+            result = lockGlobalComplete(timeoutMs);
         }
 
-        const LockResult flushLockResult = lock(resourceIdMMAPV1Flush,
-                                                _getModeForMMAPV1FlushLock());
-        if (flushLockResult != LOCK_OK) {
-            invariant(flushLockResult == LOCK_TIMEOUT);
-            invariant(unlock(resourceIdGlobal));
+        if (result == LOCK_OK) {
+            lockMMAPV1Flush();
         }
 
-        return flushLockResult;
-    }
-
-    template<bool IsForMMAPV1>
-    LockResult LockerImpl<IsForMMAPV1>::lockGlobalComplete(unsigned timeoutMs) {
-        return lockComplete(resourceIdGlobal, timeoutMs, false);
+        return result;
     }
 
     template<bool IsForMMAPV1>
     LockResult LockerImpl<IsForMMAPV1>::lockGlobalBegin(LockMode mode) {
         const LockResult result = lockBegin(resourceIdGlobal, mode);
-
         if (result == LOCK_OK) return LOCK_OK;
 
         // Currently, deadlock detection does not happen inline with lock acquisition so the only
@@ -328,6 +293,24 @@ namespace {
     }
 
     template<bool IsForMMAPV1>
+    LockResult LockerImpl<IsForMMAPV1>::lockGlobalComplete(unsigned timeoutMs) {
+        return lockComplete(resourceIdGlobal, timeoutMs, false);
+    }
+
+    template<bool IsForMMAPV1>
+    void LockerImpl<IsForMMAPV1>::lockMMAPV1Flush() {
+        if (!IsForMMAPV1) return;
+
+        // The flush lock always has a reference count of 1, because it is dropped at the end of
+        // each write unit of work in order to allow the flush thread to run. See the comments in
+        // the header for information on how the MMAP V1 journaling system works.
+        LockRequest* globalLockRequest = _requests.find(resourceIdGlobal).objAddr();
+        if (globalLockRequest->recursiveCount == 1) {
+            invariant(LOCK_OK == lock(resourceIdMMAPV1Flush, _getModeForMMAPV1FlushLock()));
+        }
+    }
+
+    template<bool IsForMMAPV1>
     void LockerImpl<IsForMMAPV1>::downgradeGlobalXtoSForMMAPV1() {
         invariant(!inAWriteUnitOfWork());
 
@@ -552,16 +535,14 @@ namespace {
         // The global lock must have been acquired just once
         stateOut->globalMode = globalRequest->mode;
         invariant(unlock(resourceIdGlobal));
-        if (IsForMMAPV1) {
-            invariant(unlock(resourceIdMMAPV1Flush));
-        }
 
         // Next, the non-global locks.
         for (LockRequestsMap::Iterator it = _requests.begin(); !it.finished(); it.next()) {
             const ResourceId resId = it.key();
 
-            // We don't support saving and restoring document-level locks.
-            invariant(RESOURCE_DATABASE == resId.getType() ||
+            // We should never have to save and restore metadata locks.
+            invariant((IsForMMAPV1 && (resourceIdMMAPV1Flush == resId)) ||
+                      RESOURCE_DATABASE == resId.getType() ||
                       RESOURCE_COLLECTION == resId.getType());
 
             // And, stuff the info into the out parameter.
@@ -585,11 +566,18 @@ namespace {
         // We shouldn't be saving and restoring lock state from inside a WriteUnitOfWork.
         invariant(!inAWriteUnitOfWork());
 
-        lockGlobal(state.globalMode); // also handles MMAPV1Flush
+        invariant(LOCK_OK == lockGlobal(state.globalMode));
 
         std::vector<OneLock>::const_iterator it = state.locks.begin();
         for (; it != state.locks.end(); it++) {
-            invariant(LOCK_OK == lock(it->resourceId, it->mode));
+            // This is a sanity check that lockGlobal restored the MMAP V1 flush lock in the
+            // expected mode.
+            if (IsForMMAPV1 && (it->resourceId == resourceIdMMAPV1Flush)) {
+                invariant(it->mode == _getModeForMMAPV1FlushLock());
+            }
+            else {
+                invariant(LOCK_OK == lock(it->resourceId, it->mode));
+            }
         }
     }
 
@@ -621,6 +609,10 @@ namespace {
                 request->compatibleFirst = true;
             }
         }
+        else {
+            // The global lock must always be acquired first
+            invariant(getLockMode(resourceIdGlobal) != MODE_NONE);
+        }
 
         _notify.clear();
 
diff --git a/src/mongo/db/concurrency/lock_state.h b/src/mongo/db/concurrency/lock_state.h
index 357b188fbd5..a017d238736 100644
--- a/src/mongo/db/concurrency/lock_state.h
+++ b/src/mongo/db/concurrency/lock_state.h
@@ -98,6 +98,7 @@ namespace mongo {
         virtual LockResult lockGlobal(LockMode mode, unsigned timeoutMs = UINT_MAX);
         virtual LockResult lockGlobalBegin(LockMode mode);
         virtual LockResult lockGlobalComplete(unsigned timeoutMs);
+        virtual void lockMMAPV1Flush();
 
         virtual void downgradeGlobalXtoSForMMAPV1();
         virtual bool unlockAll();
diff --git a/src/mongo/db/concurrency/lock_state_test.cpp b/src/mongo/db/concurrency/lock_state_test.cpp
index 04bfc4c8286..0c741a36a40 100644
--- a/src/mongo/db/concurrency/lock_state_test.cpp
+++ b/src/mongo/db/concurrency/lock_state_test.cpp
@@ -80,15 +80,15 @@ namespace {
     TEST(LockerImpl, ConflictWithTimeout) {
         const ResourceId resId(RESOURCE_COLLECTION, std::string("TestDB.collection"));
 
-        MMAPV1LockerImpl locker1;
+        DefaultLockerImpl locker1;
         ASSERT(LOCK_OK == locker1.lockGlobal(MODE_IX));
         ASSERT(LOCK_OK == locker1.lock(resId, MODE_X));
 
-        MMAPV1LockerImpl locker2;
+        DefaultLockerImpl locker2;
         ASSERT(LOCK_OK == locker2.lockGlobal(MODE_IX));
         ASSERT(LOCK_TIMEOUT == locker2.lock(resId, MODE_S, 0));
 
-        ASSERT(locker2.isLockHeldForMode(resId, MODE_NONE));
+        ASSERT(locker2.getLockMode(resId) == MODE_NONE);
 
         ASSERT(locker1.unlock(resId));
 
@@ -99,11 +99,11 @@ namespace {
     TEST(LockerImpl, ConflictUpgradeWithTimeout) {
         const ResourceId resId(RESOURCE_COLLECTION, std::string("TestDB.collection"));
 
-        MMAPV1LockerImpl locker1;
+        DefaultLockerImpl locker1;
         ASSERT(LOCK_OK == locker1.lockGlobal(MODE_IS));
         ASSERT(LOCK_OK == locker1.lock(resId, MODE_S));
 
-        MMAPV1LockerImpl locker2;
+        DefaultLockerImpl locker2;
         ASSERT(LOCK_OK == locker2.lockGlobal(MODE_IS));
         ASSERT(LOCK_OK == locker2.lock(resId, MODE_S));
 
@@ -115,7 +115,7 @@ namespace {
     }
 
     TEST(LockerImpl, ReadTransaction) {
-        MMAPV1LockerImpl locker;
+        DefaultLockerImpl locker;
 
         locker.lockGlobal(MODE_IS);
         locker.unlockAll();
@@ -135,7 +135,7 @@ namespace {
     TEST(LockerImpl, saveAndRestoreGlobal) {
         Locker::LockSnapshot lockInfo;
 
-        MMAPV1LockerImpl locker;
+        DefaultLockerImpl locker;
 
         // No lock requests made, no locks held.
         locker.saveLockStateAndUnlock(&lockInfo);
@@ -162,7 +162,7 @@ namespace {
     TEST(LockerImpl, saveAndRestoreGlobalAcquiredTwice) {
         Locker::LockSnapshot lockInfo;
 
-        MMAPV1LockerImpl locker;
+        DefaultLockerImpl locker;
 
         // No lock requests made, no locks held.
         locker.saveLockStateAndUnlock(&lockInfo);
@@ -188,29 +188,66 @@ namespace {
     TEST(LockerImpl, saveAndRestoreDBAndCollection) {
         Locker::LockSnapshot lockInfo;
 
-        MMAPV1LockerImpl locker;
+        DefaultLockerImpl locker;
 
         const ResourceId resIdDatabase(RESOURCE_DATABASE, std::string("TestDB"));
         const ResourceId resIdCollection(RESOURCE_COLLECTION, std::string("TestDB.collection"));
 
         // Lock some stuff.
         locker.lockGlobal(MODE_IX);
-        ASSERT(LOCK_OK == locker.lock(resIdDatabase, MODE_IX));
-        ASSERT(LOCK_OK == locker.lock(resIdCollection, MODE_X));
+        ASSERT_EQUALS(LOCK_OK, locker.lock(resIdDatabase, MODE_IX));
+        ASSERT_EQUALS(LOCK_OK, locker.lock(resIdCollection, MODE_X));
         locker.saveLockStateAndUnlock(&lockInfo);
 
         // Things shouldn't be locked anymore.
-        ASSERT(locker.getLockMode(resIdDatabase) == MODE_NONE);
-        ASSERT(locker.getLockMode(resIdCollection) == MODE_NONE);
+        ASSERT_EQUALS(MODE_NONE, locker.getLockMode(resIdDatabase));
+        ASSERT_EQUALS(MODE_NONE, locker.getLockMode(resIdCollection));
 
         // Restore lock state.
         locker.restoreLockState(lockInfo);
 
         // Make sure things were re-locked.
-        ASSERT(locker.getLockMode(resIdDatabase) == MODE_IX);
-        ASSERT(locker.getLockMode(resIdCollection) == MODE_X);
+        ASSERT_EQUALS(MODE_IX, locker.getLockMode(resIdDatabase));
+        ASSERT_EQUALS(MODE_X, locker.getLockMode(resIdCollection));
 
-        locker.unlockAll();
+        ASSERT(locker.unlockAll());
+    }
+
+    TEST(LockerImpl, DefaultLocker) {
+        const ResourceId resId(RESOURCE_DATABASE, std::string("TestDB"));
+
+        DefaultLockerImpl locker;
+        ASSERT_EQUALS(LOCK_OK, locker.lockGlobal(MODE_IX));
+        ASSERT_EQUALS(LOCK_OK, locker.lock(resId, MODE_X));
+
+        // Make sure the flush lock IS NOT held
+        Locker::LockerInfo info;
+        locker.getLockerInfo(&info);
+        ASSERT(!info.waitingResource.isValid());
+        ASSERT_EQUALS(2, info.locks.size());
+        ASSERT_EQUALS(RESOURCE_GLOBAL, info.locks[0].resourceId.getType());
+        ASSERT_EQUALS(resId, info.locks[1].resourceId);
+
+        ASSERT(locker.unlockAll());
+    }
+
+    TEST(LockerImpl, MMAPV1Locker) {
+        const ResourceId resId(RESOURCE_DATABASE, std::string("TestDB"));
+
+        MMAPV1LockerImpl locker;
+        ASSERT_EQUALS(LOCK_OK, locker.lockGlobal(MODE_IX));
+        ASSERT_EQUALS(LOCK_OK, locker.lock(resId, MODE_X));
+
+        // Make sure the flush lock IS held
+        Locker::LockerInfo info;
+        locker.getLockerInfo(&info);
+        ASSERT(!info.waitingResource.isValid());
+        ASSERT_EQUALS(3, info.locks.size());
+        ASSERT_EQUALS(RESOURCE_GLOBAL, info.locks[0].resourceId.getType());
+        ASSERT_EQUALS(RESOURCE_MMAPV1_FLUSH, info.locks[1].resourceId.getType());
+        ASSERT_EQUALS(resId, info.locks[2].resourceId);
+
+        ASSERT(locker.unlockAll());
     }
 
 
@@ -251,7 +288,7 @@ namespace {
                 lockers[i].reset(new LockerForTests());
             }
 
-            LockerImpl<true> locker;
+            DefaultLockerImpl locker;
 
             // Do some warm-up loops
             for (int i = 0; i < 1000; i++) {
diff --git a/src/mongo/db/concurrency/locker.h b/src/mongo/db/concurrency/locker.h
index ad79a9e0b6f..e95f98a6855 100644
--- a/src/mongo/db/concurrency/locker.h
+++ b/src/mongo/db/concurrency/locker.h
@@ -76,11 +76,7 @@ namespace mongo {
         virtual LockResult lockGlobal(LockMode mode, unsigned timeoutMs = UINT_MAX) = 0;
 
         /**
-         * Requests *only* the global lock to be acquired in the specified mode. Does not do the
-         * full MMAP V1 concurrency control functionality, which acquires the flush lock as well.
-         *
-         * Should only be used for cases, where no data reads or writes will be performed, such as
-         * replication step-down.
+         * Requests the global lock to be acquired in the specified mode.
          *
          * See the comments for lockBegin/Complete for more information on the semantics.
          */
@@ -88,6 +84,12 @@ namespace mongo {
         virtual LockResult lockGlobalComplete(unsigned timeoutMs) = 0;
 
         /**
+         * This method is used only in the MMAP V1 storage engine, otherwise it is a no-op. See the
+         * comments in the implementation for more details on how MMAP V1 journaling works.
+         */
+        virtual void lockMMAPV1Flush() = 0;
+
+        /**
          * Decrements the reference count on the global lock.  If the reference count on the
          * global lock hits zero, the transaction is over, and unlockAll unlocks all other locks.
          *