[PATCH v2] btrfs: reduce chunk_map lookups in btrfs_map_block

[PATCH v2] btrfs: reduce chunk_map lookups in btrfs_map_block

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

Reviewed-by: Qu Wenruo <wqu@suse.com>
Reviewed-by: Filipe Manana <fdmanana@suse.com>
Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.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)
+{
+	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.
 		 */
 		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

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

[Filipe Manana]

On Tue, Aug 13, 2024 at 11:49 AM 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.
>
> Reviewed-by: Qu Wenruo <wqu@suse.com>
> Reviewed-by: Filipe Manana <fdmanana@suse.com>
> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.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
>
>

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

[Johannes Thumshirn]

On 13.08.24 12:57, Filipe Manana wrote:
> On Tue, Aug 13, 2024 at 11:49 AM 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.
>>
>> Reviewed-by: Qu Wenruo <wqu@suse.com>
>> Reviewed-by: Filipe Manana <fdmanana@suse.com>
>> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.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.

No it can't:
fs/btrfs/volumes.c:5784:8: warning: type qualifiers ignored on function 
return type [-Wignored-qualifiers]
  5784 | static const int btrfs_chunk_map_num_copies(struct 
btrfs_chunk_map *map)

Or did you mean the const struct btrfs_chunk_map? That could be done but 
I don't see a reason.

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

Whoops that was fat fingered somehow.

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

[Filipe Manana]

On Tue, Aug 13, 2024 at 12:05 PM Johannes Thumshirn
<Johannes.Thumshirn@wdc.com> wrote:
>
> On 13.08.24 12:57, Filipe Manana wrote:
> > On Tue, Aug 13, 2024 at 11:49 AM 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.
> >>
> >> Reviewed-by: Qu Wenruo <wqu@suse.com>
> >> Reviewed-by: Filipe Manana <fdmanana@suse.com>
> >> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.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.
>
> No it can't:
> fs/btrfs/volumes.c:5784:8: warning: type qualifiers ignored on function
> return type [-Wignored-qualifiers]
>   5784 | static const int btrfs_chunk_map_num_copies(struct
> btrfs_chunk_map *map)
>
> Or did you mean the const struct btrfs_chunk_map? That could be done but
> I don't see a reason.

Yes, I meant const for the argument, not the return type.
It's not needed but it makes it clear for a reader that we don't
intend to change anything. It's just a best practice.

>
> >
> >> +{
> >> +       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.
> >
>
> Whoops that was fat fingered somehow.
>

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

[David Sterba]

On Tue, Aug 13, 2024 at 11:04:52AM +0000, Johannes Thumshirn wrote:
> On 13.08.24 12:57, Filipe Manana wrote:
> > On Tue, Aug 13, 2024 at 11:49 AM 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.
> >>
> >> Reviewed-by: Qu Wenruo <wqu@suse.com>
> >> Reviewed-by: Filipe Manana <fdmanana@suse.com>
> >> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.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.
> 
> No it can't:
> fs/btrfs/volumes.c:5784:8: warning: type qualifiers ignored on function 
> return type [-Wignored-qualifiers]
>   5784 | static const int btrfs_chunk_map_num_copies(struct 
> btrfs_chunk_map *map)
> 
> Or did you mean the const struct btrfs_chunk_map? That could be done but 
> I don't see a reason.

The reason is to improve coding standards. Compiler verifies accidental
changes, internal APIs are cleaner.

It's much easier to add the const when the code written first, otherwise
it's tedious to identify the functions later on. I've been doing
cleanups like that eg. in 2917f74102cf23afe or other patches with
'constify' in the subject.

There are some marginal effects on generated asm code too but it
requires sufficient amount of "constification" so there are no cases
that would otherwise ruin a potential optimization.