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 0057e2020d95d33791db800fc637775d323592f6..0f00d67d72b68bc298c366513d68514422429d49 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 b630b616021f5acb49c2d7dffce08ebab8e1e9c2..71cfc645c944b0e77bfa4af9d1193f79b710a89d 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 e2aed5e10caf0ea68bb978e170cd593aed020017..723dcf72aa9f4b72bcea127049d39fe8655513f4 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