On Tue, 2025-05-20 at 12:38 +0200, Carlos Maiolino wrote:
> On Mon, May 19, 2025 at 08:08:54AM -0700, Darrick J. Wong wrote:
> > On Sat, May 17, 2025 at 03:43:41PM +0800, Zizhi Wo wrote:
> > > From: Zizhi Wo <wozizhi@xxxxxxxxxxxxxxx>
> > >
> > > In __xfs_getfsmap_datadev(), if "pag_agno(pag) == end_ag", we don't need
> > > to check the result of query_fn(), because there won't be another iteration
> > > of the loop anyway. Also, both before and after the change, info->group
> > > will eventually be set to NULL and the reference count of xfs_group will
> > > also be decremented before exiting the iteration.
> > >
> > > The same logic applies to other similar functions as well, so related
> > > cleanup operations are performed together.
> > >
> > > Signed-off-by: Zizhi Wo <wozizhi@xxxxxxxxxxxxxxx>
> > > Signed-off-by: Zizhi Wo <wozizhi@xxxxxxxxxx>
> > > ---
> > > fs/xfs/xfs_fsmap.c | 6 ------
> > > 1 file changed, 6 deletions(-)
> > >
> > > diff --git a/fs/xfs/xfs_fsmap.c b/fs/xfs/xfs_fsmap.c
> > > index 414b27a86458..792282aa8a29 100644
> > > --- a/fs/xfs/xfs_fsmap.c
> > > +++ b/fs/xfs/xfs_fsmap.c
> > > @@ -579,8 +579,6 @@ __xfs_getfsmap_datadev(
> > > if (pag_agno(pag) == end_ag) {
> > > info->last = true;
> > > error = query_fn(tp, info, &bt_cur, priv);
> > > - if (error)
> > > - break;
> >
> > Removing these statements make the error path harder to follow. Before,
> > it was explicit that an error breaks out of the loop body. Now you have
> > to look upwards to the while loop conditional and reason about what
> > xfs_perag_next_range does when pag-> agno == end_ag to determine that
> > the loop terminates.
> >
> > This also leaves a tripping point for anyone who wants to add another
> > statement into this here if body because now they have to recognize that
> > they need to re-add the "if (error) break;" statements that you're now
> > taking out.
> >
> > You also don't claim any reduction in generated machine code size or
> > execution speed, which means all the programmers end up having to
> > perform extra reasoning when reading this code for ... what? Zero gain?
> >
> > Please stop sending overly narrowly focused "optimizations" that make
> > life harder for all of us.
>
> I do agree with Darrick. What's the point of this patch other than making code
> harder to understand? This gets rid of less than 10 machine instructions at the
> final module, and such cod is not even a hot path. making these extra instructions
> virtually negligible IMO (looking at x86 architecture). The checks are unneeded
> logically, but make the code easier to read, which is also important.
> Did you actually see any improvement on anything by applying this patch? Or was
> it crafted merely as a result of code inspection? Where are the results that make
> this change worth the extra complexity while reading it?
I too agree with Darrick and Carlos. I initially gave my RB for the change, however I didn't
consider the above points mentioned by Carlos and Darrick. Thank you for the above pointers. It was
a good learning for me too - I will keep these in mind in my future reviews and patches too.
--NR
>
> Cheers,
> Carlos
>
> > NAK.
> >
> > --D
> >
> > > }
> > > info->group = NULL;
> > > }
> > > @@ -813,8 +811,6 @@ xfs_getfsmap_rtdev_rtbitmap(
> > > info->last = true;
> > > error = xfs_getfsmap_rtdev_rtbitmap_helper(rtg, tp,
> > > &ahigh, info);
> > > - if (error)
> > > - break;
> > > }
> > >
> > > xfs_rtgroup_unlock(rtg, XFS_RTGLOCK_BITMAP_SHARED);
> > > @@ -1018,8 +1014,6 @@ xfs_getfsmap_rtdev_rmapbt(
> > > info->last = true;
> > > error = xfs_getfsmap_rtdev_rmapbt_helper(bt_cur,
> > > &info->high, info);
> > > - if (error)
> > > - break;
> > > }
> > > info->group = NULL;
> > > }
> > > --
> > > 2.39.2
> > >
> > >
