| Message ID | 20240812165931.9106-1-jth@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: reduce chunk_map lookups in btrfs_map_block | expand |
在 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);
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 > >
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).
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);