Hi,
I've had an internal report of the following error:
[145405.949506] WARNING: at /usr/src/packages/BUILD/kernel-default-2.6.32.8/linux-2.6.32/fs/sysfs/dir.c:491 sysfs_add_one+0xf4/0x120()
[145405.949516] sysfs: cannot create duplicate filename '/devices/virtual/bdi/0:26'
...
[145405.949598] Pid: 8745, comm: mount.nfs Not tainted 2.6.32.8-0.3-default #1
[145405.949601] Call Trace:
...
[145405.949644] [<ffffffff811877c4>] sysfs_add_one+0xf4/0x120
[145405.949650] [<ffffffff811883c0>] create_dir+0x60/0xb0
[145405.949656] [<ffffffff81188441>] sysfs_create_dir+0x31/0x50
[145405.949663] [<ffffffff8120a8ef>] kobject_add_internal+0xff/0x260
[145405.949669] [<ffffffff8120ac56>] kobject_add+0x46/0x70
[145405.949675] [<ffffffff812c06b6>] device_add+0xc6/0x4e0
[145405.949682] [<ffffffff812c0c2f>] device_create_vargs+0x13f/0x150
[145405.949689] [<ffffffff810eceea>] bdi_register+0x7a/0x180
[145405.949707] [<ffffffffa04fb006>] nfs_get_sb+0x316/0x430 [nfs]
[145405.949735] [<ffffffff8112195d>] vfs_kern_mount+0x7d/0x1b0
[145405.949741] [<ffffffff81121af3>] do_kern_mount+0x53/0x120
[145405.949748] [<ffffffff8113b7dc>] do_mount+0x21c/0x250
[145405.949754] [<ffffffff8113b8d0>] sys_mount+0xc0/0xf0
i.e. it tried to bdi_register 0:26 while it was already registered.
Looking through the code, it seems that if nfs_get_sb calls
nfs_bdi_register, but then doesn't set ->s_root, then ->put_super
is not called (generic_shutdown_super only calls ->put_super if s_root
is set), so the bdi is never unregistered. The next mount
would then trigger the above error.
I haven't found a way to trigger this situation, but it seems like it
should be possible.
Does this patch make sense?
Thanks,
NeilBrown
----------------------------
NFS: ensure bdi_unregister is called on mount failure.
bdi_unregister is called by nfs_put_super which is only called by
generic_shutdown_super if ->s_root is not NULL. So if we error out
in a circumstance where we called nfs_bdi_register (i.e. server !=
NULL) but have not set s_root, then we need to call bdi_unregister
explicitly in nfs_get_sb.
Signed-off-by: NeilBrown <[email protected]>
diff --git a/fs/nfs/super.c b/fs/nfs/super.c
index f1afee4..da11ec9 100644
--- a/fs/nfs/super.c
+++ b/fs/nfs/super.c
@@ -2214,7 +2214,7 @@ static int nfs_get_sb(struct file_system_type *fs_type,
} else {
error = nfs_bdi_register(server);
if (error)
- goto error_splat_super;
+ goto error_splat_bdi;
}
if (!s->s_root) {
@@ -2256,6 +2256,9 @@ out_err_nosb:
error_splat_root:
dput(mntroot);
error_splat_super:
+ if (server && !s->s_root)
+ bdi_unregister(&server->backing_dev_info);
+error_splat_bdi:
deactivate_locked_super(s);
goto out;
}
On Thu, 2010-03-11 at 09:13 +1100, Neil Brown wrote:
>
> Hi,
> I've had an internal report of the following error:
>
>
> [145405.949506] WARNING: at /usr/src/packages/BUILD/kernel-default-2.6.32.8/linux-2.6.32/fs/sysfs/dir.c:491 sysfs_add_one+0xf4/0x120()
> [145405.949516] sysfs: cannot create duplicate filename '/devices/virtual/bdi/0:26'
> ...
> [145405.949598] Pid: 8745, comm: mount.nfs Not tainted 2.6.32.8-0.3-default #1
> [145405.949601] Call Trace:
> ...
> [145405.949644] [<ffffffff811877c4>] sysfs_add_one+0xf4/0x120
> [145405.949650] [<ffffffff811883c0>] create_dir+0x60/0xb0
> [145405.949656] [<ffffffff81188441>] sysfs_create_dir+0x31/0x50
> [145405.949663] [<ffffffff8120a8ef>] kobject_add_internal+0xff/0x260
> [145405.949669] [<ffffffff8120ac56>] kobject_add+0x46/0x70
> [145405.949675] [<ffffffff812c06b6>] device_add+0xc6/0x4e0
> [145405.949682] [<ffffffff812c0c2f>] device_create_vargs+0x13f/0x150
> [145405.949689] [<ffffffff810eceea>] bdi_register+0x7a/0x180
> [145405.949707] [<ffffffffa04fb006>] nfs_get_sb+0x316/0x430 [nfs]
> [145405.949735] [<ffffffff8112195d>] vfs_kern_mount+0x7d/0x1b0
> [145405.949741] [<ffffffff81121af3>] do_kern_mount+0x53/0x120
> [145405.949748] [<ffffffff8113b7dc>] do_mount+0x21c/0x250
> [145405.949754] [<ffffffff8113b8d0>] sys_mount+0xc0/0xf0
>
>
> i.e. it tried to bdi_register 0:26 while it was already registered.
>
> Looking through the code, it seems that if nfs_get_sb calls
> nfs_bdi_register, but then doesn't set ->s_root, then ->put_super
> is not called (generic_shutdown_super only calls ->put_super if s_root
> is set), so the bdi is never unregistered. The next mount
> would then trigger the above error.
>
> I haven't found a way to trigger this situation, but it seems like it
> should be possible.
>
> Does this patch make sense?
>
> Thanks,
> NeilBrown
>
> ----------------------------
> NFS: ensure bdi_unregister is called on mount failure.
>
> bdi_unregister is called by nfs_put_super which is only called by
> generic_shutdown_super if ->s_root is not NULL. So if we error out
> in a circumstance where we called nfs_bdi_register (i.e. server !=
> NULL) but have not set s_root, then we need to call bdi_unregister
> explicitly in nfs_get_sb.
>
>
> Signed-off-by: NeilBrown <[email protected]>
>
> diff --git a/fs/nfs/super.c b/fs/nfs/super.c
> index f1afee4..da11ec9 100644
> --- a/fs/nfs/super.c
> +++ b/fs/nfs/super.c
> @@ -2214,7 +2214,7 @@ static int nfs_get_sb(struct file_system_type *fs_type,
> } else {
> error = nfs_bdi_register(server);
> if (error)
> - goto error_splat_super;
> + goto error_splat_bdi;
> }
>
> if (!s->s_root) {
> @@ -2256,6 +2256,9 @@ out_err_nosb:
> error_splat_root:
> dput(mntroot);
> error_splat_super:
> + if (server && !s->s_root)
> + bdi_unregister(&server->backing_dev_info);
> +error_splat_bdi:
> deactivate_locked_super(s);
> goto out;
> }
Yes. It looks as if you have indeed found a bug.
My main comment is that in order to be complete the patch should also
address the cases of nfs_xdev_get_sb(), nfs4_remote_get_sb(),
nfs4_xdev_get_sb(), and nfs4_remote_referral_get_sb(). They appear to
suffer from the same problem...
Cheers
Trond
On Wed, 10 Mar 2010 18:01:47 -0500
Trond Myklebust <[email protected]> wrote:
> Yes. It looks as if you have indeed found a bug.
>
> My main comment is that in order to be complete the patch should also
> address the cases of nfs_xdev_get_sb(), nfs4_remote_get_sb(),
> nfs4_xdev_get_sb(), and nfs4_remote_referral_get_sb(). They appear to
> suffer from the same problem...
>
Good point. Patch follows.
I have compile-tested this but nothing more. I'm not even sure how I can
test it as it only affect error paths that are not easy to force.
Thanks,
NeilBrown
>From 8db673fbc28ea018ab0b80c3b1f53bad0d684251 Mon Sep 17 00:00:00 2001
From: NeilBrown <[email protected]>
Date: Thu, 11 Mar 2010 11:20:17 +1100
Subject: [PATCH] NFS: ensure bdi_unregister is called on mount failure.
bdi_unregister is called by nfs_put_super which is only called by
generic_shutdown_super if ->s_root is not NULL. So if we error out
in a circumstance where we called nfs_bdi_register (i.e. server !=
NULL) but have not set s_root, then we need to call bdi_unregister
explicitly in nfs_get_sb and various other *_get_sb() functions.
Signed-off-by: NeilBrown <[email protected]>
diff --git a/fs/nfs/super.c b/fs/nfs/super.c
index f1afee4..6baf9a3 100644
--- a/fs/nfs/super.c
+++ b/fs/nfs/super.c
@@ -2214,7 +2214,7 @@ static int nfs_get_sb(struct file_system_type *fs_type,
} else {
error = nfs_bdi_register(server);
if (error)
- goto error_splat_super;
+ goto error_splat_bdi;
}
if (!s->s_root) {
@@ -2256,6 +2256,9 @@ out_err_nosb:
error_splat_root:
dput(mntroot);
error_splat_super:
+ if (server && !s->s_root)
+ bdi_unregister(&server->backing_dev_info);
+error_splat_bdi:
deactivate_locked_super(s);
goto out;
}
@@ -2326,7 +2329,7 @@ static int nfs_xdev_get_sb(struct file_system_type *fs_type, int flags,
} else {
error = nfs_bdi_register(server);
if (error)
- goto error_splat_super;
+ goto error_splat_bdi;
}
if (!s->s_root) {
@@ -2363,6 +2366,9 @@ out_err_noserver:
return error;
error_splat_super:
+ if (server && !s->s_root)
+ bdi_unregister(&server->backing_dev_info);
+error_splat_bdi:
deactivate_locked_super(s);
dprintk("<-- nfs_xdev_get_sb() = %d [splat]\n", error);
return error;
@@ -2578,7 +2584,7 @@ static int nfs4_remote_get_sb(struct file_system_type *fs_type,
} else {
error = nfs_bdi_register(server);
if (error)
- goto error_splat_super;
+ goto error_splat_bdi;
}
if (!s->s_root) {
@@ -2616,6 +2622,9 @@ out_free:
error_splat_root:
dput(mntroot);
error_splat_super:
+ if (server && !s->s_root)
+ bdi_unregister(&server->backing_dev_info);
+error_splat_bdi:
deactivate_locked_super(s);
goto out;
}
@@ -2811,7 +2820,7 @@ static int nfs4_xdev_get_sb(struct file_system_type *fs_type, int flags,
} else {
error = nfs_bdi_register(server);
if (error)
- goto error_splat_super;
+ goto error_splat_bdi;
}
if (!s->s_root) {
@@ -2847,6 +2856,9 @@ out_err_noserver:
return error;
error_splat_super:
+ if (server && !s->s_root)
+ bdi_unregister(&server->backing_dev_info);
+error_splat_bdi:
deactivate_locked_super(s);
dprintk("<-- nfs4_xdev_get_sb() = %d [splat]\n", error);
return error;
@@ -2893,7 +2905,7 @@ static int nfs4_remote_referral_get_sb(struct file_system_type *fs_type,
} else {
error = nfs_bdi_register(server);
if (error)
- goto error_splat_super;
+ goto error_splat_bdi;
}
if (!s->s_root) {
@@ -2929,6 +2941,9 @@ out_err_noserver:
return error;
error_splat_super:
+ if (server && !s->s_root)
+ bdi_unregister(&server->backing_dev_info);
+error_splat_bdi:
deactivate_locked_super(s);
dprintk("<-- nfs4_referral_get_sb() = %d [splat]\n", error);
return error;
On Thu, 2010-03-11 at 11:22 +1100, Neil Brown wrote:
> On Wed, 10 Mar 2010 18:01:47 -0500
> Trond Myklebust <[email protected]> wrote:
>
> > Yes. It looks as if you have indeed found a bug.
> >
> > My main comment is that in order to be complete the patch should also
> > address the cases of nfs_xdev_get_sb(), nfs4_remote_get_sb(),
> > nfs4_xdev_get_sb(), and nfs4_remote_referral_get_sb(). They appear to
> > suffer from the same problem...
> >
>
> Good point. Patch follows.
> I have compile-tested this but nothing more. I'm not even sure how I can
> test it as it only affect error paths that are not easy to force.
Agreed that it is faith based patching :-) but I'm willing to go with
that unless someone can show me that your argument is incorrect.
Will apply...
Cheers
Trond