Re: [PATCH V8 16/19] xfs: Conditionally upgrade existing inodes to use large extent counters

[Dave Chinner <david@fromorbit.com>]

On Mon, Mar 21, 2022 at 10:47:47AM +0530, Chandan Babu R wrote:
> This commit enables upgrading existing inodes to use large extent counters
> provided that underlying filesystem's superblock has large extent counter
> feature enabled.
> 
> Signed-off-by: Chandan Babu R <chandan.babu@oracle.com>
> ---
>  fs/xfs/libxfs/xfs_attr.c       |  9 ++++++-
>  fs/xfs/libxfs/xfs_bmap.c       | 10 ++++++--
>  fs/xfs/libxfs/xfs_inode_fork.c | 27 +++++++++++++++++++++
>  fs/xfs/libxfs/xfs_inode_fork.h |  2 ++
>  fs/xfs/xfs_bmap_item.c         |  8 ++++++-
>  fs/xfs/xfs_bmap_util.c         | 43 ++++++++++++++++++++++++++++++----
>  fs/xfs/xfs_dquot.c             |  9 ++++++-
>  fs/xfs/xfs_iomap.c             | 17 ++++++++++++--
>  fs/xfs/xfs_reflink.c           | 17 ++++++++++++--
>  fs/xfs/xfs_rtalloc.c           |  9 ++++++-
>  10 files changed, 136 insertions(+), 15 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> index 23523b802539..6e56aa17fd82 100644
> --- a/fs/xfs/libxfs/xfs_attr.c
> +++ b/fs/xfs/libxfs/xfs_attr.c
> @@ -776,8 +776,15 @@ xfs_attr_set(
>  	if (args->value || xfs_inode_hasattr(dp)) {
>  		error = xfs_iext_count_may_overflow(dp, XFS_ATTR_FORK,
>  				XFS_IEXT_ATTR_MANIP_CNT(rmt_blks));
> -		if (error)
> +		if (error && error != -EFBIG)
>  			goto out_trans_cancel;
> +
> +		if (error == -EFBIG) {
> +			error = xfs_iext_count_upgrade(args->trans, dp,
> +					XFS_IEXT_ATTR_MANIP_CNT(rmt_blks));
> +			if (error)
> +				goto out_trans_cancel;
> +		}

Neater and more compact to do this by checking explicitly for
-EFBIG:

		error = xfs_iext_count_may_overflow(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;
	}
>  
>  	error = xfs_attr_lookup(args);
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index 5a089674c666..0cb915bf8285 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -4524,13 +4524,19 @@ xfs_bmapi_convert_delalloc(
>  		return error;
>  
>  	xfs_ilock(ip, XFS_ILOCK_EXCL);
> +	xfs_trans_ijoin(tp, ip, 0);
>  
>  	error = xfs_iext_count_may_overflow(ip, whichfork,
>  			XFS_IEXT_ADD_NOSPLIT_CNT);
> -	if (error)
> +	if (error && error != -EFBIG)
>  		goto out_trans_cancel;
>  
> -	xfs_trans_ijoin(tp, ip, 0);
> +	if (error == -EFBIG) {
> +		error = xfs_iext_count_upgrade(tp, ip,
> +				XFS_IEXT_ADD_NOSPLIT_CNT);
> +		if (error)
> +			goto out_trans_cancel;
> +	}
>  
>  	if (!xfs_iext_lookup_extent(ip, ifp, offset_fsb, &bma.icur, &bma.got) ||
>  	    bma.got.br_startoff > offset_fsb) {
> diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c
> index bb5d841aac58..aff9242db829 100644
> --- a/fs/xfs/libxfs/xfs_inode_fork.c
> +++ b/fs/xfs/libxfs/xfs_inode_fork.c
> @@ -756,3 +756,30 @@ xfs_iext_count_may_overflow(
>  
>  	return 0;
>  }
> +
> +int
> +xfs_iext_count_upgrade(
> +	struct xfs_trans	*tp,
> +	struct xfs_inode	*ip,
> +	int			nr_to_add)

nr_to_add can only be positive, so should be unsigned.

> +{
> +	if (!xfs_has_large_extent_counts(ip->i_mount) ||
> +	    (ip->i_diflags2 & XFS_DIFLAG2_NREXT64) ||
> +	    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);
> +
> +	/*
> +	 * The value of nr_to_add cannot be larger than 2^17
> +	 *
> +	 * - XFS_MAX_EXTCNT_ATTR_FORK_LARGE - XFS_MAX_EXTCNT_ATTR_FORK_SMALL
> +	 *   i.e. 2^32 - 2^15
> +	 * - XFS_MAX_EXTCNT_DATA_FORK_LARGE - XFS_MAX_EXTCNT_DATA_FORK_SMALL
> +	 *   i.e. 2^48 - 2^31
> +	 */
> +	ASSERT(nr_to_add <= (1 << 17));

That's a comment for the function head and/or the format
documentation in xfs_format.h, not hidden in the code itself as a
magic number. i.e. it is a format definition because it is bound by
on-disk format constants, not by code constratints. Hence this
should probably be defined in xfs_format.h alongside the large/small
extent counts, such as:

#define XFS_MAX_EXTCNT_UPGRADE_NR	\
	min(XFS_MAX_EXTCNT_ATTR_FORK_LARGE - XFS_MAX_EXTCNT_ATTR_FORK_SMALL, \
	    XFS_MAX_EXTCNT_DATA_FORK_LARGE - XFS_MAX_EXTCNT_DATA_FORK_SMALL)

And the ASSERT checking the incoming nr_to_add placed right at the
top of the function because the assert then documents API
constraints and always catches violations of them.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com