On Tue, May 20, 2025 at 07:57:19PM +0800, Zizhi Wo wrote:
>
>
> 在 2025/5/20 18:38, Carlos Maiolino 写道:
> > 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?
> >
>
> Yes, I was simply thinking about this while looking at the fsmap-related
> code. Since the current for loop always exits after iterating to the
> last AG, I thought off the top of my head that the error check for
> last_ag might be unnecessary. As you and Darrick mentioned, though,
> removing it would increase the difficulty of reading the code and could
> also affect future developers.:(
>
> If doing so could bring significant performance benefits, then it might
> be worth considering — perhaps with some added comments to clarify? But
> in this case, the performance gain is indeed negligible, I admit. Thank
> you for pointing that out.
As anything, it's always a trade-off.
The key thing to take away here is your patch make the whole code it touches
harder to understand without bringing any real benefit. Even if you could
prove this adds small performance gain to the fsmap interface, perhaps it
would still not be worth the performance gain in lieu of poor code reliability,
giving the interface you changed. So, it's always about discussing the
change's trade-off between complexity and performance. In your case, there
seems to be no real trade-off.
