Re: [PATCH 5/6] xfs: call xfs_bmap_exact_minlen_extent_alloc from xfs_bmap_btalloc

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

On Tue, Aug 20, 2024 at 07:04:56PM +0200, Christoph Hellwig wrote:
> xfs_bmap_exact_minlen_extent_alloc duplicates the args setup in
> xfs_bmap_btalloc.  Switch to call it from xfs_bmap_btalloc after
> doing the basic setup.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

AFAICT this looks good, though it might be useful to get an ack from
Chandan since he wrote the minlen allocator and might know about any
weird subtleties.

Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  fs/xfs/libxfs/xfs_bmap.c | 61 +++++++++-------------------------------
>  1 file changed, 13 insertions(+), 48 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index 1db9d084a44c47..b5eeaea164ee46 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -3471,28 +3471,17 @@ xfs_bmap_process_allocated_extent(
>  #ifdef DEBUG
>  static int
>  xfs_bmap_exact_minlen_extent_alloc(
> -	struct xfs_bmalloca	*ap)
> +	struct xfs_bmalloca	*ap,
> +	struct xfs_alloc_arg	*args)
>  {
> -	struct xfs_mount	*mp = ap->ip->i_mount;
> -	struct xfs_alloc_arg	args = { .tp = ap->tp, .mp = mp };
> -	xfs_fileoff_t		orig_offset;
> -	xfs_extlen_t		orig_length;
> -	int			error;
> -
> -	ASSERT(ap->length);
> -
>  	if (ap->minlen != 1) {
> -		ap->blkno = NULLFSBLOCK;
> -		ap->length = 0;
> +		args->fsbno = NULLFSBLOCK;
>  		return 0;
>  	}
>  
> -	orig_offset = ap->offset;
> -	orig_length = ap->length;
> -
> -	args.alloc_minlen_only = 1;
> -
> -	xfs_bmap_compute_alignments(ap, &args);
> +	args->alloc_minlen_only = 1;
> +	args->minlen = args->maxlen = ap->minlen;
> +	args->total = ap->total;
>  
>  	/*
>  	 * Unlike the longest extent available in an AG, we don't track
> @@ -3502,33 +3491,9 @@ xfs_bmap_exact_minlen_extent_alloc(
>  	 * we need not be concerned about a drop in performance in
>  	 * "debug only" code paths.
>  	 */
> -	ap->blkno = XFS_AGB_TO_FSB(mp, 0, 0);
> +	ap->blkno = XFS_AGB_TO_FSB(ap->ip->i_mount, 0, 0);
>  
> -	args.oinfo = XFS_RMAP_OINFO_SKIP_UPDATE;
> -	args.minlen = args.maxlen = ap->minlen;
> -	args.total = ap->total;
> -
> -	args.alignment = 1;
> -	args.minalignslop = 0;
> -
> -	args.minleft = ap->minleft;
> -	args.wasdel = ap->wasdel;
> -	args.resv = XFS_AG_RESV_NONE;
> -	args.datatype = ap->datatype;
> -
> -	error = xfs_alloc_vextent_first_ag(&args, ap->blkno);
> -	if (error)
> -		return error;
> -
> -	if (args.fsbno != NULLFSBLOCK) {
> -		xfs_bmap_process_allocated_extent(ap, &args, orig_offset,
> -			orig_length);
> -	} else {
> -		ap->blkno = NULLFSBLOCK;
> -		ap->length = 0;
> -	}
> -
> -	return 0;
> +	return xfs_alloc_vextent_first_ag(args, ap->blkno);
>  }
>  #else
>  
> @@ -3792,8 +3757,11 @@ xfs_bmap_btalloc(
>  	/* Trim the allocation back to the maximum an AG can fit. */
>  	args.maxlen = min(ap->length, mp->m_ag_max_usable);
>  
> -	if ((ap->datatype & XFS_ALLOC_USERDATA) &&
> -	    xfs_inode_is_filestream(ap->ip))
> +	if (unlikely(XFS_TEST_ERROR(false, mp,
> +			XFS_ERRTAG_BMAP_ALLOC_MINLEN_EXTENT)))
> +		error = xfs_bmap_exact_minlen_extent_alloc(ap, &args);
> +	else if ((ap->datatype & XFS_ALLOC_USERDATA) &&
> +			xfs_inode_is_filestream(ap->ip))
>  		error = xfs_bmap_btalloc_filestreams(ap, &args, stripe_align);
>  	else
>  		error = xfs_bmap_btalloc_best_length(ap, &args, stripe_align);
> @@ -4208,9 +4176,6 @@ xfs_bmapi_allocate(
>  	if ((bma->datatype & XFS_ALLOC_USERDATA) &&
>  	    XFS_IS_REALTIME_INODE(bma->ip))
>  		error = xfs_bmap_rtalloc(bma);
> -	else if (unlikely(XFS_TEST_ERROR(false, mp,
> -			XFS_ERRTAG_BMAP_ALLOC_MINLEN_EXTENT)))
> -		error = xfs_bmap_exact_minlen_extent_alloc(bma);
>  	else
>  		error = xfs_bmap_btalloc(bma);
>  	if (error)
> -- 
> 2.43.0
> 
>