SERVER-4328 cleaning of d_concurrency for readability. removed the 't' state for temprelease. that was making the code complicated it wasn't work it.

we can track that in LockState if we like, which would be good for assertion puproses, but this is better than the way it was.

4 files changed, 50 insertions, 68 deletions

diff --git a/src/mongo/db/d_concurrency.cpp b/src/mongo/db/d_concurrency.cpp
index be1aeb6a7c8..3775235cb48 100755
--- a/src/mongo/db/d_concurrency.cpp
+++ b/src/mongo/db/d_concurrency.cpp
@@ -156,10 +156,7 @@ namespace mongo {
     }
     static void lock_W() { 
         LockState& ls = lockState();
-        if( ls.threadState == 't' ) { 
-            DEV warning() << "W locking inside a temprelease, seems nonideal" << endl;
-        }
-        else if(  ls.threadState ) {
+        if(  ls.threadState ) {
             log() << "can't lock_W, threadState=" << (int) ls.threadState << endl;
             fassert(0,false);
         }
@@ -171,11 +168,10 @@ namespace mongo {
         }
         locked_W();
     }
-    static void unlock_W(char oldState = 0) { 
+    static void unlock_W() { 
         wassert( threadState() == 'W' );
         unlocking_W();
-        dassert( oldState == 0 ||  oldState == 't' );
-        threadState() = oldState;
+        threadState() = 0;
         q.unlock_W();
     }
     static void lock_R() { 
@@ -192,7 +188,7 @@ namespace mongo {
     }    
     static void lock_w() { 
         char &ts = threadState();
-        assert( ts == 0 || ts == 't' );
+        assert( ts == 0 );
         getDur().commitIfNeeded();
         ts = 'w';
         Acquiring a('w');
@@ -206,7 +202,7 @@ namespace mongo {
     }
     static void lock_r() {
         char& ts = threadState();
-        assert( ts == 0 || ts == 't' );
+        assert( ts == 0 );
         ts = 'r';
         Acquiring a('r');
         q.lock_r();
@@ -354,7 +350,7 @@ namespace mongo {
             error() << "TempRelease called but threadState()=" << type << endl;
             assert(false);
         }
-        ls.threadState = 't';
+        ls.threadState = 0;
         dassert( ls.recursive == 0 );
     }
     Lock::TempRelease::~TempRelease() {
@@ -364,7 +360,7 @@ namespace mongo {
         LockState& ls = lockState();
         dassert( ls.recursive == 0 );
         DESTRUCTOR_GUARD( 
-            fassert(0, ls.threadState == 't' || ls.threadState == 0);
+            fassert(0, ls.threadState == 0);
             ls.threadState = 0;
             switch( type ) {
             case 'W':
@@ -392,35 +388,37 @@ namespace mongo {
             }
         )
     }
-    Lock::GlobalWrite::GlobalWrite(bool sg) : stopGreed(sg), old(threadState()) {
-        if( old == 'W' ) { 
+
+    Lock::GlobalWrite::GlobalWrite(bool sg) : 
+        stoppedGreed(sg)
+    {
+        char ts = threadState();
+        if( ts == 'W' ) { 
             recursive()++;
-            DEV if( stopGreed ) { 
+            DEV if( sg ) { 
                 log() << "info Lock::GlobalWrite does not stop greed on recursive invocation" << endl;
             }
+            return;
         }
-        else {
-            if( stopGreed ) {
-                lock_W_stop_greed();
-            } else {
-                lock_W();
-            }
+        if( sg ) {
+            lock_W_stop_greed();
+        } else {
+            lock_W();
         }
     }
     Lock::GlobalWrite::~GlobalWrite() {
         if( recursive() ) { 
             recursive()--;
+            return;
+        }
+        if( threadState() == 'R' ) { // we downgraded
+            unlock_R();
         }
         else {
-            if( threadState() == 'R' ) { // downgraded
-                unlock_R();
-            }
-            else {
-                unlock_W(old);
-            }
-            if( stopGreed ) {
-                q.start_greed();
-            }
+            unlock_W();
+        }
+        if( stoppedGreed ) {
+            q.start_greed();
         }
         fassert( 0, recursive() < 1000 );
     }
@@ -471,9 +469,8 @@ namespace mongo {
         case 'W' :
             ls.recursive++;
             return true; // lock nothing further
-        default: // 't'
+        default:
             assert(false);
-        case 't':
         case  0  : 
             ;
         }
@@ -485,7 +482,6 @@ namespace mongo {
             break;
         default:
             assert(false);
-        case 't':
         case  0  : 
             lock_w();
             locked_w = true;
@@ -579,10 +575,6 @@ namespace mongo {
             return false;
         default:
             assert(false);
-        case 't' :
-            {
-                LOG(2) << "info relocking inside a temprelease" << endl;
-            }
         case  0  : 
             ;
         }
@@ -595,7 +587,6 @@ namespace mongo {
             break;
         default:
             assert(false);
-        case 't' :
         case  0  : 
             lock_r();
             locked_r = true;
@@ -676,9 +667,8 @@ namespace mongo {
    }
 }
 
-// legacy hooks
+// legacy hooks and glue
 namespace mongo { 
-
     writelock::writelock() { 
         lk1.reset( new Lock::GlobalWrite() );
     }
@@ -690,7 +680,6 @@ namespace mongo {
             lk2.reset( new Lock::DBWrite(ns) );
         }
     }
-
     writelocktry::writelocktry( int tryms ) : 
         _got( lock_W_try(tryms) )
     { }
@@ -698,7 +687,6 @@ namespace mongo {
         if( _got )
             unlock_W();
     }
-
     // note: the 'already' concept here might be a bad idea as a temprelease wouldn't notice it is nested then
     readlocktry::readlocktry( int tryms )      
     {
@@ -715,7 +703,6 @@ namespace mongo {
         if( !_already && _got )
             unlock_R();
     }
-
     readlock::readlock() {
         lk1.reset( new Lock::GlobalRead() );
     }
@@ -727,12 +714,6 @@ namespace mongo {
             lk2.reset( new Lock::DBRead(ns) );
         }
     }
-
-}
-
-// legacy MongoMutex glue
-namespace mongo {
-
     /* backward compatible glue. it could be that the assumption was that 
        it's a global read lock, so 'r' and 'w' don't qualify.
        */ 
@@ -757,7 +738,6 @@ namespace mongo {
             msgasserted(16102, "expected read lock");
         }
     }
-
     void locked_W() {
         d.dbMutex._minfo.entered(); // hopefully eliminate one day 
     }
@@ -769,10 +749,8 @@ namespace mongo {
         d.dbMutex._minfo.leaving();
         dur::releasingWriteLock();
     }
-
     MongoMutex::MongoMutex() {
         static int n = 0;
         assert( ++n == 1 );
     }
-
 }
diff --git a/src/mongo/db/d_concurrency.h b/src/mongo/db/d_concurrency.h
index d2cd75563c3..d26211a961b 100644
--- a/src/mongo/db/d_concurrency.h
+++ b/src/mongo/db/d_concurrency.h
@@ -32,9 +32,12 @@ namespace mongo {
             int local;
         };
         class GlobalWrite : boost::noncopyable { // recursive is ok
-            const bool stopGreed;
-            const char old;
+            const bool stoppedGreed;
         public:
+            /** @param stopGreed after acquisition stop greediness of other threads for write locks. this should generally not 
+                                 be used it is for exceptional circumstances. journaling uses it. perhaps this should go away, 
+                                 it makes the software more complicated.
+            */
             GlobalWrite(bool stopGreed = false); 
             ~GlobalWrite();
             void downgrade(); // W -> R
diff --git a/src/mongo/db/dur.cpp b/src/mongo/db/dur.cpp
index d33dde7a768..83466e63993 100644
--- a/src/mongo/db/dur.cpp
+++ b/src/mongo/db/dur.cpp
@@ -710,18 +710,11 @@ namespace mongo {
                     return;
             }
 
-            // starvation on read locks could occur.  so if read lock acquisition is slow, try to get a
-            // write lock instead.  otherwise journaling could be delayed too long (too much data will 
-            // not accumulate though, as commitIfNeeded logic will have executed in the meantime if there 
-            // has been writes)
-            //
-            // we could greedily get an R lock, but we need to remapprivateview anyway.
-            // todo : 
-            //   greedily get R 
-            //   group commit
-            //   upgrade to W
-            //   remap private view
-            Lock::GlobalWrite w(true);
+            // we get a write lock, downgrade, do work, upgrade, finish work.
+            // getting a write lock is helpful also as we need to be greedy and not be starved here
+            // note our "stopgreed" parm -- to stop greed by others while we are working. you can't write 
+            // anytime soon anyway if we are journaling for a while, that was the idea.
+            Lock::GlobalWrite w(/*stopgreed:*/true);
             w.downgrade();
             groupCommit(&w);
         }
diff --git a/src/mongo/dbtests/threadedtests.cpp b/src/mongo/dbtests/threadedtests.cpp
index d70955d0167..6fc97850fa4 100644
--- a/src/mongo/dbtests/threadedtests.cpp
+++ b/src/mongo/dbtests/threadedtests.cpp
@@ -69,7 +69,7 @@ namespace ThreadedTests {
     //const int nthr=7;
     class MongoMutexTest : public ThreadedTest<nthr> {
 #if defined(_DEBUG)
-        enum { N = 5000 };
+        enum { N = 2000 };
 #else
         enum { N = 4000/*0*/ };
 #endif
@@ -166,7 +166,8 @@ namespace ThreadedTests {
                 }
                 else if( i % 7 == 6 ) {
                     if( i > N/2 ) { 
-                        if( i % 11 == 0 ) { 
+                        int q = i % 11;
+                        if( q == 0 ) { 
                             Lock::DBRead r("foo");
                             Lock::DBRead r2("foo");
                             Lock::DBRead r3("local");
@@ -174,7 +175,14 @@ namespace ThreadedTests {
                                 Lock::TempRelease t;
                             }
                         }
-                        else { 
+                        else if( q == 1 ) {
+                            // test locking local only -- with no preceeding lock
+                            // TODO { Lock::DBRead  x("local"); }
+                            //{ Lock::DBWrite x("local"); }
+                        } else if( q == 1 ) {
+                            // TODO { Lock::DBRead  x("admin"); }
+                            //{ Lock::DBWrite x("admin"); }
+                        } else { 
                             Lock::DBWrite w("foo");
                             {
                                 Lock::TempRelease t;