Re: [PATCH v2 5/5] btrfs: reflow btrfs_free_tree_block

[Filipe Manana]

On Thu, Nov 23, 2023 at 3:48 PM Johannes Thumshirn
<johannes.thumshirn@xxxxxxx> wrote:
>
> Reflow btrfs_free_tree_block() so that there is one level of indentation
> needed.
>
> This patch has no functional changes.
>
> Reviewed-by: Christoph Hellwig <hch@xxxxxx>
> Reviewed-by: Josef Bacik <josef@xxxxxxxxxxxxxx>
> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@xxxxxxx>
> ---
>  fs/btrfs/extent-tree.c | 97 +++++++++++++++++++++++++-------------------------
>  1 file changed, 49 insertions(+), 48 deletions(-)
>
> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> index 4044102271e9..093aaf7aeb3a 100644
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -3426,6 +3426,7 @@ void btrfs_free_tree_block(struct btrfs_trans_handle *trans,
>  {
>         struct btrfs_fs_info *fs_info = trans->fs_info;
>         struct btrfs_ref generic_ref = { 0 };
> +       struct btrfs_block_group *cache;

While at it, can we rename 'cache' to something like 'bg'?

The cache name comes from the times where the structure was named
btrfs_block_group_cache, and having it named cache is always
confusing.

Nevertheless, the change looks fine to me.

Reviewed-by: Filipe Manana <fdmanana@xxxxxxxx>

>         int ret;
>
>         btrfs_init_generic_ref(&generic_ref, BTRFS_DROP_DELAYED_REF,
> @@ -3439,64 +3440,64 @@ void btrfs_free_tree_block(struct btrfs_trans_handle *trans,
>                 BUG_ON(ret); /* -ENOMEM */
>         }
>
> -       if (last_ref && btrfs_header_generation(buf) == trans->transid) {
> -               struct btrfs_block_group *cache;
> -               bool must_pin = false;
> -
> -               if (root_id != BTRFS_TREE_LOG_OBJECTID) {
> -                       ret = check_ref_cleanup(trans, buf->start);
> -                       if (!ret)
> -                               goto out;
> -               }
> +       if (!last_ref)
> +               return;
>
> -               cache = btrfs_lookup_block_group(fs_info, buf->start);
> +       if (btrfs_header_generation(buf) != trans->transid)
> +               goto out;
>
> -               if (btrfs_header_flag(buf, BTRFS_HEADER_FLAG_WRITTEN)) {
> -                       pin_down_extent(trans, cache, buf->start, buf->len, 1);
> -                       btrfs_put_block_group(cache);
> +       if (root_id != BTRFS_TREE_LOG_OBJECTID) {
> +               ret = check_ref_cleanup(trans, buf->start);
> +               if (!ret)
>                         goto out;
> -               }
> +       }
>
> -               /*
> -                * If there are tree mod log users we may have recorded mod log
> -                * operations for this node.  If we re-allocate this node we
> -                * could replay operations on this node that happened when it
> -                * existed in a completely different root.  For example if it
> -                * was part of root A, then was reallocated to root B, and we
> -                * are doing a btrfs_old_search_slot(root b), we could replay
> -                * operations that happened when the block was part of root A,
> -                * giving us an inconsistent view of the btree.
> -                *
> -                * We are safe from races here because at this point no other
> -                * node or root points to this extent buffer, so if after this
> -                * check a new tree mod log user joins we will not have an
> -                * existing log of operations on this node that we have to
> -                * contend with.
> -                */
> -               if (test_bit(BTRFS_FS_TREE_MOD_LOG_USERS, &fs_info->flags))
> -                       must_pin = true;
> +       cache = btrfs_lookup_block_group(fs_info, buf->start);
>
> -               if (must_pin || btrfs_is_zoned(fs_info)) {
> -                       pin_down_extent(trans, cache, buf->start, buf->len, 1);
> -                       btrfs_put_block_group(cache);
> -                       goto out;
> -               }
> +       if (btrfs_header_flag(buf, BTRFS_HEADER_FLAG_WRITTEN)) {
> +               pin_down_extent(trans, cache, buf->start, buf->len, 1);
> +               btrfs_put_block_group(cache);
> +               goto out;
> +       }
>
> -               WARN_ON(test_bit(EXTENT_BUFFER_DIRTY, &buf->bflags));
> +       /*
> +        * If there are tree mod log users we may have recorded mod log
> +        * operations for this node.  If we re-allocate this node we
> +        * could replay operations on this node that happened when it
> +        * existed in a completely different root.  For example if it
> +        * was part of root A, then was reallocated to root B, and we
> +        * are doing a btrfs_old_search_slot(root b), we could replay
> +        * operations that happened when the block was part of root A,
> +        * giving us an inconsistent view of the btree.
> +        *
> +        * We are safe from races here because at this point no other
> +        * node or root points to this extent buffer, so if after this
> +        * check a new tree mod log user joins we will not have an
> +        * existing log of operations on this node that we have to
> +        * contend with.
> +        */
>
> -               btrfs_add_free_space(cache, buf->start, buf->len);
> -               btrfs_free_reserved_bytes(cache, buf->len, 0);
> +       if (test_bit(BTRFS_FS_TREE_MOD_LOG_USERS, &fs_info->flags)
> +                    || btrfs_is_zoned(fs_info)) {
> +               pin_down_extent(trans, cache, buf->start, buf->len, 1);
>                 btrfs_put_block_group(cache);
> -               trace_btrfs_reserved_extent_free(fs_info, buf->start, buf->len);
> +               goto out;
>         }
> +
> +       WARN_ON(test_bit(EXTENT_BUFFER_DIRTY, &buf->bflags));
> +
> +       btrfs_add_free_space(cache, buf->start, buf->len);
> +       btrfs_free_reserved_bytes(cache, buf->len, 0);
> +       btrfs_put_block_group(cache);
> +       trace_btrfs_reserved_extent_free(fs_info, buf->start, buf->len);
> +
>  out:
> -       if (last_ref) {
> -               /*
> -                * Deleting the buffer, clear the corrupt flag since it doesn't
> -                * matter anymore.
> -                */
> -               clear_bit(EXTENT_BUFFER_CORRUPT, &buf->bflags);
> -       }
> +
> +       /*
> +        * Deleting the buffer, clear the corrupt flag since it doesn't
> +        * matter anymore.
> +        */
> +       clear_bit(EXTENT_BUFFER_CORRUPT, &buf->bflags);
>  }
>
>  /* Can return -ENOMEM */
>
> --
> 2.41.0
>
>