[PATCH 0/7] btrfs: remove ASSERT()s for folio_order() and folio_test_large()

[PATCH 0/7] btrfs: remove ASSERT()s for folio_order() and folio_test_large()

[Qu Wenruo]

There are quite some ASSERT()s checking folio_order() or
folio_test_large() on data folios.

They are the blockage for the incoming larger data folios.

This series will remove most of them, with the following exceptions
remaining:

- wait_dev_supers() and write_dev_supers()
  They are folios from block device cache, will not be affected by our
  larger data folios support.

- relocation_one_folio()
  It's about data relocation inode, and we can disable larger data folios
  for such special inode easily.

- btrfs_attach_subpage()
  The folio_test_large() is only for metadata, just change it to be a
  metadata specific check.

- defrage_prepare_one_folio()
  It rejects larger folios gracefully so no need to handle it yet.
  Although we still need some extra work to make defrag to properly
  support larger folios.

- btrfs_end_repair_bio()
  This will be covered by a dedicated series converting the bio layer to
  use folio interfaces other than bvec interfaces.
  Until then the ASSERT() will stay.

The first patch is just a small cleanup for send, exposed during the
removal of ASSERT()s.

Qu Wenruo (7):
  btrfs: send: remove the again label inside put_file_data()
  btrfs: send: prepare put_file_data() for larger data folios
  btrfs: prepare btrfs_page_mkwrite() for larger data folios
  btrfs: prepare prepare_one_folio() for larger data folios
  btrfs: prepare end_bbio_data_write() for larger data folios
  btrfs: subpage: prepare for larger data folios
  btrfs: zlib: prepare copy_data_into_buffer() for larger data folios

 fs/btrfs/extent_io.c |  3 ---
 fs/btrfs/file.c      |  6 +-----
 fs/btrfs/send.c      | 29 +++++++++++++----------------
 fs/btrfs/subpage.c   |  6 ++----
 fs/btrfs/zlib.c      |  2 --
 5 files changed, 16 insertions(+), 30 deletions(-)

-- 
2.48.1

[PATCH 1/7] btrfs: send: remove the again label inside put_file_data()

[Qu Wenruo]

The again label is not really necessary and can be replaced by a simple
continue.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/send.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
index 0c8c58c4f29b..43c29295f477 100644
--- a/fs/btrfs/send.c
+++ b/fs/btrfs/send.c
@@ -5280,7 +5280,6 @@ static int put_file_data(struct send_ctx *sctx, u64 offset, u32 len)
 		unsigned cur_len = min_t(unsigned, len,
 					 PAGE_SIZE - pg_offset);
 
