From 20b0c65a289e2b55fb6922c8f60e873f1f4c6f97 Mon Sep 17 00:00:00 2001 From: Andrew Deason Date: Wed, 31 Oct 2012 15:55:35 -0500 Subject: [PATCH 1/1] afs: Never use GetNewDSlot after init 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 Tested-by: BuildBot --- src/afs/afs_chunkops.h | 15 ++++++++++++--- src/afs/afs_dcache.c | 34 ++++++++++++++++++++++------------ src/afs/afs_prototypes.h | 4 ++-- 3 files changed, 36 insertions(+), 17 deletions(-) diff --git a/src/afs/afs_chunkops.h b/src/afs/afs_chunkops.h index 1a4d38b..3babf89 100644 --- a/src/afs/afs_chunkops.h +++ b/src/afs/afs_chunkops.h @@ -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) { diff --git a/src/afs/afs_dcache.c b/src/afs/afs_dcache.c index 6dafb01..647f912 100644 --- a/src/afs/afs_dcache.c +++ b/src/afs/afs_dcache.c @@ -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; diff --git a/src/afs/afs_prototypes.h b/src/afs/afs_prototypes.h index 1283d07..0c0f445 100644 --- a/src/afs/afs_prototypes.h +++ b/src/afs/afs_prototypes.h @@ -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); -- 1.9.4