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 d1aa1e06c24fc48e1e4814e3fd12035505421960..1fe0ade4e4a9c30976c7dde335a90ce5716c3f67 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 ebc4c24ef47d22f88161ff6f986a3da71cb8e335..089335bcdea906de2472ebf800a5d83ff1021b9c 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 c5a10b0d19fd2a95d4b1fc6bdc3a3ee1c3172c0a..0b07c051b91a1fc928c6e7856493d73829b918db 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 fadcc812f1917ef664f95219c1e9d87ea0cd5090..396a57c15c0249bfd26d4b03c667108d3223ed91 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);