[PATCH] fs: Work around NFS wreckage

[PATCH] fs: Work around NFS wreckage

[Thomas Gleixner]

The dcache scalability work broke NFS root filesystems.

"cd /" results in the following problem:

    link_path_walk("/",...);
	jumps to return_reval
	need_reval_dot() returns true for NFS
	d_revalidate()
		dentry->d_op->d_revalidate(dentry, nd);
		 	returns -ECHILD due to nd->flags & LOOKUP_RCU
		nameidata_dentry_drop_rcu()
			spin_lock(&parent->d_lock);
			spin_lock_nested(&dentry->d_lock, DENTRY_D_LOCK_NESTED);

			This deadlocks because dentry == parent

This problem exists for any filesystem which implements d_revalidate.

Uwe bisected is down to commit 34286d6(fs: rcu-walk aware d_revalidate
method), but reverting that patch causes different wreckage to show up.

Check for parent equal dentry and skip the nested lock to avoid the
deadlock. I'm sure this is the wrong fix, but at least it "works" :)

Reported-by: Uwe Kleine-Koenig <u.kleine-koenig@pengutronix.de>
Reported-by: "Ramirez Luna, Omar" <omar.ramirez@ti.com>
Not-Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 fs/namei.c |    4 ++++
 1 file changed, 4 insertions(+)

Index: linux-2.6/fs/namei.c
===================================================================
--- linux-2.6.orig/fs/namei.c
+++ linux-2.6/fs/namei.c
@@ -487,6 +487,8 @@ static int nameidata_dentry_drop_rcu(str
 			goto err_root;
 	}
 	spin_lock(&parent->d_lock);
+	if (parent == dentry)
+		goto same;
 	spin_lock_nested(&dentry->d_lock, DENTRY_D_LOCK_NESTED);
 	if (!__d_rcu_to_refcount(dentry, nd->seq))
 		goto err;
