Re: [PATCH 3/6] xfs: xfs_itruncate_extents has no extent count limitation

[Chandan Babu R <chandanrlinux@gmail.com>]

On 27 May 2021 at 10:21, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> Ever since we moved to freeing of extents by deferred operations,
> we've already freed extents via individual transactions. Hence the
> only limitation of how many extents we can mark for freeing in a
> single xfs_bunmapi() call bound only by how many deferrals we want
> to queue.
>
> That is xfs_bunmapi() doesn't actually do any AG based extent
> freeing, so there's no actually transaction reservation used up by
> calling bunmapi with a large count of extents to be freed. RT
> extents have always been freed directly by bunmapi, but that doesn't
> require modification of large number of blocks as there are no
> btrees to split.
>
> Some callers of xfs_bunmapi assume that the extent count being freed
> is bound by geometry (e.g. directories) and these can ask bunmapi to
> free up to 64 extents in a single call. These functions just work as
> tehy stand, so there's no reason for truncate to have a limit of
> just two extents per bunmapi call anymore.
>
> Increase XFS_ITRUNC_MAX_EXTENTS to 64 to match the number of extents
> that can be deferred in a single loop to match what the directory
> code already uses.
>
> For realtime inodes, where xfs_bunmapi() directly frees extents,
> leave the limit at 2 extents per loop as this is all the space that
> the transaction reservation will cover.
>
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
>  fs/xfs/xfs_inode.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index 0369eb22c1bb..db220eaa34b8 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -40,9 +40,18 @@ kmem_zone_t *xfs_inode_zone;
>
>  /*
>   * Used in xfs_itruncate_extents().  This is the maximum number of extents
> - * freed from a file in a single transaction.
> + * we will unmap and defer for freeing in a single call to xfs_bunmapi().
> + * Realtime inodes directly free extents in xfs_bunmapi(), so are bound
> + * by transaction reservation size to 2 extents.
>   */
> -#define	XFS_ITRUNC_MAX_EXTENTS	2
> +static inline int
> +xfs_itrunc_max_extents(
> +	struct xfs_inode	*ip)
> +{
> +	if (XFS_IS_REALTIME_INODE(ip))
> +		return 2;
> +	return 64;
> +}
>
>  STATIC int xfs_iunlink(struct xfs_trans *, struct xfs_inode *);
>  STATIC int xfs_iunlink_remove(struct xfs_trans *, struct xfs_inode *);
> @@ -1402,7 +1411,7 @@ xfs_itruncate_extents_flags(
>  	while (unmap_len > 0) {
>  		ASSERT(tp->t_firstblock == NULLFSBLOCK);
>  		error = __xfs_bunmapi(tp, ip, first_unmap_block, &unmap_len,
> -				flags, XFS_ITRUNC_MAX_EXTENTS);
> +				flags, xfs_itrunc_max_extents(ip));
>  		if (error)
>  			goto out;

The list of free extent items at xfs_defer_pending->dfp_work could
now contain XFS_EFI_MAX_FAST_EXTENTS (i.e. 16) entries in the worst case.

For a single transaction, xfs_calc_itruncate_reservation() reserves space for
logging only 4 extents (i.e. 4 exts * 2 trees * (2 * max depth - 1) * block
size). But with the above change, a single transaction can now free upto 16
extents. Wouldn't this overflow the reserved log space?

--
chandan