Windows: split cm_buf_t.flags field to ensure proper locking

It turns out that for all these years the locks protecting
the cm_buf_t flags field have been racy.  Some of the flags
were protected by the cm_buf_t mutex and others by the
buf_globalLock.  This patchset splits the flags field so that
the appropriate lock will be used exclusively to protect a
common set of flags.

LICENSE MIT

Change-Id: I85c98c85244c987df30a811d405f7d897e407ba2
Reviewed-on: http://gerrit.openafs.org/1759
Tested-by: Jeffrey Altman <jaltman@openafs.org>
Reviewed-by: Jeffrey Altman <jaltman@openafs.org>

diff --git a/src/WINNT/afsd/cm_buf.c b/src/WINNT/afsd/cm_buf.c
index 10c0ea7..3920844 100644 (file)
--- a/src/WINNT/afsd/cm_buf.c
+++ b/src/WINNT/afsd/cm_buf.c
@@ -161,13 +161,13 @@ void buf_ReleaseLocked(cm_buf_t *bp, afs_uint32 writeLocked)
             lock_ConvertRToW(&buf_globalLock);
 
         if (bp->refCount == 0 &&
-            !(bp->flags & CM_BUF_INLRU)) {
+            !(bp->qFlags & CM_BUF_QINLRU)) {
             osi_QAdd((osi_queue_t **) &cm_data.buf_freeListp, &bp->q);
 
             /* watch for transition from empty to one element */
             if (!cm_data.buf_freeListEndp)
                 cm_data.buf_freeListEndp = cm_data.buf_freeListp;
-            bp->flags |= CM_BUF_INLRU;
+            bp->qFlags |= CM_BUF_QINLRU;
         }
 
         if (!writeLocked)
@@ -201,13 +201,13 @@ void buf_Release(cm_buf_t *bp)
     if (refCount == 0) {
         lock_ObtainWrite(&buf_globalLock);
         if (bp->refCount == 0 && 
-            !(bp->flags & CM_BUF_INLRU)) {
+            !(bp->qFlags & CM_BUF_QINLRU)) {
             osi_QAdd((osi_queue_t **) &cm_data.buf_freeListp, &bp->q);
 
             /* watch for transition from empty to one element */
             if (!cm_data.buf_freeListEndp)
                 cm_data.buf_freeListEndp = cm_data.buf_freeListp;
-            bp->flags |= CM_BUF_INLRU;
+            bp->qFlags |= CM_BUF_QINLRU;
         }
         lock_ReleaseWrite(&buf_globalLock);
     }
@@ -232,7 +232,7 @@ buf_Sync(int quitOnShutdown)
         */
         lock_ObtainMutex(&bp->mx);
 
