Skip to content Skip to conversation

Should we expect close-to-open consistency on directories?

PATCH In review Neil Brown 3 participants 10 messages Apr 20, 2010 – May 10, 2010 1 message not in the archive

Subsystems (3)

VFS NFS client NFS server

Share & tools

Neil Brown

author

2010-04-20 07:22 UTC #0 lore raw

PATCH 1/1 Should we expect close-to-open consistency on directories?

Hi Trond et al,

It has come to my attention that NFS directories don't behave consistently in terms of cache consistency.

If, on the client, you have a loop like:

while true; do sleep 1; ls -l $dirname ; done

and then on the server you make changes to the named directory, there are some cases where you will see changes promptly and some where you wont.

In particular, if $dirname is '.' or the name of an NFS mountpoint, then changes can be delayed by up to acdirmax. If it is any other path, i.e. with a non-trivial path component that is in the NFS filesystem, then changes are seen promptly.

This seems to me to relate to "close to open" consistency. Of course with directories the 'close' side isn't relevant, but I still think it should be that when you open a directory it validates the 'change' attribute on that directory over the wire.

However the Linux VFS never tells NFS when a directory is opened. The current correct behaviour for most directories is achieved through d_revalidate == nfs_lookup_revalidate.

For '.' and mountpoints we need a different approach. Possibly the VFS could be changed to tell the filesystem when such a directory is opened. However I don't feel up to that at the moment.

An alternative is to do a revalidation in nfs_readdir as below. i.e. when readdir see f_pos == 0, it requests a revalidation of the page cache. This has two problems:
1/ a seek before the first read would cause the revalidation to be skipped.
This can be fixed by putting a similar test in nfs_llseek_dir, or maybe triggering off 'dir_cookie == NULL' rather than 'f_pos == 0'.
2/ A normal open/readdir sequence will validate a directory twice, once in the
lookup and once in the readdir. This is probably undesirable, but it is not clear to me how to fix it.

So: is it reasonable to view the current behaviour as 'wrong'?
any suggestions on how to craft a less problematic fix?

Thanks,
NeilBrown

fs/nfs/dir.c +3 -0

index a1f6b44..df4f0a6 100644
@@ -560,6 +560,9 @@ static int nfs_readdir(struct file *filp, void *dirent, filldir_t filldir)
 	desc->entry = &my_entry;
 
 	nfs_block_sillyrename(dentry);
+	if (filp->f_pos == 0)
+		/* Force attribute validity at open */
+		NFS_I(inode)->cache_validity |= NFS_INO_REVAL_PAGECACHE;
 	res = nfs_revalidate_mapping(inode, filp->f_mapping);
 	if (res < 0)
 		goto out;

Trond Myklebust

2010-04-20 13:02 UTC #1 lore raw

Quoted 22 lines · Neil Brown

Hi Trond et al,
It has come to my attention that NFS directories don't behave consistently in terms of cache consistency.
If, on the client, you have a loop like:
while true; do sleep 1; ls -l $dirname ; done
and then on the server you make changes to the named directory, there are some cases where you will see changes promptly and some where you wont.
In particular, if $dirname is '.' or the name of an NFS mountpoint, then changes can be delayed by up to acdirmax. If it is any other path, i.e. with a non-trivial path component that is in the NFS filesystem, then changes are seen promptly.
This seems to me to relate to "close to open" consistency. Of course with directories the 'close' side isn't relevant, but I still think it should be that when you open a directory it validates the 'change' attribute on that directory over the wire.

Quoted 7 lines

However the Linux VFS never tells NFS when a directory is opened. The current correct behaviour for most directories is achieved through d_revalidate == nfs_lookup_revalidate.
For '.' and mountpoints we need a different approach. Possibly the VFS could be changed to tell the filesystem when such a directory is opened. However I don't feel up to that at the moment.

I agree that mountpoints are problematic in this case, however why isn't '.' working correctly? Is the FS_REVAL_DOT mechanism broken?

The other thing is that we should definitely expect the VFS to call nfs_opendir() once it has opened the file.

Quoted 11 lines

An alternative is to do a revalidation in nfs_readdir as below. i.e. when readdir see f_pos == 0, it requests a revalidation of the page cache. This has two problems:
1/ a seek before the first read would cause the revalidation to be skipped.
This can be fixed by putting a similar test in nfs_llseek_dir, or maybe triggering off 'dir_cookie == NULL' rather than 'f_pos == 0'.
2/ A normal open/readdir sequence will validate a directory twice, once in the
lookup and once in the readdir. This is probably undesirable, but it is not clear to me how to fix it.

