RE: historical question: nfs_rename()

RE: historical question: nfs_rename()

[Lever, Charles]

> Actually, this case should fail with EISDIR, as it does.  Mixing
> directory and files in a rename(2) will fail with EISDIR or ENOTDIR.
>=20
> However, renaming one directory to another directory, when the target
> directory is "empty", should not fail.

the specific case i'm whining about today is renaming a directory to an
empty directory.  the desired semantic is that the empty directory will
be replaced.  the existing semantic on the Linux NFS client is that
rename(2) returns -EBUSY.

as far as we can tell, the Solaris client works as desired.  the issue
is a portability problem that arises because Linux's rename(2) doesn't
work precisely the same way as Solaris'.


-------------------------------------------------------
This SF.Net email is sponsored by:
Power Architecture Resource Center: Free content, downloads, discussions,
and more. http://solutions.newsforge.com/ibmarch.tmpl
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: historical question: nfs_rename()

[Peter Staubach]

Lever, Charles wrote:

>>Actually, this case should fail with EISDIR, as it does.  Mixing
>>directory and files in a rename(2) will fail with EISDIR or ENOTDIR.
>>
>>However, renaming one directory to another directory, when the target
>>directory is "empty", should not fail.
>>    
>>
>
>the specific case i'm whining about today is renaming a directory to an
>empty directory.  the desired semantic is that the empty directory will
>be replaced.  the existing semantic on the Linux NFS client is that
>rename(2) returns -EBUSY.
>
>as far as we can tell, the Solaris client works as desired.  the issue
>is a portability problem that arises because Linux's rename(2) doesn't
>work precisely the same way as Solaris'.
>  
>
I think that this is actually a POSIX/SUS3 thing...

    Thanx...

       ps


-------------------------------------------------------
This SF.Net email is sponsored by:
Power Architecture Resource Center: Free content, downloads, discussions,
and more. http://solutions.newsforge.com/ibmarch.tmpl
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

RE: historical question: nfs_rename()

[Trond Myklebust]

ty den 18.10.2005 klokka 10:59 (-0700) skreiv Lever, Charles:
> the specific case i'm whining about today is renaming a directory to an
> empty directory.  the desired semantic is that the empty directory will
> be replaced.  the existing semantic on the Linux NFS client is that
> rename(2) returns -EBUSY.

A well-written testcase speaks louder than 1000 words (and avoids much
confusion on my part). ;-)

Cheers,
  Trond



-------------------------------------------------------
This SF.Net email is sponsored by:
Power Architecture Resource Center: Free content, downloads, discussions,
and more. http://solutions.newsforge.com/ibmarch.tmpl
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

RE: historical question: nfs_rename()

[Lever, Charles]

this is going in the right direction.

should we quickly look at other test cases?  what return code should/do
you get when you try to rename a directory onto a target directory that
still has stuff in it?

have you tested both the file onto directory and directory onto file
cases?


