From e69ee66caf1aa4131e2516d990c3d766a6d060d5 Mon Sep 17 00:00:00 2001 From: Andrew Deason Date: Tue, 15 Feb 2011 12:04:32 -0600 Subject: [PATCH] libafs: Drop xvcache for AllocCBR Normally when we AllocCBR, we are holding xvcache write-locked, since it is called from FlushVCache. Before a309e274632993c5aeec04c6e090f5ac95837a40, when AllocCBR needs to flush CBRs due to a lack of space, we hit the net, giving up callbacks on fileservers. This can cause a problem if one of those fileservers needs to contact us in order to complete that request, since the callback service thread may be waiting for xvcache, causing a deadlock (that is eventually broken by network timeouts). To avoid this, drop xvcache if AllocCBR looks like it does not have sufficient space. Fix all callers of afs_FlushVCache to handle the case where we sleep, since with this change, afs_FlushVCache can sleep on all platforms. This partially reverts a309e274632993c5aeec04c6e090f5ac95837a40, as it contains an alternative method of avoiding the xvcache lock in this situation. This commit restores much of the code path to be much more similar to how it used to be, except that it allows for dropping xvcache for AllocCBR. This should make any change to our prior behavior smaller/simpler, and thus safer and more consistent with existing clients. This reintroduces the hard limit to how much space we allocate for CBRs, although the part of a309e274632993c5aeec04c6e090f5ac95837a40 that raised this limit is retained. Reviewed-on: http://gerrit.openafs.org/3958 Reviewed-by: Derrick Brashear Tested-by: BuildBot (cherry picked from commit 76158df491f47de56d1febe1d1d2d17d316c9a74) Change-Id: I6d2d7512682b93e6524f8f60bb8e15818d888e00 Reviewed-on: http://gerrit.openafs.org/4603 Tested-by: Derrick Brashear Reviewed-by: Derrick Brashear --- src/afs/IRIX/osi_vfsops.c | 7 ++++- src/afs/LINUX/osi_vfsops.c | 6 +++- src/afs/LINUX24/osi_vfsops.c | 6 +++- src/afs/afs.h | 1 - src/afs/afs_call.c | 1 - src/afs/afs_vcache.c | 56 ++++++++++++++++++++++-------------- 6 files changed, 50 insertions(+), 27 deletions(-) diff --git a/src/afs/IRIX/osi_vfsops.c b/src/afs/IRIX/osi_vfsops.c index d8bfda327..7aa2a316f 100644 --- a/src/afs/IRIX/osi_vfsops.c +++ b/src/afs/IRIX/osi_vfsops.c @@ -216,11 +216,12 @@ afs_unmount(OSI_VFS_ARG(afsp), flags, cr) * EBUSY if any still in use */ ObtainWriteLock(&afs_xvcache, 172); + retry: for (tq = VLRU.prev; tq != &VLRU; tq = uq) { tvc = QTOV(tq); uq = QPrev(tq); vp = (vnode_t *) tvc; - if (error = afs_FlushVCache(tvc, &fv_slept)) + if (error = afs_FlushVCache(tvc, &fv_slept)) { if (vp->v_flag & VROOT) { rootvp = vp; continue; @@ -228,6 +229,10 @@ afs_unmount(OSI_VFS_ARG(afsp), flags, cr) ReleaseWriteLock(&afs_xvcache); return error; } + } + if (fv_slept) { + goto retry; + } } /* diff --git a/src/afs/LINUX/osi_vfsops.c b/src/afs/LINUX/osi_vfsops.c index 8abe6b149..04ddafc9f 100644 --- a/src/afs/LINUX/osi_vfsops.c +++ b/src/afs/LINUX/osi_vfsops.c @@ -477,13 +477,17 @@ osi_linux_free_inode_pages(void) struct vcache *tvc, *nvc; extern struct vcache *afs_vhashT[VCSIZE]; + retry: for (i = 0; i < VCSIZE; i++) { for (tvc = afs_vhashT[i]; tvc; ) { int slept; nvc = tvc->hnext; - if (afs_FlushVCache(tvc, &slept)) /* slept always 0 for linux? */ + if (afs_FlushVCache(tvc, &slept)) printf("Failed to invalidate all pages on inode 0x%p\n", tvc); + if (slept) { + goto retry; + } tvc = nvc; } } diff --git a/src/afs/LINUX24/osi_vfsops.c b/src/afs/LINUX24/osi_vfsops.c index 81aa3a019..702177c0a 100644 --- a/src/afs/LINUX24/osi_vfsops.c +++ b/src/afs/LINUX24/osi_vfsops.c @@ -465,13 +465,17 @@ osi_linux_free_inode_pages(void) struct vcache *tvc, *nvc; extern struct vcache *afs_vhashT[VCSIZE]; + retry: for (i = 0; i < VCSIZE; i++) { for (tvc = afs_vhashT[i]; tvc; ) { int slept; nvc = tvc->hnext; - if (afs_FlushVCache(tvc, &slept)) /* slept always 0 for linux? */ + if (afs_FlushVCache(tvc, &slept)) printf("Failed to invalidate all pages on inode 0x%p\n", tvc); + if (slept) { + goto retry; + } tvc = nvc; } } diff --git a/src/afs/afs.h b/src/afs/afs.h index c23a0498a..c44c2bb6f 100644 --- a/src/afs/afs.h +++ b/src/afs/afs.h @@ -260,7 +260,6 @@ struct afs_cbr { struct afs_cbr *hash_next; struct AFSFid fid; - unsigned int dynalloc:1; }; #ifdef AFS_LINUX22_ENV diff --git a/src/afs/afs_call.c b/src/afs/afs_call.c index ddacaf7f6..02db212b9 100644 --- a/src/afs/afs_call.c +++ b/src/afs/afs_call.c @@ -1245,7 +1245,6 @@ afs_shutdown(void) if (afs_shuttingdown) return; - afs_FlushVCBs(2); /* Reasonable effort to free dynamically allocated callback returns */ afs_shuttingdown = 1; if (afs_cold_shutdown) diff --git a/src/afs/afs_vcache.c b/src/afs/afs_vcache.c index 58c56cd4a..3d79d23d2 100644 --- a/src/afs/afs_vcache.c +++ b/src/afs/afs_vcache.c @@ -83,7 +83,7 @@ static int afs_nextVcacheSlot = 0; static struct afs_slotlist *afs_freeSlotList = NULL; /* Forward declarations */ -static afs_int32 afs_QueueVCB(struct vcache *avc); +static afs_int32 afs_QueueVCB(struct vcache *avc, int *slept); /*! * Generate an index into the hash table for a given Fid. @@ -205,7 +205,8 @@ afs_FlushVCache(struct vcache *avc, int *slept) vn_reinit(AFSTOV(avc)); #endif afs_FreeAllAxs(&(avc->Access)); - afs_QueueVCB(avc); + if (!afs_shuttingdown) + afs_QueueVCB(avc, slept); ObtainWriteLock(&afs_xcbhash, 460); afs_DequeueCallback(avc); /* remove it from queued callbacks list */ avc->f.states &= ~(CStatd | CUnique); @@ -300,14 +301,10 @@ afs_AllocCBR(void) struct afs_cbr *tsp; int i; - if (!afs_cbrSpace) { - afs_osi_CancelWait(&AFS_WaitHandler); /* trigger FlushVCBs asap */ - + while (!afs_cbrSpace) { if (afs_stats_cmperf.CallBackAlloced >= sizeof(afs_cbrHeads)/sizeof(afs_cbrHeads[0])) { /* don't allocate more than 16 * AFS_NCBRS for now */ - tsp = (struct afs_cbr *)osi_AllocSmallSpace(sizeof(*tsp)); - tsp->dynalloc = 1; - tsp->next = NULL; + afs_FlushVCBs(0); afs_stats_cmperf.CallBackFlushes++; } else { /* try allocating */ @@ -316,18 +313,15 @@ afs_AllocCBR(void) sizeof(struct afs_cbr)); for (i = 0; i < AFS_NCBRS - 1; i++) { tsp[i].next = &tsp[i + 1]; - tsp[i].dynalloc = 0; } tsp[AFS_NCBRS - 1].next = 0; - tsp[AFS_NCBRS - 1].dynalloc = 0; - afs_cbrSpace = tsp->next; + afs_cbrSpace = tsp; afs_cbrHeads[afs_stats_cmperf.CallBackAlloced] = tsp; afs_stats_cmperf.CallBackAlloced++; } - } else { - tsp = afs_cbrSpace; - afs_cbrSpace = tsp->next; } + tsp = afs_cbrSpace; + afs_cbrSpace = tsp->next; return tsp; } @@ -351,12 +345,8 @@ afs_FreeCBR(struct afs_cbr *asp) if (asp->hash_next) asp->hash_next->hash_pprev = asp->hash_pprev; - if (asp->dynalloc) { - osi_FreeSmallSpace(asp); - } else { - asp->next = afs_cbrSpace; - afs_cbrSpace = asp; - } + asp->next = afs_cbrSpace; + afs_cbrSpace = asp; return 0; } @@ -486,17 +476,20 @@ afs_FlushVCBs(afs_int32 lockit) * Environment: * Locks the xvcb lock. * Called when the xvcache lock is already held. + * RACE: afs_xvcache may be dropped and reacquired * * \param avc vcache entry + * \param slep Set to 1 if we dropped afs_xvcache * \return 1 if queued, 0 otherwise */ static afs_int32 -afs_QueueVCB(struct vcache *avc) +afs_QueueVCB(struct vcache *avc, int *slept) { int queued = 0; struct server *tsp; struct afs_cbr *tcbp; + int reacquire = 0; AFS_STATCNT(afs_QueueVCB); @@ -513,6 +506,15 @@ afs_QueueVCB(struct vcache *avc) /* The callback is really just a struct server ptr. */ tsp = (struct server *)(avc->callback); + if (!afs_cbrSpace) { + /* If we don't have CBR space, AllocCBR may block or hit the net for + * clearing up CBRs. Hitting the net may involve a fileserver + * needing to contact us, so we must drop xvcache so we don't block + * those requests from going through. */ + reacquire = *slept = 1; + ReleaseWriteLock(&afs_xvcache); + } + /* we now have a pointer to the server, so we just allocate * a queue entry and queue it. */ @@ -532,6 +534,11 @@ afs_QueueVCB(struct vcache *avc) done: /* now release locks and return */ ReleaseWriteLock(&afs_xvcb); + + if (reacquire) { + /* make sure this is after dropping xvcb, for locking order */ + ObtainWriteLock(&afs_xvcache, 279); + } return queued; } @@ -3068,13 +3075,18 @@ afs_DisconGiveUpCallbacks(void) ObtainWriteLock(&afs_xvcache, 1002); /* XXX - should be a unique number */ + retry: /* Somehow, walk the set of vcaches, with each one coming out as tvc */ for (i = 0; i < VCSIZE; i++) { for (tvc = afs_vhashT[i]; tvc; tvc = tvc->hnext) { - if (afs_QueueVCB(tvc)) { + int slept = 0; + if (afs_QueueVCB(tvc, &slept)) { tvc->callback = NULL; nq++; } + if (slept) { + goto retry; + } } } -- 2.39.5