[PATCH v2 0/2] btrfs: subpage: pack all subpage bitmaps into a larger bitmap

[PATCH v2 0/2] btrfs: subpage: pack all subpage bitmaps into a larger bitmap

[Qu Wenruo]

Currently we use u16 bitmap to make 4k sectorsize work for 64K page
size.

But this u16 bitmap is not large enough to contain larger page size like
128K, nor is space efficient for 16K page size.

To handle both cases, here we pack all subpage bitmaps into a larger
bitmap, now btrfs_subpage::bitmaps[] will be the ultimate bitmap for
subpage usage.

This is the first step towards more page size support.

Although to really enable extra page size like 16K and 128K, we need to
rework the metadata alignment check.
Which will happen in another patchset.

Changelog:
v2:
- Add two refactor patches to make btrfs_alloc_subpage() more readable
- Fix a break inside two loops bug
- Rename subpage_info::*_start to subpage_info::*_offset
- Add extra comment on what each subpage_info::*_offset works


Qu Wenruo (4):
  btrfs: only call btrfs_alloc_subpage() when sectorsize is smaller than
    PAGE_SIZE
  btrfs: make btrfs_alloc_subpage() to return struct btrfs_subpage *
    directly
  btrfs: introduce btrfs_subpage_bitmap_info
  btrfs: subpage: pack all subpage bitmaps into a larger bitmap

 fs/btrfs/ctree.h     |   1 +
 fs/btrfs/disk-io.c   |  12 ++-
 fs/btrfs/extent_io.c |  76 +++++++++-------
 fs/btrfs/subpage.c   | 205 ++++++++++++++++++++++++++++++-------------
 fs/btrfs/subpage.h   |  54 ++++++++----
 5 files changed, 238 insertions(+), 110 deletions(-)

-- 
2.32.0

[PATCH v2 1/4] btrfs: only call btrfs_alloc_subpage() when sectorsize is smaller than PAGE_SIZE

[Qu Wenruo]

There are two call sites of btrfs_alloc_subpage():

- btrfs_attach_subpage()
  We have ensured sectorsize is smaller than PAGE_SIZE

- alloc_extent_buffer()
  We call btrfs_alloc_subpage() unconditionally.

The alloc_extent_buffer() forces us to check the sectorsize size against
page size inside btrfs_alloc_subpage().