-        if (bp->flags & CM_BUF_DIRTY && !(bp->flags & CM_BUF_REDIR)) {
+        if (bp->flags & CM_BUF_DIRTY && !(bp->qFlags & CM_BUF_QREDIR)) {
             /* start cleaning the buffer; don't touch log pages since
              * the log code counts on knowing exactly who is writing
              * a log page at any given instant.
@@ -270,7 +270,7 @@ buf_Sync(int quitOnShutdown)
 #endif
             *bpp = bp->dirtyp;
             bp->dirtyp = NULL;
-            bp->flags &= ~CM_BUF_INDL;
+            bp->qFlags &= ~CM_BUF_QINDL;
             if (cm_data.buf_dirtyListp == NULL)
                 cm_data.buf_dirtyListEndp = NULL;
             else if (cm_data.buf_dirtyListEndp == bp)
@@ -483,7 +483,7 @@ long buf_Init(int newFile, cm_buf_ops_t *opsp, afs_uint64 nbuffers)
                 cm_data.buf_allp = bp;
                 
                 osi_QAdd((osi_queue_t **)&cm_data.buf_freeListp, &bp->q);
-                bp->flags |= CM_BUF_INLRU;
+                bp->qFlags |= CM_BUF_QINLRU;
                 lock_InitializeMutex(&bp->mx, "Buffer mutex", LOCK_HIERARCHY_BUFFER);
                 
                 /* grab appropriate number of bytes from aligned zone */
@@ -861,7 +861,7 @@ void buf_Recycle(cm_buf_t *bp)
                 "incorrect cm_buf_t flags");
     lock_AssertWrite(&buf_globalLock);
 
-    if (bp->flags & CM_BUF_INHASH) {
+    if (bp->qFlags & CM_BUF_QINHASH) {
         /* Remove from hash */
 
         i = BUF_HASH(&bp->fid, &bp->offset);
@@ -891,7 +891,7 @@ void buf_Recycle(cm_buf_t *bp)
         if (nextBp)
             nextBp->fileHashBackp = prevBp;
 
-        bp->flags &= ~CM_BUF_INHASH;
+        bp->qFlags &= ~CM_BUF_QINHASH;
     }
 
     /* make the fid unrecognizable */
@@ -981,7 +981,7 @@ long buf_GetNewLocked(struct cm_scache *scp, osi_hyper_t *offsetp, cm_req_t *req
              */
 
             /* Don't recycle a buffer held by the redirector. */
-            if (bp->flags & CM_BUF_REDIR)
+            if (bp->qFlags & CM_BUF_QREDIR)
                 continue;
 
             /* don't recycle someone in our own chunk */
@@ -1038,7 +1038,7 @@ long buf_GetNewLocked(struct cm_scache *scp, osi_hyper_t *offsetp, cm_req_t *req
              * appropriate label, if requested.
              */
             if (scp) {
-                bp->flags |= CM_BUF_INHASH;
+                bp->qFlags |= CM_BUF_QINHASH;
                 bp->fid = scp->fid;
 #ifdef DEBUG
                bp->scp = scp;
@@ -1059,7 +1059,7 @@ long buf_GetNewLocked(struct cm_scache *scp, osi_hyper_t *offsetp, cm_req_t *req
             /* we should move it from the lru queue.  It better still be there,
              * since we've held the global (big) lock since we found it there.
              */
-            osi_assertx(bp->flags & CM_BUF_INLRU,
+            osi_assertx(bp->qFlags & CM_BUF_QINLRU,
                          "buf_GetNewLocked: LRU screwup");
 
             if (cm_data.buf_freeListEndp == bp) {
@@ -1067,7 +1067,7 @@ long buf_GetNewLocked(struct cm_scache *scp, osi_hyper_t *offsetp, cm_req_t *req
                 cm_data.buf_freeListEndp = (cm_buf_t *) osi_QPrev(&bp->q);
             }
             osi_QRemove((osi_queue_t **) &cm_data.buf_freeListp, &bp->q);
-            bp->flags &= ~CM_BUF_INLRU;
+            bp->qFlags &= ~CM_BUF_QINLRU;
 
             /* prepare to return it.  Give it a refcount */
             bp->refCount = 1;
@@ -1281,11 +1281,11 @@ long buf_Get(struct cm_scache *scp, osi_hyper_t *offsetp, cm_req_t *reqp, cm_buf
      * being recycled) when we're done in buf_Release.
      */
     lock_ObtainWrite(&buf_globalLock);
-    if (bp->flags & CM_BUF_INLRU) {
+    if (bp->qFlags & CM_BUF_QINLRU) {
         if (cm_data.buf_freeListEndp == bp)
             cm_data.buf_freeListEndp = (cm_buf_t *) osi_QPrev(&bp->q);
         osi_QRemove((osi_queue_t **) &cm_data.buf_freeListp, &bp->q);
-        bp->flags &= ~CM_BUF_INLRU;
+        bp->qFlags &= ~CM_BUF_QINLRU;
     }
     lock_ReleaseWrite(&buf_globalLock);
 
@@ -1314,7 +1314,7 @@ long buf_CountFreeList(void)
          * has been invalidate (by having its DV stomped upon), then
          * count it as free, since it isn't really being utilized.
          */
-        if (!(bufp->flags & CM_BUF_INHASH) || bufp->dataVersion == CM_BUF_VERSION_BAD)
+        if (!(bufp->qFlags & CM_BUF_QINHASH) || bufp->dataVersion == CM_BUF_VERSION_BAD)
             count++;
     }       
     lock_ReleaseRead(&buf_globalLock);
@@ -1399,7 +1399,7 @@ void buf_SetDirty(cm_buf_t *bp, afs_uint32 offset, afs_uint32 length, cm_user_t
          * already there.
          */
         lock_ObtainWrite(&buf_globalLock);
-        if (!(bp->flags & CM_BUF_INDL)) {
+        if (!(bp->qFlags & CM_BUF_QINDL)) {
             buf_HoldLocked(bp);
             if (!cm_data.buf_dirtyListp) {
                 cm_data.buf_dirtyListp = cm_data.buf_dirtyListEndp = bp;
@@ -1408,7 +1408,7 @@ void buf_SetDirty(cm_buf_t *bp, afs_uint32 offset, afs_uint32 length, cm_user_t
                 cm_data.buf_dirtyListEndp = bp;
             }
             bp->dirtyp = NULL;
-            bp->flags |= CM_BUF_INDL;
+            bp->qFlags |= CM_BUF_QINDL;
         }
         lock_ReleaseWrite(&buf_globalLock);
     }
@@ -1921,10 +1921,10 @@ int cm_DumpBufHashTable(FILE *outputFile, char *cookie, int lock)
            StringCbPrintfA(output, sizeof(output), 
                            "%s bp=0x%08X, hash=%d, fid (cell=%d, volume=%d, "
                            "vnode=%d, unique=%d), offset=%x:%08x, dv=%I64d, "
-                           "flags=0x%x, cmFlags=0x%x, error=0x%x, refCount=%d\r\n",
+                           "flags=0x%x, qFlags=0x%x cmFlags=0x%x, error=0x%x, refCount=%d\r\n",
                             cookie, (void *)bp, i, bp->fid.cell, bp->fid.volume, 
                             bp->fid.vnode, bp->fid.unique, bp->offset.HighPart, 
-                            bp->offset.LowPart, bp->dataVersion, bp->flags, 
+                            bp->offset.LowPart, bp->dataVersion, bp->flags, bp->qFlags,
                             bp->cmFlags, bp->error, bp->refCount);
            WriteFile(outputFile, output, (DWORD)strlen(output), &zilch, NULL);
         }
@@ -1939,10 +1939,10 @@ int cm_DumpBufHashTable(FILE *outputFile, char *cookie, int lock)
        StringCbPrintfA(output, sizeof(output), 
                         "%s bp=0x%08X, fid (cell=%d, volume=%d, "
                         "vnode=%d, unique=%d), offset=%x:%08x, dv=%I64d, "
-                        "flags=0x%x, cmFlags=0x%x, error=0x%x, refCount=%d\r\n",
+                        "flags=0x%x, qFlags=0x%x, cmFlags=0x%x, error=0x%x, refCount=%d\r\n",
                         cookie, (void *)bp, bp->fid.cell, bp->fid.volume, 
                         bp->fid.vnode, bp->fid.unique, bp->offset.HighPart, 
-                        bp->offset.LowPart, bp->dataVersion, bp->flags, 
+                        bp->offset.LowPart, bp->dataVersion, bp->flags, bp->qFlags,
                         bp->cmFlags, bp->error, bp->refCount);
        WriteFile(outputFile, output, (DWORD)strlen(output), &zilch, NULL);
     }
@@ -1955,10 +1955,10 @@ int cm_DumpBufHashTable(FILE *outputFile, char *cookie, int lock)
        StringCbPrintfA(output, sizeof(output), 
                         "%s bp=0x%08X, fid (cell=%d, volume=%d, "
                         "vnode=%d, unique=%d), offset=%x:%08x, dv=%I64d, "
-                        "flags=0x%x, cmFlags=0x%x, error=0x%x, refCount=%d\r\n",
+                        "flags=0x%x, qFlags=0x%x, cmFlags=0x%x, error=0x%x, refCount=%d\r\n",
                         cookie, (void *)bp, bp->fid.cell, bp->fid.volume, 
                         bp->fid.vnode, bp->fid.unique, bp->offset.HighPart, 
-                        bp->offset.LowPart, bp->dataVersion, bp->flags, 
+                        bp->offset.LowPart, bp->dataVersion, bp->flags, bp->qFlags,
                         bp->cmFlags, bp->error, bp->refCount);
        WriteFile(outputFile, output, (DWORD)strlen(output), &zilch, NULL);
     }

diff --git a/src/WINNT/afsd/cm_buf.h b/src/WINNT/afsd/cm_buf.h
index 6ca613c..5f80ba6 100644 (file)
--- a/src/WINNT/afsd/cm_buf.h
+++ b/src/WINNT/afsd/cm_buf.h
@@ -62,7 +62,8 @@ typedef struct cm_buf {
     afs_uint32 dirtyCounter;   /* bumped at each dirty->clean transition */
     osi_hyper_t offset;                /* offset */
     cm_fid_t fid;              /* file ID */
-    afs_uint32 flags;          /* flags we're using */
+    afs_uint16 flags;          /* flags we're using - mx */
+    afs_uint16 qFlags;         /* queue/hash state flags - buf_globalLock */
     char *datap;               /* data in this buffer */
     afs_uint32 error;          /* last error code, if CM_BUF_ERROR is set */
     cm_user_t *userp;          /* user who wrote to the buffer last */
@@ -99,16 +100,19 @@ typedef struct cm_buf {
 /* waiting is done based on scp->flags.  Removing bits from cmFlags
    should be followed by waking the scp. */
 
+/* values for qFlags */
+#define CM_BUF_QINHASH 1       /* in the hash table */
+#define CM_BUF_QINLRU  2       /* in lru queue (aka free list) */
+#define CM_BUF_QINDL    4       /* in the dirty list */
+#define CM_BUF_QREDIR   8       /* buffer held by the redirector */
+
+/* values for flags */
 #define CM_BUF_READING 1       /* now reading buffer from the disk */
 #define CM_BUF_WRITING 2       /* now writing buffer to the disk */
-#define CM_BUF_INHASH  4       /* in the hash table */
 #define CM_BUF_DIRTY   8       /* buffer is dirty */
-#define CM_BUF_INLRU   0x10    /* in lru queue (aka free list) */
 #define CM_BUF_ERROR   0x20    /* something went wrong on delayed write */
 #define CM_BUF_WAITING 0x40    /* someone's waiting for a flag to change */
-#define CM_BUF_INDL     0x80    /* in the dirty list */
-#define CM_BUF_EOF     0x100   /* read 0 bytes; used for detecting EOF */
-#define CM_BUF_REDIR    0x200   /* buffer held by the redirector */
+#define CM_BUF_EOF     0x80    /* read 0 bytes; used for detecting EOF */
 
 typedef struct cm_buf_ops {
     long (*Writep)(void *, osi_hyper_t *, long, long, struct cm_user *,

diff --git a/src/WINNT/afsd/cm_scache.c b/src/WINNT/afsd/cm_scache.c
index 9c1dd73..261ad34 100644 (file)
--- a/src/WINNT/afsd/cm_scache.c
+++ b/src/WINNT/afsd/cm_scache.c
@@ -1738,7 +1738,7 @@ void cm_MergeStatus(cm_scache_t *dscp,
                     *lbpp = bp->hashp; /* hash out */
                     bp->hashp = NULL;
 
-                    bp->flags &= ~CM_BUF_INHASH;
+                    bp->qFlags &= ~CM_BUF_QINHASH;
                 }
                 lock_ReleaseMutex(&bp->mx);
             }