btrfs: avoid duplicated chunk lookup during btrfs_map_block()

Commit Message

Qu Wenruo June 27, 2023, 6:45 a.m. UTC

[INEFFICIENCY]
Inside btrfs_map_block() we will call btrfs_get_chunk_map() twice in a
row:

  btrfs_map_block()
  |- btrfs_num_copies()
  |  \- btrfs_get_chunk_map()
  \- btrfs_get_chunk_map()

This is duplicated and has no special benefit.

[ENHANCEMENT]
Instead of the duplicated btrfs_get_chunk_map() calls, just calculate
the number of copies using the same extent map.

This would reduce one unnecessary rb tree lookup for the pretty hot
btrfs_map_block().

Also to reduce the duplicated code on calculating the number of mirrors
on RAID56, extract a helper, map_num_copies(), to do the extra RAID56
related checks.

Signed-off-by: Qu Wenruo <[e-mail]>
---
 fs/btrfs/volumes.c | 48 +++++++++++++++++++++++-----------------------
 1 file changed, 24 insertions(+), 24 deletions(-)

Comments

Johannes Thumshirn June 27, 2023, 7:49 a.m. UTC | #1

Looks good,
Reviewed-by: Johannes Thumshirn <[e-mail]>

Christoph Hellwig June 27, 2023, 4:03 p.m. UTC | #2

On Tue, Jun 27, 2023 at 02:45:18PM +0800, Qu Wenruo wrote:
> +	num_copies = map_num_copies(map);
> +
> +	if (mirror_num > num_copies) {

num_copies is only used for this single comparism, so I think we'd be
better off just dropping the variable and writing this as:

	if (mirror_num > map_num_copies(map)) {

> +		free_extent_map(em);
> +		return -EINVAL;

And I'd probably add a free_extent_map label to the end of the
function and jump to that to avoid extra error returns.

Patch

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index a536d0e0e055..ed15c89d4350 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -5758,11 +5758,25 @@  void btrfs_mapping_tree_free(struct extent_map_tree *tree)
 	}
 }
 
+static int map_num_copies(struct map_lookup *map)
+{
+	if (map->type & BTRFS_BLOCK_GROUP_RAID5)
+		return 2;
+	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.
+		 */
+		return map->num_stripes;
+	return btrfs_bg_type_to_factor(map->type);
+}
+
 int btrfs_num_copies(struct btrfs_fs_info *fs_info, u64 logical, u64 len)
 {
 	struct extent_map *em;
-	struct map_lookup *map;
-	enum btrfs_raid_types index;
 	int ret = 1;
 
 	em = btrfs_get_chunk_map(fs_info, logical, len);
@@ -5775,23 +5789,7 @@  int btrfs_num_copies(struct btrfs_fs_info *fs_info, u64 logical, u64 len)
 		 */
 		return 1;
 
-	map = em->map_lookup;
-	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 = map_num_copies(em->map_lookup);
 	free_extent_map(em);
 	return ret;
 }
@@ -6257,15 +6255,17 @@  int btrfs_map_block(struct btrfs_fs_info *fs_info, enum btrfs_map_op op,
 
 	ASSERT(bioc_ret);
 
-	num_copies = btrfs_num_copies(fs_info, logical, fs_info->sectorsize);
-	if (mirror_num > num_copies)
-		return -EINVAL;
-
 	em = btrfs_get_chunk_map(fs_info, logical, *length);
 	if (IS_ERR(em))
 		return PTR_ERR(em);
-
 	map = em->map_lookup;
+	num_copies = map_num_copies(map);
+
+	if (mirror_num > num_copies) {
+		free_extent_map(em);
+		return -EINVAL;
+	}
+
 	data_stripes = nr_data_stripes(map);
 
 	map_offset = logical - em->start;