Re: [PATCH 2.4.19pre8][RFC] remove-NFS-close-to-open from VFS (was Re: [PATCHSET] 2.4.19-pre8-jp12)

[Jan Harkes <jaharkes@cs.cmu.edu>]

Hi,

I'm pulling up this old thread again, because there are some serious
issues with a patch that was merged in 2.4.19-pre3.

Basically it boils down to this, NFS had problems with '.' and '..'
lookups as they were bypassing the dcache revalidation/lookup. However,
the patch that got merged in 2.4.19-pre3 to fix this is causing some
significant problems, it changed the semantics of d_revalidate.

Originally, returning a error from d_revalidate in cached_lookup would
force a subsequent real_lookup. After the patch, returning an error from
d_revalidate propagates (in some cases) ESTALE back up to userspace.

NFS seems to do it's own replacement for real_lookup in dentry_revalidate,
but most other filesystems (like Samba) 'fixed' the problem by simply
revalidating the cached attributes of the object with the server. This
ofcourse breaks when an object is simply renamed, as the revalidation
will reinstantiate the old and incorrect lookup path for the object in
the dcache.

Now either every filesystem will have to follow NFS's lead and
implement some form of real_lookup inside of the dentry_revalidate
function which is not the prettiest solution. Or the VFS patch should be
reverted/fixed to actually walk the tree for '.' and '..' lookups.

btw. the patch also leaks dentries when a 'stale dentry' happens to have
children because it doesn't properly check and handle the d_invalidate
returncode.

Jan

For reference here is the thread from last May when Joerg Prante tracked
down his problems in supermount to the NFS patch.

    http://marc.theaimsgroup.com/?l=linux-kernel&m=102158542315351&w=2

Here is the 'controversial patch', cut-n-pasted out from linux.bkbits.net.

--- 1.19/fs/namei.c	Thu Feb 28 05:57:29 2002
+++ 1.20/fs/namei.c	Tue Mar 12 07:35:02 2002
@@ -457,7 +457,7 @@
    while (*name=='/')
	name++;
    if (!*name)
-		goto return_base;
+		goto return_reval;
 
    inode = nd->dentry->d_inode;
    if (current->link_count)
@@ -576,7 +576,7 @@
		inode = nd->dentry->d_inode;
		/* fallthrough */
	    case 1:
-				goto return_base;
+				goto return_reval;
	}
	if (nd->dentry->d_op && nd->dentry->d_op->d_hash) {
	    err = nd->dentry->d_op->d_hash(nd->dentry, &this);
@@ -627,6 +627,19 @@
	    nd->last_type = LAST_DOT;
	else if (this.len == 2 && this.name[1] == '.')
	    nd->last_type = LAST_DOTDOT;
+return_reval:
+		/*
+		 * We bypassed the ordinary revalidation routines.
+		 * Check the cached dentry for staleness.
+		 */
+		dentry = nd->dentry;
+		if (dentry && dentry->d_op && dentry->d_op->d_revalidate) {
+			err = -ESTALE;
+			if (!dentry->d_op->d_revalidate(dentry, 0)) {
+				d_invalidate(dentry);
+				break;
+			}
+		}
 return_base:
	return 0;
 out_dput: