afs: Never use GetNewDSlot after init
authorAndrew Deason <adeason@sinenomine.net>
Wed, 31 Oct 2012 20:55:35 +0000 (15:55 -0500)
committerDerrick Brashear <shadow@your-file-system.com>
Tue, 13 Nov 2012 18:37:31 +0000 (10:37 -0800)
Currently there are two ways to get a dcache via a slot number:
afs_GetNewDSot and afs_GetValidDSlot. afs_GetValidDSlot assumes that
the given slot number refers to a dcache entry that is valid on disk;
with afs_GetNewDSlot, the given slot may not be valid, and if it is
not, an empty 'template' dcache is returned.

afs_GetNewDSlot is useful for initializing cache files, since if a
given dcache slot exists on disk and contains valid data, we use the
dcache like normal. If it does not already exist or does not contain
valid data, we fill in the missing data after afs_GetNewDSlot returns.

However, for all other uses, afs_GetNewDSlot is incorrect, and causes
various serious problems. After we have initialized our dcache
entries, any attempt to read a dcache by slot number should succeed,
since the number of dcache entries never changes after we are started,
and we initialized all of them during client startup.

Some code outside of afs_InitCacheFile was still using
afs_GetNewDSlot; code that reads in a dslot from the free or discard
list. In these cases, if there is any error reading the dcache slot
from disk, we will be given a dcache that has some of its fields not
filled in properly. Notably, we assume that the entry is not on the
global hash table (we set tdc->f.fid.Fid.Volume to 0), and the
tdc->f.inode field is not initialized at all, leaving it set to
whatever was in memory for that tdc before we tried to read the slot
from disk.

This can cause cache corruption, since tdc->f.inode can point to the
inoder for a different existing cache file, so writing to that dcache
modifies the data for another cached file.

To avoid this, modify the non-afs_InitCacheFile callers of
afs_GetNewDSlot to avoid afs_GetNewDSlot. Since these callers read
from the free/discard list, the contents of the dcache entries are not
valid (the cell, volume, dv, etc are not valid), though they must
exist on disk (we have a valid inode number for them). So, create a
new function, afs_GetUnusedDSlot, to get a dcache that must exist on
disk, but does not represent any valid data. Use this for callers that
must get a dslot from the free/discard list.

Add some comments to try and help explain what is going on.

Change-Id: I5b1b7ef4886102a9e145320932b2fd650b5c6f2f
Reviewed-on: http://gerrit.openafs.org/8370
Reviewed-by: Derrick Brashear <shadow@your-file-system.com>
Tested-by: BuildBot <buildbot@rampaginggeek.com>

src/afs/afs_chunkops.h
src/afs/afs_dcache.c
src/afs/afs_prototypes.h

index 1a4d38b..3babf89 100644 (file)
@@ -60,7 +60,7 @@ struct afs_cacheOps {
     int (*close) (struct osi_file * fp);
     int (*vreadUIO) (afs_dcache_id_t *, struct uio *);
     int (*vwriteUIO) (struct vcache *, afs_dcache_id_t *, struct uio *);
-    struct dcache *(*GetDSlot) (afs_int32 aslot, int needvalid);
+    struct dcache *(*GetDSlot) (afs_int32 aslot, int indexvalid, int datavalid);
     struct volume *(*GetVolSlot) (void);
     int (*HandleLink) (struct vcache * avc, struct vrequest * areq);
 };
@@ -71,11 +71,20 @@ struct afs_cacheOps {
 #define        afs_CFileRead(file, offset, data, size) (*(afs_cacheType->fread))(file, offset, data, size)
 #define        afs_CFileWrite(file, offset, data, size) (*(afs_cacheType->fwrite))(file, offset, data, size)
 #define        afs_CFileClose(handle)          (*(afs_cacheType->close))(handle)
-#define        afs_GetValidDSlot(slot) (*(afs_cacheType->GetDSlot))(slot, 1)
-#define        afs_GetNewDSlot(slot)   (*(afs_cacheType->GetDSlot))(slot, 0)
 #define        afs_GetVolSlot()                (*(afs_cacheType->GetVolSlot))()
 #define        afs_HandleLink(avc, areq)       (*(afs_cacheType->HandleLink))(avc, areq)
 
+/* Use afs_GetValidDSlot to get a dcache from a dcache slot number when we
+ * know the dcache contains data we want (e.g. it's on the hash table) */
+#define        afs_GetValidDSlot(slot) (*(afs_cacheType->GetDSlot))(slot, 1, 1)
+/* Use afs_GetUnusedDSlot when loading a dcache entry that is on the free or
+ * discard lists (the dcache does not contain valid data, but we know the
+ * dcache entry itself exists). */
+#define        afs_GetUnusedDSlot(slot)        (*(afs_cacheType->GetDSlot))(slot, 1, 0)
+/* Use afs_GetNewDSlot only when initializing dcache slots (the given slot
+ * number may not exist at all). */
+#define        afs_GetNewDSlot(slot)   (*(afs_cacheType->GetDSlot))(slot, 0, 0)
+
 /* These memcpys should get optimised to simple assignments when afs_dcache_id_t
  * is simple */
 static_inline void afs_copy_inode(afs_dcache_id_t *dst, afs_dcache_id_t *src) {
index 6dafb01..647f912 100644 (file)
@@ -1087,7 +1087,8 @@ afs_FreeDiscardedDCache(void)
     /*
      * Get an entry from the list of discarded cache elements
      */
-    tdc = afs_GetNewDSlot(afs_discardDCList);
+    tdc = afs_GetUnusedDSlot(afs_discardDCList);
+    osi_Assert(tdc);
     osi_Assert(tdc->refCount == 1);
     ReleaseReadLock(&tdc->tlock);
 
@@ -1507,7 +1508,8 @@ afs_AllocDCache(struct vcache *avc, afs_int32 chunk, afs_int32 lock,
        || ((lock & 2) && afs_freeDCList != NULLIDX)) {
 
        afs_indexFlags[afs_freeDCList] &= ~IFFree;
-       tdc = afs_GetNewDSlot(afs_freeDCList);
+       tdc = afs_GetUnusedDSlot(afs_freeDCList);
+       osi_Assert(tdc);
        osi_Assert(tdc->refCount == 1);
        ReleaseReadLock(&tdc->tlock);
        ObtainWriteLock(&tdc->lock, 604);
@@ -1515,7 +1517,8 @@ afs_AllocDCache(struct vcache *avc, afs_int32 chunk, afs_int32 lock,
        afs_freeDCCount--;
     } else {
        afs_indexFlags[afs_discardDCList] &= ~IFDiscarded;
-       tdc = afs_GetNewDSlot(afs_discardDCList);
+       tdc = afs_GetUnusedDSlot(afs_discardDCList);
+       osi_Assert(tdc);
        osi_Assert(tdc->refCount == 1);
        ReleaseReadLock(&tdc->tlock);
        ObtainWriteLock(&tdc->lock, 605);
@@ -2605,7 +2608,7 @@ afs_WriteThroughDSlots(void)
  */
 
 struct dcache *
-afs_MemGetDSlot(afs_int32 aslot, int needvalid)
+afs_MemGetDSlot(afs_int32 aslot, int indexvalid, int datavalid)
 {
     struct dcache *tdc;
     int existing = 0;
@@ -2626,10 +2629,10 @@ afs_MemGetDSlot(afs_int32 aslot, int needvalid)
        return tdc;
     }
 
-    /* if 'needvalid' is true, the slot must already exist and be populated
+    /* if 'indexvalid' is true, the slot must already exist and be populated
      * somewhere. for memcache, the only place that dcache entries exist is
      * in memory, so if we did not find it above, something is very wrong. */
-    osi_Assert(!needvalid);
+    osi_Assert(!indexvalid);
 
     if (!afs_freeDSList)
        afs_GetDownDSlot(4);
@@ -2690,13 +2693,20 @@ unsigned int last_error = 0, lasterrtime = 0;
  *
  * Parameters:
  *     aslot : Dcache slot to look at.
- *      needvalid : Whether the specified slot should already exist
+ *      indexvalid : 1 if we know the slot we're giving is valid, and thus
+ *                   reading the dcache from the disk index should succeed. 0
+ *                   if we are initializing a new dcache, and so reading from
+ *                   the disk index may fail.
+ *      datavalid : 0 if we are loading a dcache entry from the free or
+ *                  discard list, so we know the data in the given dcache is
+ *                  not valid. 1 if we are loading a known used dcache, so the
+ *                  data in the dcache must be valid.
  *
  * Environment:
  *     afs_xdcache lock write-locked.
  */
 struct dcache *
-afs_UFSGetDSlot(afs_int32 aslot, int needvalid)
+afs_UFSGetDSlot(afs_int32 aslot, int indexvalid, int datavalid)
 {
     afs_int32 code;
     struct dcache *tdc;
@@ -2759,7 +2769,7 @@ afs_UFSGetDSlot(afs_int32 aslot, int needvalid)
        last_error = code;
 #endif
        lasterrtime = osi_Time();
-       if (needvalid) {
+       if (indexvalid) {
            struct osi_stat tstat;
            if (afs_osi_Stat(afs_cacheInodep, &tstat)) {
                tstat.size = -1;
@@ -2779,19 +2789,19 @@ afs_UFSGetDSlot(afs_int32 aslot, int needvalid)
     }
     if (!afs_CellNumValid(tdc->f.fid.Cell)) {
        entryok = 0;
-       if (needvalid) {
+       if (datavalid) {
            osi_Panic("afs: needed valid dcache but index %d off %d has "
                      "invalid cell num %d\n",
                      (int)aslot, off, (int)tdc->f.fid.Cell);
        }
     }
 
-    if (needvalid && tdc->f.fid.Fid.Volume == 0) {
+    if (datavalid && tdc->f.fid.Fid.Volume == 0) {
        osi_Panic("afs: invalid zero-volume dcache entry at slot %d off %d",
                  (int)aslot, off);
     }
 
-    if (!entryok) {
+    if (!entryok || !datavalid) {
        tdc->f.fid.Cell = 0;
        tdc->f.fid.Fid.Volume = 0;
        tdc->f.chunk = -1;
index 1283d07..0c0f445 100644 (file)
@@ -252,7 +252,7 @@ extern void afs_FlushDCache(struct dcache *adc);
 extern void shutdown_dcache(void);
 extern void afs_CacheTruncateDaemon(void);
 extern afs_int32 afs_fsfragsize;
-extern struct dcache *afs_MemGetDSlot(afs_int32 aslot, int needvalid);
+extern struct dcache *afs_MemGetDSlot(afs_int32 aslot, int indexvalid, int datavalid);
 extern struct dcache *afs_GetDCache(struct vcache *avc,
                                    afs_size_t abyte,
                                    struct vrequest *areq,
@@ -274,7 +274,7 @@ extern void afs_TryToSmush(struct vcache *avc,
 extern void updateV2DC(int lockVc, struct vcache *v, struct dcache *d,
                       int src);
 extern void afs_WriteThroughDSlots(void);
-extern struct dcache *afs_UFSGetDSlot(afs_int32 aslot, int needvalid);
+extern struct dcache *afs_UFSGetDSlot(afs_int32 aslot, int indexvalid, int datavalid);
 extern int afs_WriteDCache(struct dcache *adc, int atime);
 extern int afs_wakeup(struct vcache *avc);
 extern int afs_InitCacheFile(char *afile, ino_t ainode);