Re: Stale NFS handles on 2.4.2

[Trond Myklebust]

>>>>> " " == Neil Brown <[e-mail]> writes:

     > In short, I really don't think that NFS_INO_STALE (or any other
     > item if information received from the server) should be
     > considered to be permanent and never rechecked.

There are 2 problems of inode corruption if you allow inodes to die
and then reappear at will:

  1) is that several servers, including the userspace nfsd, have
     problems with filehandle reuse. You do not want to fall victim
     either to corruption either of the inode cache or (worse) the
     file itself.

     In the same vein, consider all these horror stories about people
     sharing CDROMs over NFS, mounting/umounting them at will...

  2) Even on servers that strictly respect the uniqueness of
     filehandles you can risk cache/file corruption due to inode
     aliasing when for instance you are using shared mmap between 2
     processes.

     For instance: under a lookup() operation, the client notices that
     the inode is stale, creates a new one (after unhashing the old
     one of course but otherwise not invalidating it).
     If the old inode is then magically declared to be OK, you
     suddenly have 2 inodes with 2 different caches that are
     supposed to represent the same file.

Of course, the second problem is not really relevant to the root
directory inode (which should never get aliased anyway). Perhaps the
correct thing to do would be to allow root inodes to clear the
NFS_INO_STALE flag? Something like the following...

Cheers,
   Trond

--- linux-2.4.2/fs/nfs/inode.c.orig	Wed Feb 14 01:14:28 2001
+++ linux-2.4.2/fs/nfs/inode.c	Thu Mar  1 02:08:59 2001
@@ -819,24 +819,22 @@
 int
 __nfs_revalidate_inode(struct nfs_server *server, struct inode *inode)
 {
-	int		 status = 0;
+	int		 status = -ESTALE;
 	struct nfs_fattr fattr;
 
 	dfprintk(PAGECACHE, "NFS: revalidating (%x/%Ld)\n",
 		inode->i_dev, (long long)NFS_FILEID(inode));
 
 	lock_kernel();
-	if (!inode || is_bad_inode(inode) || NFS_STALE(inode)) {
-		unlock_kernel();
-		return -ESTALE;
-	}
+	if (!inode || is_bad_inode(inode))
+ 		goto out_nowait;
+	if (NFS_STALE(inode) && inode != inode->i_sb->s_root->d_inode)
+ 		goto out_nowait;
 
 	while (NFS_REVALIDATING(inode)) {
 		status = nfs_wait_on_inode(inode, NFS_INO_REVALIDATING);
-		if (status < 0) {
-			unlock_kernel();
-			return status;
-		}
+		if (status < 0)
+			goto out_nowait;
 		if (time_before(jiffies,NFS_READTIME(inode)+NFS_ATTRTIMEO(inode))) {
 			status = NFS_STALE(inode) ? -ESTALE : 0;
 			goto out_nowait;
@@ -850,7 +848,8 @@
 			 inode->i_dev, (long long)NFS_FILEID(inode), status);
 		if (status == -ESTALE) {
 			NFS_FLAGS(inode) |= NFS_INO_STALE;
-			remove_inode_hash(inode);
+			if (inode != inode->i_sb->s_root->d_inode)
+				remove_inode_hash(inode);
 		}
 		goto out;
 	}
@@ -863,6 +862,8 @@
 	}
 	dfprintk(PAGECACHE, "NFS: (%x/%Ld) revalidation complete\n",
 		inode->i_dev, (long long)NFS_FILEID(inode));
+
+	NFS_FLAGS(inode) &= ~NFS_INO_STALE;
 out:
 	NFS_FLAGS(inode) &= ~NFS_INO_REVALIDATING;
 	wake_up(&inode->i_wait);