[PATCH v2 0/3] btrfs: small cleanups mostly for subpage cases

[PATCH v2 0/3] btrfs: small cleanups mostly for subpage cases

[Qu Wenruo]

Changelog:
v2:
- Fix an offset-by-one bug in the 3rd patch
  Unlock extent range end is inclusive.

During my hunt on the subpage uptodate desync bugs reported from Matt, I
exposed several PageUptodate usage which results inefficiency for
subpage cases.

Those two are fixed in the first two patches.

Furthermore I found processed_extent infrastructure is no longer needed
especially after all the csum verification is moved to storage layer (or
bio.c inside btrfs), we can easily unlock the full range without the
need for the infrastructure.

Thus the last patch would delete the processed_extent infrastructure
completely.

Qu Wenruo (3):
  btrfs: make alloc_extent_buffer() handle previously uptodate range
    more efficient for subpage
  btrfs: use the same @uptodate variable for end_bio_extent_readpage()
  btrfs: remove processed_extent infrastructure

 fs/btrfs/extent_io.c | 89 +++++---------------------------------------
 1 file changed, 10 insertions(+), 79 deletions(-)

-- 
2.40.1

[PATCH v2 1/3] btrfs: make alloc_extent_buffer() handle previously uptodate range more efficient for subpage

[Qu Wenruo]

Currently alloc_extent_buffer() would make the extent buffer uptodate if
the corresponding pages are also uptodate.

But this check is only checking PageUptodate, which is fine for regular
cases, but not for subpage cases, as we can have multiple extent buffers
in the same page.

So here we go btrfs_page_test_uptodate() instead.

The old code doesn't cause any problem, but not efficient, as it would
cause extra metadata read even if the range is already uptodate.

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

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index bbf212c7a43f..9afdcf0c70dd 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -3708,7 +3708,7 @@ struct extent_buffer *alloc_extent_buffer(struct btrfs_fs_info *fs_info,
 
 		WARN_ON(btrfs_page_test_dirty(fs_info, p, eb->start, eb->len));
 		eb->pages[i] = p;
-		if (!PageUptodate(p))
+		if (!btrfs_page_test_uptodate(fs_info, p, eb->start, eb->len))
 			uptodate = 0;
 
 		/*
-- 
2.40.1

[PATCH v2 2/3] btrfs: use the same @uptodate variable for end_bio_extent_readpage()

[Qu Wenruo]

In function end_bio_extent_readpage() we call
endio_readpage_release_extent() to unlock the extent io tree.

However we pass PageUptodate(page) as @uptodate parameter for it, while
for previous end_page_read() call, we use a dedicated @uptodate local
variable.

This is not a big deal, as even for subpage cases, either the bio only
covers part of the page, then the @uptodate is always false, and the
subpage ranges can still be merged.

But for the sake of consistency, always use @uptodate variable when
possible.

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

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 9afdcf0c70dd..2d228cc8b401 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -745,7 +745,7 @@ static void end_bio_extent_readpage(struct btrfs_bio *bbio)
 		/* Update page status and unlock. */
 		end_page_read(page, uptodate, start, len);
 		endio_readpage_release_extent(&processed, BTRFS_I(inode),
-					      start, end, PageUptodate(page));
+					      start, end, uptodate);
 
 		ASSERT(bio_offset + len > bio_offset);
 		bio_offset += len;
-- 
2.40.1

[PATCH v2 3/3] btrfs: remove processed_extent infrastructure

[Qu Wenruo]

The structure processed_extent and the helper
endio_readpage_release_extent() are used to reduce the number of calls of
unlock_extent() during end_bio_extent_readpage()

This is done by merging the range and only call the function either the
status (uptodate or not) changes or the range is no longer continuous.

However the behavior has been changed:

- The range is always continuous
  Since it's the endio function of a btrfs bio, it's ensured the range
  is always continuous inside the same file.

- The uptodate status is now per-bio (aka, will not change)
  Since commit 7609afac6775 ("btrfs: handle checksum validation and
  repair at the storage layer"), the function end_bio_extent_readpage()
  no longer handles the metadata/data verification.

  This means the @uptodate variable will not change during the function
  end_bio_extent_readpage()

Thus there is no longer the need for processed_extent and the helper
endio_readpage_release_extent().

Just call unlock_extent() at the end of end_bio_extent_readpage().

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

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 2d228cc8b401..4f5d26194768 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -581,75 +581,6 @@ static void end_bio_extent_writepage(struct btrfs_bio *bbio)
 	bio_put(bio);
 }
 
-/*
- * Record previously processed extent range
- *
- * For endio_readpage_release_extent() to handle a full extent range, reducing
- * the extent io operations.
- */
-struct processed_extent {
-	struct btrfs_inode *inode;
-	/* Start of the range in @inode */
-	u64 start;
-	/* End of the range in @inode */
-	u64 end;
-	bool uptodate;
-};
-
-/*
- * Try to release processed extent range
- *
- * May not release the extent range right now if the current range is
- * contiguous to processed extent.
- *
- * Will release processed extent when any of @inode, @uptodate, the range is
- * no longer contiguous to the processed range.
- *
- * Passing @inode == NULL will force processed extent to be released.
- */
-static void endio_readpage_release_extent(struct processed_extent *processed,
-			      struct btrfs_inode *inode, u64 start, u64 end,
-			      bool uptodate)
-{
-	struct extent_state *cached = NULL;
-	struct extent_io_tree *tree;
-
-	/* The first extent, initialize @processed */
-	if (!processed->inode)
-		goto update;
-
-	/*
-	 * Contiguous to processed extent, just uptodate the end.
-	 *
-	 * Several things to notice:
-	 *
-	 * - bio can be merged as long as on-disk bytenr is contiguous
-	 *   This means we can have page belonging to other inodes, thus need to
-	 *   check if the inode still matches.
-	 * - bvec can contain range beyond current page for multi-page bvec
-	 *   Thus we need to do processed->end + 1 >= start check
-	 */
-	if (processed->inode == inode && processed->uptodate == uptodate &&
-	    processed->end + 1 >= start && end >= processed->end) {
-		processed->end = end;
-		return;
-	}
-
-	tree = &processed->inode->io_tree;
-	/*
-	 * Now we don't have range contiguous to the processed range, release
-	 * the processed range now.
-	 */
-	unlock_extent(tree, processed->start, processed->end, &cached);
-
-update:
-	/* Update processed to current range */
-	processed->inode = inode;
-	processed->start = start;
-	processed->end = end;
-	processed->uptodate = uptodate;
-}
-
 static void begin_page_read(struct btrfs_fs_info *fs_info, struct page *page)
 {
 	ASSERT(PageLocked(page));
@@ -674,20 +605,21 @@ static void begin_page_read(struct btrfs_fs_info *fs_info, struct page *page)
 static void end_bio_extent_readpage(struct btrfs_bio *bbio)
 {
 	struct bio *bio = &bbio->bio;
+	struct inode *inode = bio_first_page_all(bio)->mapping->host;
 	struct bio_vec *bvec;
-	struct processed_extent processed = { 0 };
+	struct bvec_iter_all iter_all;
+	bool uptodate = !bio->bi_status;
+	u64 file_offset = page_offset(bio_first_page_all(bio)) +
+			  bio_first_bvec_all(bio)->bv_offset;
 	/*
 	 * The offset to the beginning of a bio, since one bio can never be
 	 * larger than UINT_MAX, u32 here is enough.
 	 */
 	u32 bio_offset = 0;
-	struct bvec_iter_all iter_all;
 
 	ASSERT(!bio_flagged(bio, BIO_CLONED));
 	bio_for_each_segment_all(bvec, bio, iter_all) {
-		bool uptodate = !bio->bi_status;
 		struct page *page = bvec->bv_page;
-		struct inode *inode = page->mapping->host;
 		struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
 		const u32 sectorsize = fs_info->sectorsize;
 		u64 start;
@@ -742,17 +674,16 @@ static void end_bio_extent_readpage(struct btrfs_bio *bbio)
 			}
 		}
 
-		/* Update page status and unlock. */
+		/* Update page status. */
 		end_page_read(page, uptodate, start, len);
-		endio_readpage_release_extent(&processed, BTRFS_I(inode),
-					      start, end, uptodate);
 
 		ASSERT(bio_offset + len > bio_offset);
 		bio_offset += len;
 
 	}
-	/* Release the last extent */
-	endio_readpage_release_extent(&processed, NULL, 0, 0, false);
+	/* Unlock the extent io tree. */
+	unlock_extent(&BTRFS_I(inode)->io_tree, file_offset,
+		      file_offset + bio_offset - 1, NULL);
 	bio_put(bio);
 }
 
-- 
2.40.1

Re: [PATCH v2 1/3] btrfs: make alloc_extent_buffer() handle previously uptodate range more efficient for subpage

[Christoph Hellwig]

On Tue, May 30, 2023 at 09:45:27AM +0800, Qu Wenruo wrote:
> Currently alloc_extent_buffer() would make the extent buffer uptodate if
> the corresponding pages are also uptodate.
> 
> But this check is only checking PageUptodate, which is fine for regular
> cases, but not for subpage cases, as we can have multiple extent buffers
> in the same page.
> 
> So here we go btrfs_page_test_uptodate() instead.
> 
> The old code doesn't cause any problem, but not efficient, as it would
> cause extra metadata read even if the range is already uptodate.

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH v2 2/3] btrfs: use the same @uptodate variable for end_bio_extent_readpage()

[Christoph Hellwig]

On Tue, May 30, 2023 at 09:45:28AM +0800, Qu Wenruo wrote:
> In function end_bio_extent_readpage() we call
> endio_readpage_release_extent() to unlock the extent io tree.
> 
> However we pass PageUptodate(page) as @uptodate parameter for it, while
> for previous end_page_read() call, we use a dedicated @uptodate local
> variable.
> 
> This is not a big deal, as even for subpage cases, either the bio only
> covers part of the page, then the @uptodate is always false, and the
> subpage ranges can still be merged.
> 
> But for the sake of consistency, always use @uptodate variable when
> possible.

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH v2 3/3] btrfs: remove processed_extent infrastructure

[Christoph Hellwig]

I recently posted an equivalent patch:

https://www.spinics.net/lists/linux-btrfs/msg134188.html

but it turns out this actually breaks with compression enabled, where
due to the ompression we can get gaps in the logical addresses.  IIRC
you had to do an auto run with -o compress to catch it.

Re: [PATCH v2 2/3] btrfs: use the same @uptodate variable for end_bio_extent_readpage()

[Anand Jain]

On 30/5/23 09:45, Qu Wenruo wrote:
> In function end_bio_extent_readpage() we call
> endio_readpage_release_extent() to unlock the extent io tree.
> 
> However we pass PageUptodate(page) as @uptodate parameter for it, while
> for previous end_page_read() call, we use a dedicated @uptodate local
> variable.
> 
> This is not a big deal, as even for subpage cases, either the bio only
> covers part of the page, then the @uptodate is always false, and the
> subpage ranges can still be merged.
> 
> But for the sake of consistency, always use @uptodate variable when
> possible.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>

LGTM
Reviewed-by: Anand Jain <anand.jain@oracle.com>


> ---
>   fs/btrfs/extent_io.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index 9afdcf0c70dd..2d228cc8b401 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -745,7 +745,7 @@ static void end_bio_extent_readpage(struct btrfs_bio *bbio)
>   		/* Update page status and unlock. */
>   		end_page_read(page, uptodate, start, len);
>   		endio_readpage_release_extent(&processed, BTRFS_I(inode),
> -					      start, end, PageUptodate(page));
> +					      start, end, uptodate);
>   
>   		ASSERT(bio_offset + len > bio_offset);
>   		bio_offset += len;

Re: [PATCH v2 3/3] btrfs: remove processed_extent infrastructure

[Qu Wenruo]

On 2023/5/30 13:43, Christoph Hellwig wrote:
> I recently posted an equivalent patch:
>
> https://www.spinics.net/lists/linux-btrfs/msg134188.html
>
> but it turns out this actually breaks with compression enabled, where
> due to the ompression we can get gaps in the logical addresses.  IIRC
> you had to do an auto run with -o compress to catch it.
>
Great thanks for the mentioning on the existing patch.

However I'm still not sure why compression can be broken.

Yes, data read would also use the same endio hook for both compressed
and regular reads.

But that endio hook is always handling file pages, thus the range it's
handling should always be continuous.
Or did I miss something?

Thanks,
Qu

Re: [PATCH v2 0/3] btrfs: small cleanups mostly for subpage cases

[David Sterba]

On Tue, May 30, 2023 at 09:45:26AM +0800, Qu Wenruo wrote:
> Changelog:
> v2:
> - Fix an offset-by-one bug in the 3rd patch
>   Unlock extent range end is inclusive.
> 
> During my hunt on the subpage uptodate desync bugs reported from Matt, I
> exposed several PageUptodate usage which results inefficiency for
> subpage cases.
> 
> Those two are fixed in the first two patches.
> 
> Furthermore I found processed_extent infrastructure is no longer needed
> especially after all the csum verification is moved to storage layer (or
> bio.c inside btrfs), we can easily unlock the full range without the
> need for the infrastructure.
> 
> Thus the last patch would delete the processed_extent infrastructure
> completely.
> 
> Qu Wenruo (3):
>   btrfs: make alloc_extent_buffer() handle previously uptodate range
>     more efficient for subpage
>   btrfs: use the same @uptodate variable for end_bio_extent_readpage()

1 and 2 added to devel.

>   btrfs: remove processed_extent infrastructure

As Christoph pointed out this needs some investigation.

Re: [PATCH v2 3/3] btrfs: remove processed_extent infrastructure

[Qu Wenruo]

On 2023/5/30 14:19, Qu Wenruo wrote:
>
>
> On 2023/5/30 13:43, Christoph Hellwig wrote:
>> I recently posted an equivalent patch:
>>
>> https://www.spinics.net/lists/linux-btrfs/msg134188.html
>>
>> but it turns out this actually breaks with compression enabled, where
>> due to the ompression we can get gaps in the logical addresses.  IIRC
>> you had to do an auto run with -o compress to catch it.
>>
> Great thanks for the mentioning on the existing patch.
>
> However I'm still not sure why compression can be broken.
>
> Yes, data read would also use the same endio hook for both compressed
> and regular reads.
>
> But that endio hook is always handling file pages, thus the range it's
> handling should always be continuous.
> Or did I miss something?

All my bad, indeed we can submit a btrfs_bio that contains two or more
ranges which are not continuous, for compressed read.

For compressed read, we always use the the on-disk bytenr as the
@disk_bytenr for submit_extent_page().

Which means, if we have the following file layout, we will submit just
one btrfs_bio:

	item 6 key (257 EXTENT_DATA 0) itemoff 15816 itemsize 53
		generation 7 type 1 (regular)
		extent data disk byte 13631488 nr 4096
		extent data offset 0 nr 4096 ram 32768
		extent compression 1 (zlib)
	item 7 key (257 EXTENT_DATA 4096) itemoff 15763 itemsize 53
		generation 8 type 1 (regular)
		extent data disk byte 0 nr 0
		extent data offset 0 nr 4096 ram 4096
		extent compression 0 (none)
	item 8 key (257 EXTENT_DATA 8192) itemoff 15710 itemsize 53
		generation 7 type 1 (regular)
		extent data disk byte 13631488 nr 4096
		extent data offset 8192 nr 24576 ram 32768
		extent compression 1 (zlib)

When reading the whole file, at file range [0, 4k), we will use 13631488
as @disk_bytenr, but at this stage we don't submit the btrfs_bio yet.

Then we hit the whole, just zeroing the pages.

Finally we hit the range [8K, 32K), we still use 13631488 as
@disk_bytenr, and since we have a btrfs_bio with the same disk_bytenr
and it's compressed read, we merge it.

This results the btrfs_bio containing two ranges, [0, 4K) and [8K 32K).

The objective looks like an optimization, if we submit two btrfs_bio for
the two ranges, we will read the compressed extents twice, and do
de-compression twice.

Although this is anti-instinct, it at least has a valid reason.

But I can argue that, this only works for holes, as if the range [4K,
8K) is not a hole, then we still need to submit two different btrfs_bio
for [0, 4K) and [8K, 32K).

Maybe it's time for us to determine whether the behavior is worthy.

Thanks,
Qu

>
> Thanks,
> Qu

Re: [PATCH v2 3/3] btrfs: remove processed_extent infrastructure

[Christoph Hellwig]

On Wed, May 31, 2023 at 07:35:44AM +0800, Qu Wenruo wrote:
> The objective looks like an optimization, if we submit two btrfs_bio for
> the two ranges, we will read the compressed extents twice, and do
> de-compression twice.
> 
> Although this is anti-instinct, it at least has a valid reason.
> 
> But I can argue that, this only works for holes, as if the range [4K,
> 8K) is not a hole, then we still need to submit two different btrfs_bio
> for [0, 4K) and [8K, 32K).
> 
> Maybe it's time for us to determine whether the behavior is worthy.

To me this behaviour looks reaѕonable and worthwhile, but a comment
in end_bio_extent_readpage would probably have saved both of us a
fair amount of time..