Windows: Fix cm_serverRef ref counts

Use Interlocked operations consistently

Simplify cm_ServerInsertList().  It no longer increments the
refCount on the serverRef object.  Instead it leaves the refCount
as is.  Its the caller's responsibility to add a reference if
required.

Add reference counts and hold locks in places where the
volume server list was used unprotected.

Reviewed-on: http://gerrit.openafs.org/5248
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Derrick Brashear <shadow@dementix.org>
(cherry picked from commit 8f446c7463c9183d59a30343682e31ad9f85b307)

Change-Id: I0ed8ea1551527e0d62e57967da6816415a3b36b5
Reviewed-on: http://gerrit.openafs.org/5254
Reviewed-by: Derrick Brashear <shadow@dementix.org>
Tested-by: Derrick Brashear <shadow@dementix.org>

diff --git a/src/WINNT/afsd/cm_cell.c b/src/WINNT/afsd/cm_cell.c
index ff82e7961f2edd0f11e6a3f4d4e4a4e89a1d87dd..5a6c134d76e87afda6806497d5de7c6755249d5d 100644 (file)
--- a/src/WINNT/afsd/cm_cell.c
+++ b/src/WINNT/afsd/cm_cell.c
@@ -62,10 +62,6 @@ long cm_AddCellProc(void *rockp, struct sockaddr_in *addrp, char *hostnamep, uns
     /* Insert the vlserver into a sorted list, sorted by server rank */
     tsrp = cm_NewServerRef(tsp, 0);
     cm_InsertServerList(&cellp->vlServersp, tsrp);
-    /* drop the allocation reference */
-    lock_ObtainWrite(&cm_serverLock);
-    tsrp->refCount--;
-    lock_ReleaseWrite(&cm_serverLock);
 
     return 0;
 }

diff --git a/src/WINNT/afsd/cm_server.c b/src/WINNT/afsd/cm_server.c
index be8ff4cc3841f1918aef2ac65740550471eda698..9d3f4acef17a0f5f66da4fbb19dfb13cd9987c38 100644 (file)
--- a/src/WINNT/afsd/cm_server.c
+++ b/src/WINNT/afsd/cm_server.c
@@ -947,6 +947,10 @@ cm_server_vols_t *cm_NewServerVols(void) {
     return tsvp;
 }
 
+/*
+ * cm_NewServerRef() returns with the allocated cm_serverRef_t
+ * with a refCount of 1.
+ */
 cm_serverRef_t *cm_NewServerRef(cm_server_t *serverp, afs_uint32 volID)
 {
     cm_serverRef_t *tsrp;
@@ -1007,6 +1011,34 @@ cm_serverRef_t *cm_NewServerRef(cm_server_t *serverp, afs_uint32 volID)
     return tsrp;
 }
 
+void cm_GetServerRef(cm_serverRef_t *tsrp, int locked)
+{
+    afs_int32 refCount;
+
+    if (!locked)
+        lock_ObtainRead(&cm_serverLock);
+    refCount = InterlockedIncrement(&tsrp->refCount);
+    if (!locked)
+        lock_ReleaseRead(&cm_serverLock);
+}
+
+afs_int32 cm_PutServerRef(cm_serverRef_t *tsrp, int locked)
+{
+    afs_int32 refCount;
+
+    if (!locked)
+        lock_ObtainRead(&cm_serverLock);
+    refCount = InterlockedDecrement(&tsrp->refCount);
+    osi_assertx(refCount >= 0, "cm_serverRef_t refCount underflow");
+
+    if (!locked)
+        lock_ReleaseRead(&cm_serverLock);
+
+    return refCount;
+}
+
+
+
 LONG_PTR cm_ChecksumServerList(cm_serverRef_t *serversp)
 {
     LONG_PTR sum = 0;
@@ -1032,7 +1064,7 @@ LONG_PTR cm_ChecksumServerList(cm_serverRef_t *serversp)
 ** Insert a server into the server list keeping the list sorted in
 ** ascending order of ipRank.
 **
-** The refCount of the cm_serverRef_t is increased
+** The refCount of the cm_serverRef_t is not altered.
 */
 void cm_InsertServerList(cm_serverRef_t** list, cm_serverRef_t* element)
 {
@@ -1043,8 +1075,6 @@ void cm_InsertServerList(cm_serverRef_t** list, cm_serverRef_t* element)
     current=*list;
     ipRank = element->server->ipRank;
 
-    element->refCount++;                /* increase refCount */
-
     /* insertion into empty list  or at the beginning of the list */
     if ( !current || (current->server->ipRank > ipRank) )
     {
@@ -1062,6 +1092,7 @@ void cm_InsertServerList(cm_serverRef_t** list, cm_serverRef_t* element)
     }
     element->next = current->next;
     current->next = element;
+
     lock_ReleaseWrite(&cm_serverLock);
 }
 /*
@@ -1090,7 +1121,7 @@ long cm_ChangeRankServer(cm_serverRef_t** list, cm_server_t*      server)
         if ( (*current)->server == server)
         {
             element = (*current);
-            *current = (*current)->next; /* delete it */
+            *current = element->next; /* delete it */
             break;
         }
         current = & ( (*current)->next);
