viced: Avoid endless BCB loop
authorAndrew Deason <adeason@sinenomine.net>
Mon, 12 Aug 2013 22:37:29 +0000 (17:37 -0500)
committerDerrick Brashear <shadow@your-file-system.com>
Tue, 8 Oct 2013 12:50:32 +0000 (05:50 -0700)
Without this commit, when we break callbacks for a fid, we loop over
all callbacks for the fid, break a few of them, and then start over.
We do this repeatedly until we run out of callbacks. If a client sees
a callback break, and then establishes a new callback promise while
the fileserver is still breaking callbacks, the fileserver can break
the same callback for the same host again and again. This can continue
forever, if the client establishes its new callback promises quickly
enough.

So to avoid this, when we start breaking callbacks, flag all of the
callback structures that we want to look at. Then when we repeatedly
loop through all of the callbacks for the fid, only look at the
flagged callback structures.

This adds a 'flags' field to struct CallBack, and defines a single
flag, CBFLAG_BREAKING.

This is an alternative fix to the issue also fixed in 843d705c. This
implementation avoids allocating extra memory under locks, and has the
slight benefit of not breaking callbacks that were elsewhere deleted
during the BCB. This comes at the cost of a single extra traversal
through our callback list, and the cost of claiming one of the bits in
the CallBack structure.

Change-Id: I6418bd404de61ec7a531261ecf581eeea719a2d4
Reviewed-on: http://gerrit.openafs.org/10172
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Benjamin Kaduk <kaduk@mit.edu>
Reviewed-by: Derrick Brashear <shadow@your-file-system.com>

src/viced/callback.c
src/viced/callback.h

index 975e1c7..4944e7b 100644 (file)
@@ -627,6 +627,7 @@ AddCallBack1_r(struct host *host, AFSFid * fid, afs_uint32 * thead, int type,
        cb->cnext = 0;
        cb->fhead = fetoi(fe);
        cb->status = type;
+       cb->flags = 0;
        HAdd(cb, host);
        TAdd(cb, Thead);
     }
@@ -836,13 +837,31 @@ BreakCallBack(struct host *xhost, AFSFid * fid, int flag)
     tf.AFSCBFids_len = 1;
     tf.AFSCBFids_val = fid;
 
+    /* Set CBFLAG_BREAKING flag on all CBs we're looking at. We do this so we
+     * can loop through all relevant CBs while dropping H_LOCK, and not lose
+     * track of which CBs we want to look at. If we look at all CBs over and
+     * over again, we can loop indefinitely as new CBs are added. */
+    for (; cb; cb = nextcb) {
+       nextcb = itocb(cb->cnext);
+
+       if ((cb->hhead != hostindex || flag)
+           && (cb->status == CB_BULK || cb->status == CB_NORMAL
+               || cb->status == CB_VOLUME)) {
+           cb->flags |= CBFLAG_BREAKING;
+       }
+    }
+
+    cb = itocb(fe->firstcb);
+    opr_Assert(cb);
+
+    /* loop through all CBs, only looking at ones with the CBFLAG_BREAKING
+     * flag set */
     for (; cb;) {
        for (ncbas = 0; cb && ncbas < MAX_CB_HOSTS; cb = nextcb) {
            nextcb = itocb(cb->cnext);
-           if ((cb->hhead != hostindex || flag)
-               && (cb->status == CB_BULK || cb->status == CB_NORMAL
-                   || cb->status == CB_VOLUME)) {
+           if ((cb->flags & CBFLAG_BREAKING)) {
                struct host *thishost = h_itoh(cb->hhead);
+               cb->flags &= ~CBFLAG_BREAKING;
                if (!thishost) {
                    ViceLog(0, ("BCB: BOGUS! cb->hhead is NULL!\n"));
                } else if (thishost->hostFlags & VENUSDOWN) {
index 8f52177..9fe0399 100644 (file)
@@ -67,7 +67,8 @@ struct CallBack {
     afs_uint32 fhead;          /* index of associated FE */
     u_byte thead;              /* Head of timeout chain */
     u_byte status;             /* Call back status; see definitions, below */
-    unsigned short spare;      /* ensure proper alignment */
+    u_byte flags;      /* see CBFLAG_* definitions below */
+    u_byte spare;      /* ensure proper alignment */
     afs_uint32 hhead;          /* Head of host table chain */
     afs_uint32 tprev, tnext;   /* per-timeout circular list of callbacks */
     afs_uint32 hprev, hnext;   /* per-host circular list of callbacks */
@@ -99,6 +100,9 @@ struct VCBParams {
 #define CB_VOLUME   3          /* Callback for a volume */
 #define CB_BULK     4          /* Normal callbacks, handed out from FetchBulkStatus */
 
+/* values for the 'flags' field of CallBack structure */
+#define CBFLAG_BREAKING        0x1     /* this CB is marked for breaking / is getting broken */
+
 /* call back indices to pointers, and vice-versa */
 #define itocb(i)    ((i)?CB+(i):0)
 #define cbtoi(cbp)  ((afs_uint32)(!(cbp)?0:(cbp)-CB))