From 5c59d1b500ae4ba5223184fbfe18837891ea30c3 Mon Sep 17 00:00:00 2001 From: Jeffrey Altman Date: Sat, 12 Nov 2011 17:32:06 -0500 Subject: [PATCH] Windows: cm_GetSCache avoid holding cm_scacheLock cm_GetSCache used to hold cm_scacheLock write-locked from start to finish except that it didn't. There were several places where cm_scacheLock was dropped and reacquired due to lock ordering requirements. Unfortunately, this has two problems. First, the function isn't very fast in the most common case since cm_scacheLock is write-locked for the search for an existing FID. Second, there is a race that results when cm_GetNewSCache() drops the cm_scacheLock. To make things faster, use a read-lock for the common case. To avoid the race, if the FID cannot be located, call cm_GetNewSCache() first and then obtain the cell and volume information. Then perform a second lookup for the FID while holding cm_scacheLock write-locked. If we lost the race or there was an error obtaining the cell and volume info, put the new cm_scache_t back onto the end of the LRU queue. Change-Id: Idb94275d23c160ee0a2dc8fdcfd0f09506ecb2d3 Reviewed-on: http://gerrit.openafs.org/6003 Tested-by: BuildBot Reviewed-by: Jeffrey Altman Tested-by: Jeffrey Altman --- src/WINNT/afsd/cm_scache.c | 119 +++++++++++++++++++++++++++++---------------- src/WINNT/afsd/cm_scache.h | 2 +- 2 files changed, 79 insertions(+), 42 deletions(-) diff --git a/src/WINNT/afsd/cm_scache.c b/src/WINNT/afsd/cm_scache.c index a2290b3..e556908 100644 --- a/src/WINNT/afsd/cm_scache.c +++ b/src/WINNT/afsd/cm_scache.c @@ -229,12 +229,17 @@ long cm_RecycleSCache(cm_scache_t *scp, afs_int32 flags) * Can allocate a new one if desperate, or if below quota (cm_data.maxSCaches). * returns scp->rw write-locked. */ -cm_scache_t *cm_GetNewSCache(void) +cm_scache_t * +cm_GetNewSCache(afs_uint32 locked) { - cm_scache_t *scp; + cm_scache_t *scp = NULL; int retry = 0; - lock_AssertWrite(&cm_scacheLock); + if (locked) + lock_AssertWrite(&cm_scacheLock); + else + lock_ObtainWrite(&cm_scacheLock); + if (cm_data.currentSCaches >= cm_data.maxSCaches) { /* There were no deleted scache objects that we could use. Try to find * one that simply hasn't been used in a while. @@ -252,7 +257,16 @@ cm_scache_t *cm_GetNewSCache(void) * it unless we have to. */ if (scp->refCount == 0 && scp->bufReadsp == NULL && scp->bufWritesp == NULL) { - if (!buf_DirtyBuffersExist(&scp->fid) && !buf_RDRBuffersExist(&scp->fid)) { + afs_uint32 buf_dirty = 0; + afs_uint32 buf_rdr = 0; + + lock_ReleaseWrite(&cm_scacheLock); + buf_dirty = buf_DirtyBuffersExist(&scp->fid); + if (!buf_dirty) + buf_rdr = buf_RDRBuffersExist(&scp->fid); + lock_ObtainWrite(&cm_scacheLock); + + if (!buf_dirty && !buf_rdr) { cm_fid_t fid; afs_uint32 fileType; @@ -287,7 +301,8 @@ cm_scache_t *cm_GetNewSCache(void) } /* and we're done */ - return scp; + osi_assertx(!(scp->flags & CM_SCACHEFLAG_INHASH), "CM_SCACHEFLAG_INHASH set"); + goto done; } lock_ReleaseWrite(&scp->rw); } else { @@ -297,7 +312,7 @@ cm_scache_t *cm_GetNewSCache(void) } osi_Log1(afsd_logp, "GetNewSCache all scache entries in use (retry = %d)", retry); } - return NULL; + goto done; } /* if we get here, we should allocate a new scache entry. We either are below @@ -317,14 +332,17 @@ cm_scache_t *cm_GetNewSCache(void) scp->serverLock = -1; /* and put it in the LRU queue */ - 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); cm_data.currentSCaches++; cm_dnlcPurgedp(scp); /* make doubly sure that this is not in dnlc */ cm_dnlcPurgevp(scp); scp->allNextp = cm_data.allSCachesp; cm_data.allSCachesp = scp; + + done: + if (!locked) + lock_ReleaseWrite(&cm_scacheLock); + return scp; } @@ -655,6 +673,7 @@ long cm_GetSCache(cm_fid_t *fidp, cm_scache_t **outScpp, cm_user_t *userp, { long hash; cm_scache_t *scp = NULL; + cm_scache_t *newScp = NULL; long code; cm_volume_t *volp = NULL; cm_cell_t *cellp; @@ -678,7 +697,7 @@ long cm_GetSCache(cm_fid_t *fidp, cm_scache_t **outScpp, cm_user_t *userp, // yj: check if we have the scp, if so, we don't need // to do anything else - lock_ObtainWrite(&cm_scacheLock); + lock_ObtainRead(&cm_scacheLock); for (scp=cm_data.scacheHashTablep[hash]; scp; scp=scp->nextp) { if (cm_FidCmp(fidp, &scp->fid) == 0) { #ifdef DEBUG_REFCOUNT @@ -692,11 +711,13 @@ long cm_GetSCache(cm_fid_t *fidp, cm_scache_t **outScpp, cm_user_t *userp, #endif cm_HoldSCacheNoLock(scp); *outScpp = scp; + lock_ConvertRToW(&cm_scacheLock); cm_AdjustScacheLRU(scp); lock_ReleaseWrite(&cm_scacheLock); return 0; } } + lock_ReleaseRead(&cm_scacheLock); // yj: when we get here, it means we don't have an scp // so we need to either load it or fake it, depending @@ -716,7 +737,6 @@ long cm_GetSCache(cm_fid_t *fidp, cm_scache_t **outScpp, cm_user_t *userp, } if (cm_freelanceEnabled && special) { - lock_ReleaseWrite(&cm_scacheLock); osi_Log0(afsd_logp,"cm_GetSCache Freelance and special"); if (cm_getLocalMountPointChange()) { @@ -724,18 +744,15 @@ long cm_GetSCache(cm_fid_t *fidp, cm_scache_t **outScpp, cm_user_t *userp, cm_reInitLocalMountPoints(); } - lock_ObtainWrite(&cm_scacheLock); if (scp == NULL) { - scp = cm_GetNewSCache(); /* returns scp->rw held */ + scp = cm_GetNewSCache(FALSE); /* returns scp->rw held */ if (scp == NULL) { osi_Log0(afsd_logp,"cm_GetSCache unable to obtain *new* scache entry"); lock_ReleaseWrite(&cm_scacheLock); return CM_ERROR_WOULDBLOCK; } } else { - lock_ReleaseWrite(&cm_scacheLock); lock_ObtainWrite(&scp->rw); - lock_ObtainWrite(&cm_scacheLock); } scp->fid = *fidp; scp->dotdotFid.cell=AFS_FAKE_ROOT_CELL_ID; @@ -743,6 +760,7 @@ long cm_GetSCache(cm_fid_t *fidp, cm_scache_t **outScpp, cm_user_t *userp, scp->dotdotFid.unique=1; scp->dotdotFid.vnode=1; _InterlockedOr(&scp->flags, (CM_SCACHEFLAG_PURERO | CM_SCACHEFLAG_RO)); + lock_ObtainWrite(&cm_scacheLock); if (!(scp->flags & CM_SCACHEFLAG_INHASH)) { scp->nextp = cm_data.scacheHashTablep[hash]; cm_data.scacheHashTablep[hash] = scp; @@ -750,6 +768,7 @@ long cm_GetSCache(cm_fid_t *fidp, cm_scache_t **outScpp, cm_user_t *userp, } scp->refCount = 1; osi_Log1(afsd_logp,"cm_GetSCache (freelance) sets refCount to 1 scp 0x%p", scp); + lock_ReleaseWrite(&cm_scacheLock); /* must be called after the scp->fid is set */ cm_FreelanceFetchMountPointString(scp); @@ -769,7 +788,6 @@ long cm_GetSCache(cm_fid_t *fidp, cm_scache_t **outScpp, cm_user_t *userp, scp->lockDataVersion=CM_SCACHE_VERSION_BAD; /* no lock yet */ scp->fsLockCount=0; lock_ReleaseWrite(&scp->rw); - lock_ReleaseWrite(&cm_scacheLock); *outScpp = scp; #ifdef DEBUG_REFCOUNT afsi_log("%s:%d cm_GetSCache (2) scp 0x%p ref %d", file, line, scp, scp->refCount); @@ -780,54 +798,72 @@ long cm_GetSCache(cm_fid_t *fidp, cm_scache_t **outScpp, cm_user_t *userp, // end of yj code #endif /* AFS_FREELANCE_CLIENT */ + /* we don't have the fid, recycle something */ + newScp = cm_GetNewSCache(FALSE); /* returns scp->rw held */ + if (newScp == NULL) { + osi_Log0(afsd_logp,"cm_GetNewSCache unable to obtain *new* scache entry"); + return CM_ERROR_WOULDBLOCK; + } +#ifdef DEBUG_REFCOUNT + afsi_log("%s:%d cm_GetNewSCache returns scp 0x%p flags 0x%x", file, line, newScp, newScp->flags); +#endif + osi_Log2(afsd_logp,"cm_GetNewSCache returns scp 0x%p flags 0x%x", newScp, newScp->flags); + /* otherwise, we need to find the volume */ if (!cm_freelanceEnabled || !isRoot) { - lock_ReleaseWrite(&cm_scacheLock); /* for perf. reasons */ cellp = cm_FindCellByID(fidp->cell, 0); - if (!cellp) + if (!cellp) { + /* put back newScp so it can be reused */ + lock_ObtainWrite(&cm_scacheLock); + newScp->flags |= CM_SCACHEFLAG_DELETED; + cm_AdjustScacheLRU(newScp); + lock_ReleaseWrite(&newScp->rw); + lock_ReleaseWrite(&cm_scacheLock); return CM_ERROR_NOSUCHCELL; + } code = cm_FindVolumeByID(cellp, fidp->volume, userp, reqp, CM_GETVOL_FLAG_CREATE, &volp); - if (code) + if (code) { + /* put back newScp so it can be reused */ + lock_ObtainWrite(&cm_scacheLock); + newScp->flags |= CM_SCACHEFLAG_DELETED; + cm_AdjustScacheLRU(newScp); + lock_ReleaseWrite(&newScp->rw); + lock_ReleaseWrite(&cm_scacheLock); return code; - lock_ObtainWrite(&cm_scacheLock); + } } - /* otherwise, we have the volume, now reverify that the scp doesn't - * exist, and proceed. + /* + * otherwise, we have the volume, now reverify that the scp doesn't + * exist, and proceed. make sure that we hold the cm_scacheLock + * write-locked until the scp is put into the hash table in order + * to avoid a race. */ + lock_ObtainWrite(&cm_scacheLock); for (scp=cm_data.scacheHashTablep[hash]; scp; scp=scp->nextp) { if (cm_FidCmp(fidp, &scp->fid) == 0) { #ifdef DEBUG_REFCOUNT afsi_log("%s:%d cm_GetSCache (3) scp 0x%p ref %d", file, line, scp, scp->refCount); osi_Log1(afsd_logp,"cm_GetSCache (3) scp 0x%p", scp); #endif + if (volp) + cm_PutVolume(volp); cm_HoldSCacheNoLock(scp); cm_AdjustScacheLRU(scp); + + /* put back newScp so it can be reused */ + newScp->flags |= CM_SCACHEFLAG_DELETED; + cm_AdjustScacheLRU(newScp); + lock_ReleaseWrite(&newScp->rw); lock_ReleaseWrite(&cm_scacheLock); - if (volp) - cm_PutVolume(volp); + *outScpp = scp; return 0; } } - /* now, if we don't have the fid, recycle something */ - scp = cm_GetNewSCache(); /* returns scp->rw held */ - if (scp == NULL) { - osi_Log0(afsd_logp,"cm_GetNewSCache unable to obtain *new* scache entry"); - lock_ReleaseWrite(&cm_scacheLock); - if (volp) - cm_PutVolume(volp); - return CM_ERROR_WOULDBLOCK; - } -#ifdef DEBUG_REFCOUNT - afsi_log("%s:%d cm_GetNewSCache returns scp 0x%p flags 0x%x", file, line, scp, scp->flags); -#endif - osi_Log2(afsd_logp,"cm_GetNewSCache returns scp 0x%p flags 0x%x", scp, scp->flags); - - osi_assertx(!(scp->flags & CM_SCACHEFLAG_INHASH), "CM_SCACHEFLAG_INHASH set"); - + scp = newScp; scp->fid = *fidp; if (!cm_freelanceEnabled || !isRoot) { /* if this scache entry represents a volume root then we need @@ -849,9 +885,11 @@ long cm_GetSCache(cm_fid_t *fidp, cm_scache_t **outScpp, cm_user_t *userp, } if (volp) cm_PutVolume(volp); + scp->nextp = cm_data.scacheHashTablep[hash]; cm_data.scacheHashTablep[hash] = scp; _InterlockedOr(&scp->flags, CM_SCACHEFLAG_INHASH); + lock_ReleaseWrite(&cm_scacheLock); lock_ReleaseWrite(&scp->rw); scp->refCount = 1; #ifdef DEBUG_REFCOUNT @@ -872,7 +910,6 @@ long cm_GetSCache(cm_fid_t *fidp, cm_scache_t **outScpp, cm_user_t *userp, afsi_log("%s:%d cm_GetSCache (4) scp 0x%p ref %d", file, line, scp, scp->refCount); osi_Log1(afsd_logp,"cm_GetSCache (4) scp 0x%p", scp); #endif - lock_ReleaseWrite(&cm_scacheLock); return 0; } diff --git a/src/WINNT/afsd/cm_scache.h b/src/WINNT/afsd/cm_scache.h index 5f8e06f..2064f01 100644 --- a/src/WINNT/afsd/cm_scache.h +++ b/src/WINNT/afsd/cm_scache.h @@ -376,7 +376,7 @@ extern long cm_GetSCache(cm_fid_t *, cm_scache_t **, struct cm_user *, struct cm_req *); #endif -extern cm_scache_t *cm_GetNewSCache(void); +extern cm_scache_t *cm_GetNewSCache(afs_uint32 locked); extern __inline int cm_FidCmp(cm_fid_t *, cm_fid_t *); -- 1.9.4