[patch 0/4] nfs: cleanups in nfs_rename()

[patch 0/4] nfs: cleanups in nfs_rename()

[Miklos Szeredi]

Hi Trond,

This small patchset cleans up accumulated cruft in nfs_rename().  It's
been lightly tested.

Thanks,
Miklos
--

[patch 1/4] nfs: remove unnecessary check from nfs_rename()

[Miklos Szeredi]

[-- Attachment #1: nfs_rename_remove_unnecessary_check.patch --]
[-- Type: text/plain, Size: 1200 bytes --]

From: Miklos Szeredi <mszeredi@suse.cz>

VFS already checks if both source and target are directories.

Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
---
 fs/nfs/dir.c |   10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

Index: linux-2.6/fs/nfs/dir.c
===================================================================
--- linux-2.6.orig/fs/nfs/dir.c	2009-12-02 13:46:12.000000000 +0100
+++ linux-2.6/fs/nfs/dir.c	2009-12-02 15:10:33.000000000 +0100
@@ -1601,13 +1601,8 @@ static int nfs_rename(struct inode *old_
 	 * silly-rename. If the silly-rename succeeds, the
 	 * copied dentry is hashed and becomes the new target.
 	 */
-	if (!new_inode)
-		goto go_ahead;
-	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) {
+	if (new_inode && !S_ISDIR(new_inode->i_mode) &&
+	    atomic_read(&new_dentry->d_count) > 2) {
 		int err;
 		/* copy the target dentry's name */
 		dentry = d_alloc(new_dentry->d_parent,
@@ -1627,7 +1622,6 @@ static int nfs_rename(struct inode *old_
 			goto out;
 	}
 
-go_ahead:
 	/*
 	 * ... prune child dentries and writebacks if needed.
 	 */

-- 

[patch 2/4] nfs: fix comments in nfs_rename()

[Miklos Szeredi]

[-- Attachment #1: nfs_rename_fix_comments.patch --]
[-- Type: text/plain, Size: 1610 bytes --]

From: Miklos Szeredi <mszeredi@suse.cz>

Comments are wrong or out of date.  In particular d_drop() doesn't
free the inode it just unhashes the dentry.  And if target is a
directory then it is not checked for being busy.

Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
---
 fs/nfs/dir.c |   12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

Index: linux-2.6/fs/nfs/dir.c
===================================================================
--- linux-2.6.orig/fs/nfs/dir.c	2009-12-02 15:10:33.000000000 +0100
+++ linux-2.6/fs/nfs/dir.c	2009-12-02 15:10:44.000000000 +0100
@@ -1581,7 +1581,7 @@ static int nfs_rename(struct inode *old_
 
 	/*
 	 * To prevent any new references to the target during the rename,
-	 * we unhash the dentry and free the inode in advance.
+	 * we unhash the dentry in advance.
 	 */
 	if (!d_unhashed(new_dentry)) {
 		d_drop(new_dentry);
@@ -1594,12 +1594,10 @@ static int nfs_rename(struct inode *old_
 		 atomic_read(&new_dentry->d_count));
 
 	/*
-	 * First check whether the target is busy ... we can't
-	 * safely do _any_ rename if the target is in use.
-	 *
-	 * For files, make a copy of the dentry and then do a 
-	 * silly-rename. If the silly-rename succeeds, the
-	 * copied dentry is hashed and becomes the new target.
+	 * For non-directories, check whether the target is busy and if so,
+	 * make a copy of the dentry and then do a silly-rename. If the
+	 * silly-rename succeeds, the copied dentry is hashed and becomes
+	 * the new target.
 	 */
 	if (new_inode && !S_ISDIR(new_inode->i_mode) &&
 	    atomic_read(&new_dentry->d_count) > 2) {

-- 

[patch 3/4] nfs: dont unhash target if renaming a directory

[Miklos Szeredi]

[-- Attachment #1: nfs_rename_dont_unhash_if_dir.patch --]
[-- Type: text/plain, Size: 2829 bytes --]

From: Miklos Szeredi <mszeredi@suse.cz>

Move unhashing the target to after the check for existence and being a
non-directory.

If renaming a directory then the VFS already unhashes the target if it
is not busy.  If it's busy then acquiring more references during the
rename makes no difference.

Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
---
 fs/nfs/dir.c |   58 ++++++++++++++++++++++++++++++----------------------------
 1 file changed, 30 insertions(+), 28 deletions(-)

Index: linux-2.6/fs/nfs/dir.c
===================================================================
--- linux-2.6.orig/fs/nfs/dir.c	2009-12-02 15:10:44.000000000 +0100
+++ linux-2.6/fs/nfs/dir.c	2009-12-02 15:11:19.000000000 +0100
@@ -1579,15 +1579,6 @@ static int nfs_rename(struct inode *old_
 	struct dentry *dentry = NULL, *rehash = NULL;
 	int error = -EBUSY;
 
-	/*
-	 * To prevent any new references to the target during the rename,
-	 * we unhash the dentry in advance.
-	 */
-	if (!d_unhashed(new_dentry)) {
-		d_drop(new_dentry);
-		rehash = new_dentry;
-	}
-
 	dfprintk(VFS, "NFS: rename(%s/%s -> %s/%s, ct=%d)\n",
 		 old_dentry->d_parent->d_name.name, old_dentry->d_name.name,
 		 new_dentry->d_parent->d_name.name, new_dentry->d_name.name,
@@ -1599,25 +1590,36 @@ static int nfs_rename(struct inode *old_
 	 * silly-rename succeeds, the copied dentry is hashed and becomes
 	 * the new target.
 	 */
-	if (new_inode && !S_ISDIR(new_inode->i_mode) &&
-	    atomic_read(&new_dentry->d_count) > 2) {
-		int err;
-		/* copy the target dentry's name */
-		dentry = d_alloc(new_dentry->d_parent,
-				 &new_dentry->d_name);
-		if (!dentry)
-			goto out;
-
-		/* silly-rename the existing target ... */
-		err = nfs_sillyrename(new_dir, new_dentry);
-		if (!err) {
-			new_dentry = rehash = dentry;
-			new_inode = NULL;
-			/* instantiate the replacement target */
-			d_instantiate(new_dentry, NULL);
-		} else if (atomic_read(&new_dentry->d_count) > 1)
-			/* dentry still busy? */
-			goto out;
+	if (new_inode && !S_ISDIR(new_inode->i_mode)) {
+		/*
+		 * To prevent any new references to the target during the
+		 * rename, we unhash the dentry in advance.
+		 */
+		if (!d_unhashed(new_dentry)) {
+			d_drop(new_dentry);
+			rehash = new_dentry;
+		}
+
+		if (atomic_read(&new_dentry->d_count) > 2) {
+			int err;
+
+			/* copy the target dentry's name */
+			dentry = d_alloc(new_dentry->d_parent,
+					 &new_dentry->d_name);
+			if (!dentry)
+				goto out;
+
+			/* silly-rename the existing target ... */
+			err = nfs_sillyrename(new_dir, new_dentry);
+			if (!err) {
+				new_dentry = rehash = dentry;
+				new_inode = NULL;
+				/* instantiate the replacement target */
+				d_instantiate(new_dentry, NULL);
+			} else if (atomic_read(&new_dentry->d_count) > 1)
+				/* dentry still busy? */
+				goto out;
+		}
 	}
 
 	/*

-- 

[patch 4/4] nfs: clean up sillyrenaming in nfs_rename()

[Miklos Szeredi]

[-- Attachment #1: nfs_rename_dont_instantiate.patch --]
[-- Type: text/plain, Size: 1199 bytes --]

From: Miklos Szeredi <mszeredi@suse.cz>

The d_instantiate(new_dentry, NULL) is superfluous, the dentry is
already negative.  Rehashing this dummy dentry isn't needed either,
d_move() works fine on an unhashed target.

The re-checking for busy after a failed nfs_sillyrename() is bogus
too: new_dentry->d_count < 2 would be a bug here.

Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
---
 fs/nfs/dir.c |   11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

Index: linux-2.6/fs/nfs/dir.c
===================================================================
--- linux-2.6.orig/fs/nfs/dir.c	2009-12-02 15:11:19.000000000 +0100
+++ linux-2.6/fs/nfs/dir.c	2009-12-02 15:11:30.000000000 +0100
@@ -1611,14 +1611,11 @@ static int nfs_rename(struct inode *old_
 
 			/* silly-rename the existing target ... */
 			err = nfs_sillyrename(new_dir, new_dentry);
-			if (!err) {
-				new_dentry = rehash = dentry;
-				new_inode = NULL;
-				/* instantiate the replacement target */
-				d_instantiate(new_dentry, NULL);
-			} else if (atomic_read(&new_dentry->d_count) > 1)
-				/* dentry still busy? */
+			if (err)
 				goto out;
+
+			new_dentry = dentry;
+			new_inode = NULL;
 		}
 	}
 

-- 

Re: [patch 4/4] nfs: clean up sillyrenaming in nfs_rename()

[hooanon05]

Miklos,

Don't we need to make the 'dentry' hashed, in sillyrename case?
The d_alloc()ed dentry is unhashed, and it was d_rehash()ed later
(before the patch).

	new_dentry = rehash = dentry;
	:::
out:
	if (rehash)
		d_rehash(rehash);

Applying your patch, the 'rehash' is not re-assigned and the 'dentry'
left unhashed. Such dentry which is visible by readdir but its d_hash is
empty, will be a problem in nfs readdir (which calls d_lookup()), I am
afraid.


J. R. Okajima


Miklos Szeredi:
> The d_instantiate(new_dentry, NULL) is superfluous, the dentry is
> already negative.  Rehashing this dummy dentry isn't needed either,
> d_move() works fine on an unhashed target.
> 
> The re-checking for busy after a failed nfs_sillyrename() is bogus
> too: new_dentry->d_count < 2 would be a bug here.
> 
> Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
> ---
>  fs/nfs/dir.c |   11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)
> 
> Index: linux-2.6/fs/nfs/dir.c
> ===================================================================
> --- linux-2.6.orig/fs/nfs/dir.c 2009-12-02 15:11:19.000000000 +0100
> +++ linux-2.6/fs/nfs/dir.c      2009-12-02 15:11:30.000000000 +0100
> @@ -1611,14 +1611,11 @@  static int nfs_rename(struct inode *old_
> 
>                         /* silly-rename the existing target ... */
>                         err = nfs_sillyrename(new_dir, new_dentry);
> -                       if (!err) {
> -                               new_dentry = rehash = dentry;
> -                               new_inode = NULL;
> -                               /* instantiate the replacement target */
> -                               d_instantiate(new_dentry, NULL);
> -                       } else if (atomic_read(&new_dentry->d_count) > 1)
> -                               /* dentry still busy? */
> +                       if (err)
>                                 goto out;
> +
> +                       new_dentry = dentry;
> +                       new_inode = NULL;
>                 }
>         }

Re: [patch 4/4] nfs: clean up sillyrenaming in nfs_rename()

[OGAWA Hirofumi]

Miklos Szeredi <miklos@szeredi.hu> writes:

> --- linux-2.6.orig/fs/nfs/dir.c	2009-12-02 15:11:19.000000000 +0100
> +++ linux-2.6/fs/nfs/dir.c	2009-12-02 15:11:30.000000000 +0100
> @@ -1611,14 +1611,11 @@ static int nfs_rename(struct inode *old_
>  
>  			/* silly-rename the existing target ... */
>  			err = nfs_sillyrename(new_dir, new_dentry);
> -			if (!err) {
> -				new_dentry = rehash = dentry;
> -				new_inode = NULL;
> -				/* instantiate the replacement target */
> -				d_instantiate(new_dentry, NULL);
> -			} else if (atomic_read(&new_dentry->d_count) > 1)
> -				/* dentry still busy? */
> +			if (err)
>  				goto out;
> +
> +			new_dentry = dentry;
> +			new_inode = NULL;

This needs to update "rehash". Sorry, this patch is still compile test
only, although the patch is clear and simple.

Thanks.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>



[PATCH] nfs: Fix d_rehash() for hashed dentry in nfs_rename()

Recent change is missing to update "rehash". With that change, it will
become the cause of adding dentry to hash twice.

This explains the reason of Oops (dereference the freed dentry in
__d_lookup()) on my machine.

Signed-off-by: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
---

 fs/nfs/dir.c |    1 +
 1 file changed, 1 insertion(+)

diff -puN fs/nfs/dir.c~nfs-d_rehash-fix fs/nfs/dir.c
--- linux-2.6/fs/nfs/dir.c~nfs-d_rehash-fix	2009-12-28 06:18:09.000000000 +0900
+++ linux-2.6-hirofumi/fs/nfs/dir.c	2009-12-28 06:18:16.000000000 +0900
@@ -1615,6 +1615,7 @@ static int nfs_rename(struct inode *old_
 				goto out;
 
 			new_dentry = dentry;
+			rehash = NULL;
 			new_inode = NULL;
 		}
 	}
_

Re: [patch 0/4] nfs: cleanups in nfs_rename()

[Trond Myklebust]

On Wed, 2009-12-02 at 16:04 +0100, Miklos Szeredi wrote: 
> Hi Trond,
> 
> This small patchset cleans up accumulated cruft in nfs_rename().  It's
> been lightly tested.
> 
> Thanks,
> Miklos
> --

Thanks Miklos! I will apply.

Cheers
  Trond