@@ -1104,10 +1135,6 @@ long cm_ChangeRankServer(cm_serverRef_t** list, cm_server_t*     server)
     /* re-insert deleted element into the list with modified rank*/
     cm_InsertServerList(list, element);
 
-    /* reduce refCount which was increased by cm_InsertServerList */
-    lock_ObtainWrite(&cm_serverLock);
-    element->refCount--;
-    lock_ReleaseWrite(&cm_serverLock);
     return 0;
 }
 /*
@@ -1252,6 +1279,7 @@ void cm_FreeServerList(cm_serverRef_t** list, afs_uint32 flags)
     cm_serverRef_t  **current;
     cm_serverRef_t  **nextp;
     cm_serverRef_t  * next;
+    afs_int32         refCount;
 
     lock_ObtainWrite(&cm_serverLock);
     current = list;
@@ -1264,7 +1292,8 @@ void cm_FreeServerList(cm_serverRef_t** list, afs_uint32 flags)
     while (*current)
     {
         nextp = &(*current)->next;
-        if (--((*current)->refCount) == 0) {
+        refCount = cm_PutServerRef(*current, TRUE);
+        if (refCount == 0) {
             next = *nextp;
 
             if ((*current)->volID)

diff --git a/src/WINNT/afsd/cm_server.h b/src/WINNT/afsd/cm_server.h
index 2bfbcf0721ea84240414be8bb2834353c7ccec8f..665e3207de27414920ecb3e4b0ad5a0135af053d 100644 (file)
--- a/src/WINNT/afsd/cm_server.h
+++ b/src/WINNT/afsd/cm_server.h
@@ -86,6 +86,10 @@ extern cm_server_t *cm_NewServer(struct sockaddr_in *addrp, int type,
 
 extern cm_serverRef_t *cm_NewServerRef(struct cm_server *serverp, afs_uint32 volID);
 
+extern void cm_GetServerRef(cm_serverRef_t *tsrp, int locked);
+
+extern afs_int32 cm_PutServerRef(cm_serverRef_t *tsrp, int locked);
+
 extern LONG_PTR cm_ChecksumServerList(cm_serverRef_t *serversp);
 
 extern void cm_GetServer(cm_server_t *);

diff --git a/src/WINNT/afsd/cm_volume.c b/src/WINNT/afsd/cm_volume.c
index b57426585412be052f64422d01690f33d6a0c8d1..cca42b151846d034c9cb7ae5d44475a344ca1f6c 100644 (file)
--- a/src/WINNT/afsd/cm_volume.c
+++ b/src/WINNT/afsd/cm_volume.c
@@ -521,6 +521,7 @@ long cm_UpdateVolumeLocation(struct cm_cell *cellp, cm_user_t *userp, cm_req_t *
             tempAddr = htonl(serverNumber[i]);
             tsockAddr.sin_addr.s_addr = tempAddr;
             tsp = cm_FindServer(&tsockAddr, CM_SERVER_FILE);
+#ifdef MULTIHOMED
             if (tsp && (method == 2) && (tsp->flags & CM_SERVERFLAG_UUID)) {
                 /*
                  * Check to see if the uuid of the server we know at this address
@@ -541,6 +542,7 @@ long cm_UpdateVolumeLocation(struct cm_cell *cellp, cm_user_t *userp, cm_req_t *
                               osi_LogSaveString(afsd_logp, hoststr));
                 }
             }
+#endif
             if (!tsp) {
                 /*
                  * cm_NewServer will probe the file server which in turn will
@@ -577,20 +579,12 @@ long cm_UpdateVolumeLocation(struct cm_cell *cellp, cm_user_t *userp, cm_req_t *
             if ((tflags & VLSF_RWVOL) && (flags & VLF_RWEXISTS)) {
                 tsrp = cm_NewServerRef(tsp, rwID);
                 cm_InsertServerList(&volp->vol[RWVOL].serversp, tsrp);
-
-                lock_ObtainWrite(&cm_serverLock);
-                tsrp->refCount--;       /* drop allocation reference */
-                lock_ReleaseWrite(&cm_serverLock);
-
                 if (!(tsp->flags & CM_SERVERFLAG_DOWN))
                     rwServers_alldown = 0;
             }
             if ((tflags & VLSF_ROVOL) && (flags & VLF_ROEXISTS)) {
                 tsrp = cm_NewServerRef(tsp, roID);
                 cm_InsertServerList(&volp->vol[ROVOL].serversp, tsrp);
-                lock_ObtainWrite(&cm_serverLock);
-                tsrp->refCount--;       /* drop allocation reference */
-                lock_ReleaseWrite(&cm_serverLock);
                 ROcount++;
 
                 if (!(tsp->flags & CM_SERVERFLAG_DOWN))
@@ -604,9 +598,6 @@ long cm_UpdateVolumeLocation(struct cm_cell *cellp, cm_user_t *userp, cm_req_t *
             if ((tflags & VLSF_RWVOL) && (flags & VLF_BACKEXISTS)) {
                 tsrp = cm_NewServerRef(tsp, bkID);
                 cm_InsertServerList(&volp->vol[BACKVOL].serversp, tsrp);
-                lock_ObtainWrite(&cm_serverLock);
-                tsrp->refCount--;       /* drop allocation reference */
-                lock_ReleaseWrite(&cm_serverLock);
 
                 if (!(tsp->flags & CM_SERVERFLAG_DOWN))
                     bkServers_alldown = 0;
@@ -1128,7 +1119,7 @@ cm_serverRef_t **cm_GetVolServers(cm_volume_t *volp, afs_uint32 volume, cm_user_
      * They will be freed by cm_FreeServerList when they get to zero
      */
     for (current = *serverspp; current; current = current->next)
-        current->refCount++;
+        cm_GetServerRef(current, TRUE);
 
     lock_ReleaseWrite(&cm_serverLock);
 
@@ -1226,6 +1217,7 @@ cm_CheckOfflineVolumeState(cm_volume_t *volp, cm_vol_state_t *statep, afs_uint32
             *volumeUpdatedp = 1;
         }
 
+        lock_ObtainRead(&cm_serverLock);
         if (statep->serversp) {
             alldown = 1;
             alldeleted = 1;
@@ -1240,6 +1232,7 @@ cm_CheckOfflineVolumeState(cm_volume_t *volp, cm_vol_state_t *statep, afs_uint32
                 if (serversp->status == srv_busy || serversp->status == srv_offline)
                     serversp->status = srv_not_busy;
             }
+            lock_ReleaseRead(&cm_serverLock);
 
             if (alldeleted && !(*volumeUpdatedp)) {
                 cm_InitReq(&req);
@@ -1280,6 +1273,7 @@ cm_CheckOfflineVolumeState(cm_volume_t *volp, cm_vol_state_t *statep, afs_uint32
                 statep->state = vl_alldown;
             }
         } else if (statep->state != vl_alldown) {
+            lock_ReleaseRead(&cm_serverLock);
             cm_VolumeStatusNotification(volp, statep->ID, statep->state, vl_alldown);
             statep->state = vl_alldown;
         }