-again:
 		folio = filemap_lock_folio(mapping, index);
 		if (IS_ERR(folio)) {
 			page_cache_sync_readahead(mapping,
@@ -5316,7 +5315,7 @@ static int put_file_data(struct send_ctx *sctx, u64 offset, u32 len)
 			if (folio->mapping != mapping) {
 				folio_unlock(folio);
 				folio_put(folio);
-				goto again;
+				continue;
 			}
 		}
 
-- 
2.48.1

[PATCH 2/7] btrfs: send: prepare put_file_data() for larger data folios

[Qu Wenruo]

Currently the function put_file_data() can only accept page sized folio.

However the function itself is not that complex, it's just copying data
from filemap folio into send buffer.

So make it support larger data folios by:

- Change the loop to use file offset instead of page index

- Calculate @pg_offset and @cur_len after getting the folio

- Remove the "WARN_ON(folio_order(folio));" line

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/send.c | 26 ++++++++++++--------------
 1 file changed, 12 insertions(+), 14 deletions(-)

diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
index 43c29295f477..4df07dfe326f 100644
--- a/fs/btrfs/send.c
+++ b/fs/btrfs/send.c
@@ -5263,10 +5263,9 @@ static int put_file_data(struct send_ctx *sctx, u64 offset, u32 len)
 {
 	struct btrfs_root *root = sctx->send_root;
 	struct btrfs_fs_info *fs_info = root->fs_info;
-	struct folio *folio;
-	pgoff_t index = offset >> PAGE_SHIFT;
-	pgoff_t last_index;
-	unsigned pg_offset = offset_in_page(offset);
+	u64 cur = offset;
+	const u64 end = offset + len;
+	const pgoff_t last_index = (end - 1) >> PAGE_SHIFT;
 	struct address_space *mapping = sctx->cur_inode->i_mapping;
 	int ret;
 
@@ -5274,11 +5273,11 @@ static int put_file_data(struct send_ctx *sctx, u64 offset, u32 len)
 	if (ret)
 		return ret;
 
-	last_index = (offset + len - 1) >> PAGE_SHIFT;
-
-	while (index <= last_index) {
-		unsigned cur_len = min_t(unsigned, len,
-					 PAGE_SIZE - pg_offset);
+	while (cur < end) {
+		pgoff_t index = cur >> PAGE_SHIFT;
+		unsigned int cur_len;
+		unsigned int pg_offset;
+		struct folio *folio;
 
 		folio = filemap_lock_folio(mapping, index);
 		if (IS_ERR(folio)) {
@@ -5292,8 +5291,9 @@ static int put_file_data(struct send_ctx *sctx, u64 offset, u32 len)
 				break;
 			}
 		}
-
-		WARN_ON(folio_order(folio));
+		pg_offset = offset_in_folio(folio, cur);
+		cur_len = min_t(unsigned int, end - cur,
+				folio_size(folio) - pg_offset);
 
 		if (folio_test_readahead(folio))
 			page_cache_async_readahead(mapping, &sctx->ra, NULL, folio,
@@ -5323,9 +5323,7 @@ static int put_file_data(struct send_ctx *sctx, u64 offset, u32 len)
 				  pg_offset, cur_len);
 		folio_unlock(folio);
 		folio_put(folio);
-		index++;
-		pg_offset = 0;
-		len -= cur_len;
+		cur += cur_len;
 		sctx->send_size += cur_len;
 	}
 
-- 
2.48.1

[PATCH 3/7] btrfs: prepare btrfs_page_mkwrite() for larger data folios

[Qu Wenruo]

The function btrfs_page_mkwrite() has an explicit ASSERT() checking the
folio order.

To make it support larger data folios, we need to:

- Remove the ASSERT(folio_order(folio) == 0)

- Use folio_contains() to check if the folio covers the last page

Otherwise the code is already supporting larger folios well.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/file.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index 262a707d8990..4213807982d6 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -1791,8 +1791,6 @@ static vm_fault_t btrfs_page_mkwrite(struct vm_fault *vmf)
 	u64 page_end;
 	u64 end;
 
-	ASSERT(folio_order(folio) == 0);
-
 	reserved_space = fsize;
 
 	sb_start_pagefault(inode->i_sb);
@@ -1857,7 +1855,7 @@ static vm_fault_t btrfs_page_mkwrite(struct vm_fault *vmf)
 		goto again;
 	}
 
-	if (folio->index == ((size - 1) >> PAGE_SHIFT)) {
+	if (folio_contains(folio, (size - 1) >> PAGE_SHIFT)) {
 		reserved_space = round_up(size - page_start, fs_info->sectorsize);
 		if (reserved_space < fsize) {
 			end = page_start + reserved_space - 1;
-- 
2.48.1

[PATCH 4/7] btrfs: prepare prepare_one_folio() for larger data folios

[Qu Wenruo]

The only blockage is the ASSERT() rejecting larger folios, just remove
it.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/file.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index 4213807982d6..c2648858772a 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -875,8 +875,6 @@ static noinline int prepare_one_folio(struct inode *inode, struct folio **folio_
 			ret = PTR_ERR(folio);
 		return ret;
 	}
-	/* Only support page sized folio yet. */
-	ASSERT(folio_order(folio) == 0);
 	ret = set_folio_extent_mapped(folio);
 	if (ret < 0) {
 		folio_unlock(folio);
-- 
2.48.1

[PATCH 5/7] btrfs: prepare end_bbio_data_write() for larger data folios

[Qu Wenruo]

The function is doing an ASSERT() checking the folio order,  but all
later functions are handling larger folios properly, thus we can safely
remove that ASSERT().

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/extent_io.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 7db74a173b77..d5d4f9b06309 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -462,9 +462,6 @@ static void end_bbio_data_write(struct btrfs_bio *bbio)
 		u64 start = folio_pos(folio) + fi.offset;
 		u32 len = fi.length;
 
-		/* Only order 0 (single page) folios are allowed for data. */
-		ASSERT(folio_order(folio) == 0);
-
 		/* Our read/write should always be sector aligned. */
 		if (!IS_ALIGNED(fi.offset, sectorsize))
 			btrfs_err(fs_info,
-- 
2.48.1

[PATCH 6/7] btrfs: subpage: prepare for larger data folios

[Qu Wenruo]

The subpage handling code has two locations not supporting larger
folios:

- btrfs_attach_subpage()
  Which is doing a metadata specific ASSERT() check.

  But for the future larger data folios support, that check is too
  generic.
  Since it's metadata specific, only check the ASSERT() for metadata.

- btrfs_subpage_assert()
  Just remove the "ASSERT(folio_order(folio) == 0)" check.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/subpage.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/fs/btrfs/subpage.c b/fs/btrfs/subpage.c
index 834161f35a00..bf7fd50ab9ec 100644
--- a/fs/btrfs/subpage.c
+++ b/fs/btrfs/subpage.c
@@ -69,7 +69,8 @@ int btrfs_attach_subpage(const struct btrfs_fs_info *fs_info,
 	struct btrfs_subpage *subpage;
 
 	/* For metadata we don't support larger folio yet. */
-	ASSERT(!folio_test_large(folio));
+	if (type == BTRFS_SUBPAGE_METADATA)
+		ASSERT(!folio_test_large(folio));
 
 	/*
 	 * We have cases like a dummy extent buffer page, which is not mapped
@@ -181,9 +182,6 @@ void btrfs_folio_dec_eb_refs(const struct btrfs_fs_info *fs_info, struct folio *
 static void btrfs_subpage_assert(const struct btrfs_fs_info *fs_info,
 				 struct folio *folio, u64 start, u32 len)
 {
-	/* For subpage support, the folio must be single page. */
-	ASSERT(folio_order(folio) == 0);
-
 	/* Basic checks */
 	ASSERT(folio_test_private(folio) && folio_get_private(folio));
 	ASSERT(IS_ALIGNED(start, fs_info->sectorsize) &&
-- 
2.48.1

[PATCH 7/7] btrfs: zlib: prepare copy_data_into_buffer() for larger data folios

[Qu Wenruo]

The function itself is already taking larger folios into consideration,
just remove the ASSERT(!folio_test_large()) line.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/zlib.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/fs/btrfs/zlib.c b/fs/btrfs/zlib.c
index 3a7d136f57b4..b32aa05b288e 100644
--- a/fs/btrfs/zlib.c
+++ b/fs/btrfs/zlib.c
@@ -120,8 +120,6 @@ static int copy_data_into_buffer(struct address_space *mapping,
 		ret = btrfs_compress_filemap_get_folio(mapping, cur, &folio);
 		if (ret < 0)
 			return ret;
-		/* No larger folio support yet. */
-		ASSERT(!folio_test_large(folio));
 
 		offset = offset_in_folio(folio, cur);
 		copy_length = min(folio_size(folio) - offset,
-- 
2.48.1

Re: [PATCH 1/7] btrfs: send: remove the again label inside put_file_data()

[David Sterba]

On Thu, Mar 13, 2025 at 02:50:43PM +1030, Qu Wenruo wrote:
> The again label is not really necessary and can be replaced by a simple
> continue.

This should also say why it's needed.

> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  fs/btrfs/send.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
> index 0c8c58c4f29b..43c29295f477 100644
> --- a/fs/btrfs/send.c
> +++ b/fs/btrfs/send.c
> @@ -5280,7 +5280,6 @@ static int put_file_data(struct send_ctx *sctx, u64 offset, u32 len)
>  		unsigned cur_len = min_t(unsigned, len,
>  					 PAGE_SIZE - pg_offset);
>  
> -again:
>  		folio = filemap_lock_folio(mapping, index);
>  		if (IS_ERR(folio)) {
>  			page_cache_sync_readahead(mapping,
> @@ -5316,7 +5315,7 @@ static int put_file_data(struct send_ctx *sctx, u64 offset, u32 len)
>  			if (folio->mapping != mapping) {
>  				folio_unlock(folio);
>  				folio_put(folio);
> -				goto again;
> +				continue;
>  			}
>  		}
>  
> -- 
> 2.48.1
> 

Re: [PATCH 1/7] btrfs: send: remove the again label inside put_file_data()

[Qu Wenruo]

在 2025/3/15 03:22, David Sterba 写道:
> On Thu, Mar 13, 2025 at 02:50:43PM +1030, Qu Wenruo wrote:
>> The again label is not really necessary and can be replaced by a simple
>> continue.
>
> This should also say why it's needed.

Do you mean why we need to continue, or why the old label was needed?

Thanks,
Qu

>
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>   fs/btrfs/send.c | 3 +--
>>   1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
>> index 0c8c58c4f29b..43c29295f477 100644
>> --- a/fs/btrfs/send.c
>> +++ b/fs/btrfs/send.c
>> @@ -5280,7 +5280,6 @@ static int put_file_data(struct send_ctx *sctx, u64 offset, u32 len)
>>   		unsigned cur_len = min_t(unsigned, len,
>>   					 PAGE_SIZE - pg_offset);
>>
>> -again:
>>   		folio = filemap_lock_folio(mapping, index);
>>   		if (IS_ERR(folio)) {
>>   			page_cache_sync_readahead(mapping,
>> @@ -5316,7 +5315,7 @@ static int put_file_data(struct send_ctx *sctx, u64 offset, u32 len)
>>   			if (folio->mapping != mapping) {
>>   				folio_unlock(folio);
>>   				folio_put(folio);
>> -				goto again;
>> +				continue;
>>   			}
>>   		}
>>
>> --
>> 2.48.1
>>
>

Re: [PATCH 1/7] btrfs: send: remove the again label inside put_file_data()

[David Sterba]

On Sat, Mar 15, 2025 at 08:20:43AM +1030, Qu Wenruo wrote:
> 在 2025/3/15 03:22, David Sterba 写道:
> > On Thu, Mar 13, 2025 at 02:50:43PM +1030, Qu Wenruo wrote:
> >> The again label is not really necessary and can be replaced by a simple
> >> continue.
> >
> > This should also say why it's needed.
> 
> Do you mean why we need to continue, or why the old label was needed?

Something about that old and new code is equivalent and what implies
that (no changes in the variables). Changes in the control flow could
change invariants.

Re: [PATCH 1/7] btrfs: send: remove the again label inside put_file_data()

[Qu Wenruo]

在 2025/3/15 09:28, David Sterba 写道:
> On Sat, Mar 15, 2025 at 08:20:43AM +1030, Qu Wenruo wrote:
>> 在 2025/3/15 03:22, David Sterba 写道:
>>> On Thu, Mar 13, 2025 at 02:50:43PM +1030, Qu Wenruo wrote:
>>>> The again label is not really necessary and can be replaced by a simple
>>>> continue.
>>>
>>> This should also say why it's needed.
>>
>> Do you mean why we need to continue, or why the old label was needed?
> 
> Something about that old and new code is equivalent and what implies
> that (no changes in the variables). Changes in the control flow could
> change invariants.

I thought all developers here should understand what "continue" keyword 
does.

Do you really want me to copy the official definition of "continue" 
keyword here as a commit message?
That doesn't make much sense to me.

Re: [PATCH 1/7] btrfs: send: remove the again label inside put_file_data()

[David Sterba]

On Sat, Mar 15, 2025 at 09:55:31AM +1030, Qu Wenruo wrote:
> 
> 
> 在 2025/3/15 09:28, David Sterba 写道:
> > On Sat, Mar 15, 2025 at 08:20:43AM +1030, Qu Wenruo wrote:
> >> 在 2025/3/15 03:22, David Sterba 写道:
> >>> On Thu, Mar 13, 2025 at 02:50:43PM +1030, Qu Wenruo wrote:
> >>>> The again label is not really necessary and can be replaced by a simple
> >>>> continue.
> >>>
> >>> This should also say why it's needed.
> >>
> >> Do you mean why we need to continue, or why the old label was needed?
> > 
> > Something about that old and new code is equivalent and what implies
> > that (no changes in the variables). Changes in the control flow could
> > change invariants.
> 
> I thought all developers here should understand what "continue" keyword 
> does.
> 
> Do you really want me to copy the official definition of "continue" 
> keyword here as a commit message?

Do you really think this is my point and that I need this kind of
information?  Or that I'm asking to give a brief explanation 'why the
change is ok', the changelog only says 'what' the code does.