path_lookup for 2.4.20-pre4 ([email protected])

This patch breaks NFS close-to-open cache consistency as it undoes those changes that provide path revalidation for the case of open(".").
The changelog entry doesn't even attempt to document this removal...

If people want to revert to the old behaviour, then there are ways of doing this that will not affect NFS. Something like the appended patch for instance...

Cheers,
Trond

diff -u --recursive --new-file linux-2.4.22-odirect/fs/namei.c linux-2.4.22-fix_cto/fs/namei.c
@@ -563,7 +563,7 @@
 	while (*name=='/')
 		name++;
 	if (!*name)
-		goto return_base;
+		goto return_reval;
 
 	inode = nd->dentry->d_inode;
 	if (current->link_count)

@@ -686,7 +686,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);

@@ -732,6 +732,24 @@
 			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.
+		 * We may need to check the cached dentry for staleness.
+		 */
+		if (nd->dentry && nd->dentry->d_sb &&
+		    (nd->dentry->d_sb->s_type->fs_flags & FS_REVAL_DOT)) {
+			struct dentry *dentry = nd->dentry;
+			unlock_nd(nd);
+			dput(pinned.dentry);
+			mntput(pinned.mnt);
+			if (!dentry->d_op->d_revalidate(dentry, 0)) {
+				d_invalidate(dentry);
+				path_release(nd);
+				return -ESTALE;
+			}
+			return 0;
+		}
 return_base:
 		unlock_nd(nd);
 		dput(pinned.dentry);

diff -u --recursive --new-file linux-2.4.22-odirect/fs/nfs/inode.c linux-2.4.22-fix_cto/fs/nfs/inode.c
@@ -1125,7 +1125,7 @@
 /*
  * File system information
  */
-static DECLARE_FSTYPE(nfs_fs_type, "nfs", nfs_read_super, FS_ODD_RENAME);
+static DECLARE_FSTYPE(nfs_fs_type, "nfs", nfs_read_super, FS_ODD_RENAME|FS_REVAL_DOT);
 
 extern int nfs_init_nfspagecache(void);
 extern void nfs_destroy_nfspagecache(void);

diff -u --recursive --new-file linux-2.4.22-odirect/include/linux/fs.h linux-2.4.22-fix_cto/include/linux/fs.h
@@ -92,6 +92,7 @@
 #define FS_SINGLE	8 /* Filesystem that can have only one superblock */
 #define FS_NOMOUNT	16 /* Never mount from userland */
 #define FS_LITTER	32 /* Keeps the tree in dcache */
+#define FS_REVAL_DOT	16384	/* Check the paths ".", ".." for staleness */
 #define FS_ODD_RENAME	32768	/* Temporary stuff; will go away as soon
 				  * as nfs_rename() will be cleaned up
 				  */

I was a little suprised it went in, it never seemed a candidate for dealing with a stable tree, just optimisation stuff that is 2.5 material only

> I was a little suprised it went in, it never seemed a candidate > for dealing with a stable tree, just optimisation stuff that is > 2.5 material only

As you can see, I screwed up on the title, so I may have confused you...
...but I do agree with your comment. The patch I meant to refer to (see revised title) does not appear in the 2.5.x tree either.

Have we BTW been shown any numbers that support the alleged benefits? I may have missed those...

Cheers,
Trond

A while ago yes - on very big SMP boxes.

Its no big problem to me since I can just back it out of -ac

just curious, because I use this patch since early 2.4.20, are there any reasons to 'back it out of -ac' for you?

anyway I totally agree that the NFS issue pointed out by Trond should be addressed ...

TIA,
Herbert

Its high risk, its got bugs as Trond already showed and it only helps performance on giant SMP boxes. Its all risk and no reward. Quota updates get you working 32bit uid quota and the interactivity stuff helps all even tho its got some risk.

