DEVEL15-windows-buf-refcount-leak-fix-20070203

Plug a buffer refcount leak.   This is why the client slows down over
time.  It runs out of buffers it can use.

(cherry picked from commit 9a9e0976c881ef6b77e7de19a79611569b1426a3)

diff --git a/src/WINNT/afsd/cm_buf.c b/src/WINNT/afsd/cm_buf.c
index 19a974162b9ed40463b353d6d18baa9c36a43500..4ad5b75897b76d34012c82df50117e27158b2da6 100644 (file)
--- a/src/WINNT/afsd/cm_buf.c
+++ b/src/WINNT/afsd/cm_buf.c
@@ -768,11 +768,12 @@ long buf_GetNewLocked(struct cm_scache *scp, osi_hyper_t *offsetp, cm_buf_t **bu
 
        /* does this fix the problem below?  it's a simple solution. */
        if (!cm_data.buf_freeListEndp)
-           {
+       {
            lock_ReleaseWrite(&buf_globalLock);
+           osi_Log0(afsd_logp, "buf_GetNewLocked: Free Buffer List is empty - sleeping 200ms");
            Sleep(200);
            goto retry;
-           }
+       }
 
         /* for debugging, assert free list isn't empty, although we
          * really should try waiting for a running tranasction to finish
@@ -874,17 +875,12 @@ long buf_GetNewLocked(struct cm_scache *scp, osi_hyper_t *offsetp, cm_buf_t **bu
                 cm_data.buf_fileHashTablepp[i] = bp;
             }
 
-            /* prepare to return it.  Start by giving it a good
-             * refcount */
-            bp->refCount = 1;
-                        
-            /* and since it has a non-zero ref count, we should move
-             * it from the lru queue.  It better be still there,
-             * since we've held the global (big) lock since we found
-             * it there.
+            /* 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,
                          "buf_GetNewLocked: LRU screwup");
+
             if (cm_data.buf_freeListEndp == bp) {
                 /* we're the last guy in this queue, so maintain it */
                 cm_data.buf_freeListEndp = (cm_buf_t *) osi_QPrev(&bp->q);
@@ -892,13 +888,19 @@ long buf_GetNewLocked(struct cm_scache *scp, osi_hyper_t *offsetp, cm_buf_t **bu
             osi_QRemove((osi_queue_t **) &cm_data.buf_freeListp, &bp->q);
             bp->flags &= ~CM_BUF_INLRU;
 
-            /* finally, grab the mutex so that people don't use it
+            /* grab the mutex so that people don't use it
              * before the caller fills it with data.  Again, no one    
              * should have been able to get to this dude to lock it.
              */
-            osi_assertx(lock_TryMutex(&bp->mx),
-                         "buf_GetNewLocked: TryMutex failed");
+           if (!lock_TryMutex(&bp->mx)) {
+               osi_Log2(afsd_logp, "buf_GetNewLocked bp 0x%p cannot be mutex locked.  refCount %d should be 0",
+                        bp, bp->refCount);
+               osi_panic("buf_GetNewLocked: TryMutex failed",__FILE__,__LINE__);
+           }
 
+           /* prepare to return it.  Give it a refcount */
+            bp->refCount = 1;
+                        
             lock_ReleaseWrite(&buf_globalLock);
             *bufpp = bp;
 
@@ -908,7 +910,8 @@ long buf_GetNewLocked(struct cm_scache *scp, osi_hyper_t *offsetp, cm_buf_t **bu
             return 0;
         } /* for all buffers in lru queue */
         lock_ReleaseWrite(&buf_globalLock);
-               Sleep(100);             /* give some time for a buffer to be freed */
+       osi_Log0(afsd_logp, "buf_GetNewLocked: Free Buffer List has no buffers with a zero refcount - sleeping 100ms");
+       Sleep(100);             /* give some time for a buffer to be freed */
     }  /* while loop over everything */
     /* not reached */
 } /* the proc */
@@ -1349,7 +1352,7 @@ long buf_Truncate(cm_scache_t *scp, cm_user_t *userp, cm_req_t *reqp,
 
     buf_HoldLocked(bufp);
     lock_ReleaseWrite(&buf_globalLock);
-    for(; bufp; bufp = nbufp) {
+    while (bufp) {
         lock_ObtainMutex(&bufp->mx);
 
         bufEnd.HighPart = 0;
@@ -1426,6 +1429,7 @@ long buf_Truncate(cm_scache_t *scp, cm_user_t *userp, cm_req_t *reqp,
        }
        buf_ReleaseLocked(bufp);
        lock_ReleaseWrite(&buf_globalLock);
+       bufp = nbufp;
     }
 
 #ifdef TESTING
@@ -1620,21 +1624,44 @@ int cm_DumpBufHashTable(FILE *outputFile, char *cookie, int lock)
     {
         for (bp = cm_data.buf_hashTablepp[i]; bp; bp=bp->hashp) 
         {
-            if (bp->refCount)
-            {
-                StringCbPrintfA(output, sizeof(output), 
-                               "%s bp=0x%08X, hash=%d, fid (cell=%d, volume=%d, "
-                               "vnode=%d, unique=%d), size=%d refCount=%d\r\n", 
-                        cookie, (void *)bp, i, bp->fid.cell, bp->fid.volume, 
-                        bp->fid.vnode, bp->fid.unique, bp->size, bp->refCount);
-                WriteFile(outputFile, output, (DWORD)strlen(output), &zilch, NULL);
-            }
+           StringCbPrintfA(output, sizeof(output), 
+                           "%s bp=0x%08X, hash=%d, fid (cell=%d, volume=%d, "
+                           "vnode=%d, unique=%d), size=%d flags=0x%x, refCount=%d\r\n", 
+                            cookie, (void *)bp, i, bp->fid.cell, bp->fid.volume, 
+                            bp->fid.vnode, bp->fid.unique, bp->size, bp->flags, bp->refCount);
+           WriteFile(outputFile, output, (DWORD)strlen(output), &zilch, NULL);
         }
     }
   
     StringCbPrintfA(output, sizeof(output), "%s - Done dumping buf_HashTable.\r\n", cookie);
     WriteFile(outputFile, output, (DWORD)strlen(output), &zilch, NULL);
 
+    StringCbPrintfA(output, sizeof(output), "%s - dumping buf_freeListEndp\r\n", cookie);
+    WriteFile(outputFile, output, (DWORD)strlen(output), &zilch, NULL);
+    for(bp = cm_data.buf_freeListEndp; bp; bp=(cm_buf_t *) osi_QPrev(&bp->q)) {
+       StringCbPrintfA(output, sizeof(output), 
+                        "%s bp=0x%08X, fid (cell=%d, volume=%d, "
+                        "vnode=%d, unique=%d), size=%d flags=0x%x, refCount=%d\r\n", 
+                        cookie, (void *)bp, bp->fid.cell, bp->fid.volume, 
+                        bp->fid.vnode, bp->fid.unique, bp->size, bp->flags, bp->refCount);
+       WriteFile(outputFile, output, (DWORD)strlen(output), &zilch, NULL);
+    }
+    StringCbPrintfA(output, sizeof(output), "%s - Done dumping buf_FreeListEndp.\r\n", cookie);
+    WriteFile(outputFile, output, (DWORD)strlen(output), &zilch, NULL);
+
+    StringCbPrintfA(output, sizeof(output), "%s - dumping buf_dirtyListEndp\r\n", cookie);
+    WriteFile(outputFile, output, (DWORD)strlen(output), &zilch, NULL);
+    for(bp = cm_data.buf_dirtyListEndp; bp; bp=(cm_buf_t *) osi_QPrev(&bp->q)) {
+       StringCbPrintfA(output, sizeof(output), 
+                        "%s bp=0x%08X, fid (cell=%d, volume=%d, "
+                        "vnode=%d, unique=%d), size=%d flags=0x%x, refCount=%d\r\n", 
+                        cookie, (void *)bp, bp->fid.cell, bp->fid.volume, 
+                        bp->fid.vnode, bp->fid.unique, bp->size, bp->flags, bp->refCount);
+       WriteFile(outputFile, output, (DWORD)strlen(output), &zilch, NULL);
+    }
+    StringCbPrintfA(output, sizeof(output), "%s - Done dumping buf_dirtyListEndp.\r\n", cookie);
+    WriteFile(outputFile, output, (DWORD)strlen(output), &zilch, NULL);
+
     if (lock)
         lock_ReleaseRead(&buf_globalLock);
     return 0;

diff --git a/src/WINNT/afsd/cm_dcache.c b/src/WINNT/afsd/cm_dcache.c
index 971c27e278044716d758b8f90d3c915492798b38..9c9a86a732bd9f13cf2d1576049c6920f11ac97e 100644 (file)
--- a/src/WINNT/afsd/cm_dcache.c
+++ b/src/WINNT/afsd/cm_dcache.c
@@ -646,7 +646,7 @@ void cm_BkgPrefetch(cm_scache_t *scp, afs_uint32 p1, afs_uint32 p2, afs_uint32 p
     long length;
     osi_hyper_t base;
     long code;
-    cm_buf_t *bp;
+    cm_buf_t *bp = NULL;
     int cpff = 0;                      /* cleared prefetch flag */
     cm_req_t req;
 
@@ -666,6 +666,8 @@ void cm_BkgPrefetch(cm_scache_t *scp, afs_uint32 p1, afs_uint32 p2, afs_uint32 p
     if (code || (bp->cmFlags & CM_BUF_CMFETCHING)) {
         scp->flags &= ~CM_SCACHEFLAG_PREFETCHING;
         lock_ReleaseMutex(&scp->mx);
+       if (bp)
+           buf_Release(bp);
         return;
     }
 

diff --git a/src/WINNT/afsd/cm_scache.c b/src/WINNT/afsd/cm_scache.c
index a53432993c4ea1765cec348367a567bb1fed23e8..67f2fed0326c182c302c454449d07e07c64f38a2 100644 (file)
--- a/src/WINNT/afsd/cm_scache.c
+++ b/src/WINNT/afsd/cm_scache.c
@@ -1212,7 +1212,9 @@ void cm_SyncOpDone(cm_scache_t *scp, cm_buf_t *bufp, afs_uint32 flags)
 
     /* now update the buffer pointer */
     if (flags & CM_SCACHESYNC_FETCHDATA) {
-        /* ensure that the buffer isn't already in the I/O list */
+       int release = 0;
+
+       /* ensure that the buffer isn't already in the I/O list */
         for(qdp = scp->bufReadsp; qdp; qdp = (osi_queueData_t *) osi_QNext(&qdp->q)) {
             tbufp = osi_GetQData(qdp);
             if (tbufp == bufp) 
@@ -1221,11 +1223,9 @@ void cm_SyncOpDone(cm_scache_t *scp, cm_buf_t *bufp, afs_uint32 flags)
        if (qdp) {
            osi_QRemove((osi_queue_t **) &scp->bufReadsp, &qdp->q);
            osi_QDFree(qdp);
+           release = 1;
        }
         if (bufp) {
-           int release = 0;
-           if (bufp->cmFlags & CM_BUF_CMFETCHING)
-               release = 1;
             bufp->cmFlags &= ~(CM_BUF_CMFETCHING | CM_BUF_CMFULLYFETCHED);
             if (bufp->flags & CM_BUF_WAITING) {
                 osi_Log2(afsd_logp, "CM SyncOpDone Waking [scp 0x%p] bufp 0x%p", scp, bufp);
@@ -1238,6 +1238,7 @@ void cm_SyncOpDone(cm_scache_t *scp, cm_buf_t *bufp, afs_uint32 flags)
 
     /* now update the buffer pointer */
     if (flags & CM_SCACHESYNC_STOREDATA) {
+       int release = 0;
         /* ensure that the buffer isn't already in the I/O list */
         for(qdp = scp->bufWritesp; qdp; qdp = (osi_queueData_t *) osi_QNext(&qdp->q)) {
             tbufp = osi_GetQData(qdp);
@@ -1247,11 +1248,9 @@ void cm_SyncOpDone(cm_scache_t *scp, cm_buf_t *bufp, afs_uint32 flags)
        if (qdp) {
            osi_QRemove((osi_queue_t **) &scp->bufWritesp, &qdp->q);
            osi_QDFree(qdp);
+           release = 1;
        }
         if (bufp) {
-           int release = 0;
-           if (bufp->cmFlags & CM_BUF_CMSTORING)
-               release = 1;
             bufp->cmFlags &= ~CM_BUF_CMSTORING;
             if (bufp->flags & CM_BUF_WAITING) {
                 osi_Log2(afsd_logp, "CM SyncOpDone Waking [scp 0x%p] bufp 0x%p", scp, bufp);
@@ -1556,8 +1555,8 @@ int cm_DumpSCache(FILE *outputFile, char *cookie, int lock)
     {
         if (scp->refCount != 0)
         {
-            sprintf(output, "%s fid (cell=%d, volume=%d, vnode=%d, unique=%d) refCount=%u\r\n", 
-                    cookie, scp->fid.cell, scp->fid.volume, scp->fid.vnode, scp->fid.unique, 
+            sprintf(output, "%s scp=0x%p, fid (cell=%d, volume=%d, vnode=%d, unique=%d) refCount=%u\r\n", 
+                    cookie, scp, scp->fid.cell, scp->fid.volume, scp->fid.vnode, scp->fid.unique, 
                     scp->refCount);
             WriteFile(outputFile, output, (DWORD)strlen(output), &zilch, NULL);
         }

diff --git a/src/WINNT/afsd/cm_vnodeops.c b/src/WINNT/afsd/cm_vnodeops.c
index 1f532985875137edc47e9c23a05d50571e216f38..a30cc5e595de06a0f6d412cde170ccef31b42f14 100644 (file)
--- a/src/WINNT/afsd/cm_vnodeops.c
+++ b/src/WINNT/afsd/cm_vnodeops.c
@@ -937,15 +937,15 @@ long cm_ReadMountPoint(cm_scache_t *scp, cm_user_t *userp, cm_req_t *reqp)
     lock_ReleaseRead(&scp->bufCreateLock);
 
     lock_ObtainMutex(&scp->mx);
-    if (code) {
+    if (code)
         return code;
-    }
+
     while (1) {
         code = cm_SyncOp(scp, bufp, userp, reqp, 0,
                           CM_SCACHESYNC_READ | CM_SCACHESYNC_NEEDCALLBACK);
-        if (code) {
+        if (code)
             goto done;
-        }
+
        cm_SyncOpDone(scp, bufp, CM_SCACHESYNC_NEEDCALLBACK | CM_SCACHESYNC_READ);
 
 
@@ -954,9 +954,8 @@ long cm_ReadMountPoint(cm_scache_t *scp, cm_user_t *userp, cm_req_t *reqp)
 
         /* otherwise load buffer */
         code = cm_GetBuffer(scp, bufp, NULL, userp, reqp);
-        if (code) {
+        if (code)
             goto done;
-        }
     }
     /* locked, has callback, has valid data in buffer */
     if ((tlen = scp->length.LowPart) > 1000)