Re: [PATCH 7/8] btrfs: use a dedicated data structure for chunk maps

[David Sterba <dsterba@suse.cz>]

On Tue, Nov 21, 2023 at 01:38:38PM +0000, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> Currently we abuse the extent_map structure for two purposes:
> 
> 1) To actually represent extents for inodes;
> 2) To represent chunk mappings.
> 
> This is odd and has several disadvantages:
> 
> 1) To create a chunk map, we need to do two memory allocations: one for
>    an extent_map structure and another one for a map_lookup structure, so
>    more potential for an allocation failure and more complicated code to
>    manage and link two structures;
> 
> 2) For a chunk map we actually only use 3 fields (24 bytes) of the
>    respective extent map structure: the 'start' field to have the logical
>    start address of the chunk, the 'len' field to have the chunk's size,
>    and the 'orig_block_len' field to contain the chunk's stripe size.
> 
>    Besides wasting a memory, it's also odd and not intuitive at all to
>    have the stripe size in a field named 'orig_block_len'.
> 
>    We are also using 'block_len' of the extent_map structure to contain
>    the chunk size, so we have 2 fields for the same value, 'len' and
>    'block_len', which is pointless;
> 
> 3) When an extent map is associated to a chunk mapping, we set the bit
>    EXTENT_FLAG_FS_MAPPING on its flags and then make its member named
>    'map_lookup' point to the associated map_lookup structure. This means
>    that for an extent map associated to an inode extent, we are not using
>    this 'map_lookup' pointer, so wasting 8 bytes (on a 64 bits platform);
> 
> 4) Extent maps associated to a chunk mapping are never merged or split so
>    it's pointless to use the existing extent map infrastructure.
> 
> So add a dedicated data structure named 'btrfs_chunk_map' to represent
> chunk mappings, this is basically the existing map_lookup structure with
> some extra fields:
> 
> 1) 'start' to contain the chunk logical address;
> 2) 'chunk_len' to contain the chunk's length;
> 3) 'stripe_size' for the stripe size;
> 4) 'rb_node' for insertion into a rb tree;
> 5) 'refs' for reference counting.
> 
> This way we do a single memory allocation for chunk mappings and we don't
> waste memory for them with unused/unnecessary fields from an extent_map.
> 
> We also save 8 bytes from the extent_map structure by removing the
> 'map_lookup' pointer, so the size of struct extent_map is reduced from
> 144 bytes down to 136 bytes, and we can now have 30 extents map per 4K
> page instead of 28.
> 
> Signed-off-by: Filipe Manana <fdmanana@suse.com>
> ---
>  fs/btrfs/block-group.c            | 165 ++++-----
>  fs/btrfs/block-group.h            |   6 +-
>  fs/btrfs/dev-replace.c            |  28 +-
>  fs/btrfs/disk-io.c                |   7 +-
>  fs/btrfs/extent_map.c             |  46 ---
>  fs/btrfs/extent_map.h             |   4 -
>  fs/btrfs/fs.h                     |   3 +-
>  fs/btrfs/inode.c                  |  25 +-
>  fs/btrfs/raid56.h                 |   2 +-
>  fs/btrfs/scrub.c                  |  39 +--
>  fs/btrfs/tests/btrfs-tests.c      |   3 +-
>  fs/btrfs/tests/btrfs-tests.h      |   1 +
>  fs/btrfs/tests/extent-map-tests.c |  40 +--
>  fs/btrfs/volumes.c                | 540 ++++++++++++++++++------------
>  fs/btrfs/volumes.h                |  45 ++-
>  fs/btrfs/zoned.c                  |  24 +-

I see a lot of errors when compiling zoned.c, there are still map_lookup
structures. Do you have the zoned mode config option enabled?

  CC [M]  fs/btrfs/zoned.o                                                                                
fs/btrfs/zoned.c:1293:40: warning: ‘struct map_lookup’ declared inside parameter list will not be visible outside of this definition or declaration
 1293 |                                 struct map_lookup *map)                                                                                                                                                      
      |                                        ^~~~~~~~~~              
fs/btrfs/zoned.c: In function ‘btrfs_load_zone_info’:                                                     
fs/btrfs/zoned.c:1296:42: error: invalid use of undefined type ‘struct map_lookup’                                                                                                                                   
 1296 |         struct btrfs_device *device = map->stripes[zone_idx].dev;                 
      |                                          ^~                                                       
fs/btrfs/zoned.c:1302:29: error: invalid use of undefined type ‘struct map_lookup’                        
 1302 |         info->physical = map->stripes[zone_idx].physical;                         
      |                             ^~                                                                                                                                                                               
fs/btrfs/zoned.c: At top level:                                                                           
fs/btrfs/zoned.c:1396:46: warning: ‘struct map_lookup’ declared inside parameter list will not be visible outside of this definition or declaration
 1396 |                                       struct map_lookup *map,                                                                                                                                                
      |                                              ^~~~~~~~~~                            
fs/btrfs/zoned.c: In function ‘btrfs_load_block_group_dup’:           
fs/btrfs/zoned.c:1402:17: error: invalid use of undefined type ‘struct map_lookup’
 1402 |         if ((map->type & BTRFS_BLOCK_GROUP_DATA) && !fs_info->stripe_root) {       
      |                 ^~
...