STABLE14-dirbuffer-fid-is-index-20050119

"The new buffer code (which I wrote) did not deal
with dcache object re-use, as I had conflated the concepts of "dcache *
reuse" and "dcache slot reuse".

This patch should fix this problem. It now stores the dcache index (aka slot number,
which is the same as the numeric part of the cache file's filename) in the
buffer instead of the ephemeral struct dcache pointer."

(cherry picked from commit 8ccd2d91d89fc3ed0170a458853ec95ff274c87d)

diff --git a/src/afs/afs.h b/src/afs/afs.h
index c5087c62a1aa9d65de10e15db847627427b9b8ba..c3f3bd611b3f6a199394c5abc9422450b562eb90 100644 (file)
--- a/src/afs/afs.h
+++ b/src/afs/afs.h
@@ -1195,7 +1195,8 @@ struct afs_fakestat_state {
 extern int afs_fakestat_enable;
 
 struct buffer {
-    struct dcache *fid;
+    afs_int32 fid;             /* is adc->index, the cache file number */
+    afs_inode_t inode;         /* is adc->f.inode, the inode number of the cache file */
     afs_int32 page;
     afs_int32 accesstime;
     struct buffer *hashNext;

diff --git a/src/afs/afs_buffer.c b/src/afs/afs_buffer.c
index b8a366caf5a2c9a74af06442c08be6f260720516..a3c1a6dd9fdbac860d0ff2393ade6ad98acf461f 100644 (file)
--- a/src/afs/afs_buffer.c
+++ b/src/afs/afs_buffer.c
@@ -63,7 +63,7 @@ RCSID
 /* page hash table size - this is pretty intertwined with pHash */
 #define PHSIZE (PHPAGEMASK + PHFIDMASK + 1)
 /* the pHash macro */
-#define pHash(fid,page) ((((afs_int32)((fid)->f.inode)) & PHFIDMASK) \
+#define pHash(fid,page) ((((afs_int32)(fid)) & PHFIDMASK) \
                         | (page & PHPAGEMASK))
 
 #ifdef dirty
@@ -88,7 +88,7 @@ static int nbuffers;
 static afs_int32 timecounter;
 
 /* Prototypes for static routines */
-static struct buffer *afs_newslot(struct dcache * afid, afs_int32 apage,
+static struct buffer *afs_newslot(struct dcache *adc, afs_int32 apage,
                                  register struct buffer *lp);
 
 static int dinit_flag = 0;
@@ -130,7 +130,8 @@ DInit(int abuffers)
 #endif
        /* Fill in each buffer with an empty indication. */
        tb = &Buffers[i];
-       dirp_Zap(tb->fid);
+       tb->fid = 0;
+       tb->inode = 0;
        tb->accesstime = 0;
        tb->lockers = 0;
 #if AFS_USEBUFFERS
@@ -150,7 +151,7 @@ DInit(int abuffers)
 }
 
 void *
-DRead(register struct dcache * fid, register int page)
+DRead(register struct dcache *adc, register int page)
 {
     /* Read a page from the disk. */
     register struct buffer *tb, *tb2;
@@ -160,7 +161,7 @@ DRead(register struct dcache * fid, register int page)
     AFS_STATCNT(DRead);
     MObtainWriteLock(&afs_bufferLock, 256);
 
-#define bufmatch(tb) (tb->page == page && dirp_Eq(tb->fid, fid))
+#define bufmatch(tb) (tb->page == page && tb->fid == adc->index)
 #define buf_Front(head,parent,p) {(parent)->hashNext = (p)->hashNext; (p)->hashNext= *(head);*(head)=(p);}
 
     /* this apparently-complicated-looking code is simply an example of
@@ -169,7 +170,7 @@ DRead(register struct dcache * fid, register int page)
      * of larger code size.  This could be simplified by better use of
      * macros. 
      */
-    if ((tb = phTable[pHash(fid, page)])) {
+    if ((tb = phTable[pHash(adc->index, page)])) {
        if (bufmatch(tb)) {
            MObtainWriteLock(&tb->lock, 257);
            ReleaseWriteLock(&afs_bufferLock);
@@ -180,7 +181,7 @@ DRead(register struct dcache * fid, register int page)
            return tb->data;
        } else {
            register struct buffer **bufhead;
-           bufhead = &(phTable[pHash(fid, page)]);
+           bufhead = &(phTable[pHash(adc->index, page)]);
            while ((tb2 = tb->hashNext)) {
                if (bufmatch(tb2)) {
                    buf_Front(bufhead, tb, tb2);
@@ -216,7 +217,7 @@ DRead(register struct dcache * fid, register int page)
      * is at least the oldest buffer on one particular hash chain, so it's 
      * a pretty good place to start looking for the truly oldest buffer.
      */
-    tb = afs_newslot(fid, page, (tb ? tb : tb2));
+    tb = afs_newslot(adc, page, (tb ? tb : tb2));
     if (!tb) {
        MReleaseWriteLock(&afs_bufferLock);
        return NULL;
@@ -224,19 +225,21 @@ DRead(register struct dcache * fid, register int page)
     MObtainWriteLock(&tb->lock, 260);
     MReleaseWriteLock(&afs_bufferLock);
     tb->lockers++;
-    if (page * AFS_BUFFER_PAGESIZE >= fid->f.chunkBytes) {
-       dirp_Zap(tb->fid);
+    if (page * AFS_BUFFER_PAGESIZE >= adc->f.chunkBytes) {
+       tb->fid = 0;
+       tb->inode = 0;
        tb->lockers--;
        MReleaseWriteLock(&tb->lock);
        return NULL;
     }
-    tfile = afs_CFileOpen(fid->f.inode);
+    tfile = afs_CFileOpen(adc->f.inode);
     code =
        afs_CFileRead(tfile, tb->page * AFS_BUFFER_PAGESIZE, tb->data,
                      AFS_BUFFER_PAGESIZE);
     afs_CFileClose(tfile);
     if (code < AFS_BUFFER_PAGESIZE) {
-       dirp_Zap(tb->fid);
+       tb->fid = 0;
+       tb->inode = 0;
        tb->lockers--;
        MReleaseWriteLock(&tb->lock);
        return NULL;
@@ -273,7 +276,7 @@ FixupBucket(register struct buffer *ap)
 
 /* lp is pointer to a fairly-old buffer */
 static struct buffer *
-afs_newslot(struct dcache * afid, afs_int32 apage, register struct buffer *lp)
+afs_newslot(struct dcache *adc, afs_int32 apage, register struct buffer *lp)
 {
     /* Find a usable buffer slot */
     register afs_int32 i;
@@ -340,7 +343,8 @@ afs_newslot(struct dcache * afid, afs_int32 apage, register struct buffer *lp)
     }
 
     if (lp->dirty) {
-       tfile = afs_CFileOpen(lp->fid->f.inode);
+       /* see DFlush for rationale for not getting and locking the dcache */
+       tfile = afs_CFileOpen(lp->inode);
        afs_CFileWrite(tfile, lp->page * AFS_BUFFER_PAGESIZE, lp->data,
                       AFS_BUFFER_PAGESIZE);
        lp->dirty = 0;
@@ -349,7 +353,8 @@ afs_newslot(struct dcache * afid, afs_int32 apage, register struct buffer *lp)
     }
 
     /* Now fill in the header. */
-    dirp_Cpy(lp->fid, afid);   /* set this */
+    lp->fid = adc->index;
+    lp->inode = adc->f.inode;
     lp->page = apage;
     lp->accesstime = timecounter++;
     FixupBucket(lp);           /* move to the right hash bucket */
@@ -432,7 +437,7 @@ DVOffset(register void *ap)
  * method of DRead...
  */
 void
-DZap(struct dcache * fid)
+DZap(struct dcache *adc)
 {
     register int i;
     /* Destroy all buffers pertaining to a particular fid. */
@@ -442,10 +447,11 @@ DZap(struct dcache * fid)
     MObtainReadLock(&afs_bufferLock);
 
     for (i = 0; i <= PHPAGEMASK; i++)
-       for (tb = phTable[pHash(fid, i)]; tb; tb = tb->hashNext)
-           if (dirp_Eq(tb->fid, fid)) {
+       for (tb = phTable[pHash(adc->index, i)]; tb; tb = tb->hashNext)
+           if (tb->fid == adc->index) {
                MObtainWriteLock(&tb->lock, 262);
-               dirp_Zap(tb->fid);
+               tb->fid = 0;
+               tb->inode = 0;
                tb->dirty = 0;
                MReleaseWriteLock(&tb->lock);
            }
@@ -469,7 +475,18 @@ DFlush(void)
            tb->lockers++;
            MReleaseReadLock(&afs_bufferLock);
            if (tb->dirty) {
-               tfile = afs_CFileOpen(tb->fid->f.inode);
+               /* it seems safe to do this I/O without having the dcache
+                * locked, since the only things that will update the data in
+                * a directory are the buffer package, which holds the relevant
+                * tb->lock while doing the write, or afs_GetDCache, which 
+                * DZap's the directory while holding the dcache lock.
+                * It is not possible to lock the dcache or even call
+                * afs_GetDSlot to map the index to the dcache since the dir
+                * package's caller has some dcache object locked already (so
+                * we cannot lock afs_xdcache). In addition, we cannot obtain
+                * a dcache lock while holding the tb->lock of the same file
+                * since that can deadlock with DRead/DNew */
+               tfile = afs_CFileOpen(tb->inode);
                afs_CFileWrite(tfile, tb->page * AFS_BUFFER_PAGESIZE,
                               tb->data, AFS_BUFFER_PAGESIZE);
                tb->dirty = 0;  /* Clear the dirty flag */
@@ -484,20 +501,24 @@ DFlush(void)
 }
 
 void *
-DNew(register struct dcache * fid, register int page)
+DNew(register struct dcache *adc, register int page)
 {
     /* Same as read, only do *not* even try to read the page, since it probably doesn't exist. */
     register struct buffer *tb;
     AFS_STATCNT(DNew);
     MObtainWriteLock(&afs_bufferLock, 264);
-    if ((tb = afs_newslot(fid, page, NULL)) == 0) {
+    if ((tb = afs_newslot(adc, page, NULL)) == 0) {
        MReleaseWriteLock(&afs_bufferLock);
        return 0;
     }
     /* extend the chunk, if needed */
-    if ((page + 1) * AFS_BUFFER_PAGESIZE > fid->f.chunkBytes) {
-        afs_AdjustSize(fid, (page + 1) * AFS_BUFFER_PAGESIZE);
-        afs_WriteDCache(fid, 1);
+    /* Do it now, not in DFlush or afs_newslot when the data is written out,
+     * since now our caller has adc->lock writelocked, and we can't acquire
+     * that lock (or even map from a fid to a dcache) in afs_newslot or
+     * DFlush due to lock hierarchy issues */
+    if ((page + 1) * AFS_BUFFER_PAGESIZE > adc->f.chunkBytes) {
+       afs_AdjustSize(adc, (page + 1) * AFS_BUFFER_PAGESIZE);
+       afs_WriteDCache(adc, 1);
     }
     MObtainWriteLock(&tb->lock, 265);
     MReleaseWriteLock(&afs_bufferLock);

diff --git a/src/afs/afs_osi.h b/src/afs/afs_osi.h
index 9471ba8e16d9529bfae7b4f17fc938047a905fab..9eeb47a8e0b7369a32956a55b8017da6e6ed3148 100644 (file)
--- a/src/afs/afs_osi.h
+++ b/src/afs/afs_osi.h
@@ -183,15 +183,6 @@ typedef struct {
 typedef struct timeval osi_timeval_t;
 #endif /* AFS_SGI61_ENV */
 
-/*
- * The following three routines provide the fid routines used by the buffer
- * and directory packages.
- */
-#define dirp_Zap(afid)    ((afid) = 0)
-#define dirp_Eq(afid, bfid) ((afid) == (bfid))
-#define dirp_Cpy(dfid,sfid) ((dfid) = (sfid))
-
-
 /*
  * osi_ThreadUnique() should yield a value that can be found in ps
  * output in order to draw correspondences between ICL traces and what