On Wed, Oct 23, 2024 at 03:37:23PM +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@xxxxxx>
Looks fine to me,
Reviewed-by: Darrick J. Wong <djwong@xxxxxxxxxx>
--D
> ---
> fs/xfs/xfs_filestream.c | 96 ++++++++++++++++++++---------------------
> 1 file changed, 46 insertions(+), 50 deletions(-)
>
> diff --git a/fs/xfs/xfs_filestream.c b/fs/xfs/xfs_filestream.c
> index 88bd23ce74cd..290ba8887d29 100644
> --- a/fs/xfs/xfs_filestream.c
> +++ b/fs/xfs/xfs_filestream.c
> @@ -67,22 +67,28 @@ xfs_filestream_pick_ag(
> xfs_extlen_t 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. */
> @@ -107,7 +113,9 @@ xfs_filestream_pick_ag(
> !(flags & XFS_PICK_USERDATA) ||
> (flags & XFS_PICK_LOWSPACE))) {
> /* Break out, retaining the reference on the AG. */
> - break;
> + if (max_pag)
> + xfs_perag_rele(max_pag);
> + goto done;
> }
> }
>
> @@ -115,56 +123,44 @@ 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;
> - }
> -
> - /*
> - * 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, pag) {
> - max_pag = pag;
> - break;
> - }
> + /*
> + * 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;
> + }
>
> - /* Bail if there are no AGs at all to select from. */
> - if (!max_pag)
> - return -ENOSPC;
> + /*
> + * 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, pag) {
> + max_pag = pag;
> + break;
> }
>
> - pag = max_pag;
> - atomic_inc(&pag->pagf_fstrms);
> - } else if (max_pag) {
> - xfs_perag_rele(max_pag);
> + /* Bail if there are no AGs at all to select from. */
> + if (!max_pag)
> + return -ENOSPC;
> }
>
> + pag = max_pag;
> + atomic_inc(&pag->pagf_fstrms);
> +done:
> trace_xfs_filestream_pick(pag, pino);
> args->pag = pag;
> return 0;
> --
> 2.45.2
>
>
