Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220819081700.96279-1-chi.minghao@zte.com.cn> <0d78d24dd26441818dbaedffde611485@AcuMS.aculab.com>
In-Reply-To: <0d78d24dd26441818dbaedffde611485@AcuMS.aculab.com>
From:   Ryusuke Konishi <konishi.ryusuke@gmail.com>
Date:   Sat, 20 Aug 2022 08:50:59 +0900
Message-ID: <CAKFNMo=LedaGL9R194rirwccCBuGyawqZfKbWiNj9JuP9A3hdg@mail.gmail.com>
Subject: Re: [PATCH] nilfs2: delete unnecessary checks before brelse()
To:     David Laight <David.Laight@aculab.com>
Cc:     "cgel.zte@gmail.com" <cgel.zte@gmail.com>,
        "linux-nilfs@vger.kernel.org" <linux-nilfs@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Minghao Chi <chi.minghao@zte.com.cn>,
        Zeal Robot <zealci@zte.com.cn>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi David,

On Sat, Aug 20, 2022 at 6:31 AM David Laight wrote:
>
> From: cgel.zte@gmail.com
> > Sent: 19 August 2022 09:17
> >
> > The brelse() function tests whether its argument is NULL
> > and then returns immediately.
> > Thus remove the tests which are not needed around the shown calls.
>

> The test is the caller makes sense if it is a 'hot' path
> and bh is normally NULL.
>
>         David

Should the commit message say "inline function" instead of "function"?

or do you have other intentions?

These tests indeed seem meaningless, since brelse() is an inline
function that tests whether bh is NULL or not, like this

 static inline void brelse(struct buffer_head *bh)
 {
        if (bh)
                __brelse(bh);
 }

and the corresponding parts are equivalent to:

  if (bh != NULL)
          if (bh)
                  __brelse(bh);


Regards,
Ryusuke Konishi

>
> >
> > Reported-by: Zeal Robot <zealci@zte.com.cn>
> > Signed-off-by: Minghao Chi <chi.minghao@zte.com.cn>
> > ---
> >  fs/nilfs2/btree.c | 6 ++----
> >  1 file changed, 2 insertions(+), 4 deletions(-)
> >
> > diff --git a/fs/nilfs2/btree.c b/fs/nilfs2/btree.c
> > index f544c22fff78..b3b5a46488c2 100644
> > --- a/fs/nilfs2/btree.c
> > +++ b/fs/nilfs2/btree.c
> > @@ -1668,8 +1668,7 @@ static int nilfs_btree_check_delete(struct nilfs_bmap *btree, __u64 key)
> >       maxkey = nilfs_btree_node_get_key(node, nchildren - 1);
> >       nextmaxkey = (nchildren > 1) ?
> >               nilfs_btree_node_get_key(node, nchildren - 2) : 0;
> > -     if (bh != NULL)
> > -             brelse(bh);
> > +     brelse(bh);
> >
> >       return (maxkey == key) && (nextmaxkey < NILFS_BMAP_LARGE_LOW);
> >  }
> > @@ -1717,8 +1716,7 @@ static int nilfs_btree_gather_data(struct nilfs_bmap *btree,
> >               ptrs[i] = le64_to_cpu(dptrs[i]);
> >       }
> >
> > -     if (bh != NULL)
> > -             brelse(bh);
> > +     brelse(bh);
> >
> >       return nitems;
> >  }
> > --
> > 2.25.1