Re: [PATCH 1/2] btrfs: introduce btrfs_subpage_bitmap_info

[Nikolay Borisov <nborisov@suse.com>]

On 16.08.21 г. 9:00, Qu Wenruo wrote:
> Currently we use fixed size u16 bitmap for subpage bitmap.
> This is fine for 4K sectorsize with 64K page size.
> 
> But for 4K sectorsize and larger page size, the bitmap is too small,
> while for smaller page size like 16K, u16 bitmaps waste too much space.
> 
> Here we introduce a new helper structure, btrfs_subpage_bitmap_info, to
> record the proper bitmap size, and where each bitmap should start at.
> 
> By this, we can later compact all subpage bitmaps into one u32 bitmap.
> 
> This patch is the first step towards such compact bitmap.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  fs/btrfs/ctree.h   |  1 +
>  fs/btrfs/disk-io.c | 12 ++++++++++--
>  fs/btrfs/subpage.c | 35 +++++++++++++++++++++++++++++++++++
>  fs/btrfs/subpage.h | 28 ++++++++++++++++++++++++++++
>  4 files changed, 74 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> index 4a69aa604ac5..a98fd6a24113 100644
> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -898,6 +898,7 @@ struct btrfs_fs_info {
>  	struct btrfs_workqueue *scrub_workers;
>  	struct btrfs_workqueue *scrub_wr_completion_workers;
>  	struct btrfs_workqueue *scrub_parity_workers;
> +	struct btrfs_subpage_info *subpage_info;
>  
>  	struct btrfs_discard_ctl discard_ctl;
>  
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index 1510a9d92858..f35b875f9e53 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -1644,6 +1644,7 @@ void btrfs_free_fs_info(struct btrfs_fs_info *fs_info)
>  	btrfs_extent_buffer_leak_debug_check(fs_info);
>  	kfree(fs_info->super_copy);
>  	kfree(fs_info->super_for_commit);
> +	kfree(fs_info->subpage_info);
>  	kvfree(fs_info);
>  }
>  
> @@ -3393,11 +3394,13 @@ int __cold open_ctree(struct super_block *sb, struct btrfs_fs_devices *fs_device
>  	}
>  
>  	if (sectorsize != PAGE_SIZE) {
> +		struct btrfs_subpage_info *subpage_info;
> +
> +		ASSERT(sectorsize < PAGE_SIZE);

nit: Simply make the check sectorsize < PAGE_SIZE and that renders the
assert redundant.

> +
>  		btrfs_warn(fs_info,
>  	"read-write for sector size %u with page size %lu is experimental",
>  			   sectorsize, PAGE_SIZE);
> -	}
> -	if (sectorsize != PAGE_SIZE) {
>  		if (btrfs_super_incompat_flags(fs_info->super_copy) &
>  			BTRFS_FEATURE_INCOMPAT_RAID56) {
>  			btrfs_err(fs_info,
> @@ -3406,6 +3409,11 @@ int __cold open_ctree(struct super_block *sb, struct btrfs_fs_devices *fs_device
>  			err = -EINVAL;
>  			goto fail_alloc;
>  		}
> +		subpage_info = kzalloc(sizeof(*subpage_info), GFP_NOFS);
> +		if (!subpage_info)
> +			goto fail_alloc;
> +		btrfs_init_subpage_info(subpage_info, sectorsize);
> +		fs_info->subpage_info = subpage_info;
>  	}
>  
>  	ret = btrfs_init_workqueues(fs_info, fs_devices);
> diff --git a/fs/btrfs/subpage.c b/fs/btrfs/subpage.c
> index a61aa33aeeee..014256d47beb 100644
> --- a/fs/btrfs/subpage.c
> +++ b/fs/btrfs/subpage.c
> @@ -63,6 +63,41 @@
>   *   This means a slightly higher tree locking latency.
>   */
>  
> +void btrfs_init_subpage_info(struct btrfs_subpage_info *subpage_info,
> +			     u32 sectorsize)
> +{
> +	unsigned int cur = 0;
> +	unsigned int nr_bits;
> +
> +	/*
> +	 * Just in case we have super large PAGE_SIZE that unsigned int is not
> +	 * enough to contain the number of sectors for the minimal sectorsize.
> +	 */
> +	BUILD_BUG_ON(UINT_MAX * SZ_4K < PAGE_SIZE);
> +
> +	ASSERT(IS_ALIGNED(PAGE_SIZE, sectorsize));
> +
> +	nr_bits = PAGE_SIZE / sectorsize;
> +	subpage_info->bitmap_nr_bits = nr_bits;
> +
> +	subpage_info->uptodate_start = cur;
> +	cur += nr_bits;
> +
> +	subpage_info->error_start = cur;
> +	cur += nr_bits;
> +
> +	subpage_info->dirty_start = cur;
> +	cur += nr_bits;
> +
> +	subpage_info->writeback_start = cur;
> +	cur += nr_bits;
> +
> +	subpage_info->ordered_start = cur;
> +	cur += nr_bits;
> +
> +	subpage_info->total_nr_bits = cur;

So those values are really consts, however due to them being allocated
on the heap you can't simply define them as const and initialize them.
What a bummer...


On the other hand the namings are a bit generic, those are really
offsets into subpage->bitmaps. As such I'd prefer something along the
lines of
writeback_bitmap_offset
ordered_bitmap_offset etc.

Also I believe a graphical representation is in order i.e


[u][u][u][u][e][e][e][e][e]
^			  ^
|-uptodate_start	  |- error_start etc

Since it's a bit unexpected to have multiple, logically independent
bitmaps be tracked in the same physical location.
> +}
> +
>  int btrfs_attach_subpage(const struct btrfs_fs_info *fs_info,
>  			 struct page *page, enum btrfs_subpage_type type)
>  {
> diff --git a/fs/btrfs/subpage.h b/fs/btrfs/subpage.h
> index 9aa40d795ba9..ea90ba42c97b 100644
> --- a/fs/btrfs/subpage.h
> +++ b/fs/btrfs/subpage.h
> @@ -11,6 +11,32 @@
>   */
>  #define BTRFS_SUBPAGE_BITMAP_SIZE	16
>  
> +/*
> + * Extra info for subpapge bitmap.
> + *
> + * For subpage we integrate all uptodate/error/dirty/writeback/ordered
> + * bitmaps into one larger bitmap.
> + * This structure records the basic info.
> + */
> +struct btrfs_subpage_info {
> +	/* Number of bits for each bitmap*/
> +	unsigned int bitmap_nr_bits;
> +
> +	/* Total number of bits for the whole bitmap */
> +	unsigned int total_nr_bits;
> +
> +	/*
> +	 * *_start indicates where the bitmap starts, the length
> +	 * is always @bitmap_size, which is calculated from
> +	 * PAGE_SIZE / sectorsize.
> +	 */
> +	unsigned int uptodate_start;
> +	unsigned int error_start;
> +	unsigned int dirty_start;
> +	unsigned int writeback_start;
> +	unsigned int ordered_start;
> +};
> +
>  /*
>   * Structure to trace status of each sector inside a page, attached to
>   * page::private for both data and metadata inodes.
> @@ -53,6 +79,8 @@ enum btrfs_subpage_type {
>  	BTRFS_SUBPAGE_DATA,
>  };
>  
> +void btrfs_init_subpage_info(struct btrfs_subpage_info *subpage_info,
> +			     u32 sectorsize);
>  int btrfs_attach_subpage(const struct btrfs_fs_info *fs_info,
>  			 struct page *page, enum btrfs_subpage_type type);
>  void btrfs_detach_subpage(const struct btrfs_fs_info *fs_info,
>