afs: Change afs_AllocDCache to return error codes 27/13227/3
authorAndrew Deason <adeason@sinenomine.net>
Thu, 28 Jun 2018 18:08:47 +0000 (13:08 -0500)
committerBenjamin Kaduk <kaduk@mit.edu>
Fri, 29 Jun 2018 02:21:54 +0000 (22:21 -0400)
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 <buildbot@rampaginggeek.com>
Reviewed-by: Benjamin Kaduk <kaduk@mit.edu>

src/afs/afs_dcache.c

index 4be5b59..3c437bd 100644 (file)
@@ -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);