* [RFC] usb: don't dput() in usbfs_rmdir()
[not found] ` <BANLkTimaMihxe75vTXDS1HivCbj8-v-pBg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2011-05-30 15:35 ` Sebastian Andrzej Siewior
[not found] ` <20110530153520.GA2386-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
2011-05-30 20:27 ` Sage Weil
0 siblings, 2 replies; 6+ messages in thread
From: Sebastian Andrzej Siewior @ 2011-05-30 15:35 UTC (permalink / raw)
To: Sage Weil
Cc: Tanya Brokhman, Huajun Li, linux-usb-u79uwXL29TY76Z2rM5mHXA,
greg-U8xfFu+wG4EAvxtiuMwx3w, ablay-sgV2jX0FEOL9JmXXK+q4OQ,
Jassi Brar, Al Viro, linux-fsdevel-u79uwXL29TY76Z2rM5mHXA
rmmod of every hcd results currently in somethig like
|uhci_hcd 0000:00:01.2: USB bus 1 deregistered
|------------[ cut here ]------------
|kernel BUG at /home/bigeasy/git/linux/fs/dcache.c:419!
|invalid opcode: 0000 [#1]
|Modules linked in: uhci_hcd(-) ehci_hcd usbcore
|
|Pid: 1736, comm: rmmod Not tainted 3.0.0-rc1-00001-geb7d864-dirty #148 Bochs Bochs
|EIP: 0060:[<810c3f6b>] EFLAGS: 00010246 CPU: 0
|EIP is at dput+0x12b/0x130
|EAX: 00000000 EBX: af3ca9a0 ECX: af3cbd00 EDX: 00000202
|ESI: af3c6000 EDI: af3cbca8 EBP: aeae7ddc ESP: aeae7dd4
| DS: 007b ES: 007b FS: 0000 GS: 0033 SS: 0068
|Process rmmod (pid: 1736, ti=aeae6000 task=ae815c00 task.ti=aeae6000)
|Stack:
| af3ca9a0 af3c6000 aeae7dfc b0a489b6 af3cb9d8 af3cbcc0 af3caa10 aefb2230
| 00000000 00000000 aeae7e28 b0a48de5 afa37140 af402300 ade0a038 aeae7e40
| 810ae9f5 81177295 b0a4dce0 00000000 00000000 aeae7e44 81049395 aefb2230
|Call Trace:
| [<b0a489b6>] fs_remove_file+0x76/0x120 [usbcore]
| [<b0a48de5>] usbfs_notify+0x35/0x2d0 [usbcore]
| [<810ae9f5>] ? kfree+0x105/0x130
| [<81177295>] ? kobject_release+0x45/0x80
| [<81049395>] notifier_call_chain+0x45/0x60
| [<81049793>] __blocking_notifier_call_chain+0x43/0x70
| [<810497df>] blocking_notifier_call_chain+0x1f/0x30
| [<b0a46a39>] usb_notify_remove_bus+0x19/0x20 [usbcore]
| [<b0a3af0b>] usb_deregister_bus+0x5b/0x70 [usbcore]
| [<b0a3b011>] usb_remove_hcd+0xf1/0x100 [usbcore]
a bisect turned up:
|commit 64252c75a2196a0cf1e0d3777143ecfe0e3ae650
|Author: Sage Weil <sage-BnTBU8nroG7k1uMJSBkQmQ@public.gmane.org>
|Date: Tue May 24 13:06:05 2011 -0700
|
| vfs: remove dget() from dentry_unhash()
|
| This serves no useful purpose that I can discern. All callers (rename,
| rmdir) hold their own reference to the dentry.
|
| A quick audit of all file systems showed no relevant checks on the value
| of d_count in vfs_rmdir/vfs_rename_dir paths.
|
| Signed-off-by: Sage Weil <sage-BnTBU8nroG7k1uMJSBkQmQ@public.gmane.org>
| Signed-off-by: Al Viro <viro-RmSDqhL/yNMiFSDQTTA3OLVCufUGDwFn@public.gmane.org>
I removed the "inner" dput() which is called on emptry dir and sets error
to zero. This looks like the same change in 64252c7 for vfs_rmdir()
where dput() is removed from the path where we have dont_mount() and
S_DEAD.
Signed-off-by: Sebastian Andrzej Siewior <bigeasy-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
---
Could one of VFS ppl look at this an NACK/ACK it?
Jessi: I've seen your patch after I had mine done. If you send me a
sign-off-by for this, I will resend it with you as Author + SOB if you
like once the VFS are fine with this.
drivers/usb/core/inode.c | 1 -
1 files changed, 0 insertions(+), 1 deletions(-)
diff --git a/drivers/usb/core/inode.c b/drivers/usb/core/inode.c
index 1b125c2..3ef9308 100644
--- a/drivers/usb/core/inode.c
+++ b/drivers/usb/core/inode.c
@@ -381,7 +381,6 @@ static int usbfs_rmdir(struct inode *dir, struct dentry *dentry)
dont_mount(dentry);
drop_nlink(dentry->d_inode);
drop_nlink(dentry->d_inode);
- dput(dentry);
inode->i_flags |= S_DEAD;
drop_nlink(dir);
error = 0;
--
1.7.4.4
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [RFC] usb: don't dput() in usbfs_rmdir()
[not found] ` <20110530153520.GA2386-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
@ 2011-05-30 15:44 ` Jassi Brar
0 siblings, 0 replies; 6+ messages in thread
From: Jassi Brar @ 2011-05-30 15:44 UTC (permalink / raw)
To: Sebastian Andrzej Siewior
Cc: Sage Weil, Tanya Brokhman, Huajun Li,
linux-usb-u79uwXL29TY76Z2rM5mHXA, greg-U8xfFu+wG4EAvxtiuMwx3w,
ablay-sgV2jX0FEOL9JmXXK+q4OQ, Al Viro,
linux-fsdevel-u79uwXL29TY76Z2rM5mHXA
On Mon, May 30, 2011 at 9:05 PM, Sebastian Andrzej Siewior
<bigeasy-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org> wrote:
> Jessi: I've seen your patch after I had mine done. If you send me a
> sign-off-by for this, I will resend it with you as Author + SOB if you
> like once the VFS are fine with this.
Signed-off-by: Jassi Brar <jaswinder.singh-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Thanks
> drivers/usb/core/inode.c | 1 -
> 1 files changed, 0 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/usb/core/inode.c b/drivers/usb/core/inode.c
> index 1b125c2..3ef9308 100644
> --- a/drivers/usb/core/inode.c
> +++ b/drivers/usb/core/inode.c
> @@ -381,7 +381,6 @@ static int usbfs_rmdir(struct inode *dir, struct dentry *dentry)
> dont_mount(dentry);
> drop_nlink(dentry->d_inode);
> drop_nlink(dentry->d_inode);
> - dput(dentry);
> inode->i_flags |= S_DEAD;
> drop_nlink(dir);
> error = 0;
> --
> 1.7.4.4
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC] usb: don't dput() in usbfs_rmdir()
2011-05-30 15:35 ` [RFC] usb: don't dput() in usbfs_rmdir() Sebastian Andrzej Siewior
[not found] ` <20110530153520.GA2386-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
@ 2011-05-30 20:27 ` Sage Weil
2011-05-31 9:53 ` Sebastian Andrzej Siewior
1 sibling, 1 reply; 6+ messages in thread
From: Sage Weil @ 2011-05-30 20:27 UTC (permalink / raw)
To: Sebastian Andrzej Siewior
Cc: Tanya Brokhman, Huajun Li, linux-usb, greg, ablay, Jassi Brar,
Al Viro, linux-fsdevel
On Mon, 30 May 2011, Sebastian Andrzej Siewior wrote:
> rmmod of every hcd results currently in somethig like
>
> |uhci_hcd 0000:00:01.2: USB bus 1 deregistered
> |------------[ cut here ]------------
> |kernel BUG at /home/bigeasy/git/linux/fs/dcache.c:419!
> |invalid opcode: 0000 [#1]
> |Modules linked in: uhci_hcd(-) ehci_hcd usbcore
> |
> |Pid: 1736, comm: rmmod Not tainted 3.0.0-rc1-00001-geb7d864-dirty #148 Bochs Bochs
> |EIP: 0060:[<810c3f6b>] EFLAGS: 00010246 CPU: 0
> |EIP is at dput+0x12b/0x130
> |EAX: 00000000 EBX: af3ca9a0 ECX: af3cbd00 EDX: 00000202
> |ESI: af3c6000 EDI: af3cbca8 EBP: aeae7ddc ESP: aeae7dd4
> | DS: 007b ES: 007b FS: 0000 GS: 0033 SS: 0068
> |Process rmmod (pid: 1736, ti=aeae6000 task=ae815c00 task.ti=aeae6000)
> |Stack:
> | af3ca9a0 af3c6000 aeae7dfc b0a489b6 af3cb9d8 af3cbcc0 af3caa10 aefb2230
> | 00000000 00000000 aeae7e28 b0a48de5 afa37140 af402300 ade0a038 aeae7e40
> | 810ae9f5 81177295 b0a4dce0 00000000 00000000 aeae7e44 81049395 aefb2230
> |Call Trace:
> | [<b0a489b6>] fs_remove_file+0x76/0x120 [usbcore]
> | [<b0a48de5>] usbfs_notify+0x35/0x2d0 [usbcore]
> | [<810ae9f5>] ? kfree+0x105/0x130
> | [<81177295>] ? kobject_release+0x45/0x80
> | [<81049395>] notifier_call_chain+0x45/0x60
> | [<81049793>] __blocking_notifier_call_chain+0x43/0x70
> | [<810497df>] blocking_notifier_call_chain+0x1f/0x30
> | [<b0a46a39>] usb_notify_remove_bus+0x19/0x20 [usbcore]
> | [<b0a3af0b>] usb_deregister_bus+0x5b/0x70 [usbcore]
> | [<b0a3b011>] usb_remove_hcd+0xf1/0x100 [usbcore]
>
> a bisect turned up:
>
> |commit 64252c75a2196a0cf1e0d3777143ecfe0e3ae650
> |Author: Sage Weil <sage@newdream.net>
> |Date: Tue May 24 13:06:05 2011 -0700
> |
> | vfs: remove dget() from dentry_unhash()
> |
> | This serves no useful purpose that I can discern. All callers (rename,
> | rmdir) hold their own reference to the dentry.
> |
> | A quick audit of all file systems showed no relevant checks on the value
> | of d_count in vfs_rmdir/vfs_rename_dir paths.
> |
> | Signed-off-by: Sage Weil <sage@newdream.net>
> | Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
>
> I removed the "inner" dput() which is called on emptry dir and sets error
> to zero. This looks like the same change in 64252c7 for vfs_rmdir()
> where dput() is removed from the path where we have dont_mount() and
> S_DEAD.
>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
> Could one of VFS ppl look at this an NACK/ACK it?
I think it's the other dput that you want to remove. 64252c75 is a
misleading because the first hunk has to remove dput() from every exit
path for the function. dentry_unhash is unconditionally doing dget,
though. I think we want
diff --git a/drivers/usb/core/inode.c b/drivers/usb/core/inode.c
index 1b125c2..2278dad 100644
--- a/drivers/usb/core/inode.c
+++ b/drivers/usb/core/inode.c
@@ -389,7 +389,6 @@ static int usbfs_rmdir(struct inode *dir, struct dentry *dentry)
mutex_unlock(&inode->i_mutex);
if (!error)
d_delete(dentry);
- dput(dentry);
return error;
}
Sorry I missed this one; I forgot there were filesystems outside of fs/.
sage
>
> Jessi: I've seen your patch after I had mine done. If you send me a
> sign-off-by for this, I will resend it with you as Author + SOB if you
> like once the VFS are fine with this.
>
> drivers/usb/core/inode.c | 1 -
> 1 files changed, 0 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/usb/core/inode.c b/drivers/usb/core/inode.c
> index 1b125c2..3ef9308 100644
> --- a/drivers/usb/core/inode.c
> +++ b/drivers/usb/core/inode.c
> @@ -381,7 +381,6 @@ static int usbfs_rmdir(struct inode *dir, struct dentry *dentry)
> dont_mount(dentry);
> drop_nlink(dentry->d_inode);
> drop_nlink(dentry->d_inode);
> - dput(dentry);
> inode->i_flags |= S_DEAD;
> drop_nlink(dir);
> error = 0;
> --
> 1.7.4.4
>
>
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [RFC] usb: don't dput() in usbfs_rmdir()
2011-05-30 20:27 ` Sage Weil
@ 2011-05-31 9:53 ` Sebastian Andrzej Siewior
2011-05-31 16:11 ` Sage Weil
0 siblings, 1 reply; 6+ messages in thread
From: Sebastian Andrzej Siewior @ 2011-05-31 9:53 UTC (permalink / raw)
To: Sage Weil
Cc: Tanya Brokhman, Huajun Li, linux-usb, greg, ablay, Jassi Brar,
Al Viro, linux-fsdevel
Sage Weil wrote:
>> Could one of VFS ppl look at this an NACK/ACK it?
>
> I think it's the other dput that you want to remove. 64252c75 is a
> misleading because the first hunk has to remove dput() from every exit
> path for the function. dentry_unhash is unconditionally doing dget,
> though. I think we want
>
> diff --git a/drivers/usb/core/inode.c b/drivers/usb/core/inode.c
> index 1b125c2..2278dad 100644
> --- a/drivers/usb/core/inode.c
> +++ b/drivers/usb/core/inode.c
> @@ -389,7 +389,6 @@ static int usbfs_rmdir(struct inode *dir, struct dentry *dentry)
> mutex_unlock(&inode->i_mutex);
> if (!error)
> d_delete(dentry);
> - dput(dentry);
> return error;
> }
Yep, this is the correct one. I added a file and removed it after the hcd
was gone and it only survived your way :)
Are you going to post a complete patch or do you want me to do it?
> Sorry I missed this one; I forgot there were filesystems outside of fs/.
Yes, they all over the place including arch/ :)
>
> sage
Sebastian
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC] usb: don't dput() in usbfs_rmdir()
2011-05-31 9:53 ` Sebastian Andrzej Siewior
@ 2011-05-31 16:11 ` Sage Weil
[not found] ` <Pine.LNX.4.64.1105310909370.25709-vIokxiIdD2AQNTJnQDzGJqxOck334EZe@public.gmane.org>
0 siblings, 1 reply; 6+ messages in thread
From: Sage Weil @ 2011-05-31 16:11 UTC (permalink / raw)
To: Al Viro, Sebastian Andrzej Siewior
Cc: Tanya Brokhman, Huajun Li, linux-usb, greg, ablay, Jassi Brar,
linux-fsdevel
On Tue, 31 May 2011, Sebastian Andrzej Siewior wrote:
> Sage Weil wrote:
> > > Could one of VFS ppl look at this an NACK/ACK it?
> >
> > I think it's the other dput that you want to remove. 64252c75 is a
> > misleading because the first hunk has to remove dput() from every exit path
> > for the function. dentry_unhash is unconditionally doing dget, though. I
> > think we want
> >
> > diff --git a/drivers/usb/core/inode.c b/drivers/usb/core/inode.c
> > index 1b125c2..2278dad 100644
> > --- a/drivers/usb/core/inode.c
> > +++ b/drivers/usb/core/inode.c
> > @@ -389,7 +389,6 @@ static int usbfs_rmdir(struct inode *dir, struct dentry
> > *dentry)
> > mutex_unlock(&inode->i_mutex);
> > if (!error)
> > d_delete(dentry);
> > - dput(dentry);
> > return error;
> > }
>
> Yep, this is the correct one. I added a file and removed it after the hcd
> was gone and it only survived your way :)
> Are you going to post a complete patch or do you want me to do it?
Patch is below. Thanks for testing!
sage
>From 2dbf6d8f7426980d4c0d8798222b2ce9eed76651 Mon Sep 17 00:00:00 2001
From: Sage Weil <sage@newdream.net>
Date: Tue, 31 May 2011 09:09:16 -0700
Subject: [PATCH] usb: remove bad dput after dentry_unhash
Commit 64252c75a removed the useless dget from dentry_unhash but didn't
fix up this caller in the usb code. There used to be exactly one dput per
dentry_unhash call; now there are none.
Tested-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Signed-off-by: Sage Weil <sage@newdream.net>
---
drivers/usb/core/inode.c | 1 -
1 files changed, 0 insertions(+), 1 deletions(-)
diff --git a/drivers/usb/core/inode.c b/drivers/usb/core/inode.c
index 1b125c2..2278dad 100644
--- a/drivers/usb/core/inode.c
+++ b/drivers/usb/core/inode.c
@@ -389,7 +389,6 @@ static int usbfs_rmdir(struct inode *dir, struct dentry *dentry)
mutex_unlock(&inode->i_mutex);
if (!error)
d_delete(dentry);
- dput(dentry);
return error;
}
--
1.7.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [RFC] usb: don't dput() in usbfs_rmdir()
[not found] ` <Pine.LNX.4.64.1105310909370.25709-vIokxiIdD2AQNTJnQDzGJqxOck334EZe@public.gmane.org>
@ 2011-05-31 16:54 ` Sergei Shtylyov
0 siblings, 0 replies; 6+ messages in thread
From: Sergei Shtylyov @ 2011-05-31 16:54 UTC (permalink / raw)
To: Sage Weil
Cc: Al Viro, Sebastian Andrzej Siewior, Tanya Brokhman, Huajun Li,
linux-usb-u79uwXL29TY76Z2rM5mHXA, greg-U8xfFu+wG4EAvxtiuMwx3w,
ablay-sgV2jX0FEOL9JmXXK+q4OQ, Jassi Brar,
linux-fsdevel-u79uwXL29TY76Z2rM5mHXA
Hello.
Sage Weil wrote:
>>>> Could one of VFS ppl look at this an NACK/ACK it?
>>> I think it's the other dput that you want to remove. 64252c75 is a
>>> misleading because the first hunk has to remove dput() from every exit path
>>> for the function. dentry_unhash is unconditionally doing dget, though. I
>>> think we want
>>> diff --git a/drivers/usb/core/inode.c b/drivers/usb/core/inode.c
>>> index 1b125c2..2278dad 100644
>>> --- a/drivers/usb/core/inode.c
>>> +++ b/drivers/usb/core/inode.c
>>> @@ -389,7 +389,6 @@ static int usbfs_rmdir(struct inode *dir, struct dentry
>>> *dentry)
>>> mutex_unlock(&inode->i_mutex);
>>> if (!error)
>>> d_delete(dentry);
>>> - dput(dentry);
>>> return error;
>>> }
>> Yep, this is the correct one. I added a file and removed it after the hcd
>> was gone and it only survived your way :)
>> Are you going to post a complete patch or do you want me to do it?
> Patch is below. Thanks for testing!
> sage
> From 2dbf6d8f7426980d4c0d8798222b2ce9eed76651 Mon Sep 17 00:00:00 2001
> From: Sage Weil <sage-BnTBU8nroG7k1uMJSBkQmQ@public.gmane.org>
> Date: Tue, 31 May 2011 09:09:16 -0700
> Subject: [PATCH] usb: remove bad dput after dentry_unhash
> Commit 64252c75a removed the useless dget from dentry_unhash but didn't
Please also specify that commit's summary in parens -- for the human readers.
Commit ID is only immediately usable to gitweb.
> fix up this caller in the usb code. There used to be exactly one dput per
> dentry_unhash call; now there are none.
> Tested-by: Sebastian Andrzej Siewior <bigeasy-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
> Signed-off-by: Sage Weil <sage-BnTBU8nroG7k1uMJSBkQmQ@public.gmane.org>
WBR, Sergei
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2011-05-31 16:54 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <c53791ee86b26aac740ff8db61454972.squirrel@www.codeaurora.org>
[not found] ` <BANLkTinspCvVeP7k5faYio4YPRqGbnzzTg@mail.gmail.com>
[not found] ` <000001cc1eb1$6aa8e500$3ffaaf00$@org>
[not found] ` <BANLkTimaMihxe75vTXDS1HivCbj8-v-pBg@mail.gmail.com>
[not found] ` <BANLkTimaMihxe75vTXDS1HivCbj8-v-pBg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2011-05-30 15:35 ` [RFC] usb: don't dput() in usbfs_rmdir() Sebastian Andrzej Siewior
[not found] ` <20110530153520.GA2386-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
2011-05-30 15:44 ` Jassi Brar
2011-05-30 20:27 ` Sage Weil
2011-05-31 9:53 ` Sebastian Andrzej Siewior
2011-05-31 16:11 ` Sage Weil
[not found] ` <Pine.LNX.4.64.1105310909370.25709-vIokxiIdD2AQNTJnQDzGJqxOck334EZe@public.gmane.org>
2011-05-31 16:54 ` Sergei Shtylyov
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.