Revert "__d_unalias() should refuse to move mountpoints"

PATCH In review Maarten Lankhorst 3 participants 16 messages Sep 24, 2012 – Dec 4, 2012

Subsystems (2)

VFS NFS client

lore

Share & tools

lore

Maarten Lankhorst

author

2012-09-24 17:45 UTC #0 lore raw

PATCH 1/1 Revert "__d_unalias() should refuse to move mountpoints"

This reverts commit ee3efa91e240f513898050ef305a49a653c8ed90.

Signed-off-by Maarten Lankhorst <[email protected]>

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.

---

fs/dcache.c +4 -5

index 16521a9..711f421 100644
@@ -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)

Eric W. Biederman

2012-09-25 03:39 UTC #1 lore raw

Quoted 10 lines · Maarten Lankhorst

This reverts commit ee3efa91e240f513898050ef305a49a653c8ed90.
Signed-off-by Maarten Lankhorst <[email protected]>
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?

Quoted 46 lines

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.

---

fs/dcache.c +4 -6

index 16521a9..711f421 100644
@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Maarten Lankhorst

author

2012-09-25 06:42 UTC #2 lore raw

Hey,

Op 25-09-12 05:39, Eric W. Biederman schreef:

Quoted 12 lines

Quoted 10 lines · Maarten Lankhorst
This reverts commit ee3efa91e240f513898050ef305a49a653c8ed90.
Signed-off-by Maarten Lankhorst <[email protected]>
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:

Quoted 9 lines

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

Eric W. Biederman

2012-09-25 07:05 UTC #3 lore raw

Quoted 22 lines · Maarten Lankhorst

Hey,
Op 25-09-12 05:39, Eric W. Biederman schreef:
Quoted 7 lines
Quoted 5 lines · Maarten Lankhorst
This reverts commit ee3efa91e240f513898050ef305a49a653c8ed90.
Signed-off-by Maarten Lankhorst <[email protected]>
My thread about the regression seemed to have been ignored, so I can
only
Quoted 3 lines
Quoted 3 lines
conclude nobody objects against a full revert of this patch.
My testcase is simply booting through netboot with / and ~/nfs as
separate
Quoted 1 line
Quoted 1 line
nfs filesystems, then doing 'ls ~/nfs' followed by 'ls ~' in a
gnome-terminal
Quoted 5 lines
Quoted 1 line
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

Quoted 8 lines

you
Quoted 1 line
need to move mount points around then that something seems much
deeper
Quoted 4 lines
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.

Quoted 11 lines

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

Maarten Lankhorst

author

2012-09-25 09:04 UTC #4 lore raw

Hey,

Op 25-09-12 09:05, Eric W. Biederman schreef:

Quoted 29 lines

Quoted 27 lines · Maarten Lankhorst
Hey,
Op 25-09-12 05:39, Eric W. Biederman schreef:
Quoted 7 lines
Quoted 5 lines · Maarten Lankhorst
This reverts commit ee3efa91e240f513898050ef305a49a653c8ed90.
Signed-off-by Maarten Lankhorst <[email protected]>
My thread about the regression seemed to have been ignored, so I can
only
Quoted 3 lines
Quoted 3 lines
conclude nobody objects against a full revert of this patch.
My testcase is simply booting through netboot with / and ~/nfs as
separate
Quoted 1 line
Quoted 1 line
nfs filesystems, then doing 'ls ~/nfs' followed by 'ls ~' in a
gnome-terminal
Quoted 5 lines
Quoted 1 line
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
Quoted 1 line
need to move mount points around then that something seems much
deeper
Quoted 2 lines
than the __d_unalias change.

Quoted 8 lines

Quoted 3 lines
Quoted 2 lines
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

Quoted 4 lines

Quoted 4 lines
Just doing
ls / is enough
to make all filesystems mounted on / return -EBUSY and disappear.

Quoted 9 lines

Quoted 7 lines
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

Eric W. Biederman

2012-09-25 10:42 UTC #5 lore raw

Quoted 37 lines · Maarten Lankhorst

Hey,
Op 25-09-12 09:05, Eric W. Biederman schreef:
Quoted 34 lines
Quoted 22 lines · Maarten Lankhorst
Hey,
Op 25-09-12 05:39, Eric W. Biederman schreef:
Quoted 7 lines
Quoted 5 lines · Maarten Lankhorst
This reverts commit ee3efa91e240f513898050ef305a49a653c8ed90.
Signed-off-by Maarten Lankhorst <[email protected]>
My thread about the regression seemed to have been ignored, so I can
only
Quoted 3 lines
Quoted 3 lines
conclude nobody objects against a full revert of this patch.
My testcase is simply booting through netboot with / and ~/nfs as
separate
Quoted 1 line
Quoted 1 line
nfs filesystems, then doing 'ls ~/nfs' followed by 'ls ~' in a
gnome-terminal
Quoted 5 lines
Quoted 1 line
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
Quoted 8 lines
you
Quoted 1 line
need to move mount points around then that something seems much
deeper
Quoted 4 lines
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?

Quoted 7 lines

Quoted 3 lines
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.

Quoted 9 lines

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

fs/dcache.c +13 -0

index 8086636..193b7be 100644
@@ -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;

Maarten Lankhorst

author

2012-09-25 11:03 UTC #6 lore raw

Op 25-09-12 12:42, Eric W. Biederman schreef:

Quoted 59 lines

