Windows: Fix cm_serverRef ref counts
authorJeffrey Altman <jaltman@your-file-system.com>
Fri, 12 Aug 2011 23:02:48 +0000 (19:02 -0400)
committerDerrick Brashear <shadow@dementix.org>
Sat, 13 Aug 2011 13:09:26 +0000 (06:09 -0700)
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.

Change-Id: Ie65cdca4461e84c675e8a29e22cef3e15679fda7
Reviewed-on: http://gerrit.openafs.org/5248
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Derrick Brashear <shadow@dementix.org>

src/WINNT/afsd/cm_cell.c
src/WINNT/afsd/cm_server.c
src/WINNT/afsd/cm_server.h
src/WINNT/afsd/cm_volume.c

index 855d0f9..b6aa482 100644 (file)
@@ -65,10 +65,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;
 }
index 1a3a02f..963c05d 100644 (file)
@@ -950,6 +950,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;
@@ -1010,6 +1014,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;
@@ -1035,7 +1067,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)
 {
@@ -1046,8 +1078,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) )
     {
@@ -1065,6 +1095,7 @@ void cm_InsertServerList(cm_serverRef_t** list, cm_serverRef_t* element)
     }
     element->next = current->next;
     current->next = element;
+
     lock_ReleaseWrite(&cm_serverLock);
 }
 /*
@@ -1093,7 +1124,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);
@@ -1107,10 +1138,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;
 }
 /*
@@ -1255,6 +1282,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;
@@ -1267,7 +1295,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)
index 74cf91d..2aa03d8 100644 (file)
@@ -85,6 +85,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 *);
index c34d5df..1c1e32a 100644 (file)
@@ -524,6 +524,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
@@ -544,6 +545,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
@@ -580,20 +582,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))
@@ -607,9 +601,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;
@@ -1131,7 +1122,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);
 
@@ -1229,6 +1220,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;
@@ -1243,6 +1235,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);
@@ -1283,6 +1276,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;
         }