[PATCH] fix nfsidem cthon test

[PATCH] fix nfsidem cthon test

[Greg Banks]

[-- Attachment #1: Type: text/plain, Size: 241 bytes --]

G'day,

The attached patch forward ports from 2.4 the fix to nfs_rename()
which makes the nfsidem test in the Connectathon test suite pass.

Greg.
-- 
Greg Banks, R&D Software Engineer, SGI Australian Software Group.
I don't speak for SGI.


[-- Attachment #2: gnb-nfs-rename --]
[-- Type: text/plain, Size: 1015 bytes --]

Fix nfs_rename() so the nfsidem test in the Connectathon test suite passes.


Signed-off-by: Greg Banks <gnb@melbourne.sgi.com>


Index: linux/fs/nfs/dir.c
===================================================================
--- linux.orig/fs/nfs/dir.c	2004-10-04 14:35:26.%N +1000
+++ linux/fs/nfs/dir.c	2004-10-21 15:45:21.%N +1000
@@ -1409,7 +1409,7 @@ static int nfs_rename(struct inode *old_
 	struct inode *old_inode = old_dentry->d_inode;
 	struct inode *new_inode = new_dentry->d_inode;
 	struct dentry *dentry = NULL, *rehash = NULL;
-	int error = -EBUSY;
+	int error;
 
 	/*
 	 * To prevent any new references to the target during the rename,
@@ -1436,6 +1436,12 @@ static int nfs_rename(struct inode *old_
 	 */
 	if (!new_inode)
 		goto go_ahead;
+	/* If target is a hard link to the source, then noop */
+	error = 0;
+	if (NFS_FILEID(new_inode) == NFS_FILEID(old_inode))
+		goto out;
+
+	error = -EBUSY;
 	if (S_ISDIR(new_inode->i_mode))
 		goto out;
 	else if (atomic_read(&new_dentry->d_count) > 1) {

Re: [PATCH] fix nfsidem cthon test

[Trond Myklebust]

to den 21.10.2004 Klokka 16:51 (+1000) skreiv Greg Banks:
> G'day,
> 
> The attached patch forward ports from 2.4 the fix to nfs_rename()
> which makes the nfsidem test in the Connectathon test suite pass.


Vetoed.

This remains a server bug, not a client bug. The default should NOT be
no_subtree_check.

Besides, that patch will screw up on "nohide" partitions which may
indeed cause 2 different files to have the same fileid.

Cheers,
  Trond

-- 
Trond Myklebust <trond.myklebust@fys.uio.no>



-------------------------------------------------------
This SF.net email is sponsored by: IT Product Guide on ITManagersJournal
Use IT products in your business? Tell us what you think of them. Give us
Your Opinions, Get Free ThinkGeek Gift Certificates! Click to find out more
http://productguide.itmanagersjournal.com/guidepromo.tmpl
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [PATCH] fix nfsidem cthon test

[Greg Banks]

On Thu, 2004-10-21 at 17:31, Trond Myklebust wrote:
> to den 21.10.2004 Klokka 16:51 (+1000) skreiv Greg Banks:
> > G'day,
> > 
> > The attached patch forward ports from 2.4 the fix to nfs_rename()
> > which makes the nfsidem test in the Connectathon test suite pass.
> 
> 
> Vetoed.

I don't get it.  Why is this patch in 2.4?

Greg.
-- 
Greg Banks, R&D Software Engineer, SGI Australian Software Group.
I don't speak for SGI.




-------------------------------------------------------
This SF.net email is sponsored by: IT Product Guide on ITManagersJournal
Use IT products in your business? Tell us what you think of them. Give us
Your Opinions, Get Free ThinkGeek Gift Certificates! Click to find out more
http://productguide.itmanagersjournal.com/guidepromo.tmpl
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [PATCH] fix nfsidem cthon test

[Trond Myklebust]

to den 21.10.2004 Klokka 17:51 (+1000) skreiv Greg Banks:
> > 
> > Vetoed.
> 
> I don't get it.  Why is this patch in 2.4?

It slipped in. That's not a reason to make the same mistake in 2.6.

Cheers,
  Trond

-- 
Trond Myklebust <trond.myklebust@fys.uio.no>



-------------------------------------------------------
This SF.net email is sponsored by: IT Product Guide on ITManagersJournal
Use IT products in your business? Tell us what you think of them. Give us
Your Opinions, Get Free ThinkGeek Gift Certificates! Click to find out more
http://productguide.itmanagersjournal.com/guidepromo.tmpl
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: Re: [PATCH] fix nfsidem cthon test

[J. Bruce Fields]

On Thu, Oct 21, 2004 at 09:31:31AM +0200, Trond Myklebust wrote:
> to den 21.10.2004 Klokka 16:51 (+1000) skreiv Greg Banks:
> > G'day,
> > 
> > The attached patch forward ports from 2.4 the fix to nfs_rename()
> > which makes the nfsidem test in the Connectathon test suite pass.
> 
> 
> Vetoed.
> 
> This remains a server bug, not a client bug. The default should NOT be
> no_subtree_check.

We could do something like this for v4, though, couldn't we?

(I'm looking at rfc3530 section 9.3, which allows the client to assume
two files are the same if the server returns the same fileid and fsid
attributes for the two filehandles and doesn't return TRUE for
unique_handles on either.)

--b.


-------------------------------------------------------
This SF.net email is sponsored by: IT Product Guide on ITManagersJournal
Use IT products in your business? Tell us what you think of them. Give us
Your Opinions, Get Free ThinkGeek Gift Certificates! Click to find out more
http://productguide.itmanagersjournal.com/guidepromo.tmpl
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: Re: [PATCH] fix nfsidem cthon test

[Greg Banks]

[-- Attachment #1: Type: text/plain, Size: 2370 bytes --]

G'day,

[compendium reply]

On Thu, 2004-10-21 at 17:31, Trond Myklebust wrote: 
> to den 21.10.2004 Klokka 16:51 (+1000) skreiv Greg Banks:
> > G'day,
> > 
> > The attached patch forward ports from 2.4 the fix to nfs_rename()
> > which makes the nfsidem test in the Connectathon test suite pass.
> 
> 
> Vetoed.
> 
> This remains a server bug, not a client bug.

Agreed.  However we work around other server bugs, for example
IRIX server bugs which have been fixed for years and are not
seen on any SGI-supported IRIX box.  Why should a Linux server
bug be any different?

>  The default should NOT be
> no_subtree_check.

The default is "subtree_check".

> Besides, that patch will screw up on "nohide" partitions which may
> indeed cause 2 different files to have the same fileid.

Yes, and I should have seen that...the attached patch deals
with nohide by comparing the whole file handle instead of just
the fileid.

On Thu, 2004-10-21 at 17:40, Trond Myklebust wrote: 
> to den 21.10.2004 Klokka 17:51 (+1000) skreiv Greg Banks:
> > > 
> > > Vetoed.
> > 
> > I don't get it.  Why is this patch in 2.4?
> 
> It slipped in. That's not a reason to make the same mistake in 2.6.

Ok, let's slip it out again.  How about, when this is resolved, I
submit a patch which syncs 2.4 to the 2.6 solution?

On Fri, 2004-10-22 at 02:13, J. Bruce Fields wrote:
> We could do something like this for v4, though, couldn't we?
> 
> (I'm looking at rfc3530 section 9.3, which allows the client to assume
> two files are the same if the server returns the same fileid and fsid
> attributes for the two filehandles and doesn't return TRUE for
> unique_handles on either.)

While we're digging in RFCs, rfc1813 section 2.6 says:
> The client stores file handles for use in a later request and
> can compare two file handles from the same server for equality
> by doing a byte-by-byte comparison, but cannot otherwise interpret
> the contents of file handles. If two file handles from the same
> server are equal, they must refer to the same file, but if they
> are not equal, no conclusions can be drawn. [...] Clients should
> use file handle comparisons only to improve performance, not for
> correct behavior.

I think this justifies the apparoach in the attached patch for v3 too.

Greg.
-- 
Greg Banks, R&D Software Engineer, SGI Australian Software Group.
I don't speak for SGI.


[-- Attachment #2: gnb-nfs-rename-2 --]
[-- Type: text/plain, Size: 1215 bytes --]

Fix nfs_rename() so the nfsidem test in the Connectathon test suite passes.


Signed-off-by: Greg Banks <gnb@melbourne.sgi.com>


Index: linux/fs/nfs/dir.c
===================================================================
--- linux.orig/fs/nfs/dir.c	2004-10-04 14:35:26.%N +1000
+++ linux/fs/nfs/dir.c	2004-10-22 09:59:30.%N +1000
@@ -1409,7 +1409,7 @@ static int nfs_rename(struct inode *old_
 	struct inode *old_inode = old_dentry->d_inode;
 	struct inode *new_inode = new_dentry->d_inode;
 	struct dentry *dentry = NULL, *rehash = NULL;
-	int error = -EBUSY;
+	int error;
 
 	/*
 	 * To prevent any new references to the target during the rename,
@@ -1436,6 +1436,18 @@ static int nfs_rename(struct inode *old_
 	 */
 	if (!new_inode)
 		goto go_ahead;
+	/*
+	 * If target is a hard link to the source, then noop.
+	 * We compare the entire file handle instead of just
+	 * the fileid to handle the nohide case.   This test
+	 * is supposed to happen on the server, but some servers
+	 * are buggy.
+	 */
+	error = 0;
+	if (nfs_compare_fh(NFS_FH(new_inode), NFS_FH(old_inode)))
+		goto out;
+
+	error = -EBUSY;
 	if (S_ISDIR(new_inode->i_mode))
 		goto out;
 	else if (atomic_read(&new_dentry->d_count) > 1) {

Re: Re: [PATCH] fix nfsidem cthon test

[J. Bruce Fields]

On Fri, Oct 22, 2004 at 12:56:21PM +1000, Greg Banks wrote:
> Yes, and I should have seen that...the attached patch deals
> with nohide by comparing the whole file handle instead of just
> the fileid.

Weird.  But I thought the bug the cthon test was seeing was the result
of the server generating two different filehandles pointing to the same
file?  (And if that isn't the problem, then why was turning on
no_subtree_check also making the test pass?)

--b.


-------------------------------------------------------
This SF.net email is sponsored by: IT Product Guide on ITManagersJournal
Use IT products in your business? Tell us what you think of them. Give us
Your Opinions, Get Free ThinkGeek Gift Certificates! Click to find out more
http://productguide.itmanagersjournal.com/guidepromo.tmpl
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: Re: [PATCH] fix nfsidem cthon test

[Greg Banks]

On Fri, 2004-10-22 at 12:43, J. Bruce Fields wrote:
> On Fri, Oct 22, 2004 at 12:56:21PM +1000, Greg Banks wrote:
> > Yes, and I should have seen that...the attached patch deals
> > with nohide by comparing the whole file handle instead of just
> > the fileid.
> 
> Weird.  But I thought the bug the cthon test was seeing was the result
> of the server generating two different filehandles pointing to the same
> file?  (And if that isn't the problem, then why was turning on
> no_subtree_check also making the test pass?)

You're right, this "feature" of subtree_check is the cause of the
problem.  My patch comparing entire filehandles is useless when the
server does things like that.  <sigh> I shouldn't have just extended
the working patch without retesting...

The test works against an IRIX server because the IRIX server always
does the equivalent of no_subtree_check, or at least the filehandles
are the same for two links to the the same inode with different parents.

This strikes me as eminently sensible behaviour, and rfc1813 agrees
but doesn't mandate it: "Servers should try to maintain a one-to-one
correspondence between file handles and files, but this is not
required".  So technically the server is behaving within the limits
of the RFC, and clients are supposed to be able to deal with it.

OTOH the server behaviour is stupid, because it results in two separate
client-side inodes and the resulting possibility of data corruption,
and not just in the Linux client.

For example, the IRIX client is only passing the test by virtue of a
client-side accident.  It appears a side effect of the link() syscall
preceeding the rename() avoids the LOOKUP call and so the client thinks
the files are the same inode and elides the RENAME call.  If just the
rename() syscall is done on a fresh mount, the IRIX client thinks
the files are different and does a RENAME call, which behaves
correctly on the server.

In other words, I really don't think a server should behave the way,
it's too confusing for clients.

Trond, I don't think it's a good idea to make no_subtree_check the
default, as this would have the undesirable side effect of defeating
directory permission checks.  Perhaps we can just make it behave
more helpfully.

Neil, is there a good reason why knfsd encodes the parent inode in the
fileid under all circumstances?  At least some filesystems (XFS, ext3)
provide apparently useful export_operations.get_parent() functions
which could be used in find_exported_dentry() instead of sending this
information out in the fileid.  This would also free up some space to
encode 64 bit inode numbers (which my hidden agenda).

Greg.
-- 
Greg Banks, R&D Software Engineer, SGI Australian Software Group.
I don't speak for SGI.




-------------------------------------------------------
This SF.net email is sponsored by: IT Product Guide on ITManagersJournal
Use IT products in your business? Tell us what you think of them. Give us
Your Opinions, Get Free ThinkGeek Gift Certificates! Click to find out more
http://productguide.itmanagersjournal.com/guidepromo.tmpl
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: Re: [PATCH] fix nfsidem cthon test

[Trond Myklebust]

m=E5 den 25.10.2004 Klokka 19:33 (+1000) skreiv Greg Banks:

> Trond, I don't think it's a good idea to make no_subtree_check the
> default, as this would have the undesirable side effect of defeating
> directory permission checks.  Perhaps we can just make it behave
> more helpfully.

If by "make it behave more helpfully" you mean get rid of the aliased
filehandles, then I'm fine with that. I'm curious to see how you propose
to do it though: as far as I can see there is no way to reconcile the
goal of proper directory permission checks and sane filehandle
behaviour.

Since the permissions problem is solvable in other ways (i.e. by only
exporting partitions/volumes), and in fact that is the way all other
known NFS server implementations expect you to solve the problem, then I
see no problems with making that the default.

A server that doesn't encourage basic data integrity tends to be frowned
upon.

> Neil, is there a good reason why knfsd encodes the parent inode in the
> fileid under all circumstances?  At least some filesystems (XFS, ext3)
> provide apparently useful export_operations.get_parent() functions
> which could be used in find_exported_dentry() instead of sending this
> information out in the fileid.  This would also free up some space to
> encode 64 bit inode numbers (which my hidden agenda).

Look more closely: get_parent() expects the parameter to be a directory.
I don't think ext2/3 or XFS encode a list of parent directories in the
on-disk inode for a regular file.

Cheers,
 Trond

--=20
Trond Myklebust <trond.myklebust@fys.uio.no>



-------------------------------------------------------
This SF.net email is sponsored by: IT Product Guide on ITManagersJournal
Use IT products in your business? Tell us what you think of them. Give us
Your Opinions, Get Free ThinkGeek Gift Certificates! Click to find out more
http://productguide.itmanagersjournal.com/guidepromo.tmpl
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: Re: [PATCH] fix nfsidem cthon test

[Greg Banks]

On Tue, 2004-10-26 at 00:41, Trond Myklebust wrote:
> m=E5 den 25.10.2004 Klokka 19:33 (+1000) skreiv Greg Banks:
>=20
> > Trond, I don't think it's a good idea to make no_subtree_check the
> > default, as this would have the undesirable side effect of defeating
> > directory permission checks.  Perhaps we can just make it behave
> > more helpfully.
>=20
> If by "make it behave more helpfully" you mean get rid of the aliased
> filehandles, then I'm fine with that. I'm curious to see how you propose
> to do it though: as far as I can see there is no way to reconcile the
> goal of proper directory permission checks and sane filehandle
> behaviour.

Ok you've convinced me, and ->splat<- is the sound of an open can
of worms hitting the Too Hard basket.

Trond, I see you've got a fix for the nfs_rename() dentry refcount bug=20
in your 2.6.10-rc1 patchset, thanks.

> Since the permissions problem is solvable in other ways (i.e. by only
> exporting partitions/volumes), and in fact that is the way all other
> known NFS server implementations expect you to solve the problem,

When the problem is solved at all...OTOH those other OSes don't
have dentry caches which want to remain connected, so Linux is a
special case.

>  then I
> see no problems with making that the default.

Well, you can have that discussion with Neil.

> Look more closely: get_parent() expects the parameter to be a directory.
> I don't think ext2/3 or XFS encode a list of parent directories in the
> on-disk inode for a regular file.

Good point.

BTW, the following hunk is in your 2.6.10-rc1 NFS_ALL.dif but missing
from the corresponding exploded patchset.


--- linux-2.6.10-rc1-up/fs/nfs/file.c	2004-10-25 15:31:13.000000000 -0400
+++ linux-2.6.10-rc1-NFS_ALL/fs/nfs/file.c	2004-10-25 19:08:55.000000000 -0=
400
@@ -396,15 +413,6 @@ nfs_lock(struct file *filp, int cmd, str
 	if ((inode->i_mode & (S_ISGID | S_IXGRP)) =3D=3D S_ISGID)
 		return -ENOLCK;
=20
-	if (NFS_PROTO(inode)->version !=3D 4) {
-		/* Fake OK code if mounted without NLM support */
-		if (NFS_SERVER(inode)->flags & NFS_MOUNT_NONLM) {
-			if (IS_GETLK(cmd))
-				return LOCK_USE_CLNT;
-			return 0;
-		}
-	}
-
 	/*
 	 * No BSD flocks over NFS allowed.
 	 * Note: we could try to fake a POSIX lock request here by

Greg.
--=20
Greg Banks, R&D Software Engineer, SGI Australian Software Group.
I don't speak for SGI.




-------------------------------------------------------
This SF.net email is sponsored by: IT Product Guide on ITManagersJournal
Use IT products in your business? Tell us what you think of them. Give us
Your Opinions, Get Free ThinkGeek Gift Certificates! Click to find out more
http://productguide.itmanagersjournal.com/guidepromo.tmpl
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: Re: [PATCH] fix nfsidem cthon test

[Trond Myklebust]

ty den 26.10.2004 Klokka 17:32 (+1000) skreiv Greg Banks:

> BTW, the following hunk is in your 2.6.10-rc1 NFS_ALL.dif but missing
> from the corresponding exploded patchset.
> 

Err. No... It should appears as the very last hunk in
linux-2.6.9-17-fix_nfs_nolock.dif

Cheers,
  Trond

-- 
Trond Myklebust <trond.myklebust@fys.uio.no>



-------------------------------------------------------
This SF.net email is sponsored by: IT Product Guide on ITManagersJournal
Use IT products in your business? Tell us what you think of them. Give us
Your Opinions, Get Free ThinkGeek Gift Certificates! Click to find out more
http://productguide.itmanagersjournal.com/guidepromo.tmpl
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: Re: [PATCH] fix nfsidem cthon test

[Greg Banks]

On Wed, 2004-10-27 at 02:55, Trond Myklebust wrote:
> ty den 26.10.2004 Klokka 17:32 (+1000) skreiv Greg Banks:
> 
> > BTW, the following hunk is in your 2.6.10-rc1 NFS_ALL.dif but missing
> > from the corresponding exploded patchset.
> > 
> 
> Err. No... It should appears as the very last hunk in
> linux-2.6.9-17-fix_nfs_nolock.dif

Oh, I see: linux-2.6.9-17-fix_nfs_nolock.dif isn't linked to
from the initial section of the page, which goes from -16 to -18.

Greg.
-- 
Greg Banks, R&D Software Engineer, SGI Australian Software Group.
I don't speak for SGI.




-------------------------------------------------------
This SF.Net email is sponsored by:
Sybase ASE Linux Express Edition - download now for FREE
LinuxWorld Reader's Choice Award Winner for best database on Linux.
http://ads.osdn.com/?ad_id=5588&alloc_id=12065&op=click
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: Re: [PATCH] fix nfsidem cthon test

[Trond Myklebust]

on den 27.10.2004 Klokka 10:40 (+1000) skreiv Greg Banks:

> Oh, I see: linux-2.6.9-17-fix_nfs_nolock.dif isn't linked to
> from the initial section of the page, which goes from -16 to -18.

Oops... I never did get round to writing a script to automate that...

Oh, well. Thanks for pointing it out!

Trond

-- 
Trond Myklebust <trond.myklebust@fys.uio.no>



-------------------------------------------------------
This SF.Net email is sponsored by:
Sybase ASE Linux Express Edition - download now for FREE
LinuxWorld Reader's Choice Award Winner for best database on Linux.
http://ads.osdn.com/?ad_id=5588&alloc_id=12065&op=click
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: Re: [PATCH] fix nfsidem cthon test

[Neil Brown]

On Friday October 22, gnb@melbourne.sgi.com wrote:
> +	/*
> +	 * If target is a hard link to the source, then noop.
> +	 * We compare the entire file handle instead of just
> +	 * the fileid to handle the nohide case.   This test
> +	 * is supposed to happen on the server, but some servers
> +	 * are buggy.
> +	 */

I don't disagree with this patch, and I don't want to assert that no
servers are buggy, or that the Linux server in particular isn't
buggy.  However I don't think the line:

    This test is supposed to happen on the server, but some servers
    are buggy.

is fair or accurate.

This test is trivial to do on the server, and I am sure it is being
done.

The real problem here is, I think, due to "sillyrename"
Consider
    create /a/fred
    ln /a/fred /b/bob
    open /b/bob
    rename /a/fred /b/bob

The rename should be a no-op because the two names refer to the same
file.   The client might be able to tell if they are the same by
inspecting the filehandles and so avoid the rename.  That is a useful
optimisation but is not guaranteed to find that they are the same when
they are.

As the client has /b/bob open, and as it thinks /b/bob might be about
to be unlinked, it replaces the
    rename /a/fred /b/bob
call with a silly-rename, viz
    rename /b/bob /b/.nfsXXXX
    rename /a/fred /a/bob

This will do something very different.  It will unlink /a/fred, which
it shouldn't.

I would suggest that silly-rename should be replaced with silly-link.
viz.
    link /b/bob /b/.nfsXXXX
    rename /a/fred /a/bob

This would then have correct semantics.  (ofcourse, the filesystem
might not support "link", in which case you could fall-back to rename
and that's not a problem, because then /a/fred and /a/bob cannot be
the same file anyway).

NeilBrown


-------------------------------------------------------
This SF.net email is sponsored by: IT Product Guide on ITManagersJournal
Use IT products in your business? Tell us what you think of them. Give us
Your Opinions, Get Free ThinkGeek Gift Certificates! Click to find out more
http://productguide.itmanagersjournal.com/guidepromo.tmpl
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: Re: [PATCH] fix nfsidem cthon test

[Greg Banks]

On Fri, 2004-10-22 at 13:18, Neil Brown wrote:
> On Friday October 22, gnb@melbourne.sgi.com wrote:
> > +	/*
> > +	 * If target is a hard link to the source, then noop.
> > +	 * We compare the entire file handle instead of just
> > +	 * the fileid to handle the nohide case.   This test
> > +	 * is supposed to happen on the server, but some servers
> > +	 * are buggy.
> > +	 */
> 
> I don't disagree with this patch, and I don't want to assert that no
> servers are buggy, or that the Linux server in particular isn't
> buggy.  However I don't think the line:
> 
>     This test is supposed to happen on the server, but some servers
>     are buggy.
> 
> is fair or accurate.

I'm happy to revise the comment ;-)

> This test is trivial to do on the server, 

Agreed.

> and I am sure it is being
> done.

I took a brief look at the server code this morning, and came to 
the conclusion that the test is done in vfs_rename() and is done
correctly, or at least would be if the only thing the client did
was a RENAME call.

> The real problem here is, I think, due to "sillyrename"
> Consider
>     create /a/fred
>     ln /a/fred /b/bob
>     open /b/bob
>     rename /a/fred /b/bob
> 
> The rename should be a no-op because the two names refer to the same
> file.   The client might be able to tell if they are the same by
> inspecting the filehandles and so avoid the rename.  That is a useful
> optimisation but is not guaranteed to find that they are the same when
> they are.

Agreed, and this is what the patch does:  it happens to make the test
pass for all the servers and backend filesystems I've seen test results
for.

> As the client has /b/bob open, and as it thinks /b/bob might be about
> to be unlinked, it replaces the
>     rename /a/fred /b/bob
> call with a silly-rename, viz
>     rename /b/bob /b/.nfsXXXX
>     rename /a/fred /a/bob
> 
> This will do something very different.  It will unlink /a/fred, which
> it shouldn't.

I agree with your analysis...yet this same client code works against
an IRIX server.  I'll grab some snoops and see what's happening on
the wire in that case.

Greg.
-- 
Greg Banks, R&D Software Engineer, SGI Australian Software Group.
I don't speak for SGI.




-------------------------------------------------------
This SF.net email is sponsored by: IT Product Guide on ITManagersJournal
Use IT products in your business? Tell us what you think of them. Give us
Your Opinions, Get Free ThinkGeek Gift Certificates! Click to find out more
http://productguide.itmanagersjournal.com/guidepromo.tmpl
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: Re: [PATCH] fix nfsidem cthon test

[Greg Banks]

[-- Attachment #1: Type: text/plain, Size: 889 bytes --]

On Fri, 2004-10-22 at 14:12, Greg Banks wrote:
> I agree with your analysis...yet this same client code works against
> an IRIX server.  I'll grab some snoops and see what's happening on
> the wire in that case.

Well, that was instructive.  There seem to be several interesting
problems; the most obvious of which is that the dentry refcount
accounting in nfs_rename() is off by one, so that the client does
a sillyrename even when there *no* file descriptors open for the
target file.  This is actually what happens in the cthon test code
and I've verified it with a small test case.

At this point in the code there should be 1 refcount for being
connected to the dentry tree and 1 for the path lookup in sys_rename().
The attached patch changes the sillyrename threshold from 1 to 2.

Greg.
-- 
Greg Banks, R&D Software Engineer, SGI Australian Software Group.
I don't speak for SGI.


[-- Attachment #2: gnb-nfs-rename-refcount --]
[-- Type: text/plain, Size: 691 bytes --]

Fix dentry refcount accounting error which causes unnecessary
sillyrenames when renaming to an existing file.

Signed-off-by: Greg Banks <gnb@melbourne.sgi.com>


Index: linux/fs/nfs/dir.c
===================================================================
--- linux.orig/fs/nfs/dir.c	2004-10-22 21:53:23.%N +1000
+++ linux/fs/nfs/dir.c	2004-10-22 21:53:46.%N +1000
@@ -1438,7 +1438,7 @@ static int nfs_rename(struct inode *old_
 		goto go_ahead;
 	if (S_ISDIR(new_inode->i_mode))
 		goto out;
-	else if (atomic_read(&new_dentry->d_count) > 1) {
+	else if (atomic_read(&new_dentry->d_count) > 2) {
 		int err;
 		/* copy the target dentry's name */
 		dentry = d_alloc(new_dentry->d_parent,

Re: Re: [PATCH] fix nfsidem cthon test

[Trond Myklebust]

fr den 22.10.2004 Klokka 22:28 (+1000) skreiv Greg Banks:

> Well, that was instructive.  There seem to be several interesting
> problems; the most obvious of which is that the dentry refcount
> accounting in nfs_rename() is off by one, so that the client does
> a sillyrename even when there *no* file descriptors open for the
> target file.  This is actually what happens in the cthon test code
> and I've verified it with a small test case.

Yes. This is correct.

Cheers,
  Trond

-- 
Trond Myklebust <trond.myklebust@fys.uio.no>



-------------------------------------------------------
This SF.net email is sponsored by: IT Product Guide on ITManagersJournal
Use IT products in your business? Tell us what you think of them. Give us
Your Opinions, Get Free ThinkGeek Gift Certificates! Click to find out more
http://productguide.itmanagersjournal.com/guidepromo.tmpl
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: Re: [PATCH] fix nfsidem cthon test

[Trond Myklebust]

fr den 22.10.2004 Klokka 13:18 (+1000) skreiv Neil Brown:

> I would suggest that silly-rename should be replaced with silly-link.
> viz.
>     link /b/bob /b/.nfsXXXX
>     rename /a/fred /a/bob
> 
> This would then have correct semantics.  (ofcourse, the filesystem
> might not support "link", in which case you could fall-back to rename
> and that's not a problem, because then /a/fred and /a/bob cannot be
> the same file anyway).

Why? That fixes the _symptom_ of sillyrename, but does nothing to
address the underlying problem.

The _real_ problem here is that the client ends up aliasing inodes
because it has no way to really know that these are the same file. That
leads to incoherent metadata+data caches for that file, resulting in
possible data corruption if file descriptors are open on both inodes at
the same time.
What's more, the cross-directory rename may result in a file descriptor
that is open on the "source" inode ending up with a stale filehandle.

Why should we encourage that sort of server behaviour?

Cheers,
  Trond

-- 
Trond Myklebust <trond.myklebust@fys.uio.no>



-------------------------------------------------------
This SF.net email is sponsored by: IT Product Guide on ITManagersJournal
Use IT products in your business? Tell us what you think of them. Give us
Your Opinions, Get Free ThinkGeek Gift Certificates! Click to find out more
http://productguide.itmanagersjournal.com/guidepromo.tmpl
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: Re: [PATCH] fix nfsidem cthon test

[Trond Myklebust]

to den 21.10.2004 Klokka 12:13 (-0400) skreiv J. Bruce Fields:

> We could do something like this for v4, though, couldn't we?
> 
> (I'm looking at rfc3530 section 9.3, which allows the client to assume
> two files are the same if the server returns the same fileid and fsid
> attributes for the two filehandles and doesn't return TRUE for
> unique_handles on either.)

I assume that the fsid does change if you cross "nohide" boundaries on
v2/v3 as well, so if you check the fsid, you can probably do it there
too.

The problem with fsid, is that it often changes under server reboot.

Cheers,
  Trond

-- 
Trond Myklebust <trond.myklebust@fys.uio.no>



-------------------------------------------------------
This SF.net email is sponsored by: IT Product Guide on ITManagersJournal
Use IT products in your business? Tell us what you think of them. Give us
Your Opinions, Get Free ThinkGeek Gift Certificates! Click to find out more
http://productguide.itmanagersjournal.com/guidepromo.tmpl
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs