fs-setserverprefs-bug-20040320

FIXES 3555

Rodney Dyer (UNCC) reported that the "fs setserverprefs" command if
issued multiple times would result in the AFSD server becoming unresponsive.
This appears to have been caused by improper use of writeLocks, a failure
to use writeLocks when necessary, and improper reference counting on the
cm_server_t data structures placed into the cm_allServersp list.

diff --git a/src/WINNT/afsd/cm_ioctl.c b/src/WINNT/afsd/cm_ioctl.c
index 0057e20..0f00d67 100644 (file)
--- a/src/WINNT/afsd/cm_ioctl.c
+++ b/src/WINNT/afsd/cm_ioctl.c
@@ -1060,7 +1060,8 @@ long cm_IoctlSetSPrefs(struct smb_ioctl *ioctlp, struct cm_user *userp)
        vlonly     = spin->flags;
        if ( vlonly )
                type = CM_SERVER_VLDB;
-       else    type = CM_SERVER_FILE;
+       else    
+        type = CM_SERVER_FILE;
 
        for ( i=0; i < noServers; i++) 
        {
@@ -1070,7 +1071,7 @@ long cm_IoctlSetSPrefs(struct smb_ioctl *ioctlp, struct cm_user *userp)
                tmp.sin_family = AF_INET;
 
                tsp = cm_FindServer(&tmp, type);
-               if ( tsp )              /* an existing server */
+               if ( tsp )              /* an existing server - ref count increased */
                {
                        tsp->ipRank = rank; /* no need to protect by mutex*/
 
@@ -1086,13 +1087,13 @@ long cm_IoctlSetSPrefs(struct smb_ioctl *ioctlp, struct cm_user *userp)
                            /* set preferences for an existing vlserver */
                            cm_ChangeRankCellVLServer(tsp);
                        }
+            cm_PutServer(tsp);  /* decrease refcount */
                }
-               else                    /* add a new server without a cell*/
+               else    /* add a new server without a cell */
                {
-                       tsp = cm_NewServer(&tmp, type, NULL);
+                       tsp = cm_NewServer(&tmp, type, NULL); /* refcount = 1 */
                        tsp->ipRank = rank;
                }
-               cm_PutServer(tsp);
        }
        return 0;
 }

diff --git a/src/WINNT/afsd/cm_server.c b/src/WINNT/afsd/cm_server.c
index b630b61..71cfc64 100644 (file)
--- a/src/WINNT/afsd/cm_server.c
+++ b/src/WINNT/afsd/cm_server.c
@@ -199,17 +199,21 @@ cm_server_t *cm_NewServer(struct sockaddr_in *socketp, int type, cm_cell_t *cell
        osi_assert(socketp->sin_family == AF_INET);
 
        tsp = malloc(sizeof(*tsp));
-        memset(tsp, 0, sizeof(*tsp));
+    memset(tsp, 0, sizeof(*tsp));
        tsp->type = type;
-        tsp->cellp = cellp;
-        tsp->refCount = 1;
-       tsp->allNextp = cm_allServersp;
-        cm_allServersp = tsp;
+    tsp->cellp = cellp;
+    tsp->refCount = 1;
        lock_InitializeMutex(&tsp->mx, "cm_server_t mutex");
        tsp->addr = *socketp;
 
        cm_SetServerPrefs(tsp); 
-        return tsp;
+
+    lock_ObtainWrite(&cm_serverLock); /* get server lock */
+       tsp->allNextp = cm_allServersp;
+    cm_allServersp = tsp;
+    lock_ReleaseWrite(&cm_serverLock); /* release server lock */
+
+    return tsp;
 }
 
 /* find a server based on its properties */
@@ -326,15 +330,14 @@ long cm_ChangeRankServer(cm_serverRef_t** list, cm_server_t*      server)
                }
                current = & ( (*current)->next);        
        }
-       /* if this volume is not replicated on this server  */
-       if (!element) {
-        lock_ReleaseWrite(&cm_serverLock);
+    lock_ReleaseWrite(&cm_serverLock);
+
+    /* if this volume is not replicated on this server  */
+       if (!element)
                return 1;       /* server is not on list */
-    }
 
        /* re-insert deleted element into the list with modified rank*/
        cm_InsertServerList(list, element);
-    lock_ReleaseWrite(&cm_serverLock);
        return 0;
 }
 /*

diff --git a/src/WINNT/afsd/cm_volume.c b/src/WINNT/afsd/cm_volume.c
index e2aed5e..723dcf7 100644 (file)
--- a/src/WINNT/afsd/cm_volume.c
+++ b/src/WINNT/afsd/cm_volume.c
@@ -43,14 +43,14 @@ void cm_InitVolume(void)
 long cm_UpdateVolume(struct cm_cell *cellp, cm_user_t *userp, cm_req_t *reqp,
        cm_volume_t *volp)
 {
-        cm_conn_t *connp;
-        int i;
+    cm_conn_t *connp;
+    int i;
        cm_serverRef_t *tsrp;
-        cm_server_t *tsp;
-        struct sockaddr_in tsockAddr;
-        long tflags;
-        u_long tempAddr;
-        struct vldbentry vldbEntry;    /* don't use NVLDB yet; they're not common */
+    cm_server_t *tsp;
+    struct sockaddr_in tsockAddr;
+    long tflags;
+    u_long tempAddr;
+    struct vldbentry vldbEntry;        /* don't use NVLDB yet; they're not common */
        int ROcount = 0;
        long code;
 