Quoted 35 lines · Maarten Lankhorst
Hey,
Op 25-09-12 09:05, Eric W. Biederman schreef:
Quoted 32 lines
Quoted 30 lines · Maarten Lankhorst
Hey,
Op 25-09-12 05:39, Eric W. Biederman schreef:
Quoted 7 lines
Quoted 5 lines · Maarten Lankhorst
This reverts commit ee3efa91e240f513898050ef305a49a653c8ed90.
Signed-off-by Maarten Lankhorst <[email protected]>
My thread about the regression seemed to have been ignored, so I can
only
Quoted 3 lines
Quoted 3 lines
conclude nobody objects against a full revert of this patch.
My testcase is simply booting through netboot with / and ~/nfs as
separate
Quoted 1 line
Quoted 1 line
nfs filesystems, then doing 'ls ~/nfs' followed by 'ls ~' in a
gnome-terminal
Quoted 5 lines
Quoted 1 line
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
Quoted 1 line
need to move mount points around then that something seems much
deeper
Quoted 4 lines
than the __d_unalias change.
What filesystems do you have mounted where?
/ is a nfs filesystem, ~/nfs is a different nfs filesystem.
Quoted 8 lines
Quoted 5 lines
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.
Quoted 8 lines
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)

Quoted 4 lines

Quoted 1 line
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

Eric W. Biederman

2012-09-25 11:29 UTC #7 lore raw

Quoted 8 lines · Maarten Lankhorst

Quoted 3 lines
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

fs/dcache.c +3 -0

index 8086636..6390f0f 100644
@@ -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);

Maarten Lankhorst

author

2012-09-25 11:59 UTC #8 lore raw

Op 25-09-12 13:29, Eric W. Biederman schreef:

Quoted 9 lines

Quoted 7 lines · Maarten Lankhorst
Quoted 3 lines
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

Quoted 7 lines

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

Maarten Lankhorst

author Tested-by

2012-10-12 13:25 UTC #9 lore raw

In reply to Eric W. Biederman

Op 25-09-12 13:29, Eric W. Biederman schreef:

Quoted 23 lines

Quoted 7 lines · Maarten Lankhorst
Quoted 3 lines
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
fs/dcache.c +0 -0
index 8086636..6390f0f 100644

fs/dcache.c out_unalias: · line 2412

Quoted 9 lines

fs/dcache.c +3 -0

@@ -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 <[email protected]>

Tested-by Maarten Lankhorst <[email protected]>

~Maarten

Al Viro

maintainer

2012-11-29 20:06 UTC #10 lore raw

In reply to Eric W. Biederman

Quoted 23 lines · Eric W. Biederman

Quoted 8 lines · Maarten Lankhorst
Quoted 3 lines
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
fs/dcache.c +0 -0
index 8086636..6390f0f 100644

fs/dcache.c out_unalias: · line 2410

Quoted 7 lines

fs/dcache.c +3 -0

@@ -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?

Al Viro

maintainer

2012-11-29 20:53 UTC #11 lore raw

Quoted 41 lines · Al Viro

Quoted 23 lines · Eric W. Biederman
Quoted 8 lines · Maarten Lankhorst
Quoted 3 lines
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
fs/dcache.c +0 -0
index 8086636..6390f0f 100644
fs/dcache.c out_unalias: · line 2410
Quoted 7 lines
fs/dcache.c +3 -0
@@ -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.

Quoted 3 lines

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.

Al Viro

maintainer

2012-11-29 21:30 UTC #12 lore raw

Quoted 62 lines · Al Viro

Quoted 41 lines · Al Viro
Quoted 23 lines · Eric W. Biederman
Quoted 8 lines · Maarten Lankhorst
Quoted 3 lines
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
fs/dcache.c +0 -0
index 8086636..6390f0f 100644
fs/dcache.c out_unalias: · line 2410
Quoted 7 lines
fs/dcache.c +3 -0
@@ -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.
Quoted 3 lines
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;

Quoted 1 line

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...

fs/nfs/dir.c +4 -1

index ce8cb92..55436f5 100644
@@ -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(-)

Al Viro

maintainer

2012-11-29 22:09 UTC #13 lore raw

Quoted 8 lines · Al Viro

fs/nfs/dir.c +4 -0

+			if (d_invalidate(dentry) != 0) {
+				WARN_ON(1);
+				goto out;
+			}
 			dput(dentry);
 		}
 	}

h/openrisc/kernel/signal.c | 6 ++----

Quoted 4 lines

 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...

Maarten Lankhorst

author

2012-12-04 10:33 UTC #14 lore raw

In reply to Al Viro

Hey,

Op 29-11-12 21:06, Al Viro schreef:

Quoted 38 lines · Eric W. Biederman

Quoted 21 lines
Quoted 7 lines · Maarten Lankhorst
Quoted 3 lines
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
fs/dcache.c +0 -0
index 8086636..6390f0f 100644
fs/dcache.c out_unalias: · line 2410
Quoted 7 lines
fs/dcache.c +3 -0
@@ -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.

Quoted 4 lines

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 ]------------

Maarten Lankhorst

author

2012-12-04 10:37 UTC #15 lore raw

Op 04-12-12 11:33, Maarten Lankhorst schreef:

Quoted 66 lines

Hey,
Op 29-11-12 21:06, Al Viro schreef:
Quoted 38 lines · Eric W. Biederman
Quoted 21 lines
Quoted 7 lines · Maarten Lankhorst
Quoted 3 lines
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
fs/dcache.c +0 -0
index 8086636..6390f0f 100644
fs/dcache.c out_unalias: · line 2410
Quoted 7 lines
fs/dcache.c +3 -0
@@ -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.
Quoted 4 lines
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

Quoted 9 lines

 [<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