diff mbox series

[5/9] xfs: aligned EOF allocations don't need to scan AGs anymore

Message ID 20231004001943.349265-6-david@fromorbit.com (mailing list archive)
State New, archived
Headers show
Series xfs: push perags further into allocation routines | expand

Commit Message

Dave Chinner Oct. 4, 2023, 12:19 a.m. UTC 
From: Dave Chinner <dchinner@redhat.com>

Now that contiguous free space selection takes into account stripe
alignment, we no longer need to do an "all AGs" allocation scan in
the case the initial AG doesn't have enough contiguous free space
for a stripe aligned allocation. This cleans up
xfs_bmap_btalloc_aligned() the same for both filestreams and the
normal btree allocation code.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/libxfs/xfs_bmap.c | 40 +++++++++++++---------------------------
 1 file changed, 13 insertions(+), 27 deletions(-)

Comments

Christoph Hellwig Oct. 5, 2023, 11:59 a.m. UTC | #1 
> -	if (ap->aeof && ap->offset) {
> +	if (ap->aeof && ap->offset)
>  		error = xfs_bmap_btalloc_at_eof(ap, args, blen, stripe_align);
> -	}
> +
> +	if (error || args->fsbno != NULLFSBLOCK)
> +		goto out_perag_rele;

Maybe just personal preference, but I find it much easier to read
if the error handling is indented and sits in a block with the
condition that can cause it, i.e.

	if (ap->aeof && ap->offset) {
 		error = xfs_bmap_btalloc_at_eof(ap, args, blen, stripe_align);
		if (error || args->fsbno != NULLFSBLOCK)
			goto out_perag_rele;
	}

