Re: [PATCH 12/19] xfs: tidy up xfs_rtallocate_extent_block

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

On Thu, Dec 14, 2023 at 07:34:31AM +0100, Christoph Hellwig wrote:
> Share the xfs_rtallocate_range logic by breaking out of the loop
> instead of duplicating it, invert a check so that the early
> return case comes first instead of in an else, and handle the
> successful case in the straight line instead a branch in the tail
> of the function.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/xfs_rtalloc.c | 63 +++++++++++++++++++++-----------------------
>  1 file changed, 30 insertions(+), 33 deletions(-)
> 
> diff --git a/fs/xfs/xfs_rtalloc.c b/fs/xfs/xfs_rtalloc.c
> index fbc60658ef24bf..5f42422a976a3e 100644
> --- a/fs/xfs/xfs_rtalloc.c
> +++ b/fs/xfs/xfs_rtalloc.c
> @@ -257,13 +257,9 @@ xfs_rtallocate_extent_block(
>  			/*
>  			 * i for maxlen is all free, allocate and return that.
>  			 */
> -			error = xfs_rtallocate_range(args, i, maxlen);
> -			if (error)
> -				return error;
> -
> -			*len = maxlen;
> -			*rtx = i;
> -			return 0;
> +			bestlen = maxlen;
> +			besti = i;
> +			break;

This return->break change gave me pause for a while...

>  		}
>  		/*
>  		 * In the case where we have a variable-sized allocation
> @@ -283,43 +279,44 @@ xfs_rtallocate_extent_block(
>  		/*
>  		 * If not done yet, find the start of the next free space.
>  		 */
> -		if (next < end) {
> -			error = xfs_rtfind_forw(args, next, end, &i);
> -			if (error)
> -				return error;
> -		} else
> +		if (next >= end)
>  			break;
> +		error = xfs_rtfind_forw(args, next, end, &i);
> +		if (error)
> +			return error;
>  	}
> +
>  	/*
>  	 * Searched the whole thing & didn't find a maxlen free extent.
>  	 */
> -	if (minlen <= maxlen && besti != -1) {
> -		xfs_rtxlen_t	p;	/* amount to trim length by */
> -
> +	if (maxlen < minlen || besti == -1) {

...because I was worried about accidentally ending up in this clause
if maxlen < minlen.  I /think/ it's the case that this function never
gets called with that true, right?

(Would this be a good place to add a ebugging assertion?)

--D

>  		/*
> -		 * If size should be a multiple of prod, make that so.
> +		 * Allocation failed.  Set *nextp to the next block to try.
>  		 */
> -		if (prod > 1) {
> -			div_u64_rem(bestlen, prod, &p);
> -			if (p)
> -				bestlen -= p;
> -		}
> +		*nextp = next;
> +		return -ENOSPC;
> +	}
>  
> -		/*
> -		 * Allocate besti for bestlen & return that.
> -		 */
> -		error = xfs_rtallocate_range(args, besti, bestlen);
> -		if (error)
> -			return error;
> -		*len = bestlen;
> -		*rtx = besti;
> -		return 0;
> +	/*
> +	 * If size should be a multiple of prod, make that so.
> +	 */
> +	if (prod > 1) {
> +		xfs_rtxlen_t	p;	/* amount to trim length by */
> +
> +		div_u64_rem(bestlen, prod, &p);
> +		if (p)
> +			bestlen -= p;
>  	}
> +
>  	/*
> -	 * Allocation failed.  Set *nextp to the next block to try.
> +	 * Allocate besti for bestlen & return that.
>  	 */
> -	*nextp = next;
> -	return -ENOSPC;
> +	error = xfs_rtallocate_range(args, besti, bestlen);
> +	if (error)
> +		return error;
> +	*len = bestlen;
> +	*rtx = besti;
> +	return 0;
>  }
>  
>  /*
> -- 
> 2.39.2
> 
>