tubik: Correct use of flags_cond and version_cond

Waiters of flags_cond and version_cond were not doing so correctly;
the correct way is to acquire a lock prior to their respective checks,
and atomically drop/acquire that lock with pthread_cond_wait.
Otherwise, we could miss a wakeup if a flag changed between our check
and when we wait.

To make this possible, make versionLock a normal pthread mutex in
AFS_PTHREAD_ENV, so it is a lock we can pass to pthread_cond_wait.
Make the waiters pass versionLock to pthread_cond_wait, and eliminate
flags_mutex and version_mutex.

Change-Id: Id33a72182b907d069e342cb049e23868ab2f7eb9
Reviewed-on: http://gerrit.openafs.org/1681
Tested-by: Andrew Deason <adeason@sinenomine.net>
Reviewed-by: Derrick Brashear <shadow@dementia.org>
Tested-by: Derrick Brashear <shadow@dementia.org>

diff --git a/src/ubik/ubik.c b/src/ubik/ubik.c
index 4187ac0..9bb9a97 100644 (file)
--- a/src/ubik/ubik.c
+++ b/src/ubik/ubik.c
@@ -415,7 +415,11 @@ ubik_ServerInitCommon(afs_int32 myHost, short myPort,
     tdb->activeTrans = (struct ubik_trans *)0;
     memset(&tdb->version, 0, sizeof(struct ubik_version));
     memset(&tdb->cachedVersion, 0, sizeof(struct ubik_version));
+#ifdef AFS_PTHREAD_ENV
+    assert(pthread_mutex_init(&tdb->versionLock, NULL) == 0);
+#else
     Lock_Init(&tdb->versionLock);
+#endif
     tdb->flags = 0;
     tdb->read = uphys_read;
     tdb->write = uphys_write;
@@ -434,8 +438,6 @@ ubik_ServerInitCommon(afs_int32 myHost, short myPort,
 #ifdef AFS_PTHREAD_ENV
     assert(pthread_cond_init(&tdb->version_cond, NULL) == 0);
     assert(pthread_cond_init(&tdb->flags_cond, NULL) == 0);
-    assert(pthread_mutex_init(&tdb->version_mutex, NULL) == 0);
-    assert(pthread_mutex_init(&tdb->flags_mutex, NULL) == 0);
 #endif /* AFS_PTHREAD_ENV */
 
     /* initialize RX */
@@ -662,16 +664,15 @@ BeginTrans(register struct ubik_dbase *dbase, afs_int32 transMode,
     if (transMode == UBIK_WRITETRANS) {
        /* if we're writing already, wait */
        while (dbase->flags & DBWRITING) {
-           DBRELE(dbase);
 #ifdef AFS_PTHREAD_ENV
-           assert(pthread_mutex_lock(&dbase->flags_mutex) == 0);
-           assert(pthread_cond_wait(&dbase->flags_cond, &dbase->flags_mutex) == 0);
-           assert(pthread_mutex_unlock(&dbase->flags_mutex) == 0);
+           assert(pthread_cond_wait(&dbase->flags_cond, &dbase->versionLock) == 0);
 #else
+           DBRELE(dbase);
            LWP_WaitProcess(&dbase->flags);
-#endif
            DBHOLD(dbase);
+#endif
        }
+
        if (!ubeacon_AmSyncSite()) {
            DBRELE(dbase);
            return UNOTSYNC;
@@ -1194,16 +1195,19 @@ int
 ubik_WaitVersion(register struct ubik_dbase *adatabase,
                 register struct ubik_version *aversion)
 {
+    DBHOLD(adatabase);
     while (1) {
        /* wait until version # changes, and then return */
-       if (vcmp(*aversion, adatabase->version) != 0)
+       if (vcmp(*aversion, adatabase->version) != 0) {
+           DBRELE(adatabase);
            return 0;
+       }
 #ifdef AFS_PTHREAD_ENV
-       assert(pthread_mutex_lock(&adatabase->version_mutex) == 0);
-       assert(pthread_cond_wait(&adatabase->version_cond,&adatabase->version_mutex) == 0);
-       assert(pthread_mutex_unlock(&adatabase->version_mutex) == 0);
+       assert(pthread_cond_wait(&adatabase->version_cond, &adatabase->versionLock) == 0);
 #else
+       DBRELE(adatabase);
        LWP_WaitProcess(&adatabase->version);   /* same vers, just wait */
+       DBHOLD(adatabase);
 #endif
     }
 }

diff --git a/src/ubik/ubik.p.h b/src/ubik/ubik.p.h
index adcbdac..fcfd460 100644 (file)
--- a/src/ubik/ubik.p.h
+++ b/src/ubik/ubik.p.h
@@ -168,7 +168,9 @@ struct ubik_dbase {
     char *pathName;            /*!< root name for dbase */
     struct ubik_trans *activeTrans;    /*!< active transaction list */
     struct ubik_version version;       /*!< version number */
-#if defined(UKERNEL)
+#ifdef AFS_PTHREAD_ENV
+    pthread_mutex_t versionLock;       /*!< lock on version number */
+#elif defined(UKERNEL)
     struct afs_lock versionLock;       /*!< lock on version number */
 #else                          /* defined(UKERNEL) */
     struct Lock versionLock;   /*!< lock on version number */
@@ -195,8 +197,6 @@ struct ubik_dbase {
 #ifdef AFS_PTHREAD_ENV
     pthread_cond_t version_cond;    /*!< condition variable to manage changes to version */
     pthread_cond_t flags_cond;      /*!< condition variable to manage changes to flags */
-    pthread_mutex_t version_mutex;
-    pthread_mutex_t flags_mutex;
 #endif
 };
 
@@ -291,8 +291,13 @@ struct ubik_server {
 };
 
 /*! \name hold and release functions on a database */
-#define        DBHOLD(a)       ObtainWriteLock(&((a)->versionLock))
-#define        DBRELE(a)       ReleaseWriteLock(&((a)->versionLock))
+#ifdef AFS_PTHREAD_ENV
+# define       DBHOLD(a)       assert(pthread_mutex_lock(&((a)->versionLock)) == 0)
+# define       DBRELE(a)       assert(pthread_mutex_unlock(&((a)->versionLock)) == 0)
+#else /* !AFS_PTHREAD_ENV */
+# define       DBHOLD(a)       ObtainWriteLock(&((a)->versionLock))
+# define       DBRELE(a)       ReleaseWriteLock(&((a)->versionLock))
+#endif /* !AFS_PTHREAD_ENV */
 /*\}*/
 
 /* globals */