[v1] btrfs: reduce chunk_map lookups in btrfs_map_block

[PATCH] btrfs: reduce chunk_map lookups in btrfs_map_block

Posted by Johannes Thumshirn 1 year, 5 months ago

From: Johannes Thumshirn <johannes.thumshirn@wdc.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.

Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
---
 fs/btrfs/volumes.c | 50 +++++++++++++++++++++++++++-------------------
 1 file changed, 29 insertions(+), 21 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index e07452207426..4863bdb4d6f4 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -5781,10 +5781,33 @@ 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)
+{
+	enum btrfs_raid_types 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))
+		return btrfs_raid_array[index].ncopies;
+
+	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;
+
+	return 1;
+}
+
 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;
 
 	map = btrfs_get_chunk_map(fs_info, logical, len);
@@ -5797,22 +5820,7 @@ int btrfs_num_copies(struct btrfs_fs_info *fs_info, u64 logical, u64 len)
 		 */
 		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 +6470,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

Re: [PATCH] btrfs: reduce chunk_map lookups in btrfs_map_block

Posted by Filipe Manana 1 year, 5 months ago

On Mon, Aug 12, 2024 at 6:18 PM Johannes Thumshirn <jth@kernel.org> wrote:
>
> From: Johannes Thumshirn <johannes.thumshirn@wdc.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.
>
> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
> ---
>  fs/btrfs/volumes.c | 50 +++++++++++++++++++++++++++-------------------
>  1 file changed, 29 insertions(+), 21 deletions(-)
>
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index e07452207426..4863bdb4d6f4 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -5781,10 +5781,33 @@ 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)

Can be made const.

> +{
> +       enum btrfs_raid_types 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))
> +               return btrfs_raid_array[index].ncopies;

Since the index is only used here, it could be declared here.
Or just just shorten the function to something like this, which also
addresses Qu's comment:

static int btrfs_chunk_map_num_copies(const struct btrfs_chunk_map *map)
{
     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;
}

Less code, less one if statement, and no need for the assert Qu mentioned.

> +
> +       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.

Since this is moving the comment and it falls too short of the 80
characters line length,
it's a good opportunity to reformat it to have the lines closer to 80
characters.

Otherwise it looks fine:

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

Thanks.

> +        */
> +       if (map->type & BTRFS_BLOCK_GROUP_RAID6)
> +               return map->num_stripes;
> +
> +       return 1;
> +}
> +
>  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;
>
>         map = btrfs_get_chunk_map(fs_info, logical, len);
> @@ -5797,22 +5820,7 @@ int btrfs_num_copies(struct btrfs_fs_info *fs_info, u64 logical, u64 len)
>                  */
>                 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 +6470,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
>
>

Re: [PATCH] btrfs: reduce chunk_map lookups in btrfs_map_block

Posted by Qu Wenruo 1 year, 5 months ago


在 2024/8/13 02:29, Johannes Thumshirn 写道:
> From: Johannes Thumshirn <johannes.thumshirn@wdc.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.
>
> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Reviewed-by: Qu Wenruo <wqu@suse.com>

Just one nitpick inlined below.

> ---
>   fs/btrfs/volumes.c | 50 +++++++++++++++++++++++++++-------------------
>   1 file changed, 29 insertions(+), 21 deletions(-)
>
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index e07452207426..4863bdb4d6f4 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -5781,10 +5781,33 @@ 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)
> +{
> +	enum btrfs_raid_types 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))
> +		return btrfs_raid_array[index].ncopies;
> +
> +	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;
> +
> +	return 1;

IIRC above if()s have checks all profiles.

Thus should we call ASSERT() instead? Or return map->num_stripes since
that's the only remaining possible case.

Thanks,
Qu
> +}
> +
>   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;
>
>   	map = btrfs_get_chunk_map(fs_info, logical, len);
> @@ -5797,22 +5820,7 @@ int btrfs_num_copies(struct btrfs_fs_info *fs_info, u64 logical, u64 len)
>   		 */
>   		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 +6470,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);

Re: [PATCH] btrfs: reduce chunk_map lookups in btrfs_map_block

Posted by Johannes Thumshirn 1 year, 5 months ago

On 13.08.24 00:33, Qu Wenruo wrote:
> 
> 
> 在 2024/8/13 02:29, Johannes Thumshirn 写道:
>> From: Johannes Thumshirn <johannes.thumshirn@wdc.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.
>>
>> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
> 
> Reviewed-by: Qu Wenruo <wqu@suse.com>
> 
> Just one nitpick inlined below.
> 
>> ---
>>    fs/btrfs/volumes.c | 50 +++++++++++++++++++++++++++-------------------
>>    1 file changed, 29 insertions(+), 21 deletions(-)
>>
>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>> index e07452207426..4863bdb4d6f4 100644
>> --- a/fs/btrfs/volumes.c
>> +++ b/fs/btrfs/volumes.c
>> @@ -5781,10 +5781,33 @@ 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)
>> +{
>> +	enum btrfs_raid_types 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))
>> +		return btrfs_raid_array[index].ncopies;
>> +
>> +	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;
>> +
>> +	return 1;
> 
> IIRC above if()s have checks all profiles.
> 
> Thus should we call ASSERT() instead? Or return map->num_stripes since
> that's the only remaining possible case.

I was also thinking of an ASSERT(), but moving that case:

 >> +	/* Non-RAID56, use their ncopies from btrfs_raid_array. */
 >> +	if (!(map->type & BTRFS_BLOCK_GROUP_RAID56_MASK))
 >> +		return btrfs_raid_array[index].ncopies;

to the end would be enough (without the if obviously).