Re: [PATCH 3/3] xfs: simplify iext overflow checking and upgrade

[Dave Chinner <david@fromorbit.com>]

On Tue, Apr 30, 2024 at 02:56:02PM +0200, Christoph Hellwig wrote:
> Currently the calls to xfs_iext_count_may_overflow and
> xfs_iext_count_upgrade are always paired.  Merge them into a single
> function to simplify the callers and the actual check and upgrade
> logic itself.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  fs/xfs/libxfs/xfs_attr.c       |  5 +--
>  fs/xfs/libxfs/xfs_bmap.c       |  5 +--
>  fs/xfs/libxfs/xfs_inode_fork.c | 57 +++++++++++++++-------------------
>  fs/xfs/libxfs/xfs_inode_fork.h |  6 ++--
>  fs/xfs/xfs_bmap_item.c         |  4 +--
>  fs/xfs/xfs_bmap_util.c         | 24 +++-----------
>  fs/xfs/xfs_dquot.c             |  5 +--
>  fs/xfs/xfs_iomap.c             |  9 ++----
>  fs/xfs/xfs_reflink.c           |  9 ++----
>  fs/xfs/xfs_rtalloc.c           |  5 +--
>  10 files changed, 41 insertions(+), 88 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> index 1c2a27fce08a9d..ded92ccefe9f6d 100644
> --- a/fs/xfs/libxfs/xfs_attr.c
> +++ b/fs/xfs/libxfs/xfs_attr.c
> @@ -1050,11 +1050,8 @@ xfs_attr_set(
>  		return error;
>  
>  	if (op != XFS_ATTRUPDATE_REMOVE || xfs_inode_hasattr(dp)) {
> -		error = xfs_iext_count_may_overflow(dp, XFS_ATTR_FORK,
> +		error = xfs_iext_count_ensure(args->trans, dp, XFS_ATTR_FORK,
>  				XFS_IEXT_ATTR_MANIP_CNT(rmt_blks));
> -		if (error == -EFBIG)
> -			error = xfs_iext_count_upgrade(args->trans, dp,
> -					XFS_IEXT_ATTR_MANIP_CNT(rmt_blks));
>  		if (error)
>  			goto out_trans_cancel;
>  	}
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index 6053f5e5c71eec..3debd0d561b812 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -4621,11 +4621,8 @@ xfs_bmapi_convert_delalloc(
>  	xfs_ilock(ip, XFS_ILOCK_EXCL);
>  	xfs_trans_ijoin(tp, ip, 0);
>  
> -	error = xfs_iext_count_may_overflow(ip, whichfork,
> +	error = xfs_iext_count_ensure(tp, ip, whichfork,
>  			XFS_IEXT_ADD_NOSPLIT_CNT);
> -	if (error == -EFBIG)
> -		error = xfs_iext_count_upgrade(tp, ip,
> -				XFS_IEXT_ADD_NOSPLIT_CNT);
>  	if (error)
>  		goto out_trans_cancel;
>  
> diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c
> index 7d660a9739090a..82e670dd1212c4 100644
> --- a/fs/xfs/libxfs/xfs_inode_fork.c
> +++ b/fs/xfs/libxfs/xfs_inode_fork.c
> @@ -765,53 +765,46 @@ xfs_ifork_verify_local_attr(
>  	return 0;
>  }
>  
> +/*
> + * Check if the inode fork supports adding nr_to_add more extents.
> + *
> + * If it doesn't but we can upgrade it to large extent counters, do the upgrade.
> + * If we can't upgrade or are already using big counters but still can't fit the
> + * additional extents, return -EFBIG.
> + */
>  int
> -xfs_iext_count_may_overflow(
> +xfs_iext_count_ensure(

Everything looks fine, but the name isn't very good. What, exactly,
is this function ensuring about the iext count?  The function is
extending the iext count if needed, so to me the function name
should reflect what it actually does.

xfs_iext_count_extend() seems like a much better name - it tells the
reader what the code is actually doing (i.e. we may have to extend
the iext count before performing this operation) and it makes it
obvious when it is done out of place....

-Dave.
-- 
Dave Chinner
david@fromorbit.com