every change (if it's not a bugfix, and even those) bear a risk, what I like about the fastwalk patch is not the performance gain on giant SMP boxes, because I do not have any (unfortunately ;) but the code change from ...

if (path_init(pathname, LOOKUP_PARENT, &nd))
error = path_walk(pathname, &nd);

to

        error = path_lookup(pathname,  LOOKUP_PARENT, &nd);

and

        dentry = cached_lookup(nd->dentry, &this, 0);
        if (!dentry) {
                dentry = real_lookup(nd->dentry, &this, 0);
                err = PTR_ERR(dentry);
                if (IS_ERR(dentry))
                         break;
        }

to

	err = do_lookup(nd, &this, &next, &pinned, 0);

which (at least for me) is more read-/understandable ...

anyway, thanks for you answer,
Herbert

exactly. 2.4 is not the place for cleanups that make the code easier to read because those cleanups can introduce new bugs.

"It's not Hollywood.  War is real, war is primarily not about defeat or
victory, it is about death.  I've seen thousands and thousands of dead bodies.
Do you think I want to have an academic debate on this subject?" -- Robert Fisk

Im not going to fight too hard for this. If people want it removed that is OK. I pushed it because dcache_rcu was not going to be accepted and Fastwalk is a fair second to dcache_rcu. The change Trond pointed out was added by Al Viro after fastwalk was included in 2.5.11 which I backported.

Hanna

> The change Trond pointed out was added by Al Viro > after fastwalk was included in 2.5.11 which I backported.

IIRC, Al vetoed the NFS cto change that went into 2.4.x because he claimed he was planning on providing an alternative fix that would better fit the unionfs.
As that apparently won't materialize in 2.6.x, I'm in any case planning on presenting the open(".") patch (or some variant of it) to Linus.

That said, just backing a bugfix out of a stable kernel without providing an alternative solution should only be done in those cases where keeping it would cause a worse problem. For instance if the fix is a security issue...

Cheers,
Trond

Then I apologize for my bad memory. I dont remember that thread. This fastwalk stuff was originally written over a year ago.

Please push your bugfix as you know the NFS area much better than I do.

Thanks.

Hanna

Ok, fine. Thats the feedback I wanted when I included it yesterday.

I'm going to revert it now.

Sorry, Hanna, but Alan saved the day again, and convinced me that fastwalk is indeed a 2.5 thing.

Alan,

I included fastwalk patch because I thought it was a stable and very useful optimisation. Even for 2.4. (I was expecting comments/flames on it when I first included it.

Now if you think it is a 2.5 thing and can cause any potential problem for 2.4 I will remove it.

One thing you might want to add to avoid revalidating all pathnames that happen to start with a dot, like '.bashrc'.

Jan

> One thing you might want to add to avoid revalidating all > pathnames that happen to start with a dot, like '.bashrc'.

Oops. Well caught! Revised patch would be as follows.

Cheers,
Trond

diff -u --recursive --new-file linux-2.4.22-odirect/fs/namei.c linux-2.4.22-fix_cto/fs/namei.c
@@ -563,7 +563,7 @@
 	while (*name=='/')
 		name++;
 	if (!*name)
-		goto return_base;
+		goto return_reval;
 
 	inode = nd->dentry->d_inode;
 	if (current->link_count)

@@ -686,7 +686,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);

@@ -732,6 +732,26 @@
 			nd->last_type = LAST_DOT;
 		else if (this.len == 2 && this.name[1] == '.')
 			nd->last_type = LAST_DOTDOT;
+		else
+			goto return_base;
+return_reval:
+		/*
+		 * We bypassed the ordinary revalidation routines.
+		 * We may need to check the cached dentry for staleness.
+		 */
+		if (nd->dentry && nd->dentry->d_sb &&
+		    (nd->dentry->d_sb->s_type->fs_flags & FS_REVAL_DOT)) {
+			struct dentry *dentry = nd->dentry;
+			unlock_nd(nd);
+			dput(pinned.dentry);
+			mntput(pinned.mnt);
+			if (!dentry->d_op->d_revalidate(dentry, 0)) {
+				d_invalidate(dentry);
+				path_release(nd);
+				return -ESTALE;
+			}
+			return 0;
+		}
 return_base:
 		unlock_nd(nd);
 		dput(pinned.dentry);

diff -u --recursive --new-file linux-2.4.22-odirect/fs/nfs/inode.c linux-2.4.22-fix_cto/fs/nfs/inode.c
@@ -1125,7 +1125,7 @@
 /*
  * File system information
  */
-static DECLARE_FSTYPE(nfs_fs_type, "nfs", nfs_read_super, FS_ODD_RENAME);
+static DECLARE_FSTYPE(nfs_fs_type, "nfs", nfs_read_super, FS_ODD_RENAME|FS_REVAL_DOT);
 
 extern int nfs_init_nfspagecache(void);
 extern void nfs_destroy_nfspagecache(void);

diff -u --recursive --new-file linux-2.4.22-odirect/include/linux/fs.h linux-2.4.22-fix_cto/include/linux/fs.h
@@ -92,6 +92,7 @@
 #define FS_SINGLE	8 /* Filesystem that can have only one superblock */
 #define FS_NOMOUNT	16 /* Never mount from userland */
 #define FS_LITTER	32 /* Keeps the tree in dcache */
+#define FS_REVAL_DOT	16384	/* Check the paths ".", ".." for staleness */
 #define FS_ODD_RENAME	32768	/* Temporary stuff; will go away as soon
 				  * as nfs_rename() will be cleaned up
 				  */