From 52490d7968c2008912ab0887bdcde7fbba43b837 Mon Sep 17 00:00:00 2001 From: Jeffrey Altman Date: Thu, 12 Jun 2008 15:20:46 +0000 Subject: [PATCH] windows-scache-deletion-and-lock-verification-20080612 LICENSE MIT Add lock assertions to various functions. Obtain a missing lock around a call to cm_RemoveSCacheFromHashTable(). Correct an abstraction layer violation. cm_scache_t objects should be marked deleted in cm_Unlink() and cm_RemoveDir() and not in smb_CloseFID(). Cleanup of deleted cm_scache_t objects should be performed in cm_ReleaseSCache() when the reference count hits zero. Prototype cm_AdjustScacheLRU() and re-implement it using osi_QAddH(). --- src/WINNT/afsd/cm_scache.c | 47 +++++++++++++++++++++++++++++++----- src/WINNT/afsd/cm_scache.h | 2 ++ src/WINNT/afsd/cm_vnodeops.c | 24 ++++++++++++++++++ src/WINNT/afsd/smb.c | 9 ------- 4 files changed, 67 insertions(+), 15 deletions(-) diff --git a/src/WINNT/afsd/cm_scache.c b/src/WINNT/afsd/cm_scache.c index 610bd08cc..1f023af2e 100644 --- a/src/WINNT/afsd/cm_scache.c +++ b/src/WINNT/afsd/cm_scache.c @@ -45,21 +45,20 @@ extern osi_mutex_t cm_Freelance_Lock; /* must be called with cm_scacheLock write-locked! */ void cm_AdjustScacheLRU(cm_scache_t *scp) { - if (scp == cm_data.scacheLRULastp) - cm_data.scacheLRULastp = (cm_scache_t *) osi_QPrev(&scp->q); + lock_AssertWrite(&cm_scacheLock); osi_QRemoveHT((osi_queue_t **) &cm_data.scacheLRUFirstp, (osi_queue_t **) &cm_data.scacheLRULastp, &scp->q); - osi_QAdd((osi_queue_t **) &cm_data.scacheLRUFirstp, &scp->q); - if (!cm_data.scacheLRULastp) - cm_data.scacheLRULastp = scp; + osi_QAddH((osi_queue_t **) &cm_data.scacheLRUFirstp, (osi_queue_t **) &cm_data.scacheLRULastp, &scp->q); } -/* call with scache write-locked and mutex held */ +/* call with cm_scacheLock write-locked and scp rw held */ void cm_RemoveSCacheFromHashTable(cm_scache_t *scp) { cm_scache_t **lscpp; cm_scache_t *tscp; int i; + lock_AssertWrite(&cm_scacheLock); + lock_AssertWrite(&scp->rw); if (scp->flags & CM_SCACHEFLAG_INHASH) { /* hash it out first */ i = CM_SCACHE_HASH(&scp->fid); @@ -94,7 +93,9 @@ long cm_RecycleSCache(cm_scache_t *scp, afs_int32 flags) return -1; } + lock_ObtainWrite(&scp->rw); cm_RemoveSCacheFromHashTable(scp); + lock_ReleaseWrite(&scp->rw); #if 0 if (flags & CM_SCACHE_RECYCLEFLAG_DESTROY_BUFFERS) { @@ -233,6 +234,7 @@ cm_scache_t *cm_GetNewSCache(void) cm_scache_t *scp; int retry = 0; + lock_AssertWrite(&cm_scacheLock); #if 0 /* first pass - look for deleted objects */ for ( scp = cm_data.scacheLRULastp; @@ -1802,8 +1804,12 @@ void cm_ReleaseSCacheNoLock(cm_scache_t *scp) #endif { afs_int32 refCount; + long lockstate; + osi_assertx(scp != NULL, "null cm_scache_t"); lock_AssertAny(&cm_scacheLock); + + lockstate = lock_GetRWLockState(&cm_scacheLock); refCount = InterlockedDecrement(&scp->refCount); #ifdef DEBUG_REFCOUNT if (refCount < 0) @@ -1814,6 +1820,21 @@ void cm_ReleaseSCacheNoLock(cm_scache_t *scp) osi_Log2(afsd_logp,"cm_ReleaseSCacheNoLock scp 0x%p ref %d",scp, refCount); afsi_log("%s:%d cm_ReleaseSCacheNoLock scp 0x%p ref %d", file, line, scp, refCount); #endif + + if (scp->flags & CM_SCACHEFLAG_DELETED) { + int deleted = 0; + lock_ObtainWrite(&scp->rw); + if (scp->flags & CM_SCACHEFLAG_DELETED) + deleted = 1; + lock_ReleaseWrite(&scp->rw); + if (deleted) { + if (lockstate != OSI_RWLOCK_WRITEHELD) + lock_ConvertRToW(&cm_scacheLock); + cm_RecycleSCache(scp, 0); + if (lockstate != OSI_RWLOCK_WRITEHELD) + lock_ConvertWToR(&cm_scacheLock); + } + } } #ifdef DEBUG_REFCOUNT @@ -1836,6 +1857,20 @@ void cm_ReleaseSCache(cm_scache_t *scp) osi_Log2(afsd_logp,"cm_ReleaseSCache scp 0x%p ref %d",scp, refCount); afsi_log("%s:%d cm_ReleaseSCache scp 0x%p ref %d", file, line, scp, refCount); #endif + + if (scp->flags & CM_SCACHEFLAG_DELETED) { + int deleted = 0; + lock_ObtainWrite(&scp->rw); + if (scp->flags & CM_SCACHEFLAG_DELETED) + deleted = 1; + lock_ReleaseWrite(&scp->rw); + if (deleted) { + lock_ConvertRToW(&cm_scacheLock); + cm_RecycleSCache(scp, 0); + lock_ConvertWToR(&cm_scacheLock); + } + } + lock_ReleaseRead(&cm_scacheLock); } diff --git a/src/WINNT/afsd/cm_scache.h b/src/WINNT/afsd/cm_scache.h index 8e704fc7c..ac4990b28 100644 --- a/src/WINNT/afsd/cm_scache.h +++ b/src/WINNT/afsd/cm_scache.h @@ -405,4 +405,6 @@ extern void cm_SuspendSCache(void); extern long cm_RecycleSCache(cm_scache_t *scp, afs_int32 flags); extern void cm_RemoveSCacheFromHashTable(cm_scache_t *scp); + +extern void cm_AdjustScacheLRU(cm_scache_t *scp); #endif /* __CM_SCACHE_H_ENV__ */ diff --git a/src/WINNT/afsd/cm_vnodeops.c b/src/WINNT/afsd/cm_vnodeops.c index 2a5b211d6..8d5336154 100644 --- a/src/WINNT/afsd/cm_vnodeops.c +++ b/src/WINNT/afsd/cm_vnodeops.c @@ -1610,6 +1610,7 @@ long cm_Unlink(cm_scache_t *dscp, char *namep, char * normalizedName, AFSVolSync volSync; struct rx_connection * callp; cm_dirOp_t dirop; + cm_scache_t *scp = NULL; #ifdef AFS_FREELANCE_CLIENT if (cm_freelanceEnabled && dscp == cm_data.rootSCachep) { @@ -1619,6 +1620,8 @@ long cm_Unlink(cm_scache_t *dscp, char *namep, char * normalizedName, } #endif + code = cm_Lookup(dscp, namep, CM_FLAG_NOMOUNTCHASE, userp, reqp, &scp); + /* make sure we don't screw up the dir status during the merge */ code = cm_BeginDirOp(dscp, userp, reqp, CM_DIRLOCK_NONE, &dirop); @@ -1681,6 +1684,15 @@ long cm_Unlink(cm_scache_t *dscp, char *namep, char * normalizedName, } cm_EndDirOp(&dirop); + if (scp) { + cm_ReleaseSCache(scp); + if (code == 0) { + lock_ObtainWrite(&scp->rw); + scp->flags |= CM_SCACHEFLAG_DELETED; + lock_ReleaseWrite(&scp->rw); + } + } + return code; } @@ -3163,6 +3175,9 @@ long cm_RemoveDir(cm_scache_t *dscp, char *namep, char *normalizedNamep, AFSVolSync volSync; struct rx_connection * callp; cm_dirOp_t dirop; + cm_scache_t *scp = NULL; + + code = cm_Lookup(dscp, namep, CM_FLAG_NOMOUNTCHASE, userp, reqp, &scp); /* before starting the RPC, mark that we're changing the directory data, * so that someone who does a chmod on the dir will wait until our @@ -3225,6 +3240,15 @@ long cm_RemoveDir(cm_scache_t *dscp, char *namep, char *normalizedNamep, } cm_EndDirOp(&dirop); + if (scp) { + cm_ReleaseSCache(scp); + if (code == 0) { + lock_ObtainWrite(&scp->rw); + scp->flags |= CM_SCACHEFLAG_DELETED; + lock_ReleaseWrite(&scp->rw); + } + } + /* and return error code */ return code; } diff --git a/src/WINNT/afsd/smb.c b/src/WINNT/afsd/smb.c index bee69de43..9ef5047cd 100644 --- a/src/WINNT/afsd/smb.c +++ b/src/WINNT/afsd/smb.c @@ -6141,7 +6141,6 @@ long smb_CloseFID(smb_vc_t *vcp, smb_fid_t *fidp, cm_user_t *userp, char *pathp = NULL; cm_scache_t * scp = NULL; cm_scache_t *delscp = NULL; - int deleted = 0; int nullcreator = 0; osi_Log4(smb_logp, "smb_CloseFID Closing fidp 0x%x (fid=%d scp=0x%x vcp=0x%x)", @@ -6261,7 +6260,6 @@ long smb_CloseFID(smb_vc_t *vcp, smb_fid_t *fidp, cm_user_t *userp, if (delscp->fileType == CM_SCACHETYPE_DIRECTORY) { code = cm_RemoveDir(dscp, originalNamep, fullPathp, userp, &req); if (code == 0) { - deleted = 1; if (dscp->flags & CM_SCACHEFLAG_ANYWATCH) smb_NotifyChange(FILE_ACTION_REMOVED, FILE_NOTIFY_CHANGE_DIR_NAME | FILE_NOTIFY_CHANGE_CREATION, @@ -6270,7 +6268,6 @@ long smb_CloseFID(smb_vc_t *vcp, smb_fid_t *fidp, cm_user_t *userp, } else { code = cm_Unlink(dscp, originalNamep, fullPathp, userp, &req); if (code == 0) { - deleted = 1; if (dscp->flags & CM_SCACHEFLAG_ANYWATCH) smb_NotifyChange(FILE_ACTION_REMOVED, FILE_NOTIFY_CHANGE_FILE_NAME | FILE_NOTIFY_CHANGE_CREATION, @@ -6320,12 +6317,6 @@ long smb_CloseFID(smb_vc_t *vcp, smb_fid_t *fidp, cm_user_t *userp, cm_ReleaseSCache(dscp); if (delscp) { - if (deleted) { - lock_ObtainWrite(&delscp->rw); - if (deleted) - delscp->flags |= CM_SCACHEFLAG_DELETED; - lock_ReleaseWrite(&delscp->rw); - } cm_ReleaseSCache(delscp); } -- 2.39.5