Re: [PATCH] btrfs: decrease indendation of find_free_extent_update_loop

[Filipe Manana <fdmanana@kernel.org>]

On Tue, Mar 17, 2026 at 1:29 PM Johannes Thumshirn
<johannes.thumshirn@wdc.com> wrote:
>
> Decrease the indendation of find_free_extent_update_loop(), by inverting
> the check if the loop state is smaller than LOOP_NO_EMPTY_SIZE.
>
> This also allows for an early return from find_free_extent_update_loop(),
> in case LOOP_NO_EMPTY_SIZE is already set at this point.
>
> While at if change a

if -> it

>
> if () {
> }
> else if
> else
>
> pattern to all using curly braces and be consistent with the rest of btrfs
> code.
>
> No functional changes intended.
>
> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
> ---
>  fs/btrfs/extent-tree.c | 111 +++++++++++++++++++++--------------------
>  1 file changed, 57 insertions(+), 54 deletions(-)
>
> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> index a55b910c755b..062fc1beb26b 100644
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -4349,71 +4349,74 @@ static int find_free_extent_update_loop(struct btrfs_fs_info *fs_info,
>                 return 1;
>
>         /* See the comments for btrfs_loop_type for an explanation of the phases. */
> -       if (ffe_ctl->loop < LOOP_NO_EMPTY_SIZE) {
> -               ffe_ctl->index = 0;
> -               /*
> -                * We want to skip the LOOP_CACHING_WAIT step if we don't have
> -                * any uncached bgs and we've already done a full search
> -                * through.
> -                */
> -               if (ffe_ctl->loop == LOOP_CACHING_NOWAIT &&
> -                   (!ffe_ctl->orig_have_caching_bg && full_search))
> -                       ffe_ctl->loop++;
> +       if (ffe_ctl->loop == LOOP_NO_EMPTY_SIZE)
> +               return -ENOSPC;
> +
> +       ffe_ctl->index = 0;
> +       /*
> +        * We want to skip the LOOP_CACHING_WAIT step if we don't have
> +        * any uncached bgs and we've already done a full search
> +        * through.
> +        */
> +       if (ffe_ctl->loop == LOOP_CACHING_NOWAIT &&
> +           (!ffe_ctl->orig_have_caching_bg && full_search))
>                 ffe_ctl->loop++;
> +       ffe_ctl->loop++;
>
> -               if (ffe_ctl->loop == LOOP_ALLOC_CHUNK) {
> -                       struct btrfs_trans_handle *trans;
> -                       int exist = 0;
> +       if (ffe_ctl->loop == LOOP_ALLOC_CHUNK) {
> +               struct btrfs_trans_handle *trans;
> +               int exist = 0;

This is a good opportunity to change the variable type to bool and
give it a more meaningful and correct name:

bool have_trans = false;

>
> -                       /* Check if allocation policy allows to create a new chunk */
> -                       ret = can_allocate_chunk(fs_info, ffe_ctl);
> -                       if (ret)
> -                               return ret;
> +               /* Check if allocation policy allows to create a new chunk */

A good opportunity too to add missing punctuation to comment.

> +               ret = can_allocate_chunk(fs_info, ffe_ctl);
> +               if (ret)
> +                       return ret;
>
> -                       trans = current->journal_info;
> -                       if (trans)
> -                               exist = 1;
> -                       else
> -                               trans = btrfs_join_transaction(root);
> +               trans = current->journal_info;
> +               if (trans)
> +                       exist = 1;

And then here:

have_trans = true;

> +               else
> +                       trans = btrfs_join_transaction(root);
>
> -                       if (IS_ERR(trans))
> -                               return PTR_ERR(trans);
> +               if (IS_ERR(trans))
> +                       return PTR_ERR(trans);
>
> -                       ret = btrfs_chunk_alloc(trans, space_info, ffe_ctl->flags,
> -                                               CHUNK_ALLOC_FORCE_FOR_EXTENT);
> +               ret = btrfs_chunk_alloc(trans, space_info, ffe_ctl->flags,
> +                                       CHUNK_ALLOC_FORCE_FOR_EXTENT);
>
> -                       /* Do not bail out on ENOSPC since we can do more. */
> -                       if (ret == -ENOSPC) {
> -                               ret = 0;
> -                               ffe_ctl->loop++;
> -                       }
> -                       else if (ret < 0)
> -                               btrfs_abort_transaction(trans, ret);
> -                       else
> -                               ret = 0;
> -                       if (!exist)
> -                               btrfs_end_transaction(trans);
> -                       if (ret)
> -                               return ret;
> +               /* Do not bail out on ENOSPC since we can do more. */
> +               if (ret == -ENOSPC) {
> +                       ret = 0;
> +                       ffe_ctl->loop++;
> +               } else if (ret < 0) {
> +                       btrfs_abort_transaction(trans, ret);
> +               } else {
> +                       ret = 0;
>                 }
>
> -               if (ffe_ctl->loop == LOOP_NO_EMPTY_SIZE) {
> -                       if (ffe_ctl->policy != BTRFS_EXTENT_ALLOC_CLUSTERED)
> -                               return -ENOSPC;
> +               if (!exist)

And here:

if (!have_trans)

And have such change mentioned in the changelog too.

Otherwise it looks good:

Reviewed-by: Filipe Manana <fdmanana@suse.com>

Thanks.


> +                       btrfs_end_transaction(trans);
>
> -                       /*
> -                        * Don't loop again if we already have no empty_size and
> -                        * no empty_cluster.
> -                        */
> -                       if (ffe_ctl->empty_size == 0 &&
> -                           ffe_ctl->empty_cluster == 0)
> -                               return -ENOSPC;
> -                       ffe_ctl->empty_size = 0;
> -                       ffe_ctl->empty_cluster = 0;
> -               }
> -               return 1;
> +               if (ret)
> +                       return ret;
> +       }
> +
> +       if (ffe_ctl->loop == LOOP_NO_EMPTY_SIZE) {
> +               if (ffe_ctl->policy != BTRFS_EXTENT_ALLOC_CLUSTERED)
> +                       return -ENOSPC;
> +
> +               /*
> +                * Don't loop again if we already have no empty_size and
> +                * no empty_cluster.
> +                */
> +               if (ffe_ctl->empty_size == 0 &&
> +                   ffe_ctl->empty_cluster == 0)
> +                       return -ENOSPC;
> +               ffe_ctl->empty_size = 0;
> +               ffe_ctl->empty_cluster = 0;
>         }
> -       return -ENOSPC;
> +
> +       return 1;
>  }
>
>  static int prepare_allocation_clustered(struct btrfs_fs_info *fs_info,
> --
> 2.53.0
>
>