but except for that nipicks this looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>
Darrick J. Wong Oct. 6, 2023, 5:55 a.m. UTC | #2 
On Wed, Oct 04, 2023 at 11:19:39AM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> Now that contiguous free space selection takes into account stripe
> alignment, we no longer need to do an "all AGs" allocation scan in
> the case the initial AG doesn't have enough contiguous free space
> for a stripe aligned allocation. This cleans up
> xfs_bmap_btalloc_aligned() the same for both filestreams and the
> normal btree allocation code.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
>  fs/xfs/libxfs/xfs_bmap.c | 40 +++++++++++++---------------------------
>  1 file changed, 13 insertions(+), 27 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index 3c250c89f42e..c1e2c0707e20 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -3538,10 +3538,8 @@ xfs_bmap_btalloc_aligned(
>  	struct xfs_bmalloca	*ap,
>  	struct xfs_alloc_arg	*args,
>  	xfs_extlen_t		blen,
> -	int			stripe_align,
> -	bool			ag_only)
> +	int			stripe_align)
>  {
> -	struct xfs_perag        *caller_pag = args->pag;
>  	int			error;
>  
>  	/*
> @@ -3558,14 +3556,7 @@ xfs_bmap_btalloc_aligned(
>  	args->alignment = stripe_align;
>  	args->minalignslop = 0;
>  
> -	if (ag_only) {
> -		error = xfs_alloc_vextent_near_bno(args, ap->blkno);
> -	} else {
> -		args->pag = NULL;
> -		error = xfs_alloc_vextent_start_ag(args, ap->blkno);
> -		ASSERT(args->pag == NULL);
> -		args->pag = caller_pag;
> -	}
> +	error = xfs_alloc_vextent_near_bno(args, ap->blkno);
>  	if (error)
>  		return error;
>  
> @@ -3650,8 +3641,7 @@ xfs_bmap_btalloc_filestreams(
>  		goto out_low_space;
>  
>  	if (ap->aeof)
> -		error = xfs_bmap_btalloc_aligned(ap, args, blen, stripe_align,
> -				true);
> +		error = xfs_bmap_btalloc_aligned(ap, args, blen, stripe_align);
>  
>  	if (!error && args->fsbno == NULLFSBLOCK)
>  		error = xfs_alloc_vextent_near_bno(args, ap->blkno);
> @@ -3715,9 +3705,16 @@ xfs_bmap_btalloc_best_length(
>  		return error;
>  	ASSERT(args->pag);
>  
> -	if (ap->aeof && ap->offset) {
> +	if (ap->aeof && ap->offset)
>  		error = xfs_bmap_btalloc_at_eof(ap, args, blen, stripe_align);
> -	}
> +
> +	if (error || args->fsbno != NULLFSBLOCK)
> +		goto out_perag_rele;
> +
> +

Double blank lines here.  With that fixed,
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

I like the simplifications going on here.

--D


> +	/* attempt aligned allocation for new EOF extents */
> +	if (stripe_align)
> +		error = xfs_bmap_btalloc_aligned(ap, args, blen, stripe_align);
>  
>  	/*
>  	 * We are now done with the perag reference for the optimal allocation
> @@ -3725,24 +3722,13 @@ xfs_bmap_btalloc_best_length(
>  	 * now as we've either succeeded, had a fatal error or we are out of
>  	 * space and need to do a full filesystem scan for free space which will
>  	 * take it's own references.
> -	 *
> -	 * XXX: now that xfs_bmap_btalloc_select_lengths() selects an AG with
> -	 * enough contiguous free space in it for an aligned allocation, we
> -	 * can change the aligned allocation at EOF to just be a single AG
> -	 * allocation.
>  	 */
> +out_perag_rele:
>  	xfs_perag_rele(args->pag);
>  	args->pag = NULL;
>  	if (error || args->fsbno != NULLFSBLOCK)
>  		return error;
>  
> -	/* attempt aligned allocation for new EOF extents */
> -	if (stripe_align)
> -		error = xfs_bmap_btalloc_aligned(ap, args, blen, stripe_align,
> -				false);
> -	if (error || args->fsbno != NULLFSBLOCK)
> -		return error;
> -
>  	/* attempt unaligned allocation */
>  	error = xfs_alloc_vextent_start_ag(args, ap->blkno);
>  	if (error || args->fsbno != NULLFSBLOCK)
> -- 
> 2.40.1
>
diff mbox series

Patch

diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index 3c250c89f42e..c1e2c0707e20 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -3538,10 +3538,8 @@  xfs_bmap_btalloc_aligned(
 	struct xfs_bmalloca	*ap,
 	struct xfs_alloc_arg	*args,
 	xfs_extlen_t		blen,
-	int			stripe_align,
-	bool			ag_only)
+	int			stripe_align)
 {
-	struct xfs_perag        *caller_pag = args->pag;
 	int			error;
 
 	/*
@@ -3558,14 +3556,7 @@  xfs_bmap_btalloc_aligned(
 	args->alignment = stripe_align;
 	args->minalignslop = 0;
 
-	if (ag_only) {
-		error = xfs_alloc_vextent_near_bno(args, ap->blkno);
-	} else {
-		args->pag = NULL;
-		error = xfs_alloc_vextent_start_ag(args, ap->blkno);
-		ASSERT(args->pag == NULL);
-		args->pag = caller_pag;
-	}
+	error = xfs_alloc_vextent_near_bno(args, ap->blkno);
 	if (error)
 		return error;
 
@@ -3650,8 +3641,7 @@  xfs_bmap_btalloc_filestreams(
 		goto out_low_space;
 
 	if (ap->aeof)
-		error = xfs_bmap_btalloc_aligned(ap, args, blen, stripe_align,
-				true);
+		error = xfs_bmap_btalloc_aligned(ap, args, blen, stripe_align);
 
 	if (!error && args->fsbno == NULLFSBLOCK)
 		error = xfs_alloc_vextent_near_bno(args, ap->blkno);
@@ -3715,9 +3705,16 @@  xfs_bmap_btalloc_best_length(
 		return error;
 	ASSERT(args->pag);
 
-	if (ap->aeof && ap->offset) {
+	if (ap->aeof && ap->offset)
 		error = xfs_bmap_btalloc_at_eof(ap, args, blen, stripe_align);
-	}
+
+	if (error || args->fsbno != NULLFSBLOCK)
+		goto out_perag_rele;
+
+
+	/* attempt aligned allocation for new EOF extents */
+	if (stripe_align)
+		error = xfs_bmap_btalloc_aligned(ap, args, blen, stripe_align);
 
 	/*
 	 * We are now done with the perag reference for the optimal allocation
@@ -3725,24 +3722,13 @@  xfs_bmap_btalloc_best_length(
 	 * now as we've either succeeded, had a fatal error or we are out of
 	 * space and need to do a full filesystem scan for free space which will
 	 * take it's own references.
-	 *
-	 * XXX: now that xfs_bmap_btalloc_select_lengths() selects an AG with
-	 * enough contiguous free space in it for an aligned allocation, we
-	 * can change the aligned allocation at EOF to just be a single AG
-	 * allocation.
 	 */
+out_perag_rele:
 	xfs_perag_rele(args->pag);
 	args->pag = NULL;
 	if (error || args->fsbno != NULLFSBLOCK)
 		return error;
 
-	/* attempt aligned allocation for new EOF extents */
-	if (stripe_align)
-		error = xfs_bmap_btalloc_aligned(ap, args, blen, stripe_align,
-				false);
-	if (error || args->fsbno != NULLFSBLOCK)
-		return error;
-
 	/* attempt unaligned allocation */
 	error = xfs_alloc_vextent_start_ag(args, ap->blkno);
 	if (error || args->fsbno != NULLFSBLOCK)