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

[Dave Chinner <david@fromorbit.com>]

On Thu, Mar 28, 2024 at 08:02:54AM +0100, 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>
> ---
>  fs/xfs/libxfs/xfs_attr.c       |  5 +--
>  fs/xfs/libxfs/xfs_bmap.c       |  5 +--
>  fs/xfs/libxfs/xfs_inode_fork.c | 62 ++++++++++++++++------------------
>  fs/xfs/libxfs/xfs_inode_fork.h |  4 +--
>  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, 44 insertions(+), 88 deletions(-)

....

> diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c
> index 7d660a9739090a..235c41eca5edd7 100644
> --- a/fs/xfs/libxfs/xfs_inode_fork.c
> +++ b/fs/xfs/libxfs/xfs_inode_fork.c
> @@ -765,53 +765,49 @@ 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_upgrade(
> +	struct xfs_trans	*tp,
>  	struct xfs_inode	*ip,
>  	int			whichfork,
> -	int			nr_to_add)
> +	uint			nr_to_add)
>  {
> +	struct xfs_mount	*mp = ip->i_mount;
> +	bool			has_large =
> +		xfs_inode_has_large_extent_counts(ip);
>  	struct xfs_ifork	*ifp = xfs_ifork_ptr(ip, whichfork);
>  	uint64_t		max_exts;
>  	uint64_t		nr_exts;
>  
> +	ASSERT(nr_to_add <= XFS_MAX_EXTCNT_UPGRADE_NR);
> +
>  	if (whichfork == XFS_COW_FORK)
>  		return 0;
>  
> -	max_exts = xfs_iext_max_nextents(xfs_inode_has_large_extent_counts(ip),
> -				whichfork);
> -
> -	if (XFS_TEST_ERROR(false, ip->i_mount, XFS_ERRTAG_REDUCE_MAX_IEXTENTS))
> -		max_exts = 10;
> -
>  	nr_exts = ifp->if_nextents + nr_to_add;
> -	if (nr_exts < ifp->if_nextents || nr_exts > max_exts)
> +	if (nr_exts < ifp->if_nextents) {
> +		/* no point in upgrading if if_nextents overflows */
>  		return -EFBIG;
> +	}
>  
> -	return 0;
> -}
> -
> -/*
> - * Upgrade this inode's extent counter fields to be able to handle a potential
> - * increase in the extent count by nr_to_add.  Normally this is the same
> - * quantity that caused xfs_iext_count_may_overflow() to return -EFBIG.
> - */
> -int
> -xfs_iext_count_upgrade(
> -	struct xfs_trans	*tp,
> -	struct xfs_inode	*ip,
> -	uint			nr_to_add)
> -{
> -	ASSERT(nr_to_add <= XFS_MAX_EXTCNT_UPGRADE_NR);
> -
> -	if (!xfs_has_large_extent_counts(ip->i_mount) ||
> -	    xfs_inode_has_large_extent_counts(ip) ||
> -	    XFS_TEST_ERROR(false, ip->i_mount, XFS_ERRTAG_REDUCE_MAX_IEXTENTS))
> -		return -EFBIG;
> -
> -	ip->i_diflags2 |= XFS_DIFLAG2_NREXT64;
> -	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
> -
> +	if (XFS_TEST_ERROR(false, mp, XFS_ERRTAG_REDUCE_MAX_IEXTENTS))
> +		max_exts = 10;
> +	else
> +		max_exts = xfs_iext_max_nextents(has_large, whichfork);
> +	if (nr_exts > max_exts) {
> +		if (has_large || !xfs_has_large_extent_counts(mp) ||
> +		    XFS_TEST_ERROR(false, mp, XFS_ERRTAG_REDUCE_MAX_IEXTENTS))
> +			return -EFBIG;
> +		ip->i_diflags2 |= XFS_DIFLAG2_NREXT64;
> +		xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
> +	}

IIUC, testing the error tag twice won't always give the same result.
I think this will be more reliable, and it self-documents the error
injection case better:

	if (XFS_TEST_ERROR(false, ip->i_mount, XFS_ERRTAG_REDUCE_MAX_IEXTENTS) &&
	    nr_exts > 10))
		return -EFBIG;

	if (nr_exts > xfs_iext_max_nextents(has_large, whichfork)) {
		if (has_large || !xfs_has_large_extent_counts(mp))
			return -EFBIG;
		ip->i_diflags2 |= XFS_DIFLAG2_NREXT64;
		xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
	}
	return 0;

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