From 503d09413add1831dff5db24ee907eec59eda9e2 Mon Sep 17 00:00:00 2001 From: Jeffrey Altman Date: Sat, 14 Jan 2012 10:32:51 -0500 Subject: [PATCH] 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 Tested-by: Jeffrey Altman --- src/WINNT/afsd/cm_callback.c | 108 +++++++++++++++++++---------------- src/WINNT/afsd/cm_callback.h | 2 +- src/WINNT/afsd/cm_conn.c | 2 - src/WINNT/afsd/cm_vnodeops.c | 32 ++++++----- 4 files changed, 80 insertions(+), 64 deletions(-) diff --git a/src/WINNT/afsd/cm_callback.c b/src/WINNT/afsd/cm_callback.c index d1aa1e06c..1fe0ade4e 100644 --- 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 ebc4c24ef..089335bcd 100644 --- 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 c5a10b0d1..0b07c051b 100644 --- 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 fadcc812f..396a57c15 100644 --- 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); -- 2.39.5