| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAL3q7H5jwR75FwT213yteX5m=5G8ehKmnKxv=xYXbY+UuhP+qQ@mail.gmail.com>
Date: Tue, 13 Aug 2024 11:56:57 +0100
From: Filipe Manana <fdmanana@...nel.org>
To: Johannes Thumshirn <jth@...nel.org>
Cc: Chris Mason <clm@...com>, Josef Bacik <josef@...icpanda.com>, David Sterba <dsterba@...e.com>,
"open list:BTRFS FILE SYSTEM" <linux-btrfs@...r.kernel.org>, open list <linux-kernel@...r.kernel.org>,
Johannes Thumshirn <johannes.thumshirn@....com>, Qu Wenruo <wqu@...e.com>
Subject: Re: [PATCH v2] btrfs: reduce chunk_map lookups in btrfs_map_block
On Tue, Aug 13, 2024 at 11:49 AM Johannes Thumshirn <jth@...nel.org> wrote:
>
> From: Johannes Thumshirn <johannes.thumshirn@....com>
>
> Currently we're calling btrfs_num_copies() before btrfs_get_chunk_map() in
> btrfs_map_block(). But btrfs_num_copies() itself does a chunk map lookup
> to be able to calculate the number of copies.
>
> So split out the code getting the number of copies from btrfs_num_copies()
> into a helper called btrfs_chunk_map_num_copies() and directly call it
> from btrfs_map_block() and btrfs_num_copies().
>
> This saves us one rbtree lookup per btrfs_map_block() invocation.
>
> Reviewed-by: Qu Wenruo <wqu@...e.com>
> Reviewed-by: Filipe Manana <fdmanana@...e.com>
> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@....com>
> ---
> Changes in v2:
> - Added Reviewed-bys
> - Reflowed comments
> - Moved non RAID56 cases to the end without an if
> Link to v1:
> https://lore.kernel.org/all/20240812165931.9106-1-jth@kernel.org/
>
> fs/btrfs/volumes.c | 58 +++++++++++++++++++++++++---------------------
> 1 file changed, 32 insertions(+), 26 deletions(-)
>
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index e07452207426..796f6350a017 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -5781,38 +5781,44 @@ void btrfs_mapping_tree_free(struct btrfs_fs_info *fs_info)
> write_unlock(&fs_info->mapping_tree_lock);
> }
>
> +static int btrfs_chunk_map_num_copies(struct btrfs_chunk_map *map)
Same as commented before, can be const.
> +{
> + enum btrfs_raid_types index = btrfs_bg_flags_to_raid_index(map->type);
> +
> + if (map->type & BTRFS_BLOCK_GROUP_RAID5)
> + return 2;
> +
> + /*
> + * There could be two corrupted data stripes, we need to loop
> + * retry in order to rebuild the correct data.
> + *
> + * Fail a stripe at a time on every retry except the stripe
> + * under reconstruction.
> + */
> + if (map->type & BTRFS_BLOCK_GROUP_RAID6)
> + return map->num_stripes;
> +
> + /* Non-RAID56, use their ncopies from btrfs_raid_array. */
> + return btrfs_raid_array[index].ncopies;
> +}
> +
> int btrfs_num_copies(struct btrfs_fs_info *fs_info, u64 logical, u64 len)
> {
> struct btrfs_chunk_map *map;
> - enum btrfs_raid_types index;
> - int ret = 1;
> + int ret;
>
> map = btrfs_get_chunk_map(fs_info, logical, len);
> if (IS_ERR(map))
> /*
> - * We could return errors for these cases, but that could get
> - * ugly and we'd probably do the same thing which is just not do
> - * anything else and exit, so return 1 so the callers don't try
> - * to use other copies.
> + * We could return errors for these cases, but that
> + * could get ugly and we'd probably do the same thing
> + * which is just not do anything else and exit, so
> + * return 1 so the callers don't try to use other
> + * copies.
My previous comment about reformatting was just for the comment that
was moved, the one now inside btrfs_chunk_map_num_copies().
For this one I don't think we should do it, as we aren't moving it
around or changing its content.
It's just confusing to have this sort of unrelated change mixed in.
Thanks.
> */
> return 1;
>
> - index = btrfs_bg_flags_to_raid_index(map->type);
> -
> - /* Non-RAID56, use their ncopies from btrfs_raid_array. */
> - if (!(map->type & BTRFS_BLOCK_GROUP_RAID56_MASK))
> - ret = btrfs_raid_array[index].ncopies;
> - else if (map->type & BTRFS_BLOCK_GROUP_RAID5)
> - ret = 2;
> - else if (map->type & BTRFS_BLOCK_GROUP_RAID6)
> - /*
> - * There could be two corrupted data stripes, we need
> - * to loop retry in order to rebuild the correct data.
> - *
> - * Fail a stripe at a time on every retry except the
> - * stripe under reconstruction.
> - */
> - ret = map->num_stripes;
> + ret = btrfs_chunk_map_num_copies(map);
> btrfs_free_chunk_map(map);
> return ret;
> }
> @@ -6462,14 +6468,14 @@ int btrfs_map_block(struct btrfs_fs_info *fs_info, enum btrfs_map_op op,
> io_geom.stripe_index = 0;
> io_geom.op = op;
>
> - num_copies = btrfs_num_copies(fs_info, logical, fs_info->sectorsize);
> - if (io_geom.mirror_num > num_copies)
> - return -EINVAL;
> -
> map = btrfs_get_chunk_map(fs_info, logical, *length);
> if (IS_ERR(map))
> return PTR_ERR(map);
>
> + num_copies = btrfs_chunk_map_num_copies(map);
> + if (io_geom.mirror_num > num_copies)
> + return -EINVAL;
> +
> map_offset = logical - map->start;
> io_geom.raid56_full_stripe_start = (u64)-1;
> max_len = btrfs_max_io_len(map, map_offset, &io_geom);
> --
> 2.43.0
>
>
Powered by blists - more mailing lists