[PATCH] Revert "__d_unalias() should refuse to move mountpoints"

[PATCH] Revert "__d_unalias() should refuse to move mountpoints"

[Maarten Lankhorst]

This reverts commit ee3efa91e240f513898050ef305a49a653c8ed90.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@canonical.com>

My thread about the regression seemed to have been ignored, so I can only
conclude nobody objects against a full revert of this patch.

My testcase is simply booting through netboot with / and ~/nfs as separate
nfs filesystems, then doing 'ls ~/nfs' followed by 'ls ~' in a gnome-terminal
window, then I get:

ls: cannot access nfs: Device or resource busy

Similar things seem to happen with ls /, /dev /proc and /sys will no longer work.

Reverting this patch seems to make things work again.

---

diff --git a/fs/dcache.c b/fs/dcache.c
index 16521a9..711f421 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -2387,13 +2387,14 @@ static struct dentry *__d_unalias(struct inode *inode,
 		struct dentry *dentry, struct dentry *alias)
 {
 	struct mutex *m1 = NULL, *m2 = NULL;
-	struct dentry *ret = ERR_PTR(-EBUSY);
+	struct dentry *ret;
 
 	/* If alias and dentry share a parent, then no extra locks required */
 	if (alias->d_parent == dentry->d_parent)
 		goto out_unalias;
 
 	/* See lock_rename() */
+	ret = ERR_PTR(-EBUSY);
 	if (!mutex_trylock(&dentry->d_sb->s_vfs_rename_mutex))
 		goto out_err;
 	m1 = &dentry->d_sb->s_vfs_rename_mutex;
@@ -2401,10 +2402,8 @@ static struct dentry *__d_unalias(struct inode *inode,
 		goto out_err;
 	m2 = &alias->d_parent->d_inode->i_mutex;
 out_unalias:
-	if (likely(!d_mountpoint(alias))) {
-		__d_move(alias, dentry);
-		ret = alias;
-	}
+	__d_move(alias, dentry);
+	ret = alias;
 out_err:
 	spin_unlock(&inode->i_lock);
 	if (m2)

Re: [PATCH] Revert "__d_unalias() should refuse to move mountpoints"

[Eric W. Biederman]

Maarten Lankhorst <maarten.lankhorst@canonical.com> writes:

> This reverts commit ee3efa91e240f513898050ef305a49a653c8ed90.
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@canonical.com>
>
> My thread about the regression seemed to have been ignored, so I can only
> conclude nobody objects against a full revert of this patch.
>
> My testcase is simply booting through netboot with / and ~/nfs as separate
> nfs filesystems, then doing 'ls ~/nfs' followed by 'ls ~' in a gnome-terminal
> window, then I get:

Do I read your description correctly:  Without using a bind mount you
have the same nfs filesystem mounted on / and on ~/nfs?

Something is definitely off with your configuration but if to work you
need to move mount points around then that something seems much deeper
than the __d_unalias change.

What filesystems do you have mounted where?

> ls: cannot access nfs: Device or resource busy
>
> Similar things seem to happen with ls /, /dev /proc and /sys will no longer work.
>
> Reverting this patch seems to make things work again.
>
> ---
>
> diff --git a/fs/dcache.c b/fs/dcache.c
> index 16521a9..711f421 100644
> --- a/fs/dcache.c
> +++ b/fs/dcache.c
> @@ -2387,13 +2387,14 @@ static struct dentry *__d_unalias(struct inode *inode,
>  		struct dentry *dentry, struct dentry *alias)
>  {
>  	struct mutex *m1 = NULL, *m2 = NULL;
> -	struct dentry *ret = ERR_PTR(-EBUSY);
> +	struct dentry *ret;
>  
>  	/* If alias and dentry share a parent, then no extra locks required */
>  	if (alias->d_parent == dentry->d_parent)
>  		goto out_unalias;
>  
>  	/* See lock_rename() */
> +	ret = ERR_PTR(-EBUSY);
>  	if (!mutex_trylock(&dentry->d_sb->s_vfs_rename_mutex))
>  		goto out_err;
>  	m1 = &dentry->d_sb->s_vfs_rename_mutex;
> @@ -2401,10 +2402,8 @@ static struct dentry *__d_unalias(struct inode *inode,
>  		goto out_err;
>  	m2 = &alias->d_parent->d_inode->i_mutex;
>  out_unalias:
> -	if (likely(!d_mountpoint(alias))) {
> -		__d_move(alias, dentry);
> -		ret = alias;
> -	}
> +	__d_move(alias, dentry);
> +	ret = alias;
>  out_err:
>  	spin_unlock(&inode->i_lock);
>  	if (m2)
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Revert "__d_unalias() should refuse to move mountpoints"

[Maarten Lankhorst]

Hey,

Op 25-09-12 05:39, Eric W. Biederman schreef:
> Maarten Lankhorst <maarten.lankhorst@canonical.com> writes:
>
>> This reverts commit ee3efa91e240f513898050ef305a49a653c8ed90.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@canonical.com>
>>
>> My thread about the regression seemed to have been ignored, so I can only
>> conclude nobody objects against a full revert of this patch.
>>
>> My testcase is simply booting through netboot with / and ~/nfs as separate
>> nfs filesystems, then doing 'ls ~/nfs' followed by 'ls ~' in a gnome-terminal
>> window, then I get:
> Do I read your description correctly:  Without using a bind mount you
> have the same nfs filesystem mounted on / and on ~/nfs?
>
> Something is definitely off with your configuration but if to work you
> need to move mount points around then that something seems much deeper
> than the __d_unalias change.
>
> What filesystems do you have mounted where?
>
/ is a nfs filesystem, ~/nfs is a different nfs filesystem. Just doing ls / is enough
to make all filesystems mounted on / return -EBUSY and disappear.

I also have a subdir of ~/nfs/ bind mounted to /lib/modules/$(uname -r)/kernel
for easy debugging so just doing 'make' in the kernel tree is enough to get the
new modules + bzImage, but I don't know if it is a factor in reproducing this bug
or not.

~Maarten

Re: [PATCH] Revert "__d_unalias() should refuse to move mountpoints"

[Eric W. Biederman]

Maarten Lankhorst <maarten.lankhorst@canonical.com> wrote:

>Hey,
>
>Op 25-09-12 05:39, Eric W. Biederman schreef:
>> Maarten Lankhorst <maarten.lankhorst@canonical.com> writes:
>>
>>> This reverts commit ee3efa91e240f513898050ef305a49a653c8ed90.
>>>
>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@canonical.com>
>>>
>>> My thread about the regression seemed to have been ignored, so I can
>only
>>> conclude nobody objects against a full revert of this patch.
>>>
>>> My testcase is simply booting through netboot with / and ~/nfs as
>separate
>>> nfs filesystems, then doing 'ls ~/nfs' followed by 'ls ~' in a
>gnome-terminal
>>> window, then I get:
>> Do I read your description correctly:  Without using a bind mount you
>> have the same nfs filesystem mounted on / and on ~/nfs?
>>
>> Something is definitely off with your configuration but if to work
>you
>> need to move mount points around then that something seems much
>deeper
>> than the __d_unalias change.
>>
>> What filesystems do you have mounted where?
>>
>/ is a nfs filesystem, ~/nfs is a different nfs filesystem.

Are both filesystems on the same server?

Are the two filesystems distinct filesystem on the server?

Unless there is duplication of something somewhere the d_unalias code should not trigger.

> Just doing
>ls / is enough
>to make all filesystems mounted on / return -EBUSY and disappear.
>
>I also have a subdir of ~/nfs/ bind mounted to /lib/modules/$(uname
>-r)/kernel
>for easy debugging so just doing 'make' in the kernel tree is enough to
>get the
>new modules + bzImage, but I don't know if it is a factor in
>reproducing this bug
>or not.

Unlikely.  But interesting.  It at least fits the criteria of showing up to different places.  It should not be enough for d_materialise uniqe.

Eric

Re: [PATCH] Revert "__d_unalias() should refuse to move mountpoints"

[Maarten Lankhorst]

Hey,

Op 25-09-12 09:05, Eric W. Biederman schreef:
> Maarten Lankhorst <maarten.lankhorst@canonical.com> wrote:
>
>> Hey,
>>
>> Op 25-09-12 05:39, Eric W. Biederman schreef:
>>> Maarten Lankhorst <maarten.lankhorst@canonical.com> writes:
>>>
>>>> This reverts commit ee3efa91e240f513898050ef305a49a653c8ed90.
>>>>
>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@canonical.com>
>>>>
>>>> My thread about the regression seemed to have been ignored, so I can
>> only
>>>> conclude nobody objects against a full revert of this patch.
>>>>
>>>> My testcase is simply booting through netboot with / and ~/nfs as
>> separate
>>>> nfs filesystems, then doing 'ls ~/nfs' followed by 'ls ~' in a
>> gnome-terminal
>>>> window, then I get:
>>> Do I read your description correctly:  Without using a bind mount you
>>> have the same nfs filesystem mounted on / and on ~/nfs?
>>>
>>> Something is definitely off with your configuration but if to work
>> you
>>> need to move mount points around then that something seems much
>> deeper
>>> than the __d_unalias change.
>>>
>>> What filesystems do you have mounted where?
>>>
>> / is a nfs filesystem, ~/nfs is a different nfs filesystem.
> Are both filesystems on the same server?
>
> Are the two filesystems distinct filesystem on the server?
>
> Unless there is duplication of something somewhere the d_unalias code should not trigger.

They're both on the same physical filesystem on the server, but unique exports:
/home/mlankhorst/nfs *(no_subtree_check,insecure,rw,all_squash,anonuid=1000,anongid=1000)
/home/mlankhorst/kvm/quantal-amd64 *(no_subtree_check,insecure,rw,no_root_squash)

Rootfs is mounted by the kernel itself, I used a custom init script to mount /lib/modules
early on:

mount -t nfs -o nolock,vers=3 192.168.1.128:/home/mlankhorst/nfs /home/mlankhorst/nfs &&
mkdir -p /lib/modules/$(uname -r)/kernel &&
mount --bind /home/mlankhorst/nfs/linux /lib/modules/$(uname -r)/kernel &&
([ -f /lib/modules/$(uname -r)/modules.symbols ] || depmod)

exec /sbin/init

>> Just doing
>> ls / is enough
>> to make all filesystems mounted on / return -EBUSY and disappear.
>>
>> I also have a subdir of ~/nfs/ bind mounted to /lib/modules/$(uname
>> -r)/kernel
>> for easy debugging so just doing 'make' in the kernel tree is enough to
>> get the
>> new modules + bzImage, but I don't know if it is a factor in
>> reproducing this bug
>> or not.
> Unlikely.  But interesting.  It at least fits the criteria of showing up to different places.  It should not be enough for d_materialise uniqe.
>
Either way until the root cause is found could this patch be reverted?

~Maarten

Re: [PATCH] Revert "__d_unalias() should refuse to move mountpoints"

[Eric W. Biederman]

Maarten Lankhorst <maarten.lankhorst@canonical.com> writes:

> Hey,
>
> Op 25-09-12 09:05, Eric W. Biederman schreef:
>> Maarten Lankhorst <maarten.lankhorst@canonical.com> wrote:
>>
>>> Hey,
>>>
>>> Op 25-09-12 05:39, Eric W. Biederman schreef:
>>>> Maarten Lankhorst <maarten.lankhorst@canonical.com> writes:
>>>>
>>>>> This reverts commit ee3efa91e240f513898050ef305a49a653c8ed90.
>>>>>
>>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@canonical.com>
>>>>>
>>>>> My thread about the regression seemed to have been ignored, so I can
>>> only
>>>>> conclude nobody objects against a full revert of this patch.
>>>>>
>>>>> My testcase is simply booting through netboot with / and ~/nfs as
>>> separate
>>>>> nfs filesystems, then doing 'ls ~/nfs' followed by 'ls ~' in a
>>> gnome-terminal
>>>>> window, then I get:
>>>> Do I read your description correctly:  Without using a bind mount you
>>>> have the same nfs filesystem mounted on / and on ~/nfs?
>>>>
>>>> Something is definitely off with your configuration but if to work
>>> you
>>>> need to move mount points around then that something seems much
>>> deeper
>>>> than the __d_unalias change.
>>>>
>>>> What filesystems do you have mounted where?
>>>>
>>> / is a nfs filesystem, ~/nfs is a different nfs filesystem.
>> Are both filesystems on the same server?
>>
>> Are the two filesystems distinct filesystem on the server?
>>
>> Unless there is duplication of something somewhere the d_unalias code should not trigger.
>
> They're both on the same physical filesystem on the server, but unique exports:
> /home/mlankhorst/nfs *(no_subtree_check,insecure,rw,all_squash,anonuid=1000,anongid=1000)
> /home/mlankhorst/kvm/quantal-amd64 *(no_subtree_check,insecure,rw,no_root_squash)

Modern NFS does some interesting things with disconnected roots and the
like.  I don't think it should be connecting those two filesytems
together because there are no overlapping directories.

I really don't get why using one filesystem causes confusion in the other.

> Rootfs is mounted by the kernel itself, I used a custom init script to mount /lib/modules
> early on:
>
> mount -t nfs -o nolock,vers=3 192.168.1.128:/home/mlankhorst/nfs /home/mlankhorst/nfs &&
> mkdir -p /lib/modules/$(uname -r)/kernel &&
> mount --bind /home/mlankhorst/nfs/linux /lib/modules/$(uname -r)/kernel &&
> ([ -f /lib/modules/$(uname -r)/modules.symbols ] || depmod)
>
> exec /sbin/init

Could you try the following patch?  This should report what directories
cannot be renamed because one of them is a mount point and it gives some
real insight into what is going on.

Eric


diff --git a/fs/dcache.c b/fs/dcache.c
index 8086636..193b7be 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -2374,6 +2374,7 @@ struct dentry *d_ancestor(struct dentry *p1, struct dentry *p2)
 	return NULL;
 }
 
+static char *__dentry_path(struct dentry *dentry, char *buf, int buflen);
 /*
  * This helper attempts to cope with remotely renamed directories
  *
@@ -2401,6 +2402,18 @@ static struct dentry *__d_unalias(struct inode *inode,
 		goto out_err;
 	m2 = &alias->d_parent->d_inode->i_mutex;
 out_unalias:
+#if 1
+	if (d_mountpoint(alias)) {
+		static char buf1[8192];
+		static char buf2[8192];
+		char *alias_name, *dentry_name;
+		alias_name = __dentry_path(alias, buf1, sizeof(buf1));
+		dentry_name = __dentry_path(dentry, buf2, sizeof(buf2));
+
+		printk(KERN_DEBUG "%s: %s -> %s\n",
+		       __func__, alias_name, dentry_name);
+	}
+#endif
 	if (likely(!d_mountpoint(alias))) {
 		__d_move(alias, dentry);
 		ret = alias;

Re: [PATCH] Revert "__d_unalias() should refuse to move mountpoints"

[Maarten Lankhorst]

Op 25-09-12 12:42, Eric W. Biederman schreef:
> Maarten Lankhorst <maarten.lankhorst@canonical.com> writes:
>
>> Hey,
>>
>> Op 25-09-12 09:05, Eric W. Biederman schreef:
>>> Maarten Lankhorst <maarten.lankhorst@canonical.com> wrote:
>>>
>>>> Hey,
>>>>
>>>> Op 25-09-12 05:39, Eric W. Biederman schreef:
>>>>> Maarten Lankhorst <maarten.lankhorst@canonical.com> writes:
>>>>>
>>>>>> This reverts commit ee3efa91e240f513898050ef305a49a653c8ed90.
>>>>>>
>>>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@canonical.com>
>>>>>>
>>>>>> My thread about the regression seemed to have been ignored, so I can
>>>> only
>>>>>> conclude nobody objects against a full revert of this patch.
>>>>>>
>>>>>> My testcase is simply booting through netboot with / and ~/nfs as
>>>> separate
>>>>>> nfs filesystems, then doing 'ls ~/nfs' followed by 'ls ~' in a
>>>> gnome-terminal
>>>>>> window, then I get:
>>>>> Do I read your description correctly:  Without using a bind mount you
>>>>> have the same nfs filesystem mounted on / and on ~/nfs?
>>>>>
>>>>> Something is definitely off with your configuration but if to work
>>>> you
>>>>> need to move mount points around then that something seems much
>>>> deeper
>>>>> than the __d_unalias change.
>>>>>
>>>>> What filesystems do you have mounted where?
>>>>>
>>>> / is a nfs filesystem, ~/nfs is a different nfs filesystem.
>>> Are both filesystems on the same server?
>>>
>>> Are the two filesystems distinct filesystem on the server?
>>>
>>> Unless there is duplication of something somewhere the d_unalias code should not trigger.
>> They're both on the same physical filesystem on the server, but unique exports:
>> /home/mlankhorst/nfs *(no_subtree_check,insecure,rw,all_squash,anonuid=1000,anongid=1000)
>> /home/mlankhorst/kvm/quantal-amd64 *(no_subtree_check,insecure,rw,no_root_squash)
> Modern NFS does some interesting things with disconnected roots and the
> like.  I don't think it should be connecting those two filesytems
> together because there are no overlapping directories.
>
> I really don't get why using one filesystem causes confusion in the other.
>
>> Rootfs is mounted by the kernel itself, I used a custom init script to mount /lib/modules
>> early on:
>>
>> mount -t nfs -o nolock,vers=3 192.168.1.128:/home/mlankhorst/nfs /home/mlankhorst/nfs &&
>> mkdir -p /lib/modules/$(uname -r)/kernel &&
>> mount --bind /home/mlankhorst/nfs/linux /lib/modules/$(uname -r)/kernel &&
>> ([ -f /lib/modules/$(uname -r)/modules.symbols ] || depmod)
>>
>> exec /sbin/init
> Could you try the following patch?  This should report what directories
> cannot be renamed because one of them is a mount point and it gives some
> real insight into what is going on.

ls /
__d_unalias: /dev -> /dev
__d_unalias: /proc -> /proc
__d_unalias: /sys -> /sys

Backtrace with WARN_ON_ONCE on the if check, unsurprisingly __d_unalias was inlined:
WARNING: at fs/dcache.c:2407 d_materialise_unique+0x3ee/0x490()
Hardware name: 1215N
Modules linked in: snd_hda_codec_realtek snd_hda_intel snd_hda_codec snd_hwdep snd_pcm snd_seq_midi snd_rawmidi snd_seq_midi_event snd_seq parport_pc snd_timer snd_seq_device arc4 nouveau ppdev snd eeepc_wmi parport ath9k asus_wmi ttm mac80211 mxm_wmi i915 ath9k_common ath9k_hw drm_kms_helper drm ath soundcore cfg80211 snd_page_alloc video nfsd
Pid: 1452, comm: ls Not tainted 3.6.0-rc4-patser+ #175
Call Trace:
 [<ffffffff8104907a>] warn_slowpath_common+0x7a/0xb0
 [<ffffffff810490c5>] warn_slowpath_null+0x15/0x20
 [<ffffffff8117d52e>] d_materialise_unique+0x3ee/0x490
 [<ffffffff812477cb>] ? nfs_fhget+0x4db/0x5a0
 [<ffffffff81242b00>] nfs_lookup+0x130/0x190
 [<ffffffff8116ff68>] lookup_real+0x18/0x50
 [<ffffffff811704e3>] __lookup_hash+0x33/0x40
 [<ffffffff81708153>] lookup_slow+0x44/0xa8
 [<ffffffff81172806>] path_lookupat+0x236/0x7e0
 [<ffffffff81170702>] ? getname_flags+0x32/0x100
 [<ffffffff8115870c>] ? kmem_cache_alloc+0xdc/0x260
 [<ffffffff81170702>] ? getname_flags+0x32/0x100
 [<ffffffff81172ddc>] do_path_lookup+0x2c/0xc0
 [<ffffffff81175a14>] user_path_at_empty+0x54/0xa0
 [<ffffffff81076f0d>] ? lg_local_unlock+0x3d/0x70
 [<ffffffff8116a1a1>] ? cp_new_stat+0x111/0x130
 [<ffffffff81175a6c>] user_path_at+0xc/0x10
 [<ffffffff8116a3b5>] vfs_fstatat+0x35/0x60
 [<ffffffff8116a3f9>] vfs_lstat+0x19/0x20
 [<ffffffff8116a555>] sys_newlstat+0x15/0x30
 [<ffffffff813924ce>] ? trace_hardirqs_on_thunk+0x3a/0x3f
 [<ffffffff817191a2>] system_call_fastpath+0x16/0x1b

~Maarten

Re: [PATCH] Revert "__d_unalias() should refuse to move mountpoints"

[Eric W. Biederman]

Maarten Lankhorst <maarten.lankhorst@canonical.com> writes:

>> Could you try the following patch?  This should report what directories
>> cannot be renamed because one of them is a mount point and it gives some
>> real insight into what is going on.
>
> ls /
> __d_unalias: /dev -> /dev
> __d_unalias: /proc -> /proc
> __d_unalias: /sys -> /sys

Ok.  That is what I thought was going on.  For some reason nfs is
attempting to recreate an existing dentry.

Does this fix the nfs problem for you?

Eric

diff --git a/fs/dcache.c b/fs/dcache.c
index 8086636..6390f0f 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -2404,6 +2404,9 @@ out_unalias:
 	if (likely(!d_mountpoint(alias))) {
 		__d_move(alias, dentry);
 		ret = alias;
+	} else if ((alias->d_parent == dentry->d_parent) &&
+		   !dentry_cmp(alias, dentry->d_name.name, dentry->d_name.len))
+		ret = alias;
 	}
 out_err:
 	spin_unlock(&inode->i_lock);

Re: [PATCH] Revert "__d_unalias() should refuse to move mountpoints"

[Maarten Lankhorst]

Op 25-09-12 13:29, Eric W. Biederman schreef:
> Maarten Lankhorst <maarten.lankhorst@canonical.com> writes:
>
>>> Could you try the following patch?  This should report what directories
>>> cannot be renamed because one of them is a mount point and it gives some
>>> real insight into what is going on.
>> ls /
>> __d_unalias: /dev -> /dev
>> __d_unalias: /proc -> /proc
>> __d_unalias: /sys -> /sys
> Ok.  That is what I thought was going on.  For some reason nfs is
> attempting to recreate an existing dentry.
>
> Does this fix the nfs problem for you?
>
> Eric
>
The patch seems to fix it, thanks!

~Maarten

Re: [PATCH] Revert "__d_unalias() should refuse to move mountpoints"

[Maarten Lankhorst]

Op 25-09-12 13:29, Eric W. Biederman schreef:
> Maarten Lankhorst <maarten.lankhorst@canonical.com> writes:
>
>>> Could you try the following patch?  This should report what directories
>>> cannot be renamed because one of them is a mount point and it gives some
>>> real insight into what is going on.
>> ls /
>> __d_unalias: /dev -> /dev
>> __d_unalias: /proc -> /proc
>> __d_unalias: /sys -> /sys
> Ok.  That is what I thought was going on.  For some reason nfs is
> attempting to recreate an existing dentry.
>
> Does this fix the nfs problem for you?
>
> Eric
>
> diff --git a/fs/dcache.c b/fs/dcache.c
> index 8086636..6390f0f 100644
> --- a/fs/dcache.c
> +++ b/fs/dcache.c
> @@ -2404,6 +2404,9 @@ out_unalias:
>  	if (likely(!d_mountpoint(alias))) {
>  		__d_move(alias, dentry);
>  		ret = alias;
> +	} else if ((alias->d_parent == dentry->d_parent) &&
> +		   !dentry_cmp(alias, dentry->d_name.name, dentry->d_name.len))
> +		ret = alias;
>  	}
>  out_err:
>  	spin_unlock(&inode->i_lock);
>
>
Are you going to send this in? I don't see a fix for this in linus' kernel yet.

Reported-by: Maarten Lankhorst <maarten.lankhorst@canonical.com>
Tested-by: Maarten Lankhorst <maarten.lankhorst@canonical.com>

~Maarten

Re: [PATCH] Revert "__d_unalias() should refuse to move mountpoints"

[Al Viro]

On Tue, Sep 25, 2012 at 04:29:58AM -0700, Eric W. Biederman wrote:
> Maarten Lankhorst <maarten.lankhorst@canonical.com> writes:
> 
> >> Could you try the following patch?  This should report what directories
> >> cannot be renamed because one of them is a mount point and it gives some
> >> real insight into what is going on.
> >
> > ls /
> > __d_unalias: /dev -> /dev
> > __d_unalias: /proc -> /proc
> > __d_unalias: /sys -> /sys
> 
> Ok.  That is what I thought was going on.  For some reason nfs is
> attempting to recreate an existing dentry.
> 
> Does this fix the nfs problem for you?
> 
> Eric
> 
> diff --git a/fs/dcache.c b/fs/dcache.c
> index 8086636..6390f0f 100644
> --- a/fs/dcache.c
> +++ b/fs/dcache.c
> @@ -2404,6 +2404,9 @@ out_unalias:
>  	if (likely(!d_mountpoint(alias))) {
>  		__d_move(alias, dentry);
>  		ret = alias;
> +	} else if ((alias->d_parent == dentry->d_parent) &&
> +		   !dentry_cmp(alias, dentry->d_name.name, dentry->d_name.len))
> +		ret = alias;
>  	}

The interesting question is why the hell had it decided that preexisting
dentry was not good enough for it?  Note that we have arrived to nfs_lookup()
after we'd decided *not* to use the damn alias.  The trace posted upthread
went __lookup_hash() -> lookup_real().  It means that lookup_dcache()
has not produced this one.  And no, even if ->d_revalidate() decided it
was no good, the logics in d_invalidate() would've said "busy" and we'd
gone with that dentry anyway.  So it means that d_lookup() has not
found it at all.

IOW, something out there is blindly unhashing mountpoint dentries; that's
where the real root of the problem seems to be.  Could you slap
WARN_ON(d_mountpoint(dentry)) in __d_drop() and see what it catches?

Re: [PATCH] Revert "__d_unalias() should refuse to move mountpoints"

[Al Viro]

On Thu, Nov 29, 2012 at 08:06:12PM +0000, Al Viro wrote:
> On Tue, Sep 25, 2012 at 04:29:58AM -0700, Eric W. Biederman wrote:
> > Maarten Lankhorst <maarten.lankhorst@canonical.com> writes:
> > 
> > >> Could you try the following patch?  This should report what directories
> > >> cannot be renamed because one of them is a mount point and it gives some
> > >> real insight into what is going on.
> > >
> > > ls /
> > > __d_unalias: /dev -> /dev
> > > __d_unalias: /proc -> /proc
> > > __d_unalias: /sys -> /sys
> > 
> > Ok.  That is what I thought was going on.  For some reason nfs is
> > attempting to recreate an existing dentry.
> > 
> > Does this fix the nfs problem for you?
> > 
> > Eric
> > 
> > diff --git a/fs/dcache.c b/fs/dcache.c
> > index 8086636..6390f0f 100644
> > --- a/fs/dcache.c
> > +++ b/fs/dcache.c
> > @@ -2404,6 +2404,9 @@ out_unalias:
> >  	if (likely(!d_mountpoint(alias))) {
> >  		__d_move(alias, dentry);
> >  		ret = alias;
> > +	} else if ((alias->d_parent == dentry->d_parent) &&
> > +		   !dentry_cmp(alias, dentry->d_name.name, dentry->d_name.len))
> > +		ret = alias;
> >  	}
> 
> The interesting question is why the hell had it decided that preexisting
> dentry was not good enough for it?  Note that we have arrived to nfs_lookup()
> after we'd decided *not* to use the damn alias.  The trace posted upthread
> went __lookup_hash() -> lookup_real().  It means that lookup_dcache()
> has not produced this one.  And no, even if ->d_revalidate() decided it
> was no good, the logics in d_invalidate() would've said "busy" and we'd
> gone with that dentry anyway.  So it means that d_lookup() has not
> found it at all.
> 
> IOW, something out there is blindly unhashing mountpoint dentries; that's
> where the real root of the problem seems to be.  Could you slap
> WARN_ON(d_mountpoint(dentry)) in __d_drop() and see what it catches?

Ho-hum...  nfs_prime_dcache() seems to be the likely suspect.  What happens
if we get nfs_same_file() failing for some reason for a mountpoint there?
Or for a busy directory, for that matter...

Guys, could somebody with reproducer see if we step into the else side of
                if (nfs_same_file(dentry, entry)) {
                        nfs_refresh_inode(dentry->d_inode, entry->fattr);
                        goto out;
                } else {
                        d_drop(dentry);
                        dput(dentry);
                }
in nfs_prime_dcache() with dentry being a mountpoint?  If nothing else,
I would suggest replacing that d_drop(dentry) with
	if (d_invalidate(dentry) != 0)
		goto out;
in there.

Re: [PATCH] Revert "__d_unalias() should refuse to move mountpoints"

[Al Viro]

On Thu, Nov 29, 2012 at 08:53:34PM +0000, Al Viro wrote:
> On Thu, Nov 29, 2012 at 08:06:12PM +0000, Al Viro wrote:
> > On Tue, Sep 25, 2012 at 04:29:58AM -0700, Eric W. Biederman wrote:
> > > Maarten Lankhorst <maarten.lankhorst@canonical.com> writes:
> > > 
> > > >> Could you try the following patch?  This should report what directories
> > > >> cannot be renamed because one of them is a mount point and it gives some
> > > >> real insight into what is going on.
> > > >
> > > > ls /
> > > > __d_unalias: /dev -> /dev
> > > > __d_unalias: /proc -> /proc
> > > > __d_unalias: /sys -> /sys
> > > 
> > > Ok.  That is what I thought was going on.  For some reason nfs is
> > > attempting to recreate an existing dentry.
> > > 
> > > Does this fix the nfs problem for you?
> > > 
> > > Eric
> > > 
> > > diff --git a/fs/dcache.c b/fs/dcache.c
> > > index 8086636..6390f0f 100644
> > > --- a/fs/dcache.c
> > > +++ b/fs/dcache.c
> > > @@ -2404,6 +2404,9 @@ out_unalias:
> > >  	if (likely(!d_mountpoint(alias))) {
> > >  		__d_move(alias, dentry);
> > >  		ret = alias;
> > > +	} else if ((alias->d_parent == dentry->d_parent) &&
> > > +		   !dentry_cmp(alias, dentry->d_name.name, dentry->d_name.len))
> > > +		ret = alias;
> > >  	}
> > 
> > The interesting question is why the hell had it decided that preexisting
> > dentry was not good enough for it?  Note that we have arrived to nfs_lookup()
> > after we'd decided *not* to use the damn alias.  The trace posted upthread
> > went __lookup_hash() -> lookup_real().  It means that lookup_dcache()
> > has not produced this one.  And no, even if ->d_revalidate() decided it
> > was no good, the logics in d_invalidate() would've said "busy" and we'd
> > gone with that dentry anyway.  So it means that d_lookup() has not
> > found it at all.
> > 
> > IOW, something out there is blindly unhashing mountpoint dentries; that's
> > where the real root of the problem seems to be.  Could you slap
> > WARN_ON(d_mountpoint(dentry)) in __d_drop() and see what it catches?
> 
> Ho-hum...  nfs_prime_dcache() seems to be the likely suspect.  What happens
> if we get nfs_same_file() failing for some reason for a mountpoint there?
> Or for a busy directory, for that matter...
> 
> Guys, could somebody with reproducer see if we step into the else side of
>                 if (nfs_same_file(dentry, entry)) {
>                         nfs_refresh_inode(dentry->d_inode, entry->fattr);
>                         goto out;
>                 } else {
>                         d_drop(dentry);
>                         dput(dentry);
>                 }
> in nfs_prime_dcache() with dentry being a mountpoint?  If nothing else,
> I would suggest replacing that d_drop(dentry) with
> 	if (d_invalidate(dentry) != 0)
> 		goto out;
> in there.

Guys, could you test the following and see if it fixes the breakage?  If so,
we need to figure out what's making nfs_same_file() spew apparent false
negatives...

diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index ce8cb92..55436f5 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -450,7 +450,10 @@ void nfs_prime_dcache(struct dentry *parent, struct nfs_entry *entry)
 			nfs_refresh_inode(dentry->d_inode, entry->fattr);
 			goto out;
 		} else {
-			d_drop(dentry);
+			if (d_invalidate(dentry) != 0) {
+				WARN_ON(1);
+				goto out;
+			}
 			dput(dentry);
 		}
 	}
h/openrisc/kernel/signal.c   |    6 ++----
 arch/score/kernel/signal.c      |    7 ++-----
 arch/sh/kernel/signal_64.c      |    6 ++----
 arch/um/kernel/exec.c           |    3 ++-
 5 files changed, 9 insertions(+), 15 deletions(-)

Re: [PATCH] Revert "__d_unalias() should refuse to move mountpoints"

[Al Viro]

On Thu, Nov 29, 2012 at 09:30:34PM +0000, Al Viro wrote:

> +			if (d_invalidate(dentry) != 0) {
> +				WARN_ON(1);
> +				goto out;
> +			}
>  			dput(dentry);
>  		}
>  	}
> h/openrisc/kernel/signal.c   |    6 ++----
>  arch/score/kernel/signal.c      |    7 ++-----
>  arch/sh/kernel/signal_64.c      |    6 ++----
>  arch/um/kernel/exec.c           |    3 ++-
>  5 files changed, 9 insertions(+), 15 deletions(-)

*boggle*

Looks like scp has _not_ truncated the target until the end of transmission
(and I've done :r <file> from vi before it had done that).  Weird...  The
file looks normal now, on both boxen.  Oh, well...

Re: [PATCH] Revert "__d_unalias() should refuse to move mountpoints"

[Maarten Lankhorst]

Hey,

Op 29-11-12 21:06, Al Viro schreef:
> On Tue, Sep 25, 2012 at 04:29:58AM -0700, Eric W. Biederman wrote:
>> Maarten Lankhorst <maarten.lankhorst@canonical.com> writes:
>>
>>>> Could you try the following patch?  This should report what directories
>>>> cannot be renamed because one of them is a mount point and it gives some
>>>> real insight into what is going on.
>>> ls /
>>> __d_unalias: /dev -> /dev
>>> __d_unalias: /proc -> /proc
>>> __d_unalias: /sys -> /sys
>> Ok.  That is what I thought was going on.  For some reason nfs is
>> attempting to recreate an existing dentry.
>>
>> Does this fix the nfs problem for you?
>>
>> Eric
>>
>> diff --git a/fs/dcache.c b/fs/dcache.c
>> index 8086636..6390f0f 100644
>> --- a/fs/dcache.c
>> +++ b/fs/dcache.c
>> @@ -2404,6 +2404,9 @@ out_unalias:
>>  	if (likely(!d_mountpoint(alias))) {
>>  		__d_move(alias, dentry);
>>  		ret = alias;
>> +	} else if ((alias->d_parent == dentry->d_parent) &&
>> +		   !dentry_cmp(alias, dentry->d_name.name, dentry->d_name.len))
>> +		ret = alias;
>>  	}
> The interesting question is why the hell had it decided that preexisting
> dentry was not good enough for it?  Note that we have arrived to nfs_lookup()
> after we'd decided *not* to use the damn alias.  The trace posted upthread
> went __lookup_hash() -> lookup_real().  It means that lookup_dcache()
> has not produced this one.  And no, even if ->d_revalidate() decided it
> was no good, the logics in d_invalidate() would've said "busy" and we'd
> gone with that dentry anyway.  So it means that d_lookup() has not
> found it at all.
>
> IOW, something out there is blindly unhashing mountpoint dentries; that's
> where the real root of the problem seems to be.  Could you slap
> WARN_ON(d_mountpoint(dentry)) in __d_drop() and see what it catches?
>
Sorry for replying so late, I thought I wasn't hitting the bug any more, I was wrong..

------------[ cut here ]------------
WARNING: at fs/dcache.c:452 d_drop+0x58/0x60()
Hardware name: Aspire M3985
Modules linked in: snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_intel snd_hda_codec snd_hwdep snd_pcm snd_seq_midi snd_rawmidi snd_seq_midi_event snd_seq kvm_intel kvm snd_timer snd_seq_device radeon snd usb_storage parport_pc ttm soundcore drm_kms_helper snd_page_alloc ppdev drm parport mei agpgart netconsole configfs nfsd
Pid: 1497, comm: ls Not tainted 3.7.0-rc4-patser+ #517
Call Trace:
 [<ffffffff8104cd8f>] warn_slowpath_common+0x7f/0xc0
 [<ffffffff8104cdea>] warn_slowpath_null+0x1a/0x20
 [<ffffffff81187be8>] d_drop+0x58/0x60
 [<ffffffff81256881>] nfs_readdir_page_filler+0x271/0x460
 [<ffffffff81257e59>] nfs_readdir_xdr_to_array+0x1f9/0x2e0
 [<ffffffff81257f66>] nfs_readdir_filler+0x26/0x90
 [<ffffffff81119e15>] ? add_to_page_cache_lru+0x35/0x50
 [<ffffffff8111a622>] do_read_cache_page+0x82/0x1a0
 [<ffffffff81257f40>] ? nfs_readdir_xdr_to_array+0x2e0/0x2e0
 [<ffffffff81183d40>] ? sys_ioctl+0xb0/0xb0
 [<ffffffff8111a78c>] read_cache_page_async+0x1c/0x20
 [<ffffffff8111a79e>] read_cache_page+0xe/0x20
 [<ffffffff81258327>] nfs_readdir+0x137/0x510
 [<ffffffff811840e1>] ? vfs_readdir+0x81/0xf0
 [<ffffffff8126dfe0>] ? nfs3_xdr_dec_getattr3res+0x80/0x80
 [<ffffffff81183d40>] ? sys_ioctl+0xb0/0xb0
 [<ffffffff81184118>] vfs_readdir+0xb8/0xf0
 [<ffffffff8118426e>] sys_getdents+0x8e/0x120
 [<ffffffff81753394>] tracesys+0xdd/0xe2
---[ end trace 61d6a607ecd4e587 ]---
------------[ cut here ]------------

Re: [PATCH] Revert "__d_unalias() should refuse to move mountpoints"

[Maarten Lankhorst]

Op 04-12-12 11:33, Maarten Lankhorst schreef:
> Hey,
>
> Op 29-11-12 21:06, Al Viro schreef:
>> On Tue, Sep 25, 2012 at 04:29:58AM -0700, Eric W. Biederman wrote:
>>> Maarten Lankhorst <maarten.lankhorst@canonical.com> writes:
>>>
>>>>> Could you try the following patch?  This should report what directories
>>>>> cannot be renamed because one of them is a mount point and it gives some
>>>>> real insight into what is going on.
>>>> ls /
>>>> __d_unalias: /dev -> /dev
>>>> __d_unalias: /proc -> /proc
>>>> __d_unalias: /sys -> /sys
>>> Ok.  That is what I thought was going on.  For some reason nfs is
>>> attempting to recreate an existing dentry.
>>>
>>> Does this fix the nfs problem for you?
>>>
>>> Eric
>>>
>>> diff --git a/fs/dcache.c b/fs/dcache.c
>>> index 8086636..6390f0f 100644
>>> --- a/fs/dcache.c
>>> +++ b/fs/dcache.c
>>> @@ -2404,6 +2404,9 @@ out_unalias:
>>>  	if (likely(!d_mountpoint(alias))) {
>>>  		__d_move(alias, dentry);
>>>  		ret = alias;
>>> +	} else if ((alias->d_parent == dentry->d_parent) &&
>>> +		   !dentry_cmp(alias, dentry->d_name.name, dentry->d_name.len))
>>> +		ret = alias;
>>>  	}
>> The interesting question is why the hell had it decided that preexisting
>> dentry was not good enough for it?  Note that we have arrived to nfs_lookup()
>> after we'd decided *not* to use the damn alias.  The trace posted upthread
>> went __lookup_hash() -> lookup_real().  It means that lookup_dcache()
>> has not produced this one.  And no, even if ->d_revalidate() decided it
>> was no good, the logics in d_invalidate() would've said "busy" and we'd
>> gone with that dentry anyway.  So it means that d_lookup() has not
>> found it at all.
>>
>> IOW, something out there is blindly unhashing mountpoint dentries; that's
>> where the real root of the problem seems to be.  Could you slap
>> WARN_ON(d_mountpoint(dentry)) in __d_drop() and see what it catches?
>>
> Sorry for replying so late, I thought I wasn't hitting the bug any more, I was wrong..
>
> ------------[ cut here ]------------
> WARNING: at fs/dcache.c:452 d_drop+0x58/0x60()
> Hardware name: Aspire M3985
> Modules linked in: snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_intel snd_hda_codec snd_hwdep snd_pcm snd_seq_midi snd_rawmidi snd_seq_midi_event snd_seq kvm_intel kvm snd_timer snd_seq_device radeon snd usb_storage parport_pc ttm soundcore drm_kms_helper snd_page_alloc ppdev drm parport mei agpgart netconsole configfs nfsd
> Pid: 1497, comm: ls Not tainted 3.7.0-rc4-patser+ #517
> Call Trace:
>  [<ffffffff8104cd8f>] warn_slowpath_common+0x7f/0xc0
>  [<ffffffff8104cdea>] warn_slowpath_null+0x1a/0x20
>  [<ffffffff81187be8>] d_drop+0x58/0x60
>  [<ffffffff81256881>] nfs_readdir_page_filler+0x271/0x460
>  [<ffffffff81257e59>] nfs_readdir_xdr_to_array+0x1f9/0x2e0
>  [<ffffffff81257f66>] nfs_readdir_filler+0x26/0x90
>  [<ffffffff81119e15>] ? add_to_page_cache_lru+0x35/0x50
>  [<ffffffff8111a622>] do_read_cache_page+0x82/0x1a0
>  [<ffffffff81257f40>] ? nfs_readdir_xdr_to_array+0x2e0/0x2e0
>  [<ffffffff81183d40>] ? sys_ioctl+0xb0/0xb0
>  [<ffffffff8111a78c>] read_cache_page_async+0x1c/0x20
>  [<ffffffff8111a79e>] read_cache_page+0xe/0x20
>  [<ffffffff81258327>] nfs_readdir+0x137/0x510
>  [<ffffffff811840e1>] ? vfs_readdir+0x81/0xf0
>  [<ffffffff8126dfe0>] ? nfs3_xdr_dec_getattr3res+0x80/0x80
>  [<ffffffff81183d40>] ? sys_ioctl+0xb0/0xb0
>  [<ffffffff81184118>] vfs_readdir+0xb8/0xf0
>  [<ffffffff8118426e>] sys_getdents+0x8e/0x120
>  [<ffffffff81753394>] tracesys+0xdd/0xe2
> ---[ end trace 61d6a607ecd4e587 ]---
> ------------[ cut here ]------------
>
Using addr2line, it is indeed nfs_prime_dcache dropping the entry, in the !nfs_same_file case.

~Maarten