diff mbox series

fs/dcache: prevent repeated locking

Message ID 20211207101646.401982-1-lv.ruyi@zte.com.cn (mailing list archive)
State New, archived
Headers show
Series fs/dcache: prevent repeated locking | expand

Commit Message

CGEL Dec. 7, 2021, 10:16 a.m. UTC 
From: Lv Ruyi <lv.ruyi@zte.com.cn>

Move the spin_lock above the restart to prevent to lock twice 
when the code goto restart.

Reported-by: Zeal Robot <zealci@zte.com.cn>
Signed-off-by: Lv Ruyi <lv.ruyi@zte.com.cn>
---
 fs/dcache.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Matthew Wilcox Dec. 7, 2021, 1:17 p.m. UTC | #1 
On Tue, Dec 07, 2021 at 10:16:46AM +0000, cgel.zte@gmail.com wrote:
> From: Lv Ruyi <lv.ruyi@zte.com.cn>
> 
> Move the spin_lock above the restart to prevent to lock twice 
> when the code goto restart.

This is madness.

void d_prune_aliases(struct inode *inode)
        spin_lock(&inode->i_lock);
                        if (likely(!dentry->d_lockref.count)) {
                                __dentry_kill(dentry);
                                goto restart;
...
static void __dentry_kill(struct dentry *dentry)
        if (dentry->d_inode)
                dentry_unlink_inode(dentry);
...
static void dentry_unlink_inode(struct dentry * dentry)
        spin_unlock(&inode->i_lock);

Did you even test this patch?
Eric Biggers Dec. 7, 2021, 9:25 p.m. UTC | #2 
On Tue, Dec 07, 2021 at 01:17:41PM +0000, Matthew Wilcox wrote:
> On Tue, Dec 07, 2021 at 10:16:46AM +0000, cgel.zte@gmail.com wrote:
> > From: Lv Ruyi <lv.ruyi@zte.com.cn>
> > 
> > Move the spin_lock above the restart to prevent to lock twice 
> > when the code goto restart.
> 
> This is madness.
> 
> void d_prune_aliases(struct inode *inode)
>         spin_lock(&inode->i_lock);
>                         if (likely(!dentry->d_lockref.count)) {
>                                 __dentry_kill(dentry);
>                                 goto restart;
> ...
> static void __dentry_kill(struct dentry *dentry)
>         if (dentry->d_inode)
>                 dentry_unlink_inode(dentry);
> ...
> static void dentry_unlink_inode(struct dentry * dentry)
>         spin_unlock(&inode->i_lock);
> 
> Did you even test this patch?

This same wrong patch has been sent several times before.  I think it's fair to
say that this code could use a comment, e.g.:

	/* i_lock was dropped */
	goto restart;

- Eric
Matthew Wilcox Dec. 7, 2021, 10:11 p.m. UTC | #3 
On Tue, Dec 07, 2021 at 01:25:18PM -0800, Eric Biggers wrote:
> On Tue, Dec 07, 2021 at 01:17:41PM +0000, Matthew Wilcox wrote:
> > On Tue, Dec 07, 2021 at 10:16:46AM +0000, cgel.zte@gmail.com wrote:
> > > From: Lv Ruyi <lv.ruyi@zte.com.cn>
> > > 
> > > Move the spin_lock above the restart to prevent to lock twice 
> > > when the code goto restart.
> > 
> > This is madness.
> > 
> > void d_prune_aliases(struct inode *inode)
> >         spin_lock(&inode->i_lock);
> >                         if (likely(!dentry->d_lockref.count)) {
> >                                 __dentry_kill(dentry);
> >                                 goto restart;
> > ...
> > static void __dentry_kill(struct dentry *dentry)
> >         if (dentry->d_inode)
> >                 dentry_unlink_inode(dentry);
> > ...
> > static void dentry_unlink_inode(struct dentry * dentry)
> >         spin_unlock(&inode->i_lock);
> > 
> > Did you even test this patch?
> 
> This same wrong patch has been sent several times before.  I think it's fair to
> say that this code could use a comment, e.g.:
> 
> 	/* i_lock was dropped */
> 	goto restart;

Not sure that'll do much good.  It seems to be fools running some script
that they haven't the wit to understand.
diff mbox series

Patch

diff --git a/fs/dcache.c b/fs/dcache.c
index c84269c6e8bf..8580d51b397a 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -1102,8 +1102,8 @@  struct dentry *d_find_alias_rcu(struct inode *inode)
 void d_prune_aliases(struct inode *inode)
 {
 	struct dentry *dentry;
-restart:
 	spin_lock(&inode->i_lock);
+restart:
 	hlist_for_each_entry(dentry, &inode->i_dentry, d_u.d_alias) {
 		spin_lock(&dentry->d_lock);
 		if (!dentry->d_lockref.count) {