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@xxxxxx>
> > ---
> > 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@xxxxxxxxxxxxx
>
