From 0322dd56b20b2e2fd6eb7f217964174fb5d25cdd Mon Sep 17 00:00:00 2001 From: Andrew Deason Date: Thu, 28 Jun 2018 13:08:47 -0500 Subject: [PATCH] afs: Change afs_AllocDCache to return error codes Currently, afs_AllocDCache can fail in 2 different situations: - When we are out of dslots on the free/discard lists - When we encounter an i/o error when trying to traverse the dslot lists But afs_AllocDCache cannot distinguish between these two cases to its caller in any way, since all we have to return is a struct dcache (and so we return NULL on any error). Currently, the caller of afs_AllocDCache in afs_GetDCache is determining which of these cases happened by looking at afs_discardDCList and afs_freeDCList, to see if they look empty. This is not great for at least a couple of reasons: - We are examining afs_discardDCList/afs_freeDCList after we drop afs_xdcache (but while still holding GLOCK) - If afs_discardDCList/afs_freeDCList are somehow changed while afs_AllocDCache is running, we may infer the wrong reason why afs_AllocDCache failed. (currently impossible, but this seems fragile) And in general, this check against afs_discardDCList/afs_freeDCList is rather indirect. It may be easier to follow if afs_AllocDCache just directly returned the reason why it failed. So do that, by changing afs_AllocDCache to return an error code, and providing the struct dcache in an output argument. This involves similiarly changing several called functions in the same way, to return error codes. We only define 2 such error codes with this commit: - ENOSPC, when we are out of free/discrad dslots - EIO, when we encounter a disk i/o error when trying to examine the dslot list Note that this commit should not change any real logic; we're mostly just changing how errors are returned from these various functions. Change-Id: I07cc3d7befdcc98360889f4a2ba01fdc9de50848 Reviewed-on: https://gerrit.openafs.org/13227 Tested-by: BuildBot Reviewed-by: Benjamin Kaduk --- src/afs/afs_dcache.c | 109 ++++++++++++++++++++++++++++++--------------------- 1 file changed, 65 insertions(+), 44 deletions(-) diff --git a/src/afs/afs_dcache.c b/src/afs/afs_dcache.c index 4be5b59..3c437bd 100644 --- a/src/afs/afs_dcache.c +++ b/src/afs/afs_dcache.c @@ -1121,29 +1121,39 @@ afs_DiscardDCache(struct dcache *adc) /** * Get a dcache entry from the discard or free list * + * @param[out] adc On success, a dcache from the given list. Otherwise, NULL. * @param[in] indexp A pointer to the head of the dcache free list or discard * list (afs_freeDCList, or afs_discardDCList) * - * @return A dcache from that list, or NULL if none could be retrieved. + * @return 0 on success. If there are no dcache slots available, return ENOSPC. + * If we encountered an error in disk i/o while trying to find a + * dcache, return EIO. * * @pre afs_xdcache is write-locked */ -static struct dcache * -afs_GetDSlotFromList(afs_int32 *indexp) +static int +afs_GetDSlotFromList(struct dcache **adc, afs_int32 *indexp) { struct dcache *tdc; - if (*indexp != NULLIDX) { - tdc = afs_GetUnusedDSlot(*indexp); - if (tdc) { - osi_Assert(tdc->refCount == 1); - ReleaseReadLock(&tdc->tlock); - *indexp = afs_dvnextTbl[tdc->index]; - afs_dvnextTbl[tdc->index] = NULLIDX; - return tdc; - } + *adc = NULL; + + if (*indexp == NULLIDX) { + return ENOSPC; } - return NULL; + + tdc = afs_GetUnusedDSlot(*indexp); + if (tdc == NULL) { + return EIO; + } + + osi_Assert(tdc->refCount == 1); + ReleaseReadLock(&tdc->tlock); + *indexp = afs_dvnextTbl[tdc->index]; + afs_dvnextTbl[tdc->index] = NULLIDX; + + *adc = tdc; + return 0; } /*! @@ -1170,7 +1180,7 @@ afs_FreeDiscardedDCache(void) /* * Get an entry from the list of discarded cache elements */ - tdc = afs_GetDSlotFromList(&afs_discardDCList); + (void)afs_GetDSlotFromList(&tdc, &afs_discardDCList); if (!tdc) { ReleaseWriteLock(&afs_xdcache); return -1; @@ -1583,31 +1593,34 @@ afs_FindDCache(struct vcache *avc, afs_size_t abyte) } /*afs_FindDCache */ /* only call these from afs_AllocDCache() */ -static struct dcache * -afs_AllocFreeDSlot(void) +static int +afs_AllocFreeDSlot(struct dcache **adc) { + int code; struct dcache *tdc; - tdc = afs_GetDSlotFromList(&afs_freeDCList); - if (!tdc) { - return NULL; + code = afs_GetDSlotFromList(&tdc, &afs_freeDCList); + if (code) { + return code; } afs_indexFlags[tdc->index] &= ~IFFree; ObtainWriteLock(&tdc->lock, 604); afs_freeDCCount--; - return tdc; + *adc = tdc; + return 0; } -static struct dcache * -afs_AllocDiscardDSlot(afs_int32 lock) +static int +afs_AllocDiscardDSlot(struct dcache **adc, afs_int32 lock) { + int code; struct dcache *tdc; afs_uint32 size = 0; struct osi_file *file; - tdc = afs_GetDSlotFromList(&afs_discardDCList); - if (!tdc) { - return NULL; + code = afs_GetDSlotFromList(&tdc, &afs_discardDCList); + if (code) { + return code; } afs_indexFlags[tdc->index] &= ~IFDiscarded; ObtainWriteLock(&tdc->lock, 605); @@ -1628,12 +1641,14 @@ afs_AllocDiscardDSlot(afs_int32 lock) afs_AdjustSize(tdc, 0); } - return tdc; + *adc = tdc; + return 0; } /*! * Get a fresh dcache from the free or discarded list. * + * \param adc Set to the new dcache on success, and NULL on error. * \param avc Who's dcache is this going to be? * \param chunk The position where it will be placed in. * \param lock How are locks held. @@ -1645,29 +1660,34 @@ afs_AllocDiscardDSlot(afs_int32 lock) * - avc (R if (lock & 1) set and W otherwise) * \note It write locks the new dcache. The caller must unlock it. * - * \return The new dcache. + * \return If we're out of dslots, ENOSPC. If we encountered disk errors, EIO. + * On success, return 0. */ -static struct dcache * -afs_AllocDCache(struct vcache *avc, afs_int32 chunk, afs_int32 lock, - struct VenusFid *ashFid) +static int +afs_AllocDCache(struct dcache **adc, struct vcache *avc, afs_int32 chunk, + afs_int32 lock, struct VenusFid *ashFid) { + int code; struct dcache *tdc = NULL; + *adc = NULL; + /* if (lock & 2), prefer 'free' dcaches; otherwise, prefer 'discard' - * dcaches. In either case, try both if our first choice doesn't work. */ + * dcaches. In either case, try both if our first choice doesn't work due + * to ENOSPC. */ if ((lock & 2)) { - tdc = afs_AllocFreeDSlot(); - if (!tdc) { - tdc = afs_AllocDiscardDSlot(lock); + code = afs_AllocFreeDSlot(&tdc); + if (code == ENOSPC) { + code = afs_AllocDiscardDSlot(&tdc, lock); } } else { - tdc = afs_AllocDiscardDSlot(lock); - if (!tdc) { - tdc = afs_AllocFreeDSlot(); + code = afs_AllocDiscardDSlot(&tdc, lock); + if (code == ENOSPC) { + code = afs_AllocFreeDSlot(&tdc); } } - if (!tdc) { - return NULL; + if (code) { + return code; } /* @@ -1704,7 +1724,8 @@ afs_AllocDCache(struct vcache *avc, afs_int32 chunk, afs_int32 lock, if (tdc->lruq.prev == &tdc->lruq) osi_Panic("lruq 1"); - return tdc; + *adc = tdc; + return 0; } /* @@ -1972,10 +1993,10 @@ afs_GetDCache(struct vcache *avc, afs_size_t abyte, if (!setLocks) avc->f.states &= ~CDCLock; } - tdc = afs_AllocDCache(avc, chunk, aflags, NULL); - if (!tdc) { + code = afs_AllocDCache(&tdc, avc, chunk, aflags, NULL); + if (code) { ReleaseWriteLock(&afs_xdcache); - if (afs_discardDCList == NULLIDX && afs_freeDCList == NULLIDX) { + if (code == ENOSPC) { /* It looks like afs_AllocDCache failed because we don't * have any free dslots to use. Maybe if we wait a little * while, we'll be able to free up some slots, so try for 5 @@ -3621,7 +3642,7 @@ afs_MakeShadowDir(struct vcache *avc, struct dcache *adc) ObtainWriteLock(&afs_xdcache, 716); /* Get a fresh dcache. */ - new_dc = afs_AllocDCache(avc, 0, 0, &shadow_fid); + (void)afs_AllocDCache(&new_dc, avc, 0, 0, &shadow_fid); osi_Assert(new_dc); ObtainReadLock(&adc->mflock); -- 1.9.4