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>

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->versionLock) == 0);
-	    assert(pthread_cond_wait(&dbase->flags_cond, &dbase->flags_mutex) == 0);
-	    assert(pthread_mutex_unlock(&dbase->flags_mutex) == 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->versionLock) == 0);
-	assert(pthread_cond_wait(&adatabase->version_cond,&adatabase->version_mutex) == 0);
-	assert(pthread_mutex_unlock(&adatabase->version_mutex) == 0);
 #else
+	DBRELE(adatabase);
 	LWP_WaitProcess(&adatabase->version);	/* same vers, just wait */
+	DBHOLD(adatabase);
 #endif
     }
 }

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))
+#ifdef AFS_PTHREAD_ENV
-#define	DBRELE(a)	ReleaseWriteLock(&((a)->versionLock))
+# 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 */