Re: [PATCH 08/10] btrfs: simplify conditions in btrfs_free_chunk_map()

Filipe Manana
2024-02-19T12:28:22Z

On Mon, Feb 19, 2024 at 11:14 AM David Sterba <dsterba@suse.com> wrote:
>
> The helper is simple enough for inlining, we can further simplify it by
> removing the check for map pointer validity. After this patch all
> callers always pass a valid pointer.

So by making btrfs_free_chunk_map() to not ignore a NULL pointer, we
are adding rather
surprising behaviour and inconsistency.

Most free functions ignore a NULL pointer, take the example of the
kfree() family and even free() family in the standard C library,
as well as most of the free functions we have in btrfs as well, which
are modeled on that common pattern.

Ignoring NULL makes error handling simpler, by not having the need to
take special care to call the free function with a non-NULL pointer.

Besides that, this change doesn't seem to improve anything.

Thanks.

>
> The changes to achieve this:
>
> - in verify_one_dev_extent() return and don't jump to the out label
> that could potentially pass a NULL pointer to btrfs_free_chunk_map
>
> - in btrfs_load_block_group_zone_info() add a label that specifically
> clears the map and does not go through label out that could encounter
> a NULL pointer in cache->physical_map
>
> Signed-off-by: David Sterba <dsterba@suse.com>
> ---
> fs/btrfs/volumes.c | 3 +--
> fs/btrfs/volumes.h | 2 +-
> fs/btrfs/zoned.c | 3 ++-
> 3 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 752144a31d79..55b91807aba4 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -7979,8 +7979,7 @@ static int verify_one_dev_extent(struct btrfs_fs_info *fs_info,
> btrfs_err(fs_info,
> "dev extent physical offset %llu on devid %llu doesn't have corresponding chunk",
> physical_offset, devid);
> - ret = -EUCLEAN;
> - goto out;
> + return -EUCLEAN;
> }
>
> stripe_len = btrfs_calc_stripe_length(map);
> diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
> index 21d4de0e3f1f..ce1aa7684c74 100644
> --- a/fs/btrfs/volumes.h
> +++ b/fs/btrfs/volumes.h
> @@ -566,7 +566,7 @@ struct btrfs_chunk_map {
>
> static inline void btrfs_free_chunk_map(struct btrfs_chunk_map *map)
> {
> - if (map && refcount_dec_and_test(&map->refs)) {
> + if (refcount_dec_and_test(&map->refs)) {
> ASSERT(RB_EMPTY_NODE(&map->rb_node));
> kfree(map);
> }
> diff --git a/fs/btrfs/zoned.c b/fs/btrfs/zoned.c
> index 3317bebfca95..b9346ca82c47 100644
> --- a/fs/btrfs/zoned.c
> +++ b/fs/btrfs/zoned.c
> @@ -1564,7 +1564,7 @@ int btrfs_load_block_group_zone_info(struct btrfs_block_group *cache, bool new)
> cache->physical_map = btrfs_clone_chunk_map(map, GFP_NOFS);
> if (!cache->physical_map) {
> ret = -ENOMEM;
> - goto out;
> + goto out_free_map;
> }
>
> zone_info = kcalloc(map->num_stripes, sizeof(*zone_info), GFP_NOFS);
> @@ -1677,6 +1677,7 @@ int btrfs_load_block_group_zone_info(struct btrfs_block_group *cache, bool new)
> }
> bitmap_free(active);
> kfree(zone_info);
> +out_free_map:
> btrfs_free_chunk_map(map);
>
> return ret;
> --
> 2.42.1
>
>