Revert "Atomically collect callbacks to be broken"
authorAndrew Deason <adeason@sinenomine.net>
Wed, 21 Aug 2013 01:51:04 +0000 (20:51 -0500)
committerDerrick Brashear <shadow@your-file-system.com>
Tue, 8 Oct 2013 12:50:23 +0000 (05:50 -0700)
This mostly reverts commit 843d705ca6f0250c3760ec2aa1f3403d19de3df1.
That commit causes each callback-breaking thread to potentially use up
a large amount of memory, as well as possibly causing large memory
allocations under H_LOCK, which isn't great.

There are other ways to allow for atomic callback breaks. Revert that
commit to allow for alternative methods to be implemented in separate
subsequent commits. Do this in separate commits so pullups to stable
branches are easier.

This does not revert the change in the definition of MAX_CB_HOSTS.
That value can still be large due to the improved multi_Rx
implementation.

Change-Id: I14024b4d80696b0361658b1c5ae7af308629fab4
Reviewed-on: http://gerrit.openafs.org/10171
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Derrick Brashear <shadow@your-file-system.com>

src/viced/callback.c

index 6618f88..975e1c7 100644 (file)
@@ -805,8 +805,8 @@ BreakCallBack(struct host *xhost, AFSFid * fid, int flag)
 {
     struct FileEntry *fe;
     struct CallBack *cb, *nextcb;
-    struct cbstruct cbaDef[MAX_CB_HOSTS], *cba = cbaDef;
-    unsigned int ncbas, cbaAlloc = MAX_CB_HOSTS;
+    struct cbstruct cba[MAX_CB_HOSTS];
+    int ncbas;
     struct AFSCBFids tf;
     int hostindex;
     char hoststr[16];
@@ -836,7 +836,8 @@ BreakCallBack(struct host *xhost, AFSFid * fid, int flag)
     tf.AFSCBFids_len = 1;
     tf.AFSCBFids_val = fid;
 
-       for (ncbas = 0; cb ; cb = nextcb) {
+    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
@@ -853,21 +854,6 @@ BreakCallBack(struct host *xhost, AFSFid * fid, int flag)
                } else {
                    if (!(thishost->hostFlags & HOSTDELETED)) {
                        h_Hold_r(thishost);
-                       if (ncbas == cbaAlloc) {        /* Need more space */
-                           int curLen = cbaAlloc*sizeof(cba[0]);
-                           struct cbstruct *cbaOld = (cba == cbaDef) ? NULL : cba;
-
-                           /* There are logical contraints elsewhere that the number of hosts
-                              (i.e. h_HTSPERBLOCK*h_MAXHOSTTABLES) remains in the realm of a signed "int".
-                              cbaAlloc is defined unsigned int hence doubling below cannot overflow
-                           */
-                           cbaAlloc = cbaAlloc<<1;     /* double */
-                           cba = realloc(cbaOld, cbaAlloc * sizeof(cba[0]));
-
-                           if (cbaOld == NULL) {       /* realloc wouldn't have copied from cbaDef */
-                               memcpy(cba, cbaDef, curLen);
-                           }
-                       }
                        cba[ncbas].hp = thishost;
                        cba[ncbas].thead = cb->thead;
                        ncbas++;
@@ -881,16 +867,20 @@ BreakCallBack(struct host *xhost, AFSFid * fid, int flag)
        }
 
        if (ncbas) {
-           struct cbstruct *cba2;
-           int num;
+           MultiBreakCallBack_r(cba, ncbas, &tf);
 
-           for (cba2 = cba, num = ncbas; ncbas > 0; cba2 += num, ncbas -= num) {
-               num = (ncbas > MAX_CB_HOSTS) ? MAX_CB_HOSTS : ncbas;
-               MultiBreakCallBack_r(cba2, num, &tf);
+           /* we need to to all these initializations again because MultiBreakCallBack may block */
+           fe = FindFE(fid);
+           if (!fe) {
+               goto done;
+           }
+           cb = itocb(fe->firstcb);
+           if (!cb || ((fe->ncbs == 1) && (cb->hhead == hostindex) && !flag)) {
+               /* the most common case is what follows the || */
+               goto done;
            }
        }
-
-       if (cba != cbaDef) free(cba);
+    }
 
   done:
     H_UNLOCK;