Quoted 2 lines

So: is it reasonable to view the current behaviour as 'wrong'?
any suggestions on how to craft a less problematic fix?

nfs_opendir() should fix case 1/, but still has the issue with case 2/. How about just having it force a revalidation if we see that this is a mountpoint?

Cheers
Trond

Neil Brown

author

2010-04-21 07:03 UTC #2 lore raw

Quoted 27 lines · On Tue, 20 Apr 2010 09:02:01 -0400 Trond Myklebust…

Quoted 26 lines · Neil Brown
Hi Trond et al,
It has come to my attention that NFS directories don't behave consistently in terms of cache consistency.
If, on the client, you have a loop like:
while true; do sleep 1; ls -l $dirname ; done
and then on the server you make changes to the named directory, there are some cases where you will see changes promptly and some where you wont.
In particular, if $dirname is '.' or the name of an NFS mountpoint, then changes can be delayed by up to acdirmax. If it is any other path, i.e. with a non-trivial path component that is in the NFS filesystem, then changes are seen promptly.
This seems to me to relate to "close to open" consistency. Of course with directories the 'close' side isn't relevant, but I still think it should be that when you open a directory it validates the 'change' attribute on that directory over the wire.
However the Linux VFS never tells NFS when a directory is opened. The current correct behaviour for most directories is achieved through d_revalidate == nfs_lookup_revalidate.

Quoted 6 lines

Quoted 3 lines
For '.' and mountpoints we need a different approach. Possibly the VFS could be changed to tell the filesystem when such a directory is opened. However I don't feel up to that at the moment.
I agree that mountpoints are problematic in this case, however why isn't '.' working correctly? Is the FS_REVAL_DOT mechanism broken?

Yes, the FS_REVAL_DOT mechanism is broken.
Specifically, when you open ".", ->d_revalidate is called by link_path_walk, but LOOKUP_PARENT is set, and LOOKUP_OPEN is not set, so nfs_lookup_verify_inode doesn't force a revalidate.

Then in do_last(), LOOKUP_PARENT is no longer set, and LOOKUP_OPEN is, but do_last doesn't bother calling ->d_revalidate for LAST_DOT.

I verified this understanding with the following patch which causes "ls ." to reliably get current (rather than cached) contents of the directory.

fs/namei.c +2 -2