@@ -71,34 +71,34 @@ long cm_UpdateVolume(struct cm_cell *cellp, cm_user_t *userp, cm_req_t *reqp,
                free(tsrp);
        }
 
-        /* now we have volume structure locked and held; make RPC to fill it */
-        do {
+    /* now we have volume structure locked and held; make RPC to fill it */
+    do {
                code = cm_ConnByMServers(cellp->vlServersp, userp, reqp,
-                                        &connp);
-                if (code) continue;
+                                  &connp);
+        if (code) continue;
                osi_Log1(afsd_logp, "CALL VL_GetEntryByNameO name %s",
-                        volp->namep);
-                code = VL_GetEntryByNameO(connp->callp, volp->namep, &vldbEntry);
+                  volp->namep);
+        code = VL_GetEntryByNameO(connp->callp, volp->namep, &vldbEntry);
        } while (cm_Analyze(connp, userp, reqp, NULL, NULL, NULL, code));
-        code = cm_MapVLRPCError(code, reqp);
+    code = cm_MapVLRPCError(code, reqp);
 
-        if (code == 0) {
+    if (code == 0) {
                /* decode the response */
                lock_ObtainWrite(&cm_volumeLock);
-                if (vldbEntry.flags & VLF_RWEXISTS)
-                       volp->rwID = vldbEntry.volumeId[0];
+        if (vldbEntry.flags & VLF_RWEXISTS)
+            volp->rwID = vldbEntry.volumeId[0];
                else
-                       volp->rwID = 0;
-                if (vldbEntry.flags & VLF_ROEXISTS)
-                       volp->roID = vldbEntry.volumeId[1];
-                else
-                       volp->roID = 0;
-                if (vldbEntry.flags & VLF_BACKEXISTS)
-                       volp->bkID = vldbEntry.volumeId[2];
+            volp->rwID = 0;
+        if (vldbEntry.flags & VLF_ROEXISTS)
+            volp->roID = vldbEntry.volumeId[1];
+        else
+            volp->roID = 0;
+        if (vldbEntry.flags & VLF_BACKEXISTS)
+            volp->bkID = vldbEntry.volumeId[2];
                else
-                       volp->bkID = 0;
+            volp->bkID = 0;
                lock_ReleaseWrite(&cm_volumeLock);
-                for(i=0; i<vldbEntry.nServers; i++) {
+        for(i=0; i<vldbEntry.nServers; i++) {
                        /* create a server entry */
                        tflags = vldbEntry.serverFlags[i];
                        if (tflags & VLSF_DONTUSE) continue;
@@ -107,44 +107,44 @@ long cm_UpdateVolume(struct cm_cell *cellp, cm_user_t *userp, cm_req_t *reqp,
                        tsockAddr.sin_addr.s_addr = tempAddr;
                        tsp = cm_FindServer(&tsockAddr, CM_SERVER_FILE);
                        if (!tsp)
-                               tsp = cm_NewServer(&tsockAddr, CM_SERVER_FILE,
-                                                  cellp);
+                tsp = cm_NewServer(&tsockAddr, CM_SERVER_FILE,
+                                    cellp);
 
                        /* if this server was created by fs setserverprefs */
                        if ( !tsp->cellp ) 
                                tsp->cellp = cellp;
 
-                        osi_assert(tsp != NULL);
+            osi_assert(tsp != NULL);
                         
-                        /* and add it to the list(s). */
+            /* and add it to the list(s). */
                        /*
-                        * Each call to cm_NewServerRef() increments the
-                        * ref count of tsp.  These reference will be dropped,
+             * Each call to cm_NewServerRef() increments the
+             * ref count of tsp.  These reference will be dropped,
                         * if and when the volume is reset; see reset code
                         * earlier in this function.
                         */
                        if ((tflags & VLSF_RWVOL)
-                           && (vldbEntry.flags & VLF_RWEXISTS)) {
+                 && (vldbEntry.flags & VLF_RWEXISTS)) {
                                tsrp = cm_NewServerRef(tsp);
                                tsrp->next = volp->rwServersp;
-                               volp->rwServersp = tsrp;
+                volp->rwServersp = tsrp;
                        }
-                        if ((tflags & VLSF_ROVOL)
-                           && (vldbEntry.flags & VLF_ROEXISTS)) {
+            if ((tflags & VLSF_ROVOL)
+                 && (vldbEntry.flags & VLF_ROEXISTS)) {
                                tsrp = cm_NewServerRef(tsp);
                                cm_InsertServerList(&volp->roServersp, tsrp);
                                ROcount++;
-                        }
+            }
                        /* We don't use VLSF_BACKVOL !?! */
-                        if ((tflags & VLSF_RWVOL)
-                           && (vldbEntry.flags & VLF_BACKEXISTS)) {
+            if ((tflags & VLSF_RWVOL)
+                 && (vldbEntry.flags & VLF_BACKEXISTS)) {
                                tsrp = cm_NewServerRef(tsp);
-                               tsrp->next = volp->bkServersp;
-                                volp->bkServersp = tsrp;
+                tsrp->next = volp->bkServersp;
+                volp->bkServersp = tsrp;
                        }
                        /* Drop the reference obtained by cm_FindServer() */
                        cm_PutServer(tsp);
-                }
+        }
 
                /*
                 * Randomize RO list