在 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.
Thanks,
Zizhi Wo
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
