Re: [PATCH 1/2] xfs: streamline xfs_filestream_pick_ag

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

On Tue, Oct 22, 2024 at 02:13:37PM +0200, Christoph Hellwig wrote:
> Directly return the error from xfs_bmap_longest_free_extent instead
> of breaking from the loop and handling it there, and use a done
> label to directly jump to the exist when we found a suitable perag
> structure to reduce the indentation level and pag/max_pag check
> complexity in the tail of the function.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

So the key change here is that now the function can exit directly from
the for_each_perag_wrap loop if it finds a suitable perag, and that the
rest of the function has less indentation?

Ok, sounds good to me though the bugfix probably should've come first.

Don't really care either way, so
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  fs/xfs/xfs_filestream.c | 95 ++++++++++++++++++++---------------------
>  1 file changed, 46 insertions(+), 49 deletions(-)
> 
> diff --git a/fs/xfs/xfs_filestream.c b/fs/xfs/xfs_filestream.c
> index e3aaa055559781..f523027cc32586 100644
> --- a/fs/xfs/xfs_filestream.c
> +++ b/fs/xfs/xfs_filestream.c
> @@ -67,22 +67,28 @@ xfs_filestream_pick_ag(
>  	xfs_extlen_t		free = 0, minfree, maxfree = 0;
>  	xfs_agnumber_t		agno;
>  	bool			first_pass = true;
> -	int			err;
>  
>  	/* 2% of an AG's blocks must be free for it to be chosen. */
>  	minfree = mp->m_sb.sb_agblocks / 50;
>  
>  restart:
>  	for_each_perag_wrap(mp, start_agno, agno, pag) {
> +		int		err;
> +
>  		trace_xfs_filestream_scan(pag, pino);
> +
>  		*longest = 0;
>  		err = xfs_bmap_longest_free_extent(pag, NULL, longest);
>  		if (err) {
> -			if (err != -EAGAIN)
> -				break;
> -			/* Couldn't lock the AGF, skip this AG. */
> -			err = 0;
> -			continue;
> +			if (err == -EAGAIN) {
> +				/* Couldn't lock the AGF, skip this AG. */
> +				err = 0;
> +				continue;
> +			}
> +			xfs_perag_rele(pag);
> +			if (max_pag)
> +				xfs_perag_rele(max_pag);
> +			return err;
>  		}
>  
>  		/* Keep track of the AG with the most free blocks. */
> @@ -108,7 +114,9 @@ xfs_filestream_pick_ag(
>  			     (flags & XFS_PICK_LOWSPACE))) {
>  				/* Break out, retaining the reference on the AG. */
>  				free = pag->pagf_freeblks;
> -				break;
> +				if (max_pag)
> +					xfs_perag_rele(max_pag);
> +				goto done;
>  			}
>  		}
>  
> @@ -116,53 +124,42 @@ xfs_filestream_pick_ag(
>  		atomic_dec(&pag->pagf_fstrms);
>  	}
>  
> -	if (err) {
> -		xfs_perag_rele(pag);
> -		if (max_pag)
> -			xfs_perag_rele(max_pag);
> -		return err;
> +	/*
> +	 * Allow a second pass to give xfs_bmap_longest_free_extent() another
> +	 * attempt at locking AGFs that it might have skipped over before we
> +	 * fail.
> +	 */
> +	if (first_pass) {
> +		first_pass = false;
> +		goto restart;
>  	}
>  
> -	if (!pag) {
> -		/*
> -		 * Allow a second pass to give xfs_bmap_longest_free_extent()
> -		 * another attempt at locking AGFs that it might have skipped
> -		 * over before we fail.
> -		 */
> -		if (first_pass) {
> -			first_pass = false;
> -			goto restart;
> -		}
> -
> -		/*
> -		 * We must be low on data space, so run a final lowspace
> -		 * optimised selection pass if we haven't already.
> -		 */
> -		if (!(flags & XFS_PICK_LOWSPACE)) {
> -			flags |= XFS_PICK_LOWSPACE;
> -			goto restart;
> -		}
> +	/*
> +	 * We must be low on data space, so run a final lowspace optimised
> +	 * selection pass if we haven't already.
> +	 */
> +	if (!(flags & XFS_PICK_LOWSPACE)) {
> +		flags |= XFS_PICK_LOWSPACE;
> +		goto restart;
> +	}
>  
> -		/*
> -		 * No unassociated AGs are available, so select the AG with the
> -		 * most free space, regardless of whether it's already in use by
> -		 * another filestream. It none suit, just use whatever AG we can
> -		 * grab.
> -		 */
> -		if (!max_pag) {
> -			for_each_perag_wrap(args->mp, 0, start_agno, args->pag)
> -				break;
> -			atomic_inc(&args->pag->pagf_fstrms);
> -			*longest = 0;
> -		} else {
> -			pag = max_pag;
> -			free = maxfree;
> -			atomic_inc(&pag->pagf_fstrms);
> -		}
> -	} else if (max_pag) {
> -		xfs_perag_rele(max_pag);
> +	/*
> +	 * No unassociated AGs are available, so select the AG with the most
> +	 * free space, regardless of whether it's already in use by another
> +	 * filestream. It none suit, just use whatever AG we can grab.
> +	 */
> +	if (!max_pag) {
> +		for_each_perag_wrap(args->mp, 0, start_agno, args->pag)
> +			break;
> +		atomic_inc(&args->pag->pagf_fstrms);
> +		*longest = 0;
> +	} else {
> +		pag = max_pag;
> +		free = maxfree;
> +		atomic_inc(&pag->pagf_fstrms);
>  	}
>  
> +done:
>  	trace_xfs_filestream_pick(pag, pino, free);
>  	args->pag = pag;
>  	return 0;
> -- 
> 2.45.2
> 
>