index 48e60a1..f9204af 100644
@@ -1620,6 +1620,8 @@ static struct file *do_last(struct nameidata *nd, struct path *path,
 	switch (nd->last_type) {
 	case LAST_DOTDOT:
 		follow_dotdot(nd);
+		/* fallthrough */
+	case LAST_DOT:
 		dir = nd->path.dentry;
 		if (nd->path.mnt->mnt_sb->s_type->fs_flags & FS_REVAL_DOT) {
 			if (!dir->d_op->d_revalidate(dir, nd)) {

@@ -1627,8 +1629,6 @@ static struct file *do_last(struct nameidata *nd, struct path *path,
 				goto exit;
 			}
 		}
-		/* fallthrough */
-	case LAST_DOT:
 	case LAST_ROOT:
 		if (open_flag & O_CREAT)
 			goto exit;

Quoted 3 lines

The other thing is that we should definitely expect the VFS to call nfs_opendir() once it has opened the file.

Oh yes, I see that now. So we could force a cache revalidation there. But I'm not sure how to test if this is a mountpoint as you suggest below.

Maybe something like the following. I'm pretty sure this is wrong as it ignores the return value of d_revalidate, but I didn't know what to do with the value.
Al ??

fs/namei.c +5 -0

@@ -720,6 +720,11 @@ done:
 	path->mnt = mnt;
 	path->dentry = dentry;
 	__follow_mount(path);
+	if (path->dentry != dentry)
+		if (path->dentry && path->dentry->d_sb &&
+		    (path->dentry->d_sb->s_type->fs_flags & FS_REVAL_DOT))
+			path->dentry->d_op->d_revalidate(
+				path->dentry, nd);
 	return 0;
 
 need_lookup:

Thanks,
NeilBrown

Quoted 21 lines

Quoted 12 lines
An alternative is to do a revalidation in nfs_readdir as below. i.e. when readdir see f_pos == 0, it requests a revalidation of the page cache. This has two problems:
1/ a seek before the first read would cause the revalidation to be skipped.
This can be fixed by putting a similar test in nfs_llseek_dir, or maybe triggering off 'dir_cookie == NULL' rather than 'f_pos == 0'.
2/ A normal open/readdir sequence will validate a directory twice, once in the
lookup and once in the readdir. This is probably undesirable, but it is not clear to me how to fix it.
Quoted 2 lines
So: is it reasonable to view the current behaviour as 'wrong'?
any suggestions on how to craft a less problematic fix?
nfs_opendir() should fix case 1/, but still has the issue with case 2/. How about just having it force a revalidation if we see that this is a mountpoint?
Cheers
Trond

Neil Brown

author

2010-05-06 04:13 UTC #3 lore raw

Quoted 42 lines · On Wed, 21 Apr 2010 17:03:21 +1000 Neil Brown <[email protected]> wrote:

Quoted 27 lines · On Tue, 20 Apr 2010 09:02:01 -0400 Trond Myklebust…
Quoted 26 lines · Neil Brown
Hi Trond et al,
It has come to my attention that NFS directories don't behave consistently in terms of cache consistency.
If, on the client, you have a loop like:
while true; do sleep 1; ls -l $dirname ; done
and then on the server you make changes to the named directory, there are some cases where you will see changes promptly and some where you wont.
In particular, if $dirname is '.' or the name of an NFS mountpoint, then changes can be delayed by up to acdirmax. If it is any other path, i.e. with a non-trivial path component that is in the NFS filesystem, then changes are seen promptly.
This seems to me to relate to "close to open" consistency. Of course with directories the 'close' side isn't relevant, but I still think it should be that when you open a directory it validates the 'change' attribute on that directory over the wire.
However the Linux VFS never tells NFS when a directory is opened. The current correct behaviour for most directories is achieved through d_revalidate == nfs_lookup_revalidate.
Quoted 6 lines
Quoted 3 lines
For '.' and mountpoints we need a different approach. Possibly the VFS could be changed to tell the filesystem when such a directory is opened. However I don't feel up to that at the moment.
I agree that mountpoints are problematic in this case, however why isn't '.' working correctly? Is the FS_REVAL_DOT mechanism broken?
Yes, the FS_REVAL_DOT mechanism is broken.
Specifically, when you open ".", ->d_revalidate is called by link_path_walk, but LOOKUP_PARENT is set, and LOOKUP_OPEN is not set, so nfs_lookup_verify_inode doesn't force a revalidate.

Quoted 5 lines

Then in do_last(), LOOKUP_PARENT is no longer set, and LOOKUP_OPEN is, but do_last doesn't bother calling ->d_revalidate for LAST_DOT.
I verified this understanding with the following patch which causes "ls ." to reliably get current (rather than cached) contents of the directory.

No replies ... Maybe Al is busy.

I looked at this again, created a patch that I thought looked good and tested it to ensure it addressed both sides of the problem.

Does it look OK to you Trond?
Thanks.

NFS - ensure directory at end of path is always revalidated.

The FS_REVAL_DOT fs_type flag is meant to ensure that the final component of a path is always revalidated, even if it isn't a normal (LAST_NORM) path component (which is always revalidated).
There are two cases where this doesn't happen for NFS
One is where the last component is '.' as the revalidation happens while
LOOKUP_PARENT is set, so NFS ignores it (see nfs_lookup_check_intent).
The other is where the directory is a mountpoint, so it is LAST_NORM, but that directory is different from the mounted directory.

This patches fixes these two issues by
1/ extending do_last() to revalidate DOT as well as DOTDOT and 2/ extending do_lookup() to revalidate after a successful __follow_mount
if FS_REVAL_DOT is set.

Signed-off-by NeilBrown <[email protected]>

fs/namei.c +6 -2

index a7dce91..256ae13 100644
@@ -719,7 +719,11 @@ static int do_lookup(struct nameidata *nd, struct qstr *name,
 done:
 	path->mnt = mnt;
 	path->dentry = dentry;
-	__follow_mount(path);
+	if (__follow_mount(path) &&
+	    (path->mnt->mnt_sb->s_type->fs_flags & FS_REVAL_DOT)) {
+		if (!path->dentry->d_op->d_revalidate(path->dentry, nd))
+			return -ESTALE;
+	}
 	return 0;
 
 need_lookup:

@@ -1619,6 +1623,7 @@ static struct file *do_last(struct nameidata *nd, struct path *path,
 	switch (nd->last_type) {
 	case LAST_DOTDOT:
 		follow_dotdot(nd);
+	case LAST_DOT:
 		dir = nd->path.dentry;
 		if (nd->path.mnt->mnt_sb->s_type->fs_flags & FS_REVAL_DOT) {
 			if (!dir->d_op->d_revalidate(dir, nd)) {

@@ -1627,7 +1632,6 @@ static struct file *do_last(struct nameidata *nd, struct path *path,
 			}
 		}
 		/* fallthrough */
-	case LAST_DOT:
 	case LAST_ROOT:
 		if (open_flag & O_CREAT)
 			goto exit;

Quoted 79 lines

fs/namei.c +2 -2
index 48e60a1..f9204af 100644
@@ -1620,6 +1620,8 @@ static struct file *do_last(struct nameidata *nd, struct path *path,
 	switch (nd->last_type) {
 	case LAST_DOTDOT:
 		follow_dotdot(nd);
+		/* fallthrough */
+	case LAST_DOT:
 		dir = nd->path.dentry;
 		if (nd->path.mnt->mnt_sb->s_type->fs_flags & FS_REVAL_DOT) {
 			if (!dir->d_op->d_revalidate(dir, nd)) {
@@ -1627,8 +1629,6 @@ static struct file *do_last(struct nameidata *nd, struct path *path,
 				goto exit;
 			}
 		}
-		/* fallthrough */
-	case LAST_DOT:
 	case LAST_ROOT:
 		if (open_flag & O_CREAT)
 			goto exit;
Quoted 4 lines
The other thing is that we should definitely expect the VFS to call nfs_opendir() once it has opened the file.
Oh yes, I see that now. So we could force a cache revalidation there. But I'm not sure how to test if this is a mountpoint as you suggest below.
Maybe something like the following. I'm pretty sure this is wrong as it ignores the return value of d_revalidate, but I didn't know what to do with the value.
Al ??
fs/namei.c +5 -0
@@ -720,6 +720,11 @@ done:
 	path->mnt = mnt;
 	path->dentry = dentry;
 	__follow_mount(path);
+	if (path->dentry != dentry)
+		if (path->dentry && path->dentry->d_sb &&
+		    (path->dentry->d_sb->s_type->fs_flags & FS_REVAL_DOT))
+			path->dentry->d_op->d_revalidate(
+				path->dentry, nd);
 	return 0;
 
 need_lookup:
Thanks,
NeilBrown
Quoted 21 lines
Quoted 12 lines
An alternative is to do a revalidation in nfs_readdir as below. i.e. when readdir see f_pos == 0, it requests a revalidation of the page cache. This has two problems:
1/ a seek before the first read would cause the revalidation to be skipped.
This can be fixed by putting a similar test in nfs_llseek_dir, or maybe triggering off 'dir_cookie == NULL' rather than 'f_pos == 0'.
2/ A normal open/readdir sequence will validate a directory twice, once in the
lookup and once in the readdir. This is probably undesirable, but it is not clear to me how to fix it.
Quoted 2 lines
So: is it reasonable to view the current behaviour as 'wrong'?
any suggestions on how to craft a less problematic fix?
nfs_opendir() should fix case 1/, but still has the issue with case 2/. How about just having it force a revalidation if we see that this is a mountpoint?
Cheers
Trond

A message that is not in the archive (sent off-list or not yet received): [email protected]

Trond Myklebust

2010-05-06 13:58 UTC #5 lore raw

Quoted 84 lines · Neil Brown

Quoted 42 lines · On Wed, 21 Apr 2010 17:03:21 +1000 Neil Brown <[email protected]> wrote:
Quoted 27 lines · On Tue, 20 Apr 2010 09:02:01 -0400 Trond Myklebust…
Quoted 26 lines · Neil Brown
Hi Trond et al,
It has come to my attention that NFS directories don't behave consistently in terms of cache consistency.
If, on the client, you have a loop like:
while true; do sleep 1; ls -l $dirname ; done
and then on the server you make changes to the named directory, there are some cases where you will see changes promptly and some where you wont.
In particular, if $dirname is '.' or the name of an NFS mountpoint, then changes can be delayed by up to acdirmax. If it is any other path, i.e. with a non-trivial path component that is in the NFS filesystem, then changes are seen promptly.
This seems to me to relate to "close to open" consistency. Of course with directories the 'close' side isn't relevant, but I still think it should be that when you open a directory it validates the 'change' attribute on that directory over the wire.
However the Linux VFS never tells NFS when a directory is opened. The current correct behaviour for most directories is achieved through d_revalidate == nfs_lookup_revalidate.
Quoted 6 lines
Quoted 3 lines
For '.' and mountpoints we need a different approach. Possibly the VFS could be changed to tell the filesystem when such a directory is opened. However I don't feel up to that at the moment.
I agree that mountpoints are problematic in this case, however why isn't '.' working correctly? Is the FS_REVAL_DOT mechanism broken?
Yes, the FS_REVAL_DOT mechanism is broken.
Specifically, when you open ".", ->d_revalidate is called by link_path_walk, but LOOKUP_PARENT is set, and LOOKUP_OPEN is not set, so nfs_lookup_verify_inode doesn't force a revalidate.
Quoted 5 lines
Then in do_last(), LOOKUP_PARENT is no longer set, and LOOKUP_OPEN is, but do_last doesn't bother calling ->d_revalidate for LAST_DOT.
I verified this understanding with the following patch which causes "ls ." to reliably get current (rather than cached) contents of the directory.
No replies ... Maybe Al is busy.
I looked at this again, created a patch that I thought looked good and tested it to ensure it addressed both sides of the problem.
Does it look OK to you Trond?
Thanks.
NFS - ensure directory at end of path is always revalidated.
The FS_REVAL_DOT fs_type flag is meant to ensure that the final component of a path is always revalidated, even if it isn't a normal (LAST_NORM) path component (which is always revalidated).
There are two cases where this doesn't happen for NFS
One is where the last component is '.' as the revalidation happens while
LOOKUP_PARENT is set, so NFS ignores it (see nfs_lookup_check_intent).
The other is where the directory is a mountpoint, so it is LAST_NORM, but that directory is different from the mounted directory.
This patches fixes these two issues by
1/ extending do_last() to revalidate DOT as well as DOTDOT and 2/ extending do_lookup() to revalidate after a successful __follow_mount
if FS_REVAL_DOT is set.
Signed-off-by NeilBrown <[email protected]>
fs/namei.c +0 -0
index a7dce91..256ae13 100644

fs/namei.c static int do_lookup(struct nameidata *nd, struct qstr *name, · line 725

Quoted 8 lines

fs/namei.c +4 -1

@@ -719,7 +719,11 @@ static int do_lookup(struct nameidata *nd, struct qstr *name,
 done:
 	path->mnt = mnt;
 	path->dentry = dentry;
-	__follow_mount(path);
+	if (__follow_mount(path) &&
+	    (path->mnt->mnt_sb->s_type->fs_flags & FS_REVAL_DOT)) {
+		if (!path->dentry->d_op->d_revalidate(path->dentry, nd))
+			return -ESTALE;

Won't this prevent you from ever being able to unmount the stale filesystem?

Quoted 21 lines

fs/namei.c +2 -1

+	}
 	return 0;
 
 need_lookup:

@@ -1619,6 +1623,7 @@ static struct file *do_last(struct nameidata *nd, struct path *path,
 	switch (nd->last_type) {
 	case LAST_DOTDOT:
 		follow_dotdot(nd);
+	case LAST_DOT:
 		dir = nd->path.dentry;
 		if (nd->path.mnt->mnt_sb->s_type->fs_flags & FS_REVAL_DOT) {
 			if (!dir->d_op->d_revalidate(dir, nd)) {

@@ -1627,7 +1632,6 @@ static struct file *do_last(struct nameidata *nd, struct path *path,
 			}
 		}
 		/* fallthrough */
-	case LAST_DOT:
 	case LAST_ROOT:
 		if (open_flag & O_CREAT)
 			goto exit;

Neil Brown

author

2010-05-07 22:34 UTC #6 lore raw

Quoted 19 lines · On Thu, 06 May 2010 09:58:31 -0400 Trond Myklebust<[email protected]>…

Quoted 7 lines
fs/namei.c +0 -0
index a7dce91..256ae13 100644
fs/namei.c static int do_lookup(struct nameidata *nd, struct qstr *name, · line 725
Quoted 8 lines
fs/namei.c +4 -1
@@ -719,7 +719,11 @@ static int do_lookup(struct nameidata *nd, struct qstr *name,
 done:
 	path->mnt = mnt;
 	path->dentry = dentry;
-	__follow_mount(path);
+	if (__follow_mount(path) &&
+	    (path->mnt->mnt_sb->s_type->fs_flags & FS_REVAL_DOT)) {
+		if (!path->dentry->d_op->d_revalidate(path->dentry, nd))
+			return -ESTALE;
Won't this prevent you from ever being able to unmount the stale filesystem?

Good point - I think you are right.

It seems to me that ->d_revalidate is being used for two distinct, though related, tasks.
One is to revalidate the dentry - make sure the name still refers to the same inode. The other is to revalidate the inode - make sure the cached attributes are still valid. In NFS (v3 and v4 at least) these are both performed by one call so it makes some sense to combine them. But from the VFS perspective I would have thought they were quite separate. nfs_lookup_revalidate sometimes does a GETATTR, and sometimes does a LOOKUP, depending to some extent on the 'intent' in the nameidata. I find this makes it a bit hard to follow what is really happening, or how d_revalidate should really be used.

Maybe we should just ignore the return value above. Or maybe d_revalidate should never do a GETATTR - that should be done by ->open ??

Confused :-(

NeilBrown

Chuck Lever

2010-05-08 17:06 UTC #7 lore raw

Quoted 35 lines · Neil Brown

Quoted 19 lines · On Thu, 06 May 2010 09:58:31 -0400 Trond Myklebust<[email protected]>…
Quoted 7 lines
fs/namei.c +0 -0
index a7dce91..256ae13 100644
fs/namei.c static int do_lookup(struct nameidata *nd, struct qstr *name, · line 725
Quoted 8 lines
fs/namei.c +4 -1
@@ -719,7 +719,11 @@ static int do_lookup(struct nameidata *nd, struct qstr *name,
  done:
  	path->mnt = mnt;
  	path->dentry = dentry;
-	__follow_mount(path);
+	if (__follow_mount(path)&&
+	    (path->mnt->mnt_sb->s_type->fs_flags&  FS_REVAL_DOT)) {
+		if (!path->dentry->d_op->d_revalidate(path->dentry, nd))
+			return -ESTALE;
Won't this prevent you from ever being able to unmount the stale filesystem?
Good point - I think you are right.
It seems to me that ->d_revalidate is being used for two distinct, though related, tasks.
One is to revalidate the dentry - make sure the name still refers to the same inode. The other is to revalidate the inode - make sure the cached attributes are still valid. In NFS (v3 and v4 at least) these are both performed by one call so it makes some sense to combine them. But from the VFS perspective I would have thought they were quite separate. nfs_lookup_revalidate sometimes does a GETATTR, and sometimes does a LOOKUP, depending to some extent on the 'intent' in the nameidata. I find this makes it a bit hard to follow what is really happening, or how d_revalidate should really be used.

Quoted 4 lines

Maybe we should just ignore the return value above. Or maybe d_revalidate should never do a GETATTR - that should be done by ->open ??
Confused :-(

The reason for this arrangement is that by the time ->open is called, it's too late to return ESTALE. During the path walk, there is still an opportunity to renew the mapping between dentry and inode, should the cached value of that mapping be stale.

Signature · 1 line

chuck[dot]lever[at]oracle[dot]com

Neil Brown

author

2010-05-08 22:08 UTC #8 lore raw

Quoted 39 lines · On Sat, 08 May 2010 09:05:04 -0400 Chuck Lever <[email protected]> wrote:

Quoted 38 lines · Neil Brown
Quoted 19 lines · On Thu, 06 May 2010 09:58:31 -0400 Trond Myklebust<[email protected]>…
Quoted 7 lines
fs/namei.c +0 -0
index a7dce91..256ae13 100644
fs/namei.c static int do_lookup(struct nameidata *nd, struct qstr *name, · line 725
Quoted 8 lines
fs/namei.c +4 -1
@@ -719,7 +719,11 @@ static int do_lookup(struct nameidata *nd, struct qstr *name,
  done:
  	path->mnt = mnt;
  	path->dentry = dentry;
-	__follow_mount(path);
+	if (__follow_mount(path)&&
+	    (path->mnt->mnt_sb->s_type->fs_flags&  FS_REVAL_DOT)) {
+		if (!path->dentry->d_op->d_revalidate(path->dentry, nd))
+			return -ESTALE;
Won't this prevent you from ever being able to unmount the stale filesystem?
Good point - I think you are right.
It seems to me that ->d_revalidate is being used for two distinct, though related, tasks.
One is to revalidate the dentry - make sure the name still refers to the same inode. The other is to revalidate the inode - make sure the cached attributes are still valid. In NFS (v3 and v4 at least) these are both performed by one call so it makes some sense to combine them. But from the VFS perspective I would have thought they were quite separate. nfs_lookup_revalidate sometimes does a GETATTR, and sometimes does a LOOKUP, depending to some extent on the 'intent' in the nameidata. I find this makes it a bit hard to follow what is really happening, or how d_revalidate should really be used.
Maybe we should just ignore the return value above. Or maybe d_revalidate should never do a GETATTR - that should be done by ->open ??

Quoted 7 lines

Quoted 1 line
Confused :-(
The reason for this arrangement is that by the time ->open is called, it's too late to return ESTALE. During the path walk, there is still an opportunity to renew the mapping between dentry and inode, should the cached value of that mapping be stale.

Is it?
->open seems to be able to return an error code, and that error code seems to be propagated up. What am I missing?

I appreciate that a previous lookup might have already done the equivalent of 'open' (e.g. for O_EXCL) but it seems wrong to assume that the last lookup will always do the GETATTR required for open (because there isn't always a 'last lookup').
I would rather see the lookup (or d_revalidate) remember how much of an open it has done, and then ->open does the rest. i.e. it does a GETATTR if that hasn't already been done.
I don't think there is currently an easy way for 'open' to know how much work 'd_revalidate' or 'lookup' has done, though I suspect that could be fixed. Maybe pass (as pointer to) the whole intent structure into open rather than just the intent.open.file ??

Thanks,
NeilBrown

Chuck Lever

2010-05-10 02:31 UTC #9 lore raw

Quoted 50 lines · Neil Brown

Quoted 39 lines · On Sat, 08 May 2010 09:05:04 -0400 Chuck Lever<[email protected]> wrote:
Quoted 38 lines · Neil Brown
Quoted 19 lines · On Thu, 06 May 2010 09:58:31 -0400 Trond Myklebust<[email protected]>…
Quoted 7 lines
fs/namei.c +0 -0
index a7dce91..256ae13 100644
fs/namei.c static int do_lookup(struct nameidata *nd, struct qstr *name, · line 725
Quoted 8 lines
fs/namei.c +4 -1
@@ -719,7 +719,11 @@ static int do_lookup(struct nameidata *nd, struct qstr *name,
   done:
   	path->mnt = mnt;
   	path->dentry = dentry;
-	__follow_mount(path);
+	if (__follow_mount(path)&&
+	    (path->mnt->mnt_sb->s_type->fs_flags&   FS_REVAL_DOT)) {
+		if (!path->dentry->d_op->d_revalidate(path->dentry, nd))
+			return -ESTALE;
Won't this prevent you from ever being able to unmount the stale filesystem?
Good point - I think you are right.
It seems to me that ->d_revalidate is being used for two distinct, though related, tasks.
One is to revalidate the dentry - make sure the name still refers to the same inode. The other is to revalidate the inode - make sure the cached attributes are still valid. In NFS (v3 and v4 at least) these are both performed by one call so it makes some sense to combine them. But from the VFS perspective I would have thought they were quite separate. nfs_lookup_revalidate sometimes does a GETATTR, and sometimes does a LOOKUP, depending to some extent on the 'intent' in the nameidata. I find this makes it a bit hard to follow what is really happening, or how d_revalidate should really be used.
Maybe we should just ignore the return value above. Or maybe d_revalidate should never do a GETATTR - that should be done by ->open ??
Quoted 7 lines
Quoted 1 line
Confused :-(
The reason for this arrangement is that by the time ->open is called, it's too late to return ESTALE. During the path walk, there is still an opportunity to renew the mapping between dentry and inode, should the cached value of that mapping be stale.

Quoted 3 lines

Is it?
->open seems to be able to return an error code, and that error code seems to be propagated up. What am I missing?

"too late to return ESTALE" ... to the VFS layer. When d_revalidate returns ESTALE while a pathname is being resolved, our VFS layer re-walks the pathname from the root, passing in a flag that says "please don't use our cache, but do each LOOKUP again."

That way, we can rely on the dentry cache most of the time, and have some way to recover when file system objects are renamed on the server.

Quoted 4 lines

I appreciate that a previous lookup might have already done the equivalent of 'open' (e.g. for O_EXCL) but it seems wrong to assume that the last lookup will always do the GETATTR required for open (because there isn't always a 'last lookup').

open(".") has always been funky.

I had an argument years ago with someone who pointed out that the POSIX standards are written in such a way that it is entirely optional to revalidate ".", and so we don't. I don't recall the specifics.

Quoted 7 lines

I would rather see the lookup (or d_revalidate) remember how much of an open it has done, and then ->open does the rest. i.e. it does a GETATTR if that hasn't already been done.
I don't think there is currently an easy way for 'open' to know how much work 'd_revalidate' or 'lookup' has done, though I suspect that could be fixed. Maybe pass (as pointer to) the whole intent structure into open rather than just the intent.open.file ??

Neil Brown

author

2010-05-10 03:01 UTC #10 lore raw

Quoted 51 lines · On Sun, 09 May 2010 22:29:27 -0400 Chuck Lever <[email protected]> wrote:

Quoted 50 lines · Neil Brown
Quoted 46 lines · On Sat, 08 May 2010 09:05:04 -0400 Chuck Lever<[email protected]> wrote:
Quoted 35 lines · Neil Brown
Quoted 19 lines · On Thu, 06 May 2010 09:58:31 -0400 Trond Myklebust<[email protected]>…
Quoted 7 lines
fs/namei.c +0 -0
index a7dce91..256ae13 100644
fs/namei.c static int do_lookup(struct nameidata *nd, struct qstr *name, · line 725
Quoted 8 lines
fs/namei.c +4 -1
@@ -719,7 +719,11 @@ static int do_lookup(struct nameidata *nd, struct qstr *name,
   done:
   	path->mnt = mnt;
   	path->dentry = dentry;
-	__follow_mount(path);
+	if (__follow_mount(path)&&
+	    (path->mnt->mnt_sb->s_type->fs_flags&   FS_REVAL_DOT)) {
+		if (!path->dentry->d_op->d_revalidate(path->dentry, nd))
+			return -ESTALE;
Won't this prevent you from ever being able to unmount the stale filesystem?
Good point - I think you are right.
It seems to me that ->d_revalidate is being used for two distinct, though related, tasks.
One is to revalidate the dentry - make sure the name still refers to the same inode. The other is to revalidate the inode - make sure the cached attributes are still valid. In NFS (v3 and v4 at least) these are both performed by one call so it makes some sense to combine them. But from the VFS perspective I would have thought they were quite separate. nfs_lookup_revalidate sometimes does a GETATTR, and sometimes does a LOOKUP, depending to some extent on the 'intent' in the nameidata. I find this makes it a bit hard to follow what is really happening, or how d_revalidate should really be used.
Quoted 4 lines
Maybe we should just ignore the return value above. Or maybe d_revalidate should never do a GETATTR - that should be done by ->open ??
Confused :-(
The reason for this arrangement is that by the time ->open is called, it's too late to return ESTALE. During the path walk, there is still an opportunity to renew the mapping between dentry and inode, should the cached value of that mapping be stale.

Quoted 8 lines

Quoted 3 lines
Is it?
->open seems to be able to return an error code, and that error code seems to be propagated up. What am I missing?
"too late to return ESTALE" ... to the VFS layer. When d_revalidate returns ESTALE while a pathname is being resolved, our VFS layer re-walks the pathname from the root, passing in a flag that says "please don't use our cache, but do each LOOKUP again."

I see. path_walk does that, the flag being "LOOKUP_REVAL".
But this is just saying that 'open' is too late to validate a name, and I agree with that. I want 'open' to revalidate the cached attributes - particularly in the case where there is no name to revalidated. i.e. a mount point or '.'. In those cases we would want to return ESTALE, not try to redo the lookup (because there is no lookup to do).

Quoted 9 lines

That way, we can rely on the dentry cache most of the time, and have some way to recover when file system objects are renamed on the server.
Quoted 4 lines
I appreciate that a previous lookup might have already done the equivalent of 'open' (e.g. for O_EXCL) but it seems wrong to assume that the last lookup will always do the GETATTR required for open (because there isn't always a 'last lookup').

Quoted 5 lines

open(".") has always been funky.
I had an argument years ago with someone who pointed out that the POSIX standards are written in such a way that it is entirely optional to revalidate ".", and so we don't. I don't recall the specifics.

The rationale I fall back on is that to preserve "close-to-open-coherency" you need to get the change attribute (aka mtime) when you open something - a directory in this case.
This isn't so much "revalidating" as "refreshing".
So prima-facie 'open' should always issue a GETATTR and check if the file has changed. In practice that isn't necessary in most cases as a d_revalidate has issued a LOOKUP or a GETATTR and the cached attributes are current. But in the cases where there is no dentry to revalidate (mountpoint and "."), something more is needed.

NeilBrown

Quoted 12 lines

Quoted 8 lines
I would rather see the lookup (or d_revalidate) remember how much of an open it has done, and then ->open does the rest. i.e. it does a GETATTR if that hasn't already been done.
I don't think there is currently an easy way for 'open' to know how much work 'd_revalidate' or 'lookup' has done, though I suspect that could be fixed. Maybe pass (as pointer to) the whole intent structure into open rather than just the intent.open.file ??
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html