Messages in this thread Patch in this message | | | Date | Sat, 12 Dec 1998 04:26:21 +0000 | From | Jamie Lokier <> | Subject | [PATCH] 2.1.131, NFS locking/cache coherency fixes |
| |
Linus,
Recently I provided a patch to fix some problems with NFS cache coherency and locking, which stopped mail from being lost amongst other things. In 2.1.131 you included a much smaller solution, and the main problem is now fixed. (I've tested it).
Your fix got me wondering if it was correct w.r.t. race conditions (as I had written much more paranoid code). This led me to through the twisty maze of NFS client code, net/sunrpc and eventually to the unbalanced lock_kernel() in rpciod(). A moment of enlightenment later. I feel I have advanced one more step down into the great cellar of kernel interlocking strategies. I see that the races I saw before were illusions.
But there are still problems with 2.1.131. Unless you're going to correct me again. See the enclosed patch. The changes are (in order):
nfs/file.c
1. When a process exits, don't leave a stale lock on the file if we happen to get a signal -- we're exiting, we're not going to handle the signal, no one else will clean up the lock.
This cleans up in case of accidents only; it doesn't affect applications behaving normally.
2. Important. Revalidate cached data immediately, while we still have the kernel lock. Otherwise async replies can mean we don't flush the data when we should.
nfs/inode.c
3. Optimisation. Eliminate a second redundant attribute request round trip after each cache invalidation.
4. Maintainability fix. No difference in effect at the moment, but play safe. I think it's clearer what's intended this way.
Alan, if Linus isn't around yet could you apply this in your tree? I think this makes the difference between safe NFS locking & unsafe NFS locking (specifically point 2).
Enjoy, -- Jamie
--- linux/./fs/nfs/file.c.nfslock Sun Dec 6 21:22:19 1998 +++ linux/./fs/nfs/file.c Sat Dec 12 04:19:16 1998 @@ -245,7 +245,7 @@ * with locks.. */ status = nfs_wb_all(inode); - if (status < 0) + if (status < 0 && !(current->flags & PF_EXITING)) return status; if ((status = nlmclnt_proc(inode, cmd, fl)) < 0) @@ -254,7 +254,13 @@ /* * Make sure we re-validate anything we've got cached. * This makes locking act as a cache coherency point. + * + * For coherency, must revalidate while we still have the + * kernel lock, as a later async reply can call + * nfs_refresh_inode() with info from before the lock. */ NFS_CACHEINV(inode); + _nfs_revalidate_inode(NFS_SERVER(inode), filp->f_dentry); + return 0; } --- linux/./fs/nfs/inode.c.nfslock Sun Dec 6 21:22:22 1998 +++ linux/./fs/nfs/inode.c Fri Dec 11 10:36:47 1998 @@ -706,6 +706,7 @@ NFS_ATTRTIMEO(inode) = NFS_MAXATTRTIMEO(inode); } NFS_OLDMTIME(inode) = fattr.mtime.seconds; + NFS_READTIME(inode) = jiffies; dfprintk(PAGECACHE, "NFS: %s/%s revalidation complete\n", dentry->d_parent->d_name.name, dentry->d_name.name); out: @@ -790,13 +791,13 @@ inode->i_atime = fattr->atime.seconds; inode->i_mtime = fattr->mtime.seconds; inode->i_ctime = fattr->ctime.seconds; + error = 0; + if (invalid) + goto out_invalid; /* * Update the read time so we don't revalidate too often. */ NFS_READTIME(inode) = jiffies; - error = 0; - if (invalid) - goto out_invalid; out: return error; - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.rutgers.edu Please read the FAQ at http://www.tux.org/lkml/
| |