Since the function name, btrfs_alloc_subpage(), already indicates it
should only get called for subpage cases, do the check in
alloc_extent_buffer() and add an ASSERT() in btrfs_alloc_subpage().

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/extent_io.c | 16 +++++++++-------
 fs/btrfs/subpage.c   |  3 +--
 2 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index aaddd7225348..19889dbfcf15 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -6137,13 +6137,15 @@ struct extent_buffer *alloc_extent_buffer(struct btrfs_fs_info *fs_info,
 		 * page, but it may change in the future for 16K page size
 		 * support, so we still preallocate the memory in the loop.
 		 */
-		ret = btrfs_alloc_subpage(fs_info, &prealloc,
-					  BTRFS_SUBPAGE_METADATA);
-		if (ret < 0) {
-			unlock_page(p);
-			put_page(p);
-			exists = ERR_PTR(ret);
-			goto free_eb;
+		if (fs_info->sectorsize < PAGE_SIZE) {
+			ret = btrfs_alloc_subpage(fs_info, &prealloc,
+						  BTRFS_SUBPAGE_METADATA);
+			if (ret < 0) {
+				unlock_page(p);
+				put_page(p);
+				exists = ERR_PTR(ret);
+				goto free_eb;
+			}
 		}
 
 		spin_lock(&mapping->private_lock);
diff --git a/fs/btrfs/subpage.c b/fs/btrfs/subpage.c
index cb10e56ee31e..ff1c6ba34a4d 100644
--- a/fs/btrfs/subpage.c
+++ b/fs/btrfs/subpage.c
@@ -104,8 +104,7 @@ int btrfs_alloc_subpage(const struct btrfs_fs_info *fs_info,
 			struct btrfs_subpage **ret,
 			enum btrfs_subpage_type type)
 {
-	if (fs_info->sectorsize == PAGE_SIZE)
-		return 0;
+	ASSERT(fs_info->sectorsize < PAGE_SIZE);
 
 	*ret = kzalloc(sizeof(struct btrfs_subpage), GFP_NOFS);
 	if (!*ret)
-- 
2.32.0

[PATCH v2 2/4] btrfs: make btrfs_alloc_subpage() to return struct btrfs_subpage * directly

[Qu Wenruo]

The existing calling convention of btrfs_alloc_subpage() is pretty
awful.

Change it to a more common pattern by returning struct btrfs_subpage *
directly and let the caller to determine if the call succeeded.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/extent_io.c |  7 ++++---
 fs/btrfs/subpage.c   | 33 +++++++++++++++++++--------------
 fs/btrfs/subpage.h   |  5 ++---
 3 files changed, 25 insertions(+), 20 deletions(-)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 19889dbfcf15..3c5770d47a95 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -6138,9 +6138,10 @@ struct extent_buffer *alloc_extent_buffer(struct btrfs_fs_info *fs_info,
 		 * support, so we still preallocate the memory in the loop.
 		 */
 		if (fs_info->sectorsize < PAGE_SIZE) {
-			ret = btrfs_alloc_subpage(fs_info, &prealloc,
-						  BTRFS_SUBPAGE_METADATA);
-			if (ret < 0) {
+			prealloc = btrfs_alloc_subpage(fs_info,
+						       BTRFS_SUBPAGE_METADATA);
+			if (IS_ERR(prealloc)) {
+				ret = PTR_ERR(prealloc);
 				unlock_page(p);
 				put_page(p);
 				exists = ERR_PTR(ret);
diff --git a/fs/btrfs/subpage.c b/fs/btrfs/subpage.c
index ff1c6ba34a4d..ae6c68370a95 100644
--- a/fs/btrfs/subpage.c
+++ b/fs/btrfs/subpage.c
@@ -66,7 +66,7 @@
 int btrfs_attach_subpage(const struct btrfs_fs_info *fs_info,
 			 struct page *page, enum btrfs_subpage_type type)
 {
-	struct btrfs_subpage *subpage = NULL;
+	struct btrfs_subpage *subpage;
 	int ret;
 
 	/*
@@ -75,13 +75,16 @@ int btrfs_attach_subpage(const struct btrfs_fs_info *fs_info,
 	 */
 	if (page->mapping)
 		ASSERT(PageLocked(page));
+
 	/* Either not subpage, or the page already has private attached */
 	if (fs_info->sectorsize == PAGE_SIZE || PagePrivate(page))
 		return 0;
 
-	ret = btrfs_alloc_subpage(fs_info, &subpage, type);
-	if (ret < 0)
+	subpage = btrfs_alloc_subpage(fs_info, type);
+	if (IS_ERR(subpage)) {
+		ret = PTR_ERR(subpage);
 		return ret;
+	}
 	attach_page_private(page, subpage);
 	return 0;
 }
@@ -100,23 +103,25 @@ void btrfs_detach_subpage(const struct btrfs_fs_info *fs_info,
 	btrfs_free_subpage(subpage);
 }
 
-int btrfs_alloc_subpage(const struct btrfs_fs_info *fs_info,
-			struct btrfs_subpage **ret,
-			enum btrfs_subpage_type type)
+struct btrfs_subpage *btrfs_alloc_subpage(const struct btrfs_fs_info *fs_info,
+					  enum btrfs_subpage_type type)
 {
+	struct btrfs_subpage *ret;
+
 	ASSERT(fs_info->sectorsize < PAGE_SIZE);
 
-	*ret = kzalloc(sizeof(struct btrfs_subpage), GFP_NOFS);
-	if (!*ret)
-		return -ENOMEM;
-	spin_lock_init(&(*ret)->lock);
+	ret = kzalloc(sizeof(struct btrfs_subpage), GFP_NOFS);
+	if (!ret)
+		return ERR_PTR(-ENOMEM);
+
+	spin_lock_init(&ret->lock);
 	if (type == BTRFS_SUBPAGE_METADATA) {
-		atomic_set(&(*ret)->eb_refs, 0);
+		atomic_set(&ret->eb_refs, 0);
 	} else {
-		atomic_set(&(*ret)->readers, 0);
-		atomic_set(&(*ret)->writers, 0);
+		atomic_set(&ret->readers, 0);
+		atomic_set(&ret->writers, 0);
 	}
-	return 0;
+	return ret;
 }
 
 void btrfs_free_subpage(struct btrfs_subpage *subpage)
diff --git a/fs/btrfs/subpage.h b/fs/btrfs/subpage.h
index 0120948f37a1..fdcadc5193c1 100644
--- a/fs/btrfs/subpage.h
+++ b/fs/btrfs/subpage.h
@@ -59,9 +59,8 @@ void btrfs_detach_subpage(const struct btrfs_fs_info *fs_info,
 			  struct page *page);
 
 /* Allocate additional data where page represents more than one sector */
-int btrfs_alloc_subpage(const struct btrfs_fs_info *fs_info,
-			struct btrfs_subpage **ret,
-			enum btrfs_subpage_type type);
+struct btrfs_subpage *btrfs_alloc_subpage(const struct btrfs_fs_info *fs_info,
+					  enum btrfs_subpage_type type);
 void btrfs_free_subpage(struct btrfs_subpage *subpage);
 
 void btrfs_page_inc_eb_refs(const struct btrfs_fs_info *fs_info,
-- 
2.32.0

[PATCH v2 3/4] btrfs: introduce btrfs_subpage_bitmap_info

[Qu Wenruo]

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 | 36 ++++++++++++++++++++++++++++++++++++
 4 files changed, 81 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index f07c82fafa04..a5297748d719 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -899,6 +899,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 2f9515dccce0..3355708919d0 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);
 }
 
@@ -3392,12 +3393,12 @@ int __cold open_ctree(struct super_block *sb, struct btrfs_fs_devices *fs_device
 		goto fail_alloc;
 	}
 
-	if (sectorsize != PAGE_SIZE) {
+	if (sectorsize < PAGE_SIZE) {
+		struct btrfs_subpage_info *subpage_info;
+
 		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 +3407,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 ae6c68370a95..c4fb2ce52207 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_offset = cur;
+	cur += nr_bits;
+
+	subpage_info->error_offset = cur;
+	cur += nr_bits;
+
+	subpage_info->dirty_offset = cur;
+	cur += nr_bits;
+
+	subpage_info->writeback_offset = cur;
+	cur += nr_bits;
+
+	subpage_info->ordered_offset = cur;
+	cur += nr_bits;
+
+	subpage_info->total_nr_bits = cur;
+}
+
 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 fdcadc5193c1..e4e9cc21a712 100644
--- a/fs/btrfs/subpage.h
+++ b/fs/btrfs/subpage.h
@@ -11,6 +11,40 @@
  */
 #define BTRFS_SUBPAGE_BITMAP_SIZE	16
 
+/*
+ * Extra info for subpapge bitmap.
+ *
+ * For subpage we pack all uptodate/error/dirty/writeback/ordered
+ * bitmaps into one larger bitmap.
+ *
+ * This structure records how they are organized in such bitmap:
+ *
+ * /- uptodate_offset	/- error_offset	/- dirty_offset
+ * |			|		|
+ * v			v		v
+ * |u|u|u|u|........|u|u|e|e|.......|e|e| ...	|o|o|
+ * |<- bitmap_nr_bits ->|
+ * |<--------------- total_nr_bits ---------------->|
+ */
+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_offset;
+	unsigned int error_offset;
+	unsigned int dirty_offset;
+	unsigned int writeback_offset;
+	unsigned int ordered_offset;
+};
+
 /*
  * Structure to trace status of each sector inside a page, attached to
  * page::private for both data and metadata inodes.
@@ -53,6 +87,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,
-- 
2.32.0

Re: [PATCH v2 3/4] btrfs: introduce btrfs_subpage_bitmap_info

[Nikolay Borisov]

On 17.08.21 г. 12:38, 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 | 36 ++++++++++++++++++++++++++++++++++++
>  4 files changed, 81 insertions(+), 3 deletions(-)
> 

<snip>

> +/*
> + * Extra info for subpapge bitmap.
> + *
> + * For subpage we pack all uptodate/error/dirty/writeback/ordered
> + * bitmaps into one larger bitmap.
> + *
> + * This structure records how they are organized in such bitmap:
> + *
> + * /- uptodate_offset	/- error_offset	/- dirty_offset
> + * |			|		|
> + * v			v		v
> + * |u|u|u|u|........|u|u|e|e|.......|e|e| ...	|o|o|

nit: the 'e' that the dirty offset is pointing to should be a 'd', I'm
sure David can fix this while merging.

<snip>

Re: [PATCH v2 3/4] btrfs: introduce btrfs_subpage_bitmap_info

[David Sterba]

On Tue, Aug 17, 2021 at 01:11:43PM +0300, Nikolay Borisov wrote:
> > +/*
> > + * Extra info for subpapge bitmap.
> > + *
> > + * For subpage we pack all uptodate/error/dirty/writeback/ordered
> > + * bitmaps into one larger bitmap.
> > + *
> > + * This structure records how they are organized in such bitmap:
> > + *
> > + * /- uptodate_offset	/- error_offset	/- dirty_offset
> > + * |			|		|
> > + * v			v		v
> > + * |u|u|u|u|........|u|u|e|e|.......|e|e| ...	|o|o|
> 
> nit: the 'e' that the dirty offset is pointing to should be a 'd', I'm
> sure David can fix this while merging.

I don't see any 'e' under the dirty offset arrow, there's just 'o' and
the arrow points to end of |e|e| and continues with ...

Re: [PATCH v2 3/4] btrfs: introduce btrfs_subpage_bitmap_info

[Nikolay Borisov]

On 23.08.21 г. 19:45, David Sterba wrote:
> On Tue, Aug 17, 2021 at 01:11:43PM +0300, Nikolay Borisov wrote:
>>> +/*
>>> + * Extra info for subpapge bitmap.
>>> + *
>>> + * For subpage we pack all uptodate/error/dirty/writeback/ordered
>>> + * bitmaps into one larger bitmap.
>>> + *
>>> + * This structure records how they are organized in such bitmap:
>>> + *
>>> + * /- uptodate_offset	/- error_offset	/- dirty_offset
>>> + * |			|		|
>>> + * v			v		v
>>> + * |u|u|u|u|........|u|u|e|e|.......|e|e| ...	|o|o|
>>
>> nit: the 'e' that the dirty offset is pointing to should be a 'd', I'm
>> sure David can fix this while merging.
> 
> I don't see any 'e' under the dirty offset arrow, there's just 'o' and
> the arrow points to end of |e|e| and continues with ...
> 

What I wanted to say is that every arrow beneat the respective type -
uptodate/error/dirty should point to a letter corresponding to the first
letter of the respective word. I.e the dirty offset points into ...
whilst the |o|o| at the end likely mean "other" but is confusing i.e
dirty offset should be pointing to a |d| box.

Re: [PATCH v2 3/4] btrfs: introduce btrfs_subpage_bitmap_info

[David Sterba]

On Tue, Aug 17, 2021 at 05:38:51PM +0800, 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 | 36 ++++++++++++++++++++++++++++++++++++
>  4 files changed, 81 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> index f07c82fafa04..a5297748d719 100644
> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -899,6 +899,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 2f9515dccce0..3355708919d0 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);
>  }
>  
> @@ -3392,12 +3393,12 @@ int __cold open_ctree(struct super_block *sb, struct btrfs_fs_devices *fs_device
>  		goto fail_alloc;
>  	}
>  
> -	if (sectorsize != PAGE_SIZE) {
> +	if (sectorsize < PAGE_SIZE) {
> +		struct btrfs_subpage_info *subpage_info;
> +
>  		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 +3407,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 ae6c68370a95..c4fb2ce52207 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);

Do you seriously expect such hardware to exist? We know there are arches
with 256K page and that's perhaps the maximum we should care about now
but UINT_MAX * 4K is 16TiB, 2^44. CPUs are barely capable of addressing
2^40 physical address space, making assertions about page size orders of
magnitude larger than that is insane.

Re: [PATCH v2 3/4] btrfs: introduce btrfs_subpage_bitmap_info

[Qu Wenruo]

On 2021/8/24 上午12:41, David Sterba wrote:
> On Tue, Aug 17, 2021 at 05:38:51PM +0800, 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 | 36 ++++++++++++++++++++++++++++++++++++
>>   4 files changed, 81 insertions(+), 3 deletions(-)
>>
>> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
>> index f07c82fafa04..a5297748d719 100644
>> --- a/fs/btrfs/ctree.h
>> +++ b/fs/btrfs/ctree.h
>> @@ -899,6 +899,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 2f9515dccce0..3355708919d0 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);
>>   }
>>
>> @@ -3392,12 +3393,12 @@ int __cold open_ctree(struct super_block *sb, struct btrfs_fs_devices *fs_device
>>   		goto fail_alloc;
>>   	}
>>
>> -	if (sectorsize != PAGE_SIZE) {
>> +	if (sectorsize < PAGE_SIZE) {
>> +		struct btrfs_subpage_info *subpage_info;
>> +
>>   		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 +3407,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 ae6c68370a95..c4fb2ce52207 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);
>
> Do you seriously expect such hardware to exist? We know there are arches
> with 256K page and that's perhaps the maximum we should care about now
> but UINT_MAX * 4K is 16TiB, 2^44. CPUs are barely capable of addressing
> 2^40 physical address space, making assertions about page size orders of
> magnitude larger than that is insane.
>
My original idea is to use U8 for those members to save some bytes, for
that case, the BUILD_BUG_ON() would make sense.

But finally I moved to use unsigned int, now the BUILD_BUG_ON() makes no
sense and can be removed.

Thanks,
Qu

[PATCH v2 4/4] btrfs: subpage: pack all subpage bitmaps into a larger bitmap

[Qu Wenruo]

Currently we use u16 bitmap to make 4k sectorsize work for 64K page
size.

But this u16 bitmap is not large enough to contain larger page size like
128K, nor is space efficient for 16K page size.

To handle both cases, here we pack all subpage bitmaps into a larger
bitmap, now btrfs_subpage::bitmaps[] will be the ultimate bitmap for
subpage usage.

Each sub-bitmap will has its start bit number recorded in
btrfs_subpage_info::*_start, and its bitmap length will be recorded in
btrfs_subpage_info::bitmap_nr_bits.

All subpage bitmap operations will be converted from using direct u16
operations to bitmap operations, with above *_start calculated.

For 64K page size with 4K sectorsize, this should not cause much
difference.

While for 16K page size, we will only need 1 unsigned long (u32) to
store all the bitmaps, which saves quite some space.

Furthermore, this allows us to support larger page size like 128K and
258K.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/extent_io.c |  59 +++++++++++--------
 fs/btrfs/subpage.c   | 136 +++++++++++++++++++++++++++++--------------
 fs/btrfs/subpage.h   |  19 +-----
 3 files changed, 129 insertions(+), 85 deletions(-)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 3c5770d47a95..fcb25ff86ea9 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -3856,10 +3856,9 @@ static void find_next_dirty_byte(struct btrfs_fs_info *fs_info,
 	struct btrfs_subpage *subpage = (struct btrfs_subpage *)page->private;
 	u64 orig_start = *start;
 	/* Declare as unsigned long so we can use bitmap ops */
-	unsigned long dirty_bitmap;
 	unsigned long flags;
-	int nbits = (orig_start - page_offset(page)) >> fs_info->sectorsize_bits;
-	int range_start_bit = nbits;
+	int range_start_bit = fs_info->subpage_info->dirty_offset +
+		(offset_in_page(orig_start) >> fs_info->sectorsize_bits);
 	int range_end_bit;
 
 	/*
@@ -3874,11 +3873,14 @@ static void find_next_dirty_byte(struct btrfs_fs_info *fs_info,
 
 	/* We should have the page locked, but just in case */
 	spin_lock_irqsave(&subpage->lock, flags);
-	dirty_bitmap = subpage->dirty_bitmap;
+	bitmap_next_set_region(subpage->bitmaps, &range_start_bit, &range_end_bit,
+			       fs_info->subpage_info->dirty_offset +
+			       fs_info->subpage_info->bitmap_nr_bits);
 	spin_unlock_irqrestore(&subpage->lock, flags);
 
-	bitmap_next_set_region(&dirty_bitmap, &range_start_bit, &range_end_bit,
-			       BTRFS_SUBPAGE_BITMAP_SIZE);
+	range_start_bit -= fs_info->subpage_info->dirty_offset;
+	range_end_bit -= fs_info->subpage_info->dirty_offset;
+
 	*start = page_offset(page) + range_start_bit * fs_info->sectorsize;
 	*end = page_offset(page) + range_end_bit * fs_info->sectorsize;
 }
@@ -4602,12 +4604,11 @@ static int submit_eb_subpage(struct page *page,
 	int submitted = 0;
 	u64 page_start = page_offset(page);
 	int bit_start = 0;
-	const int nbits = BTRFS_SUBPAGE_BITMAP_SIZE;
 	int sectors_per_node = fs_info->nodesize >> fs_info->sectorsize_bits;
 	int ret;
 
 	/* Lock and write each dirty extent buffers in the range */
-	while (bit_start < nbits) {
+	while (bit_start < fs_info->subpage_info->bitmap_nr_bits) {
 		struct btrfs_subpage *subpage = (struct btrfs_subpage *)page->private;
 		struct extent_buffer *eb;
 		unsigned long flags;
@@ -4623,7 +4624,8 @@ static int submit_eb_subpage(struct page *page,
 			break;
 		}
 		spin_lock_irqsave(&subpage->lock, flags);
-		if (!((1 << bit_start) & subpage->dirty_bitmap)) {
+		if (!test_bit(bit_start + fs_info->subpage_info->dirty_offset,
+			      subpage->bitmaps)) {
 			spin_unlock_irqrestore(&subpage->lock, flags);
 			spin_unlock(&page->mapping->private_lock);
 			bit_start++;
@@ -7170,32 +7172,41 @@ void memmove_extent_buffer(const struct extent_buffer *dst,
 	}
 }
 
+#define GANG_LOOKUP_SIZE	16
 static struct extent_buffer *get_next_extent_buffer(
 		struct btrfs_fs_info *fs_info, struct page *page, u64 bytenr)
 {
-	struct extent_buffer *gang[BTRFS_SUBPAGE_BITMAP_SIZE];
+	struct extent_buffer *gang[GANG_LOOKUP_SIZE];
 	struct extent_buffer *found = NULL;
 	u64 page_start = page_offset(page);
-	int ret;
-	int i;
+	u64 cur = page_start;
 
 	ASSERT(in_range(bytenr, page_start, PAGE_SIZE));
-	ASSERT(PAGE_SIZE / fs_info->nodesize <= BTRFS_SUBPAGE_BITMAP_SIZE);
 	lockdep_assert_held(&fs_info->buffer_lock);
 
-	ret = radix_tree_gang_lookup(&fs_info->buffer_radix, (void **)gang,
-			bytenr >> fs_info->sectorsize_bits,
-			PAGE_SIZE / fs_info->nodesize);
-	for (i = 0; i < ret; i++) {
-		/* Already beyond page end */
-		if (gang[i]->start >= page_start + PAGE_SIZE)
-			break;
-		/* Found one */
-		if (gang[i]->start >= bytenr) {
-			found = gang[i];
-			break;
+	while (cur < page_start + PAGE_SIZE) {
+		int ret;
+		int i;
+
+		ret = radix_tree_gang_lookup(&fs_info->buffer_radix,
+				(void **)gang, cur >> fs_info->sectorsize_bits,
+				min_t(unsigned int, GANG_LOOKUP_SIZE,
+				      PAGE_SIZE / fs_info->nodesize));
+		if (ret == 0)
+			goto out;
+		for (i = 0; i < ret; i++) {
+			/* Already beyond page end */
+			if (gang[i]->start >= page_start + PAGE_SIZE)
+				goto out;
+			/* Found one */
+			if (gang[i]->start >= bytenr) {
+				found = gang[i];
+				goto out;
+			}
 		}
+		cur = gang[ret - 1]->start + gang[ret - 1]->len;
 	}
+out:
 	return found;
 }
 
diff --git a/fs/btrfs/subpage.c b/fs/btrfs/subpage.c
index c4fb2ce52207..578095c52a0f 100644
--- a/fs/btrfs/subpage.c
+++ b/fs/btrfs/subpage.c
@@ -142,10 +142,13 @@ struct btrfs_subpage *btrfs_alloc_subpage(const struct btrfs_fs_info *fs_info,
 					  enum btrfs_subpage_type type)
 {
 	struct btrfs_subpage *ret;
+	unsigned int real_size;
 
 	ASSERT(fs_info->sectorsize < PAGE_SIZE);
 
-	ret = kzalloc(sizeof(struct btrfs_subpage), GFP_NOFS);
+	real_size = struct_size(ret, bitmaps,
+			BITS_TO_LONGS(fs_info->subpage_info->total_nr_bits));
+	ret = kzalloc(real_size, GFP_NOFS);
 	if (!ret)
 		return ERR_PTR(-ENOMEM);
 
@@ -328,37 +331,60 @@ void btrfs_page_end_writer_lock(const struct btrfs_fs_info *fs_info,
 		unlock_page(page);
 }
 
-/*
- * Convert the [start, start + len) range into a u16 bitmap
- *
- * For example: if start == page_offset() + 16K, len = 16K, we get 0x00f0.
- */
-static u16 btrfs_subpage_calc_bitmap(const struct btrfs_fs_info *fs_info,
-		struct page *page, u64 start, u32 len)
+static bool bitmap_test_range_all_set(unsigned long *addr, unsigned int start,
+				      unsigned int nbits)
 {
-	const int bit_start = offset_in_page(start) >> fs_info->sectorsize_bits;
-	const int nbits = len >> fs_info->sectorsize_bits;
+	unsigned int found_zero;
 
-	btrfs_subpage_assert(fs_info, page, start, len);
+	found_zero = find_next_zero_bit(addr, start + nbits, start);
+	if (found_zero == start + nbits)
+		return true;
+	return false;
+}
 
-	/*
-	 * Here nbits can be 16, thus can go beyond u16 range. We make the
-	 * first left shift to be calculate in unsigned long (at least u32),
-	 * then truncate the result to u16.
-	 */
-	return (u16)(((1UL << nbits) - 1) << bit_start);
+static bool bitmap_test_range_all_zero(unsigned long *addr, unsigned int start,
+				       unsigned int nbits)
+{
+	unsigned int found_set;
+
+	found_set = find_next_bit(addr, start + nbits, start);
+	if (found_set == start + nbits)
+		return true;
+	return false;
 }
 
+#define subpage_calc_start_bit(fs_info, page, name, start, len)		\
+({									\
+	unsigned int start_bit;						\
+									\
+	btrfs_subpage_assert(fs_info, page, start, len);		\
+	start_bit = offset_in_page(start) >> fs_info->sectorsize_bits;	\
+	start_bit += fs_info->subpage_info->name##_offset;		\
+	start_bit;							\
+})
+
+#define subpage_test_bitmap_all_set(fs_info, subpage, name)		\
+	bitmap_test_range_all_set(subpage->bitmaps,			\
+			fs_info->subpage_info->name##_offset,		\
+			fs_info->subpage_info->bitmap_nr_bits)
+
+#define subpage_test_bitmap_all_zero(fs_info, subpage, name)		\
+	bitmap_test_range_all_zero(subpage->bitmaps,			\
+			fs_info->subpage_info->name##_offset,		\
+			fs_info->subpage_info->bitmap_nr_bits)
+
 void btrfs_subpage_set_uptodate(const struct btrfs_fs_info *fs_info,
 		struct page *page, u64 start, u32 len)
 {
 	struct btrfs_subpage *subpage = (struct btrfs_subpage *)page->private;
-	const u16 tmp = btrfs_subpage_calc_bitmap(fs_info, page, start, len);
+	unsigned int start_bit = subpage_calc_start_bit(fs_info, page,
+							uptodate, start, len);
 	unsigned long flags;
 
 	spin_lock_irqsave(&subpage->lock, flags);
-	subpage->uptodate_bitmap |= tmp;
-	if (subpage->uptodate_bitmap == U16_MAX)
+	bitmap_set(subpage->bitmaps, start_bit,
+		   len >> fs_info->sectorsize_bits);
+	if (subpage_test_bitmap_all_set(fs_info, subpage, uptodate))
 		SetPageUptodate(page);
 	spin_unlock_irqrestore(&subpage->lock, flags);
 }
@@ -367,11 +393,13 @@ void btrfs_subpage_clear_uptodate(const struct btrfs_fs_info *fs_info,
 		struct page *page, u64 start, u32 len)
 {
 	struct btrfs_subpage *subpage = (struct btrfs_subpage *)page->private;
-	const u16 tmp = btrfs_subpage_calc_bitmap(fs_info, page, start, len);
+	unsigned int start_bit = subpage_calc_start_bit(fs_info, page,
+							uptodate, start, len);
 	unsigned long flags;
 
 	spin_lock_irqsave(&subpage->lock, flags);
-	subpage->uptodate_bitmap &= ~tmp;
+	bitmap_clear(subpage->bitmaps, start_bit,
+		     len >> fs_info->sectorsize_bits);
 	ClearPageUptodate(page);
 	spin_unlock_irqrestore(&subpage->lock, flags);
 }
@@ -380,11 +408,13 @@ void btrfs_subpage_set_error(const struct btrfs_fs_info *fs_info,
 		struct page *page, u64 start, u32 len)
 {
 	struct btrfs_subpage *subpage = (struct btrfs_subpage *)page->private;
-	const u16 tmp = btrfs_subpage_calc_bitmap(fs_info, page, start, len);
+	unsigned int start_bit = subpage_calc_start_bit(fs_info, page,
+							error, start, len);
 	unsigned long flags;
 
 	spin_lock_irqsave(&subpage->lock, flags);
-	subpage->error_bitmap |= tmp;
+	bitmap_set(subpage->bitmaps, start_bit,
+		   len >> fs_info->sectorsize_bits);
 	SetPageError(page);
 	spin_unlock_irqrestore(&subpage->lock, flags);
 }
@@ -393,12 +423,14 @@ void btrfs_subpage_clear_error(const struct btrfs_fs_info *fs_info,
 		struct page *page, u64 start, u32 len)
 {
 	struct btrfs_subpage *subpage = (struct btrfs_subpage *)page->private;
-	const u16 tmp = btrfs_subpage_calc_bitmap(fs_info, page, start, len);
+	unsigned int start_bit = subpage_calc_start_bit(fs_info, page,
+							error, start, len);
 	unsigned long flags;
 
 	spin_lock_irqsave(&subpage->lock, flags);
-	subpage->error_bitmap &= ~tmp;
-	if (subpage->error_bitmap == 0)
+	bitmap_clear(subpage->bitmaps, start_bit,
+		     len >> fs_info->sectorsize_bits);
+	if (subpage_test_bitmap_all_zero(fs_info, subpage, error))
 		ClearPageError(page);
 	spin_unlock_irqrestore(&subpage->lock, flags);
 }
@@ -407,11 +439,13 @@ void btrfs_subpage_set_dirty(const struct btrfs_fs_info *fs_info,
 		struct page *page, u64 start, u32 len)
 {
 	struct btrfs_subpage *subpage = (struct btrfs_subpage *)page->private;
-	u16 tmp = btrfs_subpage_calc_bitmap(fs_info, page, start, len);
+	unsigned int start_bit = subpage_calc_start_bit(fs_info, page,
+							dirty, start, len);
 	unsigned long flags;
 
 	spin_lock_irqsave(&subpage->lock, flags);
-	subpage->dirty_bitmap |= tmp;
+	bitmap_set(subpage->bitmaps, start_bit,
+		   len >> fs_info->sectorsize_bits);
 	spin_unlock_irqrestore(&subpage->lock, flags);
 	set_page_dirty(page);
 }
@@ -430,13 +464,15 @@ bool btrfs_subpage_clear_and_test_dirty(const struct btrfs_fs_info *fs_info,
 		struct page *page, u64 start, u32 len)
 {
 	struct btrfs_subpage *subpage = (struct btrfs_subpage *)page->private;
-	u16 tmp = btrfs_subpage_calc_bitmap(fs_info, page, start, len);
+	unsigned int start_bit = subpage_calc_start_bit(fs_info, page,
+							dirty, start, len);
 	unsigned long flags;
 	bool last = false;
 
 	spin_lock_irqsave(&subpage->lock, flags);
-	subpage->dirty_bitmap &= ~tmp;
-	if (subpage->dirty_bitmap == 0)
+	bitmap_clear(subpage->bitmaps, start_bit,
+		     len >> fs_info->sectorsize_bits);
+	if (subpage_test_bitmap_all_zero(fs_info, subpage, dirty))
 		last = true;
 	spin_unlock_irqrestore(&subpage->lock, flags);
 	return last;
@@ -456,11 +492,13 @@ void btrfs_subpage_set_writeback(const struct btrfs_fs_info *fs_info,
 		struct page *page, u64 start, u32 len)
 {
 	struct btrfs_subpage *subpage = (struct btrfs_subpage *)page->private;
-	u16 tmp = btrfs_subpage_calc_bitmap(fs_info, page, start, len);
+	unsigned int start_bit = subpage_calc_start_bit(fs_info, page,
+							writeback, start, len);
 	unsigned long flags;
 
 	spin_lock_irqsave(&subpage->lock, flags);
-	subpage->writeback_bitmap |= tmp;
+	bitmap_set(subpage->bitmaps, start_bit,
+		   len >> fs_info->sectorsize_bits);
 	set_page_writeback(page);
 	spin_unlock_irqrestore(&subpage->lock, flags);
 }
@@ -469,12 +507,14 @@ void btrfs_subpage_clear_writeback(const struct btrfs_fs_info *fs_info,
 		struct page *page, u64 start, u32 len)
 {
 	struct btrfs_subpage *subpage = (struct btrfs_subpage *)page->private;
-	u16 tmp = btrfs_subpage_calc_bitmap(fs_info, page, start, len);
+	unsigned int start_bit = subpage_calc_start_bit(fs_info, page,
+							writeback, start, len);
 	unsigned long flags;
 
 	spin_lock_irqsave(&subpage->lock, flags);
-	subpage->writeback_bitmap &= ~tmp;
-	if (subpage->writeback_bitmap == 0) {
+	bitmap_clear(subpage->bitmaps, start_bit,
+		     len >> fs_info->sectorsize_bits);
+	if (subpage_test_bitmap_all_zero(fs_info, subpage, writeback)) {
 		ASSERT(PageWriteback(page));
 		end_page_writeback(page);
 	}
@@ -485,11 +525,13 @@ void btrfs_subpage_set_ordered(const struct btrfs_fs_info *fs_info,
 		struct page *page, u64 start, u32 len)
 {
 	struct btrfs_subpage *subpage = (struct btrfs_subpage *)page->private;
-	const u16 tmp = btrfs_subpage_calc_bitmap(fs_info, page, start, len);
+	unsigned int start_bit = subpage_calc_start_bit(fs_info, page,
+							ordered, start, len);
 	unsigned long flags;
 
 	spin_lock_irqsave(&subpage->lock, flags);
-	subpage->ordered_bitmap |= tmp;
+	bitmap_set(subpage->bitmaps, start_bit,
+		   len >> fs_info->sectorsize_bits);
 	SetPageOrdered(page);
 	spin_unlock_irqrestore(&subpage->lock, flags);
 }
@@ -498,12 +540,14 @@ void btrfs_subpage_clear_ordered(const struct btrfs_fs_info *fs_info,
 		struct page *page, u64 start, u32 len)
 {
 	struct btrfs_subpage *subpage = (struct btrfs_subpage *)page->private;
-	const u16 tmp = btrfs_subpage_calc_bitmap(fs_info, page, start, len);
+	unsigned int start_bit = subpage_calc_start_bit(fs_info, page,
+							ordered, start, len);
 	unsigned long flags;
 
 	spin_lock_irqsave(&subpage->lock, flags);
-	subpage->ordered_bitmap &= ~tmp;
-	if (subpage->ordered_bitmap == 0)
+	bitmap_clear(subpage->bitmaps, start_bit,
+		     len >> fs_info->sectorsize_bits);
+	if (subpage_test_bitmap_all_zero(fs_info, subpage, ordered))
 		ClearPageOrdered(page);
 	spin_unlock_irqrestore(&subpage->lock, flags);
 }
@@ -516,12 +560,14 @@ bool btrfs_subpage_test_##name(const struct btrfs_fs_info *fs_info,	\
 		struct page *page, u64 start, u32 len)			\
 {									\
 	struct btrfs_subpage *subpage = (struct btrfs_subpage *)page->private; \
-	const u16 tmp = btrfs_subpage_calc_bitmap(fs_info, page, start, len); \
+	unsigned int start_bit = subpage_calc_start_bit(fs_info, page,	\
+						name, start, len);	\
 	unsigned long flags;						\
 	bool ret;							\
 									\
 	spin_lock_irqsave(&subpage->lock, flags);			\
-	ret = ((subpage->name##_bitmap & tmp) == tmp);			\
+	ret = bitmap_test_range_all_set(subpage->bitmaps, start_bit,	\
+				len >> fs_info->sectorsize_bits);	\
 	spin_unlock_irqrestore(&subpage->lock, flags);			\
 	return ret;							\
 }
@@ -618,5 +664,5 @@ void btrfs_page_assert_not_dirty(const struct btrfs_fs_info *fs_info,
 		return;
 
 	ASSERT(PagePrivate(page) && page->private);
-	ASSERT(subpage->dirty_bitmap == 0);
+	ASSERT(subpage_test_bitmap_all_zero(fs_info, subpage, dirty));
 }
diff --git a/fs/btrfs/subpage.h b/fs/btrfs/subpage.h
index e4e9cc21a712..3eb4088604b1 100644
--- a/fs/btrfs/subpage.h
+++ b/fs/btrfs/subpage.h
@@ -5,12 +5,6 @@
 
 #include <linux/spinlock.h>
 
-/*
- * Maximum page size we support is 64K, minimum sector size is 4K, u16 bitmap
- * is sufficient. Regular bitmap_* is not used due to size reasons.
- */
-#define BTRFS_SUBPAGE_BITMAP_SIZE	16
-
 /*
  * Extra info for subpapge bitmap.
  *
@@ -52,10 +46,6 @@ struct btrfs_subpage_info {
 struct btrfs_subpage {
 	/* Common members for both data and metadata pages */
 	spinlock_t lock;
-	u16 uptodate_bitmap;
-	u16 error_bitmap;
-	u16 dirty_bitmap;
-	u16 writeback_bitmap;
 	/*
 	 * Both data and metadata needs to track how many readers are for the
 	 * page.
@@ -72,14 +62,11 @@ struct btrfs_subpage {
 		 * manages whether the subpage can be detached.
 		 */
 		atomic_t eb_refs;
-		/* Structures only used by data */
-		struct {
-			atomic_t writers;
 
-			/* Tracke pending ordered extent in this sector */
-			u16 ordered_bitmap;
-		};
+		/* Structures only used by data */
+		atomic_t writers;
 	};
+	unsigned long bitmaps[];
 };
 
 enum btrfs_subpage_type {
-- 
2.32.0

Re: [PATCH v2 4/4] btrfs: subpage: pack all subpage bitmaps into a larger bitmap

[Nikolay Borisov]

On 17.08.21 г. 12:38, Qu Wenruo wrote:
> Currently we use u16 bitmap to make 4k sectorsize work for 64K page
> size.
> 
> But this u16 bitmap is not large enough to contain larger page size like
> 128K, nor is space efficient for 16K page size.
> 
> To handle both cases, here we pack all subpage bitmaps into a larger
> bitmap, now btrfs_subpage::bitmaps[] will be the ultimate bitmap for
> subpage usage.
> 
> Each sub-bitmap will has its start bit number recorded in
> btrfs_subpage_info::*_start, and its bitmap length will be recorded in
> btrfs_subpage_info::bitmap_nr_bits.
> 
> All subpage bitmap operations will be converted from using direct u16
> operations to bitmap operations, with above *_start calculated.
> 
> For 64K page size with 4K sectorsize, this should not cause much
> difference.
> 
> While for 16K page size, we will only need 1 unsigned long (u32) to
> store all the bitmaps, which saves quite some space.
> 
> Furthermore, this allows us to support larger page size like 128K and
> 258K.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>

<snip>

> +#define GANG_LOOKUP_SIZE	16
>  static struct extent_buffer *get_next_extent_buffer(
>  		struct btrfs_fs_info *fs_info, struct page *page, u64 bytenr)
>  {
> -	struct extent_buffer *gang[BTRFS_SUBPAGE_BITMAP_SIZE];
> +	struct extent_buffer *gang[GANG_LOOKUP_SIZE];
>  	struct extent_buffer *found = NULL;
>  	u64 page_start = page_offset(page);
> -	int ret;
> -	int i;
> +	u64 cur = page_start;
>  
>  	ASSERT(in_range(bytenr, page_start, PAGE_SIZE));
> -	ASSERT(PAGE_SIZE / fs_info->nodesize <= BTRFS_SUBPAGE_BITMAP_SIZE);
>  	lockdep_assert_held(&fs_info->buffer_lock);
>  
> -	ret = radix_tree_gang_lookup(&fs_info->buffer_radix, (void **)gang,
> -			bytenr >> fs_info->sectorsize_bits,
> -			PAGE_SIZE / fs_info->nodesize);
> -	for (i = 0; i < ret; i++) {
> -		/* Already beyond page end */
> -		if (gang[i]->start >= page_start + PAGE_SIZE)
> -			break;
> -		/* Found one */
> -		if (gang[i]->start >= bytenr) {
> -			found = gang[i];
> -			break;
> +	while (cur < page_start + PAGE_SIZE) {
> +		int ret;
> +		int i;
> +
> +		ret = radix_tree_gang_lookup(&fs_info->buffer_radix,
> +				(void **)gang, cur >> fs_info->sectorsize_bits,
> +				min_t(unsigned int, GANG_LOOKUP_SIZE,
> +				      PAGE_SIZE / fs_info->nodesize));
> +		if (ret == 0)
> +			goto out;
> +		for (i = 0; i < ret; i++) {
> +			/* Already beyond page end */
> +			if (gang[i]->start >= page_start + PAGE_SIZE)

nit: this could be rewritten as (in_range(gang[i]->start, page_start,
PAGE_SIZE)

> +				goto out;
> +			/* Found one */
> +			if (gang[i]->start >= bytenr) {
> +				found = gang[i];
> +				goto out;
> +			}
>  		}
> +		cur = gang[ret - 1]->start + gang[ret - 1]->len;
>  	}
> +out:
>  	return found;
>  }
>  

<snip>

Re: [PATCH v2 4/4] btrfs: subpage: pack all subpage bitmaps into a larger bitmap

[David Sterba]

On Tue, Aug 17, 2021 at 04:43:56PM +0300, Nikolay Borisov wrote:
> 
> 
> On 17.08.21 г. 12:38, Qu Wenruo wrote:
> > Currently we use u16 bitmap to make 4k sectorsize work for 64K page
> > size.
> > 
> > But this u16 bitmap is not large enough to contain larger page size like
> > 128K, nor is space efficient for 16K page size.
> > 
> > To handle both cases, here we pack all subpage bitmaps into a larger
> > bitmap, now btrfs_subpage::bitmaps[] will be the ultimate bitmap for
> > subpage usage.
> > 
> > Each sub-bitmap will has its start bit number recorded in
> > btrfs_subpage_info::*_start, and its bitmap length will be recorded in
> > btrfs_subpage_info::bitmap_nr_bits.
> > 
> > All subpage bitmap operations will be converted from using direct u16
> > operations to bitmap operations, with above *_start calculated.
> > 
> > For 64K page size with 4K sectorsize, this should not cause much
> > difference.
> > 
> > While for 16K page size, we will only need 1 unsigned long (u32) to
> > store all the bitmaps, which saves quite some space.
> > 
> > Furthermore, this allows us to support larger page size like 128K and
> > 258K.
> > 
> > Signed-off-by: Qu Wenruo <wqu@suse.com>
> 
> <snip>
> 
> > +#define GANG_LOOKUP_SIZE	16
> >  static struct extent_buffer *get_next_extent_buffer(
> >  		struct btrfs_fs_info *fs_info, struct page *page, u64 bytenr)
> >  {
> > -	struct extent_buffer *gang[BTRFS_SUBPAGE_BITMAP_SIZE];
> > +	struct extent_buffer *gang[GANG_LOOKUP_SIZE];
> >  	struct extent_buffer *found = NULL;
> >  	u64 page_start = page_offset(page);
> > -	int ret;
> > -	int i;
> > +	u64 cur = page_start;
> >  
> >  	ASSERT(in_range(bytenr, page_start, PAGE_SIZE));
> > -	ASSERT(PAGE_SIZE / fs_info->nodesize <= BTRFS_SUBPAGE_BITMAP_SIZE);
> >  	lockdep_assert_held(&fs_info->buffer_lock);
> >  
> > -	ret = radix_tree_gang_lookup(&fs_info->buffer_radix, (void **)gang,
> > -			bytenr >> fs_info->sectorsize_bits,
> > -			PAGE_SIZE / fs_info->nodesize);
> > -	for (i = 0; i < ret; i++) {
> > -		/* Already beyond page end */
> > -		if (gang[i]->start >= page_start + PAGE_SIZE)
> > -			break;
> > -		/* Found one */
> > -		if (gang[i]->start >= bytenr) {
> > -			found = gang[i];
> > -			break;
> > +	while (cur < page_start + PAGE_SIZE) {
> > +		int ret;
> > +		int i;
> > +
> > +		ret = radix_tree_gang_lookup(&fs_info->buffer_radix,
> > +				(void **)gang, cur >> fs_info->sectorsize_bits,
> > +				min_t(unsigned int, GANG_LOOKUP_SIZE,
> > +				      PAGE_SIZE / fs_info->nodesize));
> > +		if (ret == 0)
> > +			goto out;
> > +		for (i = 0; i < ret; i++) {
> > +			/* Already beyond page end */
> > +			if (gang[i]->start >= page_start + PAGE_SIZE)
> 
> nit: this could be rewritten as (in_range(gang[i]->start, page_start,
> PAGE_SIZE)

Yes it could though I won't do that myself as it touches the logic and
code flow so I don't count it as trivial fixups I usually do. I'll fold
a fixup if you or Qu send it.

Re: [PATCH v2 4/4] btrfs: subpage: pack all subpage bitmaps into a larger bitmap

[David Sterba]

On Tue, Aug 17, 2021 at 05:38:52PM +0800, Qu Wenruo wrote:
> Currently we use u16 bitmap to make 4k sectorsize work for 64K page
> size.
> 
> But this u16 bitmap is not large enough to contain larger page size like
> 128K, nor is space efficient for 16K page size.
> 
> To handle both cases, here we pack all subpage bitmaps into a larger
> bitmap, now btrfs_subpage::bitmaps[] will be the ultimate bitmap for
> subpage usage.
> 
> Each sub-bitmap will has its start bit number recorded in
> btrfs_subpage_info::*_start, and its bitmap length will be recorded in
> btrfs_subpage_info::bitmap_nr_bits.
> 
> All subpage bitmap operations will be converted from using direct u16
> operations to bitmap operations, with above *_start calculated.
> 
> For 64K page size with 4K sectorsize, this should not cause much
> difference.
> 
> While for 16K page size, we will only need 1 unsigned long (u32) to
> store all the bitmaps, which saves quite some space.
> 
> Furthermore, this allows us to support larger page size like 128K and
> 258K.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  fs/btrfs/extent_io.c |  59 +++++++++++--------
>  fs/btrfs/subpage.c   | 136 +++++++++++++++++++++++++++++--------------
>  fs/btrfs/subpage.h   |  19 +-----
>  3 files changed, 129 insertions(+), 85 deletions(-)
> 
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index 3c5770d47a95..fcb25ff86ea9 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -3856,10 +3856,9 @@ static void find_next_dirty_byte(struct btrfs_fs_info *fs_info,
>  	struct btrfs_subpage *subpage = (struct btrfs_subpage *)page->private;
>  	u64 orig_start = *start;
>  	/* Declare as unsigned long so we can use bitmap ops */
> -	unsigned long dirty_bitmap;
>  	unsigned long flags;
> -	int nbits = (orig_start - page_offset(page)) >> fs_info->sectorsize_bits;
> -	int range_start_bit = nbits;
> +	int range_start_bit = fs_info->subpage_info->dirty_offset +
> +		(offset_in_page(orig_start) >> fs_info->sectorsize_bits);

There are several instances of fs_info->subpage_info so that warrants a
temporary variable.

>  	int range_end_bit;
>  
>  	/*
> @@ -3874,11 +3873,14 @@ static void find_next_dirty_byte(struct btrfs_fs_info *fs_info,
>  
>  	/* We should have the page locked, but just in case */
>  	spin_lock_irqsave(&subpage->lock, flags);
> -	dirty_bitmap = subpage->dirty_bitmap;
> +	bitmap_next_set_region(subpage->bitmaps, &range_start_bit, &range_end_bit,
> +			       fs_info->subpage_info->dirty_offset +
> +			       fs_info->subpage_info->bitmap_nr_bits);
>  	spin_unlock_irqrestore(&subpage->lock, flags);
>  
> -	bitmap_next_set_region(&dirty_bitmap, &range_start_bit, &range_end_bit,
> -			       BTRFS_SUBPAGE_BITMAP_SIZE);
> +	range_start_bit -= fs_info->subpage_info->dirty_offset;
> +	range_end_bit -= fs_info->subpage_info->dirty_offset;
> +
>  	*start = page_offset(page) + range_start_bit * fs_info->sectorsize;
>  	*end = page_offset(page) + range_end_bit * fs_info->sectorsize;
>  }
> @@ -4602,12 +4604,11 @@ static int submit_eb_subpage(struct page *page,
>  	int submitted = 0;
>  	u64 page_start = page_offset(page);
>  	int bit_start = 0;
> -	const int nbits = BTRFS_SUBPAGE_BITMAP_SIZE;
>  	int sectors_per_node = fs_info->nodesize >> fs_info->sectorsize_bits;
>  	int ret;
>  
>  	/* Lock and write each dirty extent buffers in the range */
> -	while (bit_start < nbits) {
> +	while (bit_start < fs_info->subpage_info->bitmap_nr_bits) {
>  		struct btrfs_subpage *subpage = (struct btrfs_subpage *)page->private;
>  		struct extent_buffer *eb;
>  		unsigned long flags;
> @@ -4623,7 +4624,8 @@ static int submit_eb_subpage(struct page *page,
>  			break;
>  		}
>  		spin_lock_irqsave(&subpage->lock, flags);
> -		if (!((1 << bit_start) & subpage->dirty_bitmap)) {
> +		if (!test_bit(bit_start + fs_info->subpage_info->dirty_offset,
> +			      subpage->bitmaps)) {
>  			spin_unlock_irqrestore(&subpage->lock, flags);
>  			spin_unlock(&page->mapping->private_lock);
>  			bit_start++;
> @@ -7170,32 +7172,41 @@ void memmove_extent_buffer(const struct extent_buffer *dst,
>  	}
>  }
>  
> +#define GANG_LOOKUP_SIZE	16
>  static struct extent_buffer *get_next_extent_buffer(
>  		struct btrfs_fs_info *fs_info, struct page *page, u64 bytenr)
>  {
> -	struct extent_buffer *gang[BTRFS_SUBPAGE_BITMAP_SIZE];
> +	struct extent_buffer *gang[GANG_LOOKUP_SIZE];
>  	struct extent_buffer *found = NULL;
>  	u64 page_start = page_offset(page);
> -	int ret;
> -	int i;
> +	u64 cur = page_start;
>  
>  	ASSERT(in_range(bytenr, page_start, PAGE_SIZE));
> -	ASSERT(PAGE_SIZE / fs_info->nodesize <= BTRFS_SUBPAGE_BITMAP_SIZE);
>  	lockdep_assert_held(&fs_info->buffer_lock);
>  
> -	ret = radix_tree_gang_lookup(&fs_info->buffer_radix, (void **)gang,
> -			bytenr >> fs_info->sectorsize_bits,
> -			PAGE_SIZE / fs_info->nodesize);
> -	for (i = 0; i < ret; i++) {
> -		/* Already beyond page end */
> -		if (gang[i]->start >= page_start + PAGE_SIZE)
> -			break;
> -		/* Found one */
> -		if (gang[i]->start >= bytenr) {
> -			found = gang[i];
> -			break;
> +	while (cur < page_start + PAGE_SIZE) {
> +		int ret;
> +		int i;
> +
> +		ret = radix_tree_gang_lookup(&fs_info->buffer_radix,
> +				(void **)gang, cur >> fs_info->sectorsize_bits,
> +				min_t(unsigned int, GANG_LOOKUP_SIZE,
> +				      PAGE_SIZE / fs_info->nodesize));
> +		if (ret == 0)
> +			goto out;
> +		for (i = 0; i < ret; i++) {
> +			/* Already beyond page end */
> +			if (gang[i]->start >= page_start + PAGE_SIZE)
> +				goto out;
> +			/* Found one */
> +			if (gang[i]->start >= bytenr) {
> +				found = gang[i];
> +				goto out;
> +			}
>  		}
> +		cur = gang[ret - 1]->start + gang[ret - 1]->len;
>  	}
> +out:
>  	return found;
>  }
>  
> diff --git a/fs/btrfs/subpage.c b/fs/btrfs/subpage.c
> index c4fb2ce52207..578095c52a0f 100644
> --- a/fs/btrfs/subpage.c
> +++ b/fs/btrfs/subpage.c
> @@ -142,10 +142,13 @@ struct btrfs_subpage *btrfs_alloc_subpage(const struct btrfs_fs_info *fs_info,
>  					  enum btrfs_subpage_type type)
>  {
>  	struct btrfs_subpage *ret;
> +	unsigned int real_size;
>  
>  	ASSERT(fs_info->sectorsize < PAGE_SIZE);
>  
> -	ret = kzalloc(sizeof(struct btrfs_subpage), GFP_NOFS);
> +	real_size = struct_size(ret, bitmaps,
> +			BITS_TO_LONGS(fs_info->subpage_info->total_nr_bits));
> +	ret = kzalloc(real_size, GFP_NOFS);
>  	if (!ret)
>  		return ERR_PTR(-ENOMEM);
>  
> @@ -328,37 +331,60 @@ void btrfs_page_end_writer_lock(const struct btrfs_fs_info *fs_info,
>  		unlock_page(page);
>  }
>  
> -/*
> - * Convert the [start, start + len) range into a u16 bitmap
> - *
> - * For example: if start == page_offset() + 16K, len = 16K, we get 0x00f0.
> - */
> -static u16 btrfs_subpage_calc_bitmap(const struct btrfs_fs_info *fs_info,
> -		struct page *page, u64 start, u32 len)
> +static bool bitmap_test_range_all_set(unsigned long *addr, unsigned int start,
> +				      unsigned int nbits)
>  {
> -	const int bit_start = offset_in_page(start) >> fs_info->sectorsize_bits;
> -	const int nbits = len >> fs_info->sectorsize_bits;
> +	unsigned int found_zero;
>  
> -	btrfs_subpage_assert(fs_info, page, start, len);
> +	found_zero = find_next_zero_bit(addr, start + nbits, start);
> +	if (found_zero == start + nbits)
> +		return true;
> +	return false;
> +}
>  
> -	/*
> -	 * Here nbits can be 16, thus can go beyond u16 range. We make the
> -	 * first left shift to be calculate in unsigned long (at least u32),
> -	 * then truncate the result to u16.
> -	 */
> -	return (u16)(((1UL << nbits) - 1) << bit_start);
> +static bool bitmap_test_range_all_zero(unsigned long *addr, unsigned int start,
> +				       unsigned int nbits)
> +{
> +	unsigned int found_set;
> +
> +	found_set = find_next_bit(addr, start + nbits, start);
> +	if (found_set == start + nbits)
> +		return true;
> +	return false;
>  }
>  
> +#define subpage_calc_start_bit(fs_info, page, name, start, len)		\
> +({									\
> +	unsigned int start_bit;						\
> +									\
> +	btrfs_subpage_assert(fs_info, page, start, len);		\
> +	start_bit = offset_in_page(start) >> fs_info->sectorsize_bits;	\
> +	start_bit += fs_info->subpage_info->name##_offset;		\
> +	start_bit;							\
> +})
> +
> +#define subpage_test_bitmap_all_set(fs_info, subpage, name)		\
> +	bitmap_test_range_all_set(subpage->bitmaps,			\
> +			fs_info->subpage_info->name##_offset,		\
> +			fs_info->subpage_info->bitmap_nr_bits)
> +
> +#define subpage_test_bitmap_all_zero(fs_info, subpage, name)		\
> +	bitmap_test_range_all_zero(subpage->bitmaps,			\
> +			fs_info->subpage_info->name##_offset,		\
> +			fs_info->subpage_info->bitmap_nr_bits)
> +
>  void btrfs_subpage_set_uptodate(const struct btrfs_fs_info *fs_info,
>  		struct page *page, u64 start, u32 len)
>  {
>  	struct btrfs_subpage *subpage = (struct btrfs_subpage *)page->private;
> -	const u16 tmp = btrfs_subpage_calc_bitmap(fs_info, page, start, len);
> +	unsigned int start_bit = subpage_calc_start_bit(fs_info, page,
> +							uptodate, start, len);
>  	unsigned long flags;
>  
>  	spin_lock_irqsave(&subpage->lock, flags);
> -	subpage->uptodate_bitmap |= tmp;
> -	if (subpage->uptodate_bitmap == U16_MAX)
> +	bitmap_set(subpage->bitmaps, start_bit,
> +		   len >> fs_info->sectorsize_bits);

All the bitmap_* calls like this and the parameter fit one line.

Re: [PATCH v2 4/4] btrfs: subpage: pack all subpage bitmaps into a larger bitmap

[Qu Wenruo]

On 2021/8/24 上午12:57, David Sterba wrote:
> On Tue, Aug 17, 2021 at 05:38:52PM +0800, Qu Wenruo wrote:
>> Currently we use u16 bitmap to make 4k sectorsize work for 64K page
>> size.
>>
>> But this u16 bitmap is not large enough to contain larger page size like
>> 128K, nor is space efficient for 16K page size.
>>
>> To handle both cases, here we pack all subpage bitmaps into a larger
>> bitmap, now btrfs_subpage::bitmaps[] will be the ultimate bitmap for
>> subpage usage.
>>
>> Each sub-bitmap will has its start bit number recorded in
>> btrfs_subpage_info::*_start, and its bitmap length will be recorded in
>> btrfs_subpage_info::bitmap_nr_bits.
>>
>> All subpage bitmap operations will be converted from using direct u16
>> operations to bitmap operations, with above *_start calculated.
>>
>> For 64K page size with 4K sectorsize, this should not cause much
>> difference.
>>
>> While for 16K page size, we will only need 1 unsigned long (u32) to
>> store all the bitmaps, which saves quite some space.
>>
>> Furthermore, this allows us to support larger page size like 128K and
>> 258K.
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>   fs/btrfs/extent_io.c |  59 +++++++++++--------
>>   fs/btrfs/subpage.c   | 136 +++++++++++++++++++++++++++++--------------
>>   fs/btrfs/subpage.h   |  19 +-----
>>   3 files changed, 129 insertions(+), 85 deletions(-)
>>
>> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
>> index 3c5770d47a95..fcb25ff86ea9 100644
>> --- a/fs/btrfs/extent_io.c
>> +++ b/fs/btrfs/extent_io.c
>> @@ -3856,10 +3856,9 @@ static void find_next_dirty_byte(struct btrfs_fs_info *fs_info,
>>   	struct btrfs_subpage *subpage = (struct btrfs_subpage *)page->private;
>>   	u64 orig_start = *start;
>>   	/* Declare as unsigned long so we can use bitmap ops */
>> -	unsigned long dirty_bitmap;
>>   	unsigned long flags;
>> -	int nbits = (orig_start - page_offset(page)) >> fs_info->sectorsize_bits;
>> -	int range_start_bit = nbits;
>> +	int range_start_bit = fs_info->subpage_info->dirty_offset +
>> +		(offset_in_page(orig_start) >> fs_info->sectorsize_bits);
>
> There are several instances of fs_info->subpage_info so that warrants a
> temporary variable.
>
>>   	int range_end_bit;
>>
>>   	/*
>> @@ -3874,11 +3873,14 @@ static void find_next_dirty_byte(struct btrfs_fs_info *fs_info,
>>
>>   	/* We should have the page locked, but just in case */
>>   	spin_lock_irqsave(&subpage->lock, flags);
>> -	dirty_bitmap = subpage->dirty_bitmap;
>> +	bitmap_next_set_region(subpage->bitmaps, &range_start_bit, &range_end_bit,
>> +			       fs_info->subpage_info->dirty_offset +
>> +			       fs_info->subpage_info->bitmap_nr_bits);
>>   	spin_unlock_irqrestore(&subpage->lock, flags);
>>
>> -	bitmap_next_set_region(&dirty_bitmap, &range_start_bit, &range_end_bit,
>> -			       BTRFS_SUBPAGE_BITMAP_SIZE);
>> +	range_start_bit -= fs_info->subpage_info->dirty_offset;
>> +	range_end_bit -= fs_info->subpage_info->dirty_offset;
>> +
>>   	*start = page_offset(page) + range_start_bit * fs_info->sectorsize;
>>   	*end = page_offset(page) + range_end_bit * fs_info->sectorsize;
>>   }
>> @@ -4602,12 +4604,11 @@ static int submit_eb_subpage(struct page *page,
>>   	int submitted = 0;
>>   	u64 page_start = page_offset(page);
>>   	int bit_start = 0;
>> -	const int nbits = BTRFS_SUBPAGE_BITMAP_SIZE;
>>   	int sectors_per_node = fs_info->nodesize >> fs_info->sectorsize_bits;
>>   	int ret;
>>
>>   	/* Lock and write each dirty extent buffers in the range */
>> -	while (bit_start < nbits) {
>> +	while (bit_start < fs_info->subpage_info->bitmap_nr_bits) {
>>   		struct btrfs_subpage *subpage = (struct btrfs_subpage *)page->private;
>>   		struct extent_buffer *eb;
>>   		unsigned long flags;
>> @@ -4623,7 +4624,8 @@ static int submit_eb_subpage(struct page *page,
>>   			break;
>>   		}
>>   		spin_lock_irqsave(&subpage->lock, flags);
>> -		if (!((1 << bit_start) & subpage->dirty_bitmap)) {
>> +		if (!test_bit(bit_start + fs_info->subpage_info->dirty_offset,
>> +			      subpage->bitmaps)) {
>>   			spin_unlock_irqrestore(&subpage->lock, flags);
>>   			spin_unlock(&page->mapping->private_lock);
>>   			bit_start++;
>> @@ -7170,32 +7172,41 @@ void memmove_extent_buffer(const struct extent_buffer *dst,
>>   	}
>>   }
>>
>> +#define GANG_LOOKUP_SIZE	16
>>   static struct extent_buffer *get_next_extent_buffer(
>>   		struct btrfs_fs_info *fs_info, struct page *page, u64 bytenr)
>>   {
>> -	struct extent_buffer *gang[BTRFS_SUBPAGE_BITMAP_SIZE];
>> +	struct extent_buffer *gang[GANG_LOOKUP_SIZE];
>>   	struct extent_buffer *found = NULL;
>>   	u64 page_start = page_offset(page);
>> -	int ret;
>> -	int i;
>> +	u64 cur = page_start;
>>
>>   	ASSERT(in_range(bytenr, page_start, PAGE_SIZE));
>> -	ASSERT(PAGE_SIZE / fs_info->nodesize <= BTRFS_SUBPAGE_BITMAP_SIZE);
>>   	lockdep_assert_held(&fs_info->buffer_lock);
>>
>> -	ret = radix_tree_gang_lookup(&fs_info->buffer_radix, (void **)gang,
>> -			bytenr >> fs_info->sectorsize_bits,
>> -			PAGE_SIZE / fs_info->nodesize);
>> -	for (i = 0; i < ret; i++) {
>> -		/* Already beyond page end */
>> -		if (gang[i]->start >= page_start + PAGE_SIZE)
>> -			break;
>> -		/* Found one */
>> -		if (gang[i]->start >= bytenr) {
>> -			found = gang[i];
>> -			break;
>> +	while (cur < page_start + PAGE_SIZE) {
>> +		int ret;
>> +		int i;
>> +
>> +		ret = radix_tree_gang_lookup(&fs_info->buffer_radix,
>> +				(void **)gang, cur >> fs_info->sectorsize_bits,
>> +				min_t(unsigned int, GANG_LOOKUP_SIZE,
>> +				      PAGE_SIZE / fs_info->nodesize));
>> +		if (ret == 0)
>> +			goto out;
>> +		for (i = 0; i < ret; i++) {
>> +			/* Already beyond page end */
>> +			if (gang[i]->start >= page_start + PAGE_SIZE)
>> +				goto out;
>> +			/* Found one */
>> +			if (gang[i]->start >= bytenr) {
>> +				found = gang[i];
>> +				goto out;
>> +			}
>>   		}
>> +		cur = gang[ret - 1]->start + gang[ret - 1]->len;
>>   	}
>> +out:
>>   	return found;
>>   }
>>
>> diff --git a/fs/btrfs/subpage.c b/fs/btrfs/subpage.c
>> index c4fb2ce52207..578095c52a0f 100644
>> --- a/fs/btrfs/subpage.c
>> +++ b/fs/btrfs/subpage.c
>> @@ -142,10 +142,13 @@ struct btrfs_subpage *btrfs_alloc_subpage(const struct btrfs_fs_info *fs_info,
>>   					  enum btrfs_subpage_type type)
>>   {
>>   	struct btrfs_subpage *ret;
>> +	unsigned int real_size;
>>
>>   	ASSERT(fs_info->sectorsize < PAGE_SIZE);
>>
>> -	ret = kzalloc(sizeof(struct btrfs_subpage), GFP_NOFS);
>> +	real_size = struct_size(ret, bitmaps,
>> +			BITS_TO_LONGS(fs_info->subpage_info->total_nr_bits));
>> +	ret = kzalloc(real_size, GFP_NOFS);
>>   	if (!ret)
>>   		return ERR_PTR(-ENOMEM);
>>
>> @@ -328,37 +331,60 @@ void btrfs_page_end_writer_lock(const struct btrfs_fs_info *fs_info,
>>   		unlock_page(page);
>>   }
>>
>> -/*
>> - * Convert the [start, start + len) range into a u16 bitmap
>> - *
>> - * For example: if start == page_offset() + 16K, len = 16K, we get 0x00f0.
>> - */
>> -static u16 btrfs_subpage_calc_bitmap(const struct btrfs_fs_info *fs_info,
>> -		struct page *page, u64 start, u32 len)
>> +static bool bitmap_test_range_all_set(unsigned long *addr, unsigned int start,
>> +				      unsigned int nbits)
>>   {
>> -	const int bit_start = offset_in_page(start) >> fs_info->sectorsize_bits;
>> -	const int nbits = len >> fs_info->sectorsize_bits;
>> +	unsigned int found_zero;
>>
>> -	btrfs_subpage_assert(fs_info, page, start, len);
>> +	found_zero = find_next_zero_bit(addr, start + nbits, start);
>> +	if (found_zero == start + nbits)
>> +		return true;
>> +	return false;
>> +}
>>
>> -	/*
>> -	 * Here nbits can be 16, thus can go beyond u16 range. We make the
>> -	 * first left shift to be calculate in unsigned long (at least u32),
>> -	 * then truncate the result to u16.
>> -	 */
>> -	return (u16)(((1UL << nbits) - 1) << bit_start);
>> +static bool bitmap_test_range_all_zero(unsigned long *addr, unsigned int start,
>> +				       unsigned int nbits)
>> +{
>> +	unsigned int found_set;
>> +
>> +	found_set = find_next_bit(addr, start + nbits, start);
>> +	if (found_set == start + nbits)
>> +		return true;
>> +	return false;
>>   }
>>
>> +#define subpage_calc_start_bit(fs_info, page, name, start, len)		\
>> +({									\
>> +	unsigned int start_bit;						\
>> +									\
>> +	btrfs_subpage_assert(fs_info, page, start, len);		\
>> +	start_bit = offset_in_page(start) >> fs_info->sectorsize_bits;	\
>> +	start_bit += fs_info->subpage_info->name##_offset;		\
>> +	start_bit;							\
>> +})
>> +
>> +#define subpage_test_bitmap_all_set(fs_info, subpage, name)		\
>> +	bitmap_test_range_all_set(subpage->bitmaps,			\
>> +			fs_info->subpage_info->name##_offset,		\
>> +			fs_info->subpage_info->bitmap_nr_bits)
>> +
>> +#define subpage_test_bitmap_all_zero(fs_info, subpage, name)		\
>> +	bitmap_test_range_all_zero(subpage->bitmaps,			\
>> +			fs_info->subpage_info->name##_offset,		\
>> +			fs_info->subpage_info->bitmap_nr_bits)
>> +
>>   void btrfs_subpage_set_uptodate(const struct btrfs_fs_info *fs_info,
>>   		struct page *page, u64 start, u32 len)
>>   {
>>   	struct btrfs_subpage *subpage = (struct btrfs_subpage *)page->private;
>> -	const u16 tmp = btrfs_subpage_calc_bitmap(fs_info, page, start, len);
>> +	unsigned int start_bit = subpage_calc_start_bit(fs_info, page,
>> +							uptodate, start, len);
>>   	unsigned long flags;
>>
>>   	spin_lock_irqsave(&subpage->lock, flags);
>> -	subpage->uptodate_bitmap |= tmp;
>> -	if (subpage->uptodate_bitmap == U16_MAX)
>> +	bitmap_set(subpage->bitmaps, start_bit,
>> +		   len >> fs_info->sectorsize_bits);
>
> All the bitmap_* calls like this and the parameter fit one line.
>
But that's over 80 chars.

I understand now our standard no longer requires 80 chars as hard limit,
but isn't it seill recommended to keep inside the 80 chars limit?

Thanks,
Qu

Re: [PATCH v2 4/4] btrfs: subpage: pack all subpage bitmaps into a larger bitmap

[David Sterba]

On Tue, Aug 24, 2021 at 07:16:49AM +0800, Qu Wenruo wrote:
> >>   	spin_lock_irqsave(&subpage->lock, flags);
> >> -	subpage->uptodate_bitmap |= tmp;
> >> -	if (subpage->uptodate_bitmap == U16_MAX)
> >> +	bitmap_set(subpage->bitmaps, start_bit,
> >> +		   len >> fs_info->sectorsize_bits);
> >
> > All the bitmap_* calls like this and the parameter fit one line.
> >
> But that's over 80 chars.
> 
> I understand now our standard no longer requires 80 chars as hard limit,
> but isn't it seill recommended to keep inside the 80 chars limit?

If a slight overflow does not harm readability, like if word
'sectorsize' crosses 80 chars by like 4  I take it as reasonable and
join the lines if I see it. The 80 char limit is still there, because
merging patches sometimes requires 3 files next to each other so that is
really helpful.

As a visual clue where's the 80 boundary I have the following in .vimrc

let &colorcolumn="81,91,101,+0"

the +0 is for textwidth setting.

Re: [PATCH v2 0/2] btrfs: subpage: pack all subpage bitmaps into a larger bitmap

[Nikolay Borisov]

On 17.08.21 г. 12:38, Qu Wenruo wrote:
> Currently we use u16 bitmap to make 4k sectorsize work for 64K page
> size.
> 
> But this u16 bitmap is not large enough to contain larger page size like
> 128K, nor is space efficient for 16K page size.
> 
> To handle both cases, here we pack all subpage bitmaps into a larger
> bitmap, now btrfs_subpage::bitmaps[] will be the ultimate bitmap for
> subpage usage.
> 
> This is the first step towards more page size support.
> 
> Although to really enable extra page size like 16K and 128K, we need to
> rework the metadata alignment check.
> Which will happen in another patchset.
> 
> Changelog:
> v2:
> - Add two refactor patches to make btrfs_alloc_subpage() more readable
> - Fix a break inside two loops bug
> - Rename subpage_info::*_start to subpage_info::*_offset
> - Add extra comment on what each subpage_info::*_offset works
> 
> 

Overall this looks better, with my small nits addressed:

Reviewed-by: Nikolay Borisov <nborisov@suse.com>

> Qu Wenruo (4):
>   btrfs: only call btrfs_alloc_subpage() when sectorsize is smaller than
>     PAGE_SIZE
>   btrfs: make btrfs_alloc_subpage() to return struct btrfs_subpage *
>     directly
>   btrfs: introduce btrfs_subpage_bitmap_info
>   btrfs: subpage: pack all subpage bitmaps into a larger bitmap
> 
>  fs/btrfs/ctree.h     |   1 +
>  fs/btrfs/disk-io.c   |  12 ++-
>  fs/btrfs/extent_io.c |  76 +++++++++-------
>  fs/btrfs/subpage.c   | 205 ++++++++++++++++++++++++++++++-------------
>  fs/btrfs/subpage.h   |  54 ++++++++----
>  5 files changed, 238 insertions(+), 110 deletions(-)
> 

Re: [PATCH v2 0/2] btrfs: subpage: pack all subpage bitmaps into a larger bitmap

[David Sterba]

On Tue, Aug 17, 2021 at 05:38:48PM +0800, Qu Wenruo wrote:
> Currently we use u16 bitmap to make 4k sectorsize work for 64K page
> size.
> 
> But this u16 bitmap is not large enough to contain larger page size like
> 128K, nor is space efficient for 16K page size.
> 
> To handle both cases, here we pack all subpage bitmaps into a larger
> bitmap, now btrfs_subpage::bitmaps[] will be the ultimate bitmap for
> subpage usage.
> 
> This is the first step towards more page size support.
> 
> Although to really enable extra page size like 16K and 128K, we need to
> rework the metadata alignment check.
> Which will happen in another patchset.
> 
> Changelog:
> v2:
> - Add two refactor patches to make btrfs_alloc_subpage() more readable
> - Fix a break inside two loops bug
> - Rename subpage_info::*_start to subpage_info::*_offset
> - Add extra comment on what each subpage_info::*_offset works
> 
> 
> Qu Wenruo (4):
>   btrfs: only call btrfs_alloc_subpage() when sectorsize is smaller than
>     PAGE_SIZE
>   btrfs: make btrfs_alloc_subpage() to return struct btrfs_subpage *
>     directly
>   btrfs: introduce btrfs_subpage_bitmap_info
>   btrfs: subpage: pack all subpage bitmaps into a larger bitmap

With a few fixups added to misc-next, thanks. I haven't tested it on
subpage config.