On Tue, Jun 01, 2021 at 05:53:16PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@xxxxxxxxxx>
>
> It turns out that there is a 1:1 mapping between the execute and goal
> parameters that are passed to xfs_inode_walk_ag:
>
> xfs_blockgc_scan_inode <=> XFS_ICWALK_BLOCKGC
> xfs_dqrele_inode <=> XFS_ICWALK_DQRELE
>
> Because of this exact correspondence, we don't need the execute function
> pointer and can replace it with a direct call.
>
> For the price of a forward static declaration, we can eliminate the
> indirect function call. This likely has a negligible impact on
> performance (since the execute function runs transactions), but it also
> simplifies the function signature.
>
> Signed-off-by: Darrick J. Wong <djwong@xxxxxxxxxx>
> ---
> fs/xfs/xfs_icache.c | 46 ++++++++++++++++++++++------------------------
> 1 file changed, 22 insertions(+), 24 deletions(-)
Overall looks good.
> @@ -1777,7 +1772,14 @@ xfs_inode_walk_ag(
> for (i = 0; i < nr_found; i++) {
> if (!batch[i])
> continue;
> - error = execute(batch[i], args);
> + switch (goal) {
> + case XFS_ICWALK_DQRELE:
> + error = xfs_dqrele_inode(batch[i], args);
> + break;
> + case XFS_ICWALK_BLOCKGC:
> + error = xfs_blockgc_scan_inode(batch[i], args);
> + break;
> + }
> xfs_irele(batch[i]);
I can't help but think that this should be wrapped up in a function
like xfs_icwalk_grab(), especially as we add the reclaim case to
this later on. Perhaps:
static int
xfs_icwalk_process_inode(
struct xfs_inode *ip,
enum xfs_icwalk_goal goal,
void *args)
{
int error = -EINVAL;
switch (goal) {
case XFS_ICWALK_DQRELE:
error = xfs_dqrele_inode(ip, args);
break;
case XFS_ICWALK_BLOCKGC:
error = xfs_blockgc_scan_inode(ip, args);
break;
}
xfs_irele(ip);
return error;
}
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
