Re: [PATCH 1/2] btrfs: introduce btrfs_subpage_bitmap_info
From: Nikolay Borisov <hidden>
Date: 2021-08-16 09:28:54
On 16.08.21 г. 9:00, Qu Wenruo wrote:
quoted hunk ↗ jump to hunk
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 <redacted> --- 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.
quoted hunk ↗ jump to hunk
+ 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.
quoted hunk ↗ jump to hunk
+} + 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,