afs: Avoid creating unused conns 37/14637/2
authorAndrew Deason <adeason@sinenomine.net>
Mon, 7 Jun 2021 23:23:39 +0000 (18:23 -0500)
committerBenjamin Kaduk <kaduk@mit.edu>
Thu, 10 Jun 2021 16:17:56 +0000 (12:17 -0400)
In afs_LoopServers and PCallBackAddr, we loop over our known servers
to issue an RPC, but we skip over servers on various conditions. For
most of those, we skip over the server before creating the relevant
conn (via afs_ConnBySA), but not for the
SRVADDR_ISDOWN/afs_HaveCallBacksFrom check. If we skip over the server
for that reason, we'll create an afs_conn and rx_connection, and then
release our refs without using them.

This doesn't cause any big problems, but it does create extra
connections and peers that linger around for at least a couple of
hours (until they get cleaned up by afs_GCUserData).

It's very easy to avoid these extra useless conns; just perform the
SRVADDR_ISDOWN/afs_HaveCallBacksFrom check before we call
afs_ConnBYSA. This also makes the check a bit more consistent with the
other checks we do.

Change-Id: Ie49b87e5dd755f77364502528a02c14944e8c23d
Reviewed-on: https://gerrit.openafs.org/14637
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Benjamin Kaduk <kaduk@mit.edu>

src/afs/afs_pioctl.c
src/afs/afs_server.c

index 2634879..4a705cc 100644 (file)
@@ -5239,6 +5239,11 @@ DECL_PIOCTL(PCallBackAddr)
        if (!ts->cell)          /* not really an active server, anyway, it must */
            continue;           /* have just been added by setsprefs */
 
+       if ((sa->sa_flags & SRVADDR_ISDOWN) == 0 && !afs_HaveCallBacksFrom(ts)) {
+           /* Server is up, and we have no active callbacks from it. */
+           continue;
+       }
+
        /* get a connection, even if host is down; bumps conn ref count */
        tu = afs_GetUser(areq->uid, ts->cell->cellNum, SHARED_LOCK);
        tc = afs_ConnBySA(sa, ts->cell->fsport, ts->cell->cellNum, tu,
@@ -5247,18 +5252,17 @@ DECL_PIOCTL(PCallBackAddr)
        if (!tc)
            continue;
 
-       if ((sa->sa_flags & SRVADDR_ISDOWN) || afs_HaveCallBacksFrom(ts)) {
-           if (sa->sa_flags & SRVADDR_ISDOWN) {
-               rx_SetConnDeadTime(rxconn, 3);
-           }
+       if (sa->sa_flags & SRVADDR_ISDOWN) {
+           rx_SetConnDeadTime(rxconn, 3);
+       }
 #ifdef RX_ENABLE_LOCKS
-           AFS_GUNLOCK();
+       AFS_GUNLOCK();
 #endif /* RX_ENABLE_LOCKS */
-           code = RXAFS_CallBackRxConnAddr(rxconn, &addr);
+       code = RXAFS_CallBackRxConnAddr(rxconn, &addr);
 #ifdef RX_ENABLE_LOCKS
-           AFS_GLOCK();
+       AFS_GLOCK();
 #endif /* RX_ENABLE_LOCKS */
-       }
+
        afs_PutConn(tc, rxconn, SHARED_LOCK);   /* done with it now */
     }                          /* Outer loop over addrs */
     afs_osi_Free(addrs, srvAddrCount * sizeof(*addrs));
index 24d4cd3..d4893b1 100644 (file)
@@ -707,6 +707,11 @@ afs_LoopServers(int adown, struct cell *acellp, int vlalso,
        if (!ts->cell)          /* not really an active server, anyway, it must */
            continue;           /* have just been added by setsprefs */
 
+       if ((sa->sa_flags & SRVADDR_ISDOWN) == 0 && !afs_HaveCallBacksFrom(sa->server)) {
+           /* Server is up, and we have no active callbacks from it. */
+           continue;
+       }
+
        /* get a connection, even if host is down; bumps conn ref count */
        tu = afs_GetUser(treq->uid, ts->cell->cellNum, SHARED_LOCK);
        tc = afs_ConnBySA(sa, ts->cell->fsport, ts->cell->cellNum, tu,
@@ -716,18 +721,16 @@ afs_LoopServers(int adown, struct cell *acellp, int vlalso,
        if (!tc)
            continue;
 
-       if ((sa->sa_flags & SRVADDR_ISDOWN) || afs_HaveCallBacksFrom(sa->server)) {
-           conns[nconns]=tc;
-           rxconns[nconns]=rxconn;
-           if (sa->sa_flags & SRVADDR_ISDOWN) {
-               rx_SetConnDeadTime(rxconn, 3);
-               conntimer[nconns]=1;
-           } else {
-               conntimer[nconns]=0;
-           }
-           nconns++;
-       } else /* not holding, kill ref */
-           afs_PutConn(tc, rxconn, SHARED_LOCK);
+       conns[nconns]=tc;
+       rxconns[nconns]=rxconn;
+       if (sa->sa_flags & SRVADDR_ISDOWN) {
+           rx_SetConnDeadTime(rxconn, 3);
+           conntimer[nconns]=1;
+       } else {
+           conntimer[nconns]=0;
+       }
+       nconns++;
+
     } /* Outer loop over addrs */
 
     afs_osi_Free(addrs, srvAddrCount * sizeof(*addrs));