> ty den 18.10.2005 klokka 12:57 (-0400) skreiv Peter Staubach:
> > However, renaming one directory to another directory, when=20
> the target
> > directory is "empty", should not fail.
>=20
> Ah... I see now. OK, so when I rewrite the test case to the attached
> code then it does indeed succeed.
>=20
> The following patch should do the right thing then.
>=20
> Cheers,
>   Trond
>=20
> NFS: Fix rename of directory onto empty directory
>=20
>  If someone tries to rename a directory onto an empty directory, we
>  currently fail and return EBUSY.
>  This patch ensures that we try the rename if both source and target
>  are directories, and that we fail with a correct error of EISDIR if
>  the source is not a directory.
>=20
>  Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>
> ---
>  fs/nfs/dir.c |    8 +++++---
>  1 files changed, 5 insertions(+), 3 deletions(-)
>=20
> Index: linux-2.6/fs/nfs/dir.c
> =
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> --- linux-2.6.orig/fs/nfs/dir.c
> +++ linux-2.6/fs/nfs/dir.c
> @@ -1511,9 +1511,11 @@ static int nfs_rename(struct inode *old_
>  	 */
>  	if (!new_inode)
>  		goto go_ahead;
> -	if (S_ISDIR(new_inode->i_mode))
> -		goto out;
> -	else if (atomic_read(&new_dentry->d_count) > 2) {
> +	if (S_ISDIR(new_inode->i_mode)) {
> +		error =3D -EISDIR;
> +		if (!S_ISDIR(old_inode->i_mode))
> +			goto out;
> +	} else if (atomic_read(&new_dentry->d_count) > 2) {
>  		int err;
>  		/* copy the target dentry's name */
>  		dentry =3D d_alloc(new_dentry->d_parent,
>=20
>=20


-------------------------------------------------------
This SF.Net email is sponsored by:
Power Architecture Resource Center: Free content, downloads, discussions,
and more. http://solutions.newsforge.com/ibmarch.tmpl
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

RE: historical question: nfs_rename()

[Trond Myklebust]

ty den 18.10.2005 klokka 11:15 (-0700) skreiv Lever, Charles:
> this is going in the right direction.
> 
> should we quickly look at other test cases?  what return code should/do
> you get when you try to rename a directory onto a target directory that
> still has stuff in it?

I think the server is going to have to handle that. There is no way for
the client to atomically check the directory for emptiness and rename.

> have you tested both the file onto directory and directory onto file
> cases?

Still untested, but the check I introduced should ensure that
file-onto-directory works as expected. The directory-onto-file behaviour
is unchanged.

Cheers,
  Trond



-------------------------------------------------------
This SF.Net email is sponsored by:
Power Architecture Resource Center: Free content, downloads, discussions,
and more. http://solutions.newsforge.com/ibmarch.tmpl
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

historical question: nfs_rename()

[Lever, Charles]

a customer recently pointed out that the rename(2) system call does not
allow replacing directories, even if they are empty.  RFC1813 at least
suggests that the server and protocol do allow directory replacement, as
long as the target is empty.

the culprit appears to be this check in nfs_rename()

        if (S_ISDIR(new_inode->i_mode))
                goto out;

which causes nfs_rename() to return -EBUSY if the target is a directory.

is there a historical reason why the Linux NFS client does not allow
directory replacement?  shouldn't the error code be -EISDIR for this
case?

        - Chuck Lever
--
corporate:     <cel at netapp dot com>
personal:      <chucklever at bigfoot dot com>


-------------------------------------------------------
This SF.Net email is sponsored by:
Power Architecture Resource Center: Free content, downloads, discussions,
and more. http://solutions.newsforge.com/ibmarch.tmpl
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: historical question: nfs_rename()

[Peter Staubach]

Lever, Charles wrote:

>a customer recently pointed out that the rename(2) system call does not
>allow replacing directories, even if they are empty.  RFC1813 at least
>suggests that the server and protocol do allow directory replacement, as
>long as the target is empty.
>
>the culprit appears to be this check in nfs_rename()
>
>        if (S_ISDIR(new_inode->i_mode))
>                goto out;
>
>which causes nfs_rename() to return -EBUSY if the target is a directory.
>
>is there a historical reason why the Linux NFS client does not allow
>directory replacement?  shouldn't the error code be -EISDIR for this
>case?
>

Assuming that the client can handle this situation by ensuring that all
of its caches stay updated, then it seems to me that the client should
just package up the rename and send it off to the server.  It should be
up to the server to decide whether it can handle directory renames of
this type or not.

    Thanx...

       ps


-------------------------------------------------------
This SF.Net email is sponsored by:
Power Architecture Resource Center: Free content, downloads, discussions,
and more. http://solutions.newsforge.com/ibmarch.tmpl
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: historical question: nfs_rename()

[Trond Myklebust]

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

må den 17.10.2005 klokka 12:12 (-0700) skreiv Lever, Charles:
> a customer recently pointed out that the rename(2) system call does not
> allow replacing directories, even if they are empty.  RFC1813 at least
> suggests that the server and protocol do allow directory replacement, as
> long as the target is empty.
> 
> the culprit appears to be this check in nfs_rename()
> 
>         if (S_ISDIR(new_inode->i_mode))
>                 goto out;
> 
> which causes nfs_rename() to return -EBUSY if the target is a directory.
> 
> is there a historical reason why the Linux NFS client does not allow
> directory replacement?  shouldn't the error code be -EISDIR for this
> case?

Yes, the error should be EISDIR. I'm not sure that I agree with your
assertion that rename failing is an NFS-only issue, though. The attached
testcase fails on both XFS and ext3 too with the error "Rename failed:
Is a directory".

Cheers,
  Trond


[-- Attachment #2: main.c --]
[-- Type: text/x-csrc, Size: 503 bytes --]

#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <stdio.h>
#include <stdlib.h>

#define FILENAME "gnurr"
#define DIRNAME "gnarg"

void die(char *errmsg)
{
	perror(errmsg);
	exit(1);
}

int main()
{
	int fd;
	int err;

	fd = open(FILENAME, O_CREAT|O_EXCL|O_RDWR, 0644);
	if (fd < 0)
		die("File creation failed");
	close(fd);
	err = mkdir(DIRNAME, 0700);
	if (err != 0)
		die("Directory creation failed");
	err = rename("gnurr", "gnarg");
	if (err != 0)
		die("Rename failed");
}

Re: historical question: nfs_rename()

[Peter Staubach]

Trond Myklebust wrote:

>m=E5 den 17.10.2005 klokka 12:12 (-0700) skreiv Lever, Charles:
> =20
>
>>a customer recently pointed out that the rename(2) system call does not
>>allow replacing directories, even if they are empty.  RFC1813 at least
>>suggests that the server and protocol do allow directory replacement, a=
s
>>long as the target is empty.
>>
>>the culprit appears to be this check in nfs_rename()
>>
>>        if (S_ISDIR(new_inode->i_mode))
>>                goto out;
>>
>>which causes nfs_rename() to return -EBUSY if the target is a directory.
>>
>>is there a historical reason why the Linux NFS client does not allow
>>directory replacement?  shouldn't the error code be -EISDIR for this
>>case?
>>   =20
>>
>
>Yes, the error should be EISDIR. I'm not sure that I agree with your
>assertion that rename failing is an NFS-only issue, though. The attached
>testcase fails on both XFS and ext3 too with the error "Rename failed:
>Is a directory".
>
>Cheers,
>  Trond
>
> =20
>
>------------------------------------------------------------------------
>
>#include <sys/types.h>
>#include <sys/stat.h>
>#include <fcntl.h>
>#include <stdio.h>
>#include <stdlib.h>
>
>#define FILENAME "gnurr"
>#define DIRNAME "gnarg"
>
>void die(char *errmsg)
>{
>	perror(errmsg);
>	exit(1);
>}
>
>int main()
>{
>	int fd;
>	int err;
>
>	fd =3D open(FILENAME, O_CREAT|O_EXCL|O_RDWR, 0644);
>	if (fd < 0)
>		die("File creation failed");
>	close(fd);
>	err =3D mkdir(DIRNAME, 0700);
>	if (err !=3D 0)
>		die("Directory creation failed");
>	err =3D rename("gnurr", "gnarg");
>	if (err !=3D 0)
>		die("Rename failed");
>}
> =20
>

Actually, this case should fail with EISDIR, as it does.  Mixing
directory and files in a rename(2) will fail with EISDIR or ENOTDIR.

However, renaming one directory to another directory, when the target
directory is "empty", should not fail.

       ps


-------------------------------------------------------
This SF.Net email is sponsored by:
Power Architecture Resource Center: Free content, downloads, discussions,
and more. http://solutions.newsforge.com/ibmarch.tmpl
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: historical question: nfs_rename()

[Trond Myklebust]

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

ty den 18.10.2005 klokka 12:57 (-0400) skreiv Peter Staubach:
> However, renaming one directory to another directory, when the target
> directory is "empty", should not fail.

Ah... I see now. OK, so when I rewrite the test case to the attached
code then it does indeed succeed.

The following patch should do the right thing then.

Cheers,
  Trond

NFS: Fix rename of directory onto empty directory

 If someone tries to rename a directory onto an empty directory, we
 currently fail and return EBUSY.
 This patch ensures that we try the rename if both source and target
 are directories, and that we fail with a correct error of EISDIR if
 the source is not a directory.

 Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>
---
 fs/nfs/dir.c |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)

Index: linux-2.6/fs/nfs/dir.c
===================================================================
--- linux-2.6.orig/fs/nfs/dir.c
+++ linux-2.6/fs/nfs/dir.c
@@ -1511,9 +1511,11 @@ static int nfs_rename(struct inode *old_
 	 */
 	if (!new_inode)
 		goto go_ahead;
-	if (S_ISDIR(new_inode->i_mode))
-		goto out;
-	else if (atomic_read(&new_dentry->d_count) > 2) {
+	if (S_ISDIR(new_inode->i_mode)) {
+		error = -EISDIR;
+		if (!S_ISDIR(old_inode->i_mode))
+			goto out;
+	} else if (atomic_read(&new_dentry->d_count) > 2) {
 		int err;
 		/* copy the target dentry's name */
 		dentry = d_alloc(new_dentry->d_parent,


[-- Attachment #2: main.c --]
[-- Type: text/x-csrc, Size: 491 bytes --]

#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <stdio.h>
#include <stdlib.h>

#define DIRNAME1 "gnurr"
#define DIRNAME2 "gnarg"

void die(char *errmsg)
{
	perror(errmsg);
	exit(1);
}

int main()
{
	int fd;
	int err;

	err = mkdir(DIRNAME1, 0700);
	if (err != 0)
		die("Creation of directory 1 failed");
	err = mkdir(DIRNAME2, 0770);
	if (err != 0)
		die("Creation of directory 2 failed");
	err = rename(DIRNAME1, DIRNAME2);
	if (err != 0)
		die("Rename failed");
}