Windows: Correct cm_volume locking
authorJeffrey Altman <jaltman@your-file-system.com>
Thu, 27 Jan 2011 01:10:57 +0000 (20:10 -0500)
committerJeffrey Altman <jaltman@openafs.org>
Thu, 27 Jan 2011 03:04:34 +0000 (19:04 -0800)
cm_volume_t flags was used for two categories of flags.  The first
protected by the cm_volume_t->rw lock.  The second protected by
the global cm_volumeLock.  Separate the flags field into two
afs_uint16 fields and break the flag space into FLAG and QFLAG.

Add assertions to the volume LRU functions to ensure that they
are always called with cm_volumeLock write-locked.

Correct two locations where cm_AdjustVolumeLRU() was called
read-locked instead of write-locked.

Change-Id: I7991b995a3c981cd5d08d9cbba09badc81518a5a
Reviewed-on: http://gerrit.openafs.org/3760
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Jeffrey Altman <jaltman@openafs.org>
Tested-by: Jeffrey Altman <jaltman@openafs.org>

src/WINNT/afsd/cm_memmap.h
src/WINNT/afsd/cm_volume.c
src/WINNT/afsd/cm_volume.h

index 1b1dc6d..7df0341 100644 (file)
@@ -10,7 +10,7 @@
 #ifndef CM_MEMMAP_H
 #define CM_MEMMAP_H 1
 
-#define CM_CONFIG_DATA_VERSION  8
+#define CM_CONFIG_DATA_VERSION  14
 #define CM_CONFIG_DATA_MAGIC            ('A' | 'F'<<8 | 'S'<<16 | CM_CONFIG_DATA_VERSION<<24)
 
 typedef struct cm_config_data {
index 85c7947..6d627d7 100644 (file)
@@ -461,7 +461,7 @@ long cm_UpdateVolumeLocation(struct cm_cell *cellp, cm_user_t *userp, cm_req_t *
             osi_Log2(afsd_logp, "cm_UpdateVolume name %s -> %s", 
                      osi_LogSaveString(afsd_logp,volp->namep), osi_LogSaveString(afsd_logp,name));
 
-            if (volp->flags & CM_VOLUMEFLAG_IN_HASH)
+            if (volp->qflags & CM_VOLUME_QFLAG_IN_HASH)
                 cm_RemoveVolumeFromNameHashTable(volp);
 
             strcpy(volp->namep, name);
@@ -477,37 +477,37 @@ long cm_UpdateVolumeLocation(struct cm_cell *cellp, cm_user_t *userp, cm_req_t *
 
         if (flags & VLF_RWEXISTS) {
             if (volp->vol[RWVOL].ID != rwID) {
-                if (volp->vol[RWVOL].flags & CM_VOLUMEFLAG_IN_HASH)
+                if (volp->vol[RWVOL].qflags & CM_VOLUME_QFLAG_IN_HASH)
                     cm_RemoveVolumeFromIDHashTable(volp, RWVOL);
                 volp->vol[RWVOL].ID = rwID;
                 cm_AddVolumeToIDHashTable(volp, RWVOL);
             }
         } else {
-            if (volp->vol[RWVOL].flags & CM_VOLUMEFLAG_IN_HASH)
+            if (volp->vol[RWVOL].qflags & CM_VOLUME_QFLAG_IN_HASH)
                 cm_RemoveVolumeFromIDHashTable(volp, RWVOL);
             volp->vol[RWVOL].ID = 0;
         }
         if (flags & VLF_ROEXISTS) {
             if (volp->vol[ROVOL].ID != roID) {
-                if (volp->vol[ROVOL].flags & CM_VOLUMEFLAG_IN_HASH)
+                if (volp->vol[ROVOL].qflags & CM_VOLUME_QFLAG_IN_HASH)
                     cm_RemoveVolumeFromIDHashTable(volp, ROVOL);
                 volp->vol[ROVOL].ID = roID;
                 cm_AddVolumeToIDHashTable(volp, ROVOL);
             }
         } else {
-            if (volp->vol[ROVOL].flags & CM_VOLUMEFLAG_IN_HASH)
+            if (volp->vol[ROVOL].qflags & CM_VOLUME_QFLAG_IN_HASH)
                 cm_RemoveVolumeFromIDHashTable(volp, ROVOL);
             volp->vol[ROVOL].ID = 0;
         }
         if (flags & VLF_BACKEXISTS) {
             if (volp->vol[BACKVOL].ID != bkID) {
-                if (volp->vol[BACKVOL].flags & CM_VOLUMEFLAG_IN_HASH)
+                if (volp->vol[BACKVOL].qflags & CM_VOLUME_QFLAG_IN_HASH)
                     cm_RemoveVolumeFromIDHashTable(volp, BACKVOL);
                 volp->vol[BACKVOL].ID = bkID;
                 cm_AddVolumeToIDHashTable(volp, BACKVOL);
             }
         } else {
-            if (volp->vol[BACKVOL].flags & CM_VOLUMEFLAG_IN_HASH)
+            if (volp->vol[BACKVOL].qflags & CM_VOLUME_QFLAG_IN_HASH)
                 cm_RemoveVolumeFromIDHashTable(volp, BACKVOL);
             volp->vol[BACKVOL].ID = 0;
         }
@@ -908,13 +908,13 @@ long cm_FindVolumeByName(struct cm_cell *cellp, char *volumeNamep,
              * same object.  This volp must be re-inserted back into
              * the LRU queue before this function exits.
              */
-            if (volp->flags & CM_VOLUMEFLAG_IN_LRU_QUEUE)
+            if (volp->qflags & CM_VOLUME_QFLAG_IN_LRU_QUEUE)
                 cm_RemoveVolumeFromLRU(volp);
-            if (volp->flags & CM_VOLUMEFLAG_IN_HASH)
+            if (volp->qflags & CM_VOLUME_QFLAG_IN_HASH)
                 cm_RemoveVolumeFromNameHashTable(volp);
 
             for ( volType = RWVOL; volType < NUM_VOL_TYPES; volType++) {
-                if (volp->vol[volType].flags & CM_VOLUMEFLAG_IN_HASH)
+                if (volp->vol[volType].qflags & CM_VOLUME_QFLAG_IN_HASH)
                     cm_RemoveVolumeFromIDHashTable(volp, volType);
                 if (volp->vol[volType].ID)
                     cm_VolumeStatusNotification(volp, volp->vol[volType].ID, volp->vol[volType].state, vl_unknown);
@@ -976,22 +976,22 @@ long cm_FindVolumeByName(struct cm_cell *cellp, char *volumeNamep,
     if (code == 0) {
         *outVolpp = volp;
                
-        lock_ObtainRead(&cm_volumeLock);
-        if (!(volp->flags & CM_VOLUMEFLAG_IN_LRU_QUEUE) ||
+        lock_ObtainWrite(&cm_volumeLock);
+        if (!(volp->qflags & CM_VOLUME_QFLAG_IN_LRU_QUEUE) ||
              (flags & CM_GETVOL_FLAG_NO_LRU_UPDATE))
             cm_AdjustVolumeLRU(volp);
-        lock_ReleaseRead(&cm_volumeLock);
+        lock_ReleaseWrite(&cm_volumeLock);
     } else {
         /*
          * do not return it to the caller but do insert it in the LRU
          * otherwise it will be lost
          */
-        lock_ObtainRead(&cm_volumeLock);
-        if (!(volp->flags & CM_VOLUMEFLAG_IN_LRU_QUEUE) ||
+        lock_ObtainWrite(&cm_volumeLock);
+        if (!(volp->qflags & CM_VOLUME_QFLAG_IN_LRU_QUEUE) ||
              (flags & CM_GETVOL_FLAG_NO_LRU_UPDATE))
             cm_AdjustVolumeLRU(volp);
         cm_PutVolume(volp);
-        lock_ReleaseRead(&cm_volumeLock);
+        lock_ReleaseWrite(&cm_volumeLock);
     }
 
     if (code == CM_ERROR_NOSUCHVOLUME && cellp->linkedName[0] && 
@@ -1327,7 +1327,7 @@ void cm_CheckOfflineVolumes(void)
     for (volp = cm_data.volumeLRULastp; 
          volp && !daemon_ShutdownFlag && !powerStateSuspended; 
          volp=(cm_volume_t *) osi_QPrev(&volp->q)) {
-        if (volp->flags & CM_VOLUMEFLAG_IN_HASH) {
+        if (volp->qflags & CM_VOLUME_QFLAG_IN_HASH) {
             InterlockedIncrement(&volp->refCount);
             lock_ReleaseRead(&cm_volumeLock);
             cm_CheckOfflineVolume(volp, 0);
@@ -1529,10 +1529,10 @@ int cm_DumpVolumes(FILE *outputFile, char *cookie, int lock)
         }
 
         sprintf(output,
-                "%s - volp=0x%p cell=%s name=%s rwID=%u roID=%u bkID=%u flags=0x%x "
+                "%s - volp=0x%p cell=%s name=%s rwID=%u roID=%u bkID=%u flags=0x%x:%x "
                 "cbServerpRO='%s' cbExpiresRO='%s' creationDateRO='%s' refCount=%u\r\n",
                  cookie, volp, volp->cellp->name, volp->namep, volp->vol[RWVOL].ID,
-                 volp->vol[ROVOL].ID, volp->vol[BACKVOL].ID, volp->flags,
+                 volp->vol[ROVOL].ID, volp->vol[BACKVOL].ID, volp->flags, volp->qflags,
                  srvStr ? srvStr : "<none>", cbt ? cbt : "<none>", cdrot ? cdrot : "<none>",
                  volp->refCount);
         WriteFile(outputFile, output, (DWORD)strlen(output), &zilch, NULL);
@@ -1585,14 +1585,14 @@ void cm_AddVolumeToNameHashTable(cm_volume_t *volp)
 {
     int i;
     
-    if (volp->flags & CM_VOLUMEFLAG_IN_HASH)
+    if (volp->qflags & CM_VOLUME_QFLAG_IN_HASH)
         return;
 
     i = CM_VOLUME_NAME_HASH(volp->namep);
 
     volp->nameNextp = cm_data.volumeNameHashTablep[i];
     cm_data.volumeNameHashTablep[i] = volp;
-    volp->flags |= CM_VOLUMEFLAG_IN_HASH;
+    volp->qflags |= CM_VOLUME_QFLAG_IN_HASH;
 }
 
 /* call with volume write-locked and mutex held */
@@ -1602,7 +1602,7 @@ void cm_RemoveVolumeFromNameHashTable(cm_volume_t *volp)
     cm_volume_t *tvolp;
     int i;
        
-    if (volp->flags & CM_VOLUMEFLAG_IN_HASH) {
+    if (volp->qflags & CM_VOLUME_QFLAG_IN_HASH) {
        /* hash it out first */
        i = CM_VOLUME_NAME_HASH(volp->namep);
        for (lvolpp = &cm_data.volumeNameHashTablep[i], tvolp = cm_data.volumeNameHashTablep[i];
@@ -1610,7 +1610,7 @@ void cm_RemoveVolumeFromNameHashTable(cm_volume_t *volp)
             lvolpp = &tvolp->nameNextp, tvolp = tvolp->nameNextp) {
            if (tvolp == volp) {
                *lvolpp = volp->nameNextp;
-               volp->flags &= ~CM_VOLUMEFLAG_IN_HASH;
+               volp->qflags &= ~CM_VOLUME_QFLAG_IN_HASH;
                 volp->nameNextp = NULL;
                break;
            }
@@ -1626,7 +1626,7 @@ void cm_AddVolumeToIDHashTable(cm_volume_t *volp, afs_uint32 volType)
 
     statep = cm_VolumeStateByType(volp, volType);
 
-    if (statep->flags & CM_VOLUMEFLAG_IN_HASH)
+    if (statep->qflags & CM_VOLUME_QFLAG_IN_HASH)
         return;
 
     i = CM_VOLUME_ID_HASH(statep->ID);
@@ -1645,7 +1645,7 @@ void cm_AddVolumeToIDHashTable(cm_volume_t *volp, afs_uint32 volType)
         cm_data.volumeBKIDHashTablep[i] = volp;
         break;
     }
-    statep->flags |= CM_VOLUMEFLAG_IN_HASH;
+    statep->qflags |= CM_VOLUME_QFLAG_IN_HASH;
 }
 
 
@@ -1659,7 +1659,7 @@ void cm_RemoveVolumeFromIDHashTable(cm_volume_t *volp, afs_uint32 volType)
        
     statep = cm_VolumeStateByType(volp, volType);
 
-    if (statep->flags & CM_VOLUMEFLAG_IN_HASH) {
+    if (statep->qflags & CM_VOLUME_QFLAG_IN_HASH) {
        /* hash it out first */
         i = CM_VOLUME_ID_HASH(statep->ID);
 
@@ -1682,7 +1682,7 @@ void cm_RemoveVolumeFromIDHashTable(cm_volume_t *volp, afs_uint32 volType)
        do {
            if (tvolp == volp) {
                *lvolpp = statep->nextp;
-                statep->flags &= ~CM_VOLUMEFLAG_IN_HASH;
+                statep->qflags &= ~CM_VOLUME_QFLAG_IN_HASH;
                 statep->nextp = NULL;
                break;
            }
@@ -1696,34 +1696,46 @@ void cm_RemoveVolumeFromIDHashTable(cm_volume_t *volp, afs_uint32 volType)
 /* must be called with cm_volumeLock write-locked! */
 void cm_AdjustVolumeLRU(cm_volume_t *volp)
 {
+    lock_AssertWrite(&cm_volumeLock);
+
     if (volp == cm_data.volumeLRUFirstp)
         return;
 
-    if (volp->flags & CM_VOLUMEFLAG_IN_LRU_QUEUE)
+    if (volp->qflags & CM_VOLUME_QFLAG_IN_LRU_QUEUE)
         osi_QRemoveHT((osi_queue_t **) &cm_data.volumeLRUFirstp, (osi_queue_t **) &cm_data.volumeLRULastp, &volp->q);
     osi_QAddH((osi_queue_t **) &cm_data.volumeLRUFirstp, (osi_queue_t **) &cm_data.volumeLRULastp, &volp->q);
-    volp->flags |= CM_VOLUMEFLAG_IN_LRU_QUEUE;
+    volp->qflags |= CM_VOLUME_QFLAG_IN_LRU_QUEUE;
+
+    osi_assertx(cm_data.volumeLRULastp != NULL, "null cm_data.volumeLRULastp");
 }
 
 /* must be called with cm_volumeLock write-locked! */
 void cm_MoveVolumeToLRULast(cm_volume_t *volp)
 {
+    lock_AssertWrite(&cm_volumeLock);
+
     if (volp == cm_data.volumeLRULastp)
         return;
 
-    if (volp->flags & CM_VOLUMEFLAG_IN_LRU_QUEUE)
+    if (volp->qflags & CM_VOLUME_QFLAG_IN_LRU_QUEUE)
         osi_QRemoveHT((osi_queue_t **) &cm_data.volumeLRUFirstp, (osi_queue_t **) &cm_data.volumeLRULastp, &volp->q);
     osi_QAddT((osi_queue_t **) &cm_data.volumeLRUFirstp, (osi_queue_t **) &cm_data.volumeLRULastp, &volp->q);
-    volp->flags |= CM_VOLUMEFLAG_IN_LRU_QUEUE;
+    volp->qflags |= CM_VOLUME_QFLAG_IN_LRU_QUEUE;
+
+    osi_assertx(cm_data.volumeLRULastp != NULL, "null cm_data.volumeLRULastp");
 }
 
 /* must be called with cm_volumeLock write-locked! */
 void cm_RemoveVolumeFromLRU(cm_volume_t *volp)
 {
-    if (volp->flags & CM_VOLUMEFLAG_IN_LRU_QUEUE) {
+    lock_AssertWrite(&cm_volumeLock);
+
+    if (volp->qflags & CM_VOLUME_QFLAG_IN_LRU_QUEUE) {
         osi_QRemoveHT((osi_queue_t **) &cm_data.volumeLRUFirstp, (osi_queue_t **) &cm_data.volumeLRULastp, &volp->q);
-        volp->flags &= ~CM_VOLUMEFLAG_IN_LRU_QUEUE;
+        volp->qflags &= ~CM_VOLUME_QFLAG_IN_LRU_QUEUE;
     }
+
+    osi_assertx(cm_data.volumeLRULastp != NULL, "null cm_data.volumeLRULastp");
 }
 
 static char * volstatus_str(enum volstatus vs)
index 7be726a..9d21ac9 100644 (file)
@@ -24,7 +24,8 @@ typedef struct cm_vol_state {
     struct cm_fid dotdotFid;           /* parent of volume root */
     cm_serverRef_t *serversp;           /* by cm_serverLock */
     enum volstatus  state;              /* by rw */
-    afs_uint32      flags;              /* by rw */
+    afs_uint16      flags;              /* by rw */
+    afs_uint16      qflags;             /* by cm_volumeLock */
 } cm_vol_state_t;
 
 /* RWVOL, ROVOL, BACKVOL are defined in cm.h */
@@ -40,7 +41,8 @@ typedef struct cm_volume {
                                         /* by cm_volumeLock */
     struct cm_vol_state vol[NUM_VOL_TYPES]; /* by cm_volumeLock */
     osi_rwlock_t rw;
-    afs_uint32 flags;                  /* by rw */
+    afs_uint16 flags;                  /* by rw */
+    afs_uint16 qflags;                  /* by cm_volumeLock */
     afs_int32 refCount;                        /* by Interlocked operations */
     struct cm_server *cbServerpRO;      /* server granting RO callback; by cm_scacheLock */
     time_t cbExpiresRO;                 /* latest RO expiration time; by cm_scacheLock */
@@ -49,12 +51,13 @@ typedef struct cm_volume {
 } cm_volume_t;
 
 #define CM_VOLUMEFLAG_RESET       1    /* reload this info on next use */
-#define CM_VOLUMEFLAG_IN_HASH      2
-#define CM_VOLUMEFLAG_IN_LRU_QUEUE 4
 #define CM_VOLUMEFLAG_UPDATING_VL  8
 #define CM_VOLUMEFLAG_DFS_VOLUME  16
 #define CM_VOLUMEFLAG_NOEXIST     32
 
+#define CM_VOLUME_QFLAG_IN_HASH      1
+#define CM_VOLUME_QFLAG_IN_LRU_QUEUE 2
+
 typedef struct cm_volumeRef {
     struct cm_volumeRef * next;
     afs_uint32  volID;