Windows: cm_EndCallbackGrantingCall refactoring

Refactor cm_EndCallbackGrantingCall to prevent assigning a
callback to the cm_scache object in the case where it is going
to be discarded.  If the race was lost the callback data was
already discarded by cm_RevokeCallback.  By assigning and then
discarding we are forced to issue an additional change notification
to the smb client or afs redirector.  Not only is this extra work
but the afs redirector notification can result in a deadlock with
a kernel thread that is waiting for the current thread to complete.

modify the function signature to return whether or not a race
was lost with a callback revocation.

rename 'freeFlag' to 'freeRacingRevokes' since that is what
the flag is meant to indicate.

create a new 'freeServer' flag to indicate when the server
reference should be released.  There was a leak of server
references when a race occurred.

modify all calls to cm_EndCallbackGrantingCall() that provide
an AFSCallBack structure on input to check for a lost race.
If a race occurs, cm_MergeStatus() should not be performed.

Change-Id: Ib17091ed51a24826bf84d33235125b3ccbbe47d4
Reviewed-on: http://gerrit.openafs.org/6556
Reviewed-by: Jeffrey Altman <jaltman@secure-endpoints.com>
Tested-by: Jeffrey Altman <jaltman@secure-endpoints.com>

diff --git a/src/WINNT/afsd/cm_callback.c b/src/WINNT/afsd/cm_callback.c
index d1aa1e0..1fe0ade 100644 (file)
--- a/src/WINNT/afsd/cm_callback.c
+++ b/src/WINNT/afsd/cm_callback.c
@@ -198,14 +198,14 @@ void cm_RevokeCallback(struct rx_call *callp, cm_cell_t * cellp, AFSFid *fidp)
             osi_Log4(afsd_logp, "RevokeCallback Discarding SCache scp 0x%p vol %u vn %u uniq %u",
                      scp, scp->fid.volume, scp->fid.vnode, scp->fid.unique);
 
-            if (RDR_Initialized)
-                RDR_InvalidateObject(scp->fid.cell, scp->fid.volume, scp->fid.vnode, scp->fid.unique,
-                                     scp->fid.hash, scp->fileType, AFS_INVALIDATE_CALLBACK);
-
             lock_ObtainWrite(&scp->rw);
             cm_DiscardSCache(scp);
             lock_ReleaseWrite(&scp->rw);
 
+            if (RDR_Initialized)
+                RDR_InvalidateObject(scp->fid.cell, scp->fid.volume, scp->fid.vnode, scp->fid.unique,
+                                     scp->fid.hash, scp->fileType, AFS_INVALIDATE_CALLBACK);
+
             cm_CallbackNotifyChange(scp);
 
             lock_ObtainWrite(&cm_scacheLock);
@@ -1640,52 +1640,40 @@ void cm_StartCallbackGrantingCall(cm_scache_t *scp, cm_callbackRequest_t *cbrp)
  *
  * Called with scp write locked, so we can discard the callbacks easily with
  * this locking hierarchy.
+ *
+ * Performs cleanup of the stack allocated cm_callbackRequest_t object.
+ *
+ * Returns 0 on success and non-zero if the callback lost a race.
  */
-void cm_EndCallbackGrantingCall(cm_scache_t *scp, cm_callbackRequest_t *cbrp,
-                                AFSCallBack *cbp, AFSVolSync *volSyncp, long flags)
+int
+cm_EndCallbackGrantingCall(cm_scache_t *scp, cm_callbackRequest_t *cbrp,
+                           AFSCallBack *cbp, AFSVolSync *volSyncp, long flags)
 {
     cm_racingRevokes_t *revp;          /* where we are */
     cm_racingRevokes_t *nrevp;         /* where we'll be next */
-    int freeFlag;
+    int freeRacingRevokes;
+    int freeServer;
     cm_server_t * serverp = NULL;
-    int discardScp = 0, discardVolCB = 0;
+    int lostRace = 0;
 
     lock_ObtainWrite(&cm_callbackLock);
     if (flags & CM_CALLBACK_MAINTAINCOUNT) {
         osi_assertx(cm_activeCallbackGrantingCalls > 0,
                     "CM_CALLBACK_MAINTAINCOUNT && cm_activeCallbackGrantingCalls == 0");
+        freeServer = 0;
     }
     else {
         osi_assertx(cm_activeCallbackGrantingCalls-- > 0,
                     "!CM_CALLBACK_MAINTAINCOUNT && cm_activeCallbackGrantingCalls == 0");
+        freeServer = 1;
     }
     if (cm_activeCallbackGrantingCalls == 0)
-        freeFlag = 1;
+        freeRacingRevokes = 1;
     else
-        freeFlag = 0;
+        freeRacingRevokes = 0;
 
-    /* record the callback; we'll clear it below if we really lose it */
-    if (cbrp) {
-       if (scp) {
-            if (!cm_ServerEqual(scp->cbServerp, cbrp->serverp)) {
-                serverp = scp->cbServerp;
-                if (!freeFlag)
-                    cm_GetServer(cbrp->serverp);
-                scp->cbServerp = cbrp->serverp;
-            } else {
-                if (freeFlag)
-                    serverp = cbrp->serverp;
-            }
-            scp->cbExpires = cbrp->startTime + cbp->ExpirationTime;
-        } else {
-            if (freeFlag)
-                serverp = cbrp->serverp;
-        }
-        if (freeFlag)
-            cbrp->serverp = NULL;
-    }
-
-    /* a callback was actually revoked during our granting call, so
+    /*
+     * a callback was revoked during our granting call, so
      * run down the list of revoked fids, looking for ours.
      * If activeCallbackGrantingCalls is zero, free the elements, too.
      *
@@ -1714,31 +1702,39 @@ void cm_EndCallbackGrantingCall(cm_scache_t *scp, cm_callbackRequest_t *cbrp,
                       scp,
                       cbrp->callbackCount, revp->callbackCount,
                       cm_callbackCount);
-            discardScp = 1;
+            lostRace = 1;
             if ((scp->flags & CM_SCACHEFLAG_PURERO) &&
                  (revp->flags & CM_RACINGFLAG_ALL))
                 cm_callbackDiscardROVolumeByFID(&scp->fid);
         }
-        if (freeFlag)
+        if (freeRacingRevokes)
             free(revp);
     }
 
     /* if we freed the list, zap the pointer to it */
-    if (freeFlag)
+    if (freeRacingRevokes)
         cm_racingRevokesp = NULL;
-
     lock_ReleaseWrite(&cm_callbackLock);
 
-    if ( discardScp ) {
-        cm_DiscardSCache(scp);
-        lock_ReleaseWrite(&scp->rw);
-        cm_CallbackNotifyChange(scp);
-        if (RDR_Initialized)
-            RDR_InvalidateObject(scp->fid.cell, scp->fid.volume, scp->fid.vnode, scp->fid.unique,
-                                 scp->fid.hash, scp->fileType, AFS_INVALIDATE_CALLBACK);
-        lock_ObtainWrite(&scp->rw);
-    } else {
-        if (scp && scp->flags & CM_SCACHEFLAG_PURERO) {
+    /* record the callback if we didn't lose it */
+    if (!lostRace && scp) {
+        if (cbrp) {
+            if (!cm_ServerEqual(scp->cbServerp, cbrp->serverp)) {
+                serverp = scp->cbServerp;
+                scp->cbServerp = cbrp->serverp;
+                if (freeServer) {
+                    cbrp->serverp = NULL;
+                } else {
+                    cm_GetServer(scp->cbServerp);
+                }
+            } else if (freeServer) {
+                serverp = cbrp->serverp;
+                cbrp->serverp = NULL;
+            }
+            scp->cbExpires = cbrp->startTime + cbp->ExpirationTime;
+        }
+
+        if (scp->flags & CM_SCACHEFLAG_PURERO) {
             cm_volume_t * volp = cm_GetVolumeByFID(&scp->fid);
             if (volp) {
                 if (volSyncp) {
@@ -1757,12 +1753,27 @@ void cm_EndCallbackGrantingCall(cm_scache_t *scp, cm_callbackRequest_t *cbrp,
             }
         }
     }
+    else
+    {
+        /*
+         * No need to discard the cm_scache object or notify the
+         * afs redirector or smb client about a change if we did lose
+         * the race.  That was done when the callback was processed in
+         * cm_RevokeCallback().
+         */
+        if (cbrp && freeServer) {
+            serverp = cbrp->serverp;
+            cbrp->serverp = NULL;
+        }
+    }
 
     if ( serverp ) {
         lock_ObtainWrite(&cm_serverLock);
         cm_FreeServer(serverp);
         lock_ReleaseWrite(&cm_serverLock);
     }
+
+    return lostRace;
 }
 
 /* if flags is 1, we want to force the code to make one call, anyway.
@@ -1861,8 +1872,9 @@ long cm_GetCallback(cm_scache_t *scp, struct cm_user *userp,
 
         lock_ObtainWrite(&scp->rw);
         if (code == 0) {
-            cm_EndCallbackGrantingCall(scp, &cbr, &callback, &volSync, 0);
-            cm_MergeStatus(NULL, scp, &afsStatus, &volSync, userp, reqp, 0);
+            int lostRace = cm_EndCallbackGrantingCall(scp, &cbr, &callback, &volSync, 0);
+            if (!lostRace)
+                cm_MergeStatus(NULL, scp, &afsStatus, &volSync, userp, reqp, 0);
         } else {
             cm_EndCallbackGrantingCall(NULL, &cbr, NULL, NULL, 0);
             InterlockedDecrement(&scp->activeRPCs);

diff --git a/src/WINNT/afsd/cm_callback.h b/src/WINNT/afsd/cm_callback.h
index ebc4c24..089335b 100644 (file)
--- a/src/WINNT/afsd/cm_callback.h
+++ b/src/WINNT/afsd/cm_callback.h
@@ -56,7 +56,7 @@ extern int cm_HaveCallback(struct cm_scache *);
 
 extern void cm_StartCallbackGrantingCall(struct cm_scache *, cm_callbackRequest_t *);
 
-extern void cm_EndCallbackGrantingCall(struct cm_scache *, cm_callbackRequest_t *,
+extern int cm_EndCallbackGrantingCall(struct cm_scache *, cm_callbackRequest_t *,
        struct AFSCallBack *, struct AFSVolSync *, long);
 
 extern long cm_GetCallback(struct cm_scache *, struct cm_user *,

diff --git a/src/WINNT/afsd/cm_conn.c b/src/WINNT/afsd/cm_conn.c
index c5a10b0..0b07c05 100644 (file)
--- a/src/WINNT/afsd/cm_conn.c
+++ b/src/WINNT/afsd/cm_conn.c
@@ -275,9 +275,7 @@ cm_Analyze(cm_conn_t *connp, cm_user_t *userp, cm_req_t *reqp,
         } else {
             cm_GetServer(serverp);
         }
-        lock_ObtainWrite(&cm_callbackLock);
         cbrp->serverp = serverp;
-        lock_ReleaseWrite(&cm_callbackLock);
     }
 
     /* if timeout - check that it did not exceed the HardDead timeout

diff --git a/src/WINNT/afsd/cm_vnodeops.c b/src/WINNT/afsd/cm_vnodeops.c
index fadcc81..396a57c 100644 (file)
--- a/src/WINNT/afsd/cm_vnodeops.c
+++ b/src/WINNT/afsd/cm_vnodeops.c
@@ -2396,6 +2396,7 @@ cm_TryBulkStatRPC(cm_scache_t *dscp, cm_bulkStat_t *bbp, cm_user_t *userp, cm_re
     long filex;
     AFSVolSync volSync;
     cm_callbackRequest_t cbReq;
+    int lostRace;
     long filesThisCall;
     long i;
     long j;
@@ -2549,12 +2550,13 @@ cm_TryBulkStatRPC(cm_scache_t *dscp, cm_bulkStat_t *bbp, cm_user_t *userp, cm_re
                      (scp->flags & CM_SCACHEFLAG_EACCESS))
                 {
                     lock_ConvertRToW(&scp->rw);
-                    cm_EndCallbackGrantingCall(scp, &cbReq,
-                                               &bbp->callbacks[j],
-                                               &volSync,
-                                               CM_CALLBACK_MAINTAINCOUNT);
+                    lostRace = cm_EndCallbackGrantingCall(scp, &cbReq,
+                                                          &bbp->callbacks[j],
+                                                          &volSync,
+                                                          CM_CALLBACK_MAINTAINCOUNT);
                     InterlockedIncrement(&scp->activeRPCs);
-                    cm_MergeStatus(dscp, scp, &bbp->stats[j], &volSync, userp, reqp, 0);
+                    if (!lostRace)
+                        cm_MergeStatus(dscp, scp, &bbp->stats[j], &volSync, userp, reqp, 0);
                     lock_ReleaseWrite(&scp->rw);
                 } else {
                     lock_ReleaseRead(&scp->rw);
@@ -2839,6 +2841,7 @@ long cm_Create(cm_scache_t *dscp, clientchar_t *cnamep, long flags, cm_attr_t *a
     cm_fid_t newFid;
     cm_scache_t *scp = NULL;
     int didEnd;
+    int lostRace;
     AFSStoreStatus inStatus;
     AFSFetchStatus updatedDirStatus;
     AFSFetchStatus newFileStatus;
@@ -2955,11 +2958,12 @@ long cm_Create(cm_scache_t *dscp, clientchar_t *cnamep, long flags, cm_attr_t *a
             lock_ObtainWrite(&scp->rw);
            scp->creator = userp;               /* remember who created it */
             if (!cm_HaveCallback(scp)) {
-                cm_EndCallbackGrantingCall(scp, &cbReq,
-                                           &newFileCallback, &volSync, 0);
+                lostRace = cm_EndCallbackGrantingCall(scp, &cbReq,
+                                                      &newFileCallback, &volSync, 0);
                 InterlockedIncrement(&scp->activeRPCs);
-                cm_MergeStatus(dscp, scp, &newFileStatus, &volSync,
-                               userp, reqp, 0);
+                if (!lostRace)
+                    cm_MergeStatus(dscp, scp, &newFileStatus, &volSync,
+                                   userp, reqp, 0);
                 didEnd = 1;
             }
             lock_ReleaseWrite(&scp->rw);
@@ -3030,6 +3034,7 @@ long cm_MakeDir(cm_scache_t *dscp, clientchar_t *cnamep, long flags, cm_attr_t *
     cm_fid_t newFid;
     cm_scache_t *scp = NULL;
     int didEnd;
+    int lostRace;
     AFSStoreStatus inStatus;
     AFSFetchStatus updatedDirStatus;
     AFSFetchStatus newDirStatus;
@@ -3144,11 +3149,12 @@ long cm_MakeDir(cm_scache_t *dscp, clientchar_t *cnamep, long flags, cm_attr_t *
         if (code == 0) {
             lock_ObtainWrite(&scp->rw);
             if (!cm_HaveCallback(scp)) {
-                cm_EndCallbackGrantingCall(scp, &cbReq,
-                                            &newDirCallback, &volSync, 0);
+                lostRace = cm_EndCallbackGrantingCall(scp, &cbReq,
+                                                      &newDirCallback, &volSync, 0);
                 InterlockedIncrement(&scp->activeRPCs);
-                cm_MergeStatus(dscp, scp, &newDirStatus, &volSync,
-                                userp, reqp, 0);
+                if (!lostRace)
+                    cm_MergeStatus(dscp, scp, &newDirStatus, &volSync,
+                                   userp, reqp, 0);
                 didEnd = 1;
             }
             lock_ReleaseWrite(&scp->rw);