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

["Darrick J. Wong" <djwong@kernel.org>]

On Fri, Mar 29, 2024 at 09:04:43AM +1100, Dave Chinner wrote:
> 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;

Agreed, that looks better to me than sampling the errtag twice.

--D

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