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?
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
> >
> >
>