@@ -499,6 +501,8 @@ static int nameidata_dentry_drop_rcu(str
 	BUG_ON(!parent->d_count);
 	parent->d_count++;
 	spin_unlock(&dentry->d_lock);
+
+same:
 	spin_unlock(&parent->d_lock);
 	if (nd->root.mnt) {
 		path_get(&nd->root);

Re: [PATCH] fs: Work around NFS wreckage

[J. R. Okajima]

Thomas Gleixner:
> The dcache scalability work broke NFS root filesystems.
	:::
> Check for parent equal dentry and skip the nested lock to avoid the
> deadlock. I'm sure this is the wrong fix, but at least it "works" :)

With this patch, can you unmount it cleanly?
It skips incrementing d_count for both dentries.
Do we need at least one?


J. R. Okajima

Re: [PATCH] fs: Work around NFS wreckage

[Uwe Kleine-König]

Hello,

On Thu, Jan 13, 2011 at 02:54:30PM +0100, Thomas Gleixner wrote:
> The dcache scalability work broke NFS root filesystems.
> 
> "cd /" results in the following problem:
> 
>     link_path_walk("/",...);
> 	jumps to return_reval
> 	need_reval_dot() returns true for NFS
> 	d_revalidate()
> 		dentry->d_op->d_revalidate(dentry, nd);
> 		 	returns -ECHILD due to nd->flags & LOOKUP_RCU
> 		nameidata_dentry_drop_rcu()
> 			spin_lock(&parent->d_lock);
> 			spin_lock_nested(&dentry->d_lock, DENTRY_D_LOCK_NESTED);
> 
> 			This deadlocks because dentry == parent
> 
> This problem exists for any filesystem which implements d_revalidate.
> 
> Uwe bisected is down to commit 34286d6(fs: rcu-walk aware d_revalidate
> method), but reverting that patch causes different wreckage to show up.
> 
> Check for parent equal dentry and skip the nested lock to avoid the
> deadlock. I'm sure this is the wrong fix, but at least it "works" :)
> 
> Reported-by: Uwe Kleine-Koenig <u.kleine-koenig@pengutronix.de>
> Reported-by: "Ramirez Luna, Omar" <omar.ramirez@ti.com>
> Not-Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
>  fs/namei.c |    4 ++++
>  1 file changed, 4 insertions(+)
> 
> Index: linux-2.6/fs/namei.c
> ===================================================================
> --- linux-2.6.orig/fs/namei.c
> +++ linux-2.6/fs/namei.c
> @@ -487,6 +487,8 @@ static int nameidata_dentry_drop_rcu(str
>  			goto err_root;
>  	}
>  	spin_lock(&parent->d_lock);
> +	if (parent == dentry)
> +		goto same;
>  	spin_lock_nested(&dentry->d_lock, DENTRY_D_LOCK_NESTED);
>  	if (!__d_rcu_to_refcount(dentry, nd->seq))
>  		goto err;
> @@ -499,6 +501,8 @@ static int nameidata_dentry_drop_rcu(str
>  	BUG_ON(!parent->d_count);
>  	parent->d_count++;
>  	spin_unlock(&dentry->d_lock);
> +
> +same:
>  	spin_unlock(&parent->d_lock);
>  	if (nd->root.mnt) {
>  		path_get(&nd->root);
> 
Note there is a different patch available in the thread here:

	http://thread.gmane.org/gmane.linux.kernel/1087013/focus=1087048

The differences are that it tests for IS_ROOT(dentry) instead of parent ==
dentry (which looks more reasonable IMVHO) and that it increases
parent->d_count even if the test triggered.
(And it doesn't skip the BUG_ONs which hopefully doesn't make a
difference.)

Note I really have no glue about the code below fs/, but I wonder if
the toplevel directories of mounts need some treatment here, too.  (But
I expect that they don't.  So I ask just in case ...)

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH] fs: Work around NFS wreckage

[Uwe Kleine-König]

Hello,

On Thu, Jan 13, 2011 at 11:10:45PM +0900, J. R. Okajima wrote:
> Thomas Gleixner:
> > The dcache scalability work broke NFS root filesystems.
> 	:::
> > Check for parent equal dentry and skip the nested lock to avoid the
> > deadlock. I'm sure this is the wrong fix, but at least it "works" :)
> 
> With this patch, can you unmount it cleanly?
> It skips incrementing d_count for both dentries.
> Do we need at least one?
If you tell me how to test that, I volunteer to try.
Something involving pivot_root?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH] fs: Work around NFS wreckage

[Thomas Gleixner]

On Thu, 13 Jan 2011, J. R. Okajima wrote:
> Thomas Gleixner:
> > The dcache scalability work broke NFS root filesystems.
> 	:::
> > Check for parent equal dentry and skip the nested lock to avoid the
> > deadlock. I'm sure this is the wrong fix, but at least it "works" :)
> 
> With this patch, can you unmount it cleanly?
> It skips incrementing d_count for both dentries.
> Do we need at least one?

Not sure, as we should not end up there at all I guess.

Re: [PATCH] fs: Work around NFS wreckage

[Nick Piggin]

On Fri, Jan 14, 2011 at 3:41 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> On Thu, 13 Jan 2011, J. R. Okajima wrote:
>> Thomas Gleixner:
>> > The dcache scalability work broke NFS root filesystems.
>>       :::
>> > Check for parent equal dentry and skip the nested lock to avoid the
>> > deadlock. I'm sure this is the wrong fix, but at least it "works" :)
>>
>> With this patch, can you unmount it cleanly?
>> It skips incrementing d_count for both dentries.
>> Do we need at least one?
>

Yeah thanks everyone for reporting this.

I have a fix for it in a slightly different way, and a couple of other reported
bugs, which are going through stress testing now.

Thanks,
Nick

Re: [PATCH] fs: Work around NFS wreckage

[J. R. Okajima]

Uwe Kleine-Knig:
> > With this patch, can you unmount it cleanly?
> > It skips incrementing d_count for both dentries.
> > Do we need at least one?
> If you tell me how to test that, I volunteer to try.
> Something involving pivot_root?

For example,
# mount host:/dir /nfs
# chroot /nfs
# umount /nfs

It is ok if /nfs is empty.


J. R. Okajima

Re: [PATCH] fs: Work around NFS wreckage

[Thomas Gleixner]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 840 bytes --]

On Fri, 14 Jan 2011, Nick Piggin wrote:

> On Fri, Jan 14, 2011 at 3:41 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> > On Thu, 13 Jan 2011, J. R. Okajima wrote:
> >> Thomas Gleixner:
> >> > The dcache scalability work broke NFS root filesystems.
> >>       :::
> >> > Check for parent equal dentry and skip the nested lock to avoid the
> >> > deadlock. I'm sure this is the wrong fix, but at least it "works" :)
> >>
> >> With this patch, can you unmount it cleanly?
> >> It skips incrementing d_count for both dentries.
> >> Do we need at least one?
> >
> 
> Yeah thanks everyone for reporting this.
> 
> I have a fix for it in a slightly different way, and a couple of other reported
> bugs, which are going through stress testing now.

Can you point us to the patches, so we can run our own set of
tests in paralell?

Thanks,

	tglx

Re: [PATCH] fs: Work around NFS wreckage

[Sedat Dilek]

On Fri, Jan 14, 2011 at 10:29 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> On Fri, 14 Jan 2011, Nick Piggin wrote:
>
>> On Fri, Jan 14, 2011 at 3:41 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
>> > On Thu, 13 Jan 2011, J. R. Okajima wrote:
>> >> Thomas Gleixner:
>> >> > The dcache scalability work broke NFS root filesystems.
>> >>       :::
>> >> > Check for parent equal dentry and skip the nested lock to avoid the
>> >> > deadlock. I'm sure this is the wrong fix, but at least it "works" :)
>> >>
>> >> With this patch, can you unmount it cleanly?
>> >> It skips incrementing d_count for both dentries.
>> >> Do we need at least one?
>> >
>>
>> Yeah thanks everyone for reporting this.
>>
>> I have a fix for it in a slightly different way, and a couple of other reported
>> bugs, which are going through stress testing now.
>
> Can you point us to the patches, so we can run our own set of
> tests in paralell?
>
> Thanks,
>
>        tglx

I guess several mentionned patches are now in [1] and I am testing
them against linux-next (next-20110114).

- Sedat -

[1] http://git.kernel.org/?p=linux/kernel/git/npiggin/linux-npiggin.git;a=shortlog;h=refs/heads/vfs-scale-working

Re: [PATCH] fs: Work around NFS wreckage

[Linus Torvalds]

On Fri, Jan 14, 2011 at 1:29 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
>
> Can you point us to the patches, so we can run our own set of
> tests in paralell?

They should be in current -git as of yesterday evening.

In particular, I think this one is commit 90dbb77ba48d.

                      Linus

Re: [PATCH] fs: Work around NFS wreckage

[Thomas Gleixner]

On Fri, 14 Jan 2011, Linus Torvalds wrote:

> On Fri, Jan 14, 2011 at 1:29 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> >
> > Can you point us to the patches, so we can run our own set of
> > tests in paralell?
> 
> They should be in current -git as of yesterday evening.
> 
> In particular, I think this one is commit 90dbb77ba48d.

Yep, that works - though it needs the pending fixes from Nick latest
pull request as well

Thanks,

	tglx

Re: [PATCH] fs: Work around NFS wreckage

[Nick Piggin]

On Sat, Jan 15, 2011 at 3:40 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> On Fri, 14 Jan 2011, Linus Torvalds wrote:
>
>> On Fri, Jan 14, 2011 at 1:29 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
>> >
>> > Can you point us to the patches, so we can run our own set of
>> > tests in paralell?
>>
>> They should be in current -git as of yesterday evening.
>>
>> In particular, I think this one is commit 90dbb77ba48d.
>
> Yep, that works - though it needs the pending fixes from Nick latest
> pull request as well

Thanks for reporting and testing.

Nick