volser: ensure GCTrans transaction walk remains valid

[openafs.git] / src / volser / voltrans.c

diff --git a/src/volser/voltrans.c b/src/volser/voltrans.c
index 6a0f365..7a7af3e 100644 (file)
--- a/src/volser/voltrans.c
+++ b/src/volser/voltrans.c
@@ -17,30 +17,19 @@
 #include <afsconfig.h>
 #include <afs/param.h>
 
+#include <roken.h>
 
-#ifdef AFS_NT40_ENV
-#include <afs/afsutil.h>
-#else
-#include <sys/time.h>
+#include <afs/opr.h>
+#ifdef AFS_PTHREAD_ENV
+# include <opr/lock.h>
 #endif
 
-#include <stdio.h>
-#include <sys/types.h>
-#include <errno.h>
-#include <string.h>
 #ifdef AFS_NT40_ENV
-#include <fcntl.h>
-#include <winsock2.h>
-#else
-#include <sys/file.h>
-#include <netinet/in.h>
-#include <unistd.h>
+#include <afs/afsutil.h>
 #endif
-#include <dirent.h>
-#include <sys/stat.h>
+
+#include <rx/rx_queue.h>
 #include <afs/afsint.h>
-#include <signal.h>
-#include <afs/afs_assert.h>
 #include <afs/prs_fs.h>
 #include <afs/nfs.h>
 #include <lwp.h>
@@ -65,14 +54,13 @@ static afs_int32 transCounter = 1;
 
 /* create a new transaction, returning ptr to same with high ref count */
 struct volser_trans *
-NewTrans(afs_uint32 avol, afs_int32 apart)
+NewTrans(VolumeId avol, afs_int32 apart)
 {
     /* set volid, next, partition */
     struct volser_trans *tt, *newtt;
     struct timeval tp;
-    struct timezone tzp;
 
-    newtt = (struct volser_trans *)malloc(sizeof(struct volser_trans));
+    newtt = calloc(1, sizeof(struct volser_trans));
     VTRANS_LOCK;
     /* don't allow the same volume to be attached twice */
     for (tt = allTrans; tt; tt = tt->next) {
@@ -83,13 +71,12 @@ NewTrans(afs_uint32 avol, afs_int32 apart)
        }
     }
     tt = newtt;
-    memset(tt, 0, sizeof(struct volser_trans));
     tt->volid = avol;
     tt->partition = apart;
     tt->refCount = 1;
     tt->rxCallPtr = (struct rx_call *)0;
     strcpy(tt->lastProcName, "");
-    gettimeofday(&tp, &tzp);
+    gettimeofday(&tp, NULL);
     tt->creationTime = tp.tv_sec;
     tt->time = FT_ApproxTime();
     tt->tid = transCounter++;
@@ -193,17 +180,14 @@ GCTrans(void)
 
     VTRANS_LOCK;
     for (tt = allTrans; tt; tt = nt) {
-       nt = tt->next;          /* remember in case we zap it */
        if (tt->time + OLDTRANSWARN < now) {
-           Log("trans %u on volume %u %s than %d seconds\n", tt->tid,
-               tt->volid,
+           Log("trans %u on volume %" AFS_VOLID_FMT " %s than %d seconds\n", tt->tid,
+               afs_printable_VolumeId_lu(tt->volid),
                ((tt->refCount > 0) ? "is older" : "has been idle for more"),
                (((now - tt->time) / GCWAKEUP) * GCWAKEUP));
        }
-       if (tt->refCount > 0)
-           continue;
-       if (tt->time + OLDTRANSTIME < now) {
-           Log("trans %u on volume %u has timed out\n", tt->tid, tt->volid);
+       if ((tt->refCount <= 0) && (tt->time + OLDTRANSTIME < now)) {
+           Log("trans %u on volume %" AFS_VOLID_FMT " has timed out\n", tt->tid, afs_printable_VolumeId_lu(tt->volid));
 
            tt->refCount++;     /* we're using it now */
 
@@ -221,12 +205,27 @@ GCTrans(void)
 
                VTRANS_UNLOCK;
                VPurgeVolume(&error, tt->volume);
+               /*
+                * While the lock was dropped, tt->next may have changed.
+                * Therefore, defer reading tt->next until _after_ we regain the lock.
+                */
                VTRANS_LOCK;
            }
-
-           DeleteTrans(tt, 0); /* drops refCount or deletes it */
+           nt = tt->next;
+           /*
+            * DeleteTrans() will decrement tt->refCount; if it falls to 0, it will
+            * also delete tt itself.  Therefore, we must read tt->next _before_
+            * calling DeleteTrans().
+            */
+           DeleteTrans(tt, 0);
            GCDeletes++;
+           continue;
        }
+       /*
+        * This path never dropped VTRANS_LOCK or modified the allTrans list.
+        * Therefore, no special care is required to determine the next trans in the chain.
+        */
+       nt = tt->next;
     }
     VTRANS_UNLOCK;
     return 0;