[PATCH 0/2] Fix another !PageUptodate related warning

[PATCH 0/2] Fix another !PageUptodate related warning

[Josef Bacik]

We hit a warning in generic/281 in our overnight testing.  This is the same
error that I thought I fixed with c2e39305299f01 ("btrfs: clear extent buffer
uptodate when we fail to write it"), however all I did was make the race window
much smaller.

The race is relatively simple

Task 1					Task 2
switch_commit_roots()
					btrfs_search_slot(path->search_commit_root)
btrfs_write_and_wait_transaction()
					start processing path->nodes[0]
write failure
end_bio_extent_buffer_writepage
	ClearPageUptodate()
					try to read from the extent buffer
						trigger warning

There's no real way to stop this without adding some heavy handed locking in
here to make sure we don't invalidate an extent buffer while we're reading it.
And we can't really add more locking because this particular path uses
->skip_locking, so we'd have to be very intentional about what we're wanting to
do.

To fix this we need two things.  First is to be consistent with how we use
PageError.  Everybody uses it for writes, and in fact that's how we use it with
the exception of one error path on the extent buffer read.  So first fix that so
we don't ever have PageError without a write error.

Secondly change assert_eb_page_uptodate() to only WARN_ON if !Uptodate &&
!Error, so we get a warning if we didn't properly read an extent buffer, but not
if we failed to write the buffer out.  With this I'm now able to run 100 runs of
generic/281 without warnings, whereas before it reproduced in around 10 runs.
Thanks,

Josef


Josef Bacik (2):
  btrfs: do not SetPageError on a read error for extent buffers
  btrfs: do not WARN_ON() if we have PageError set

 fs/btrfs/extent_io.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

-- 
2.26.3

[PATCH 1/2] btrfs: do not SetPageError on a read error for extent buffers

[Josef Bacik]

For reads the page is marked !uptodate and that indicates that there was
a problem.  We should only be using SetPageError for write errors, and
in fact do that everywhere except for extent buffer reads.  Fix this so
we maintain that PageError == write error.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/extent_io.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 16b6820c913d..bb3c29984fcd 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -6671,7 +6671,6 @@ int read_extent_buffer_pages(struct extent_buffer *eb, int wait, int mirror_num)
 				 * i.e unlock page/set error bit.
 				 */
 				ret = err;
-				SetPageError(page);
 				unlock_page(page);
 				atomic_dec(&eb->io_pages);
 			}
-- 
2.26.3

[PATCH 2/2] btrfs: do not WARN_ON() if we have PageError set

[Josef Bacik]

Whenever we do any extent buffer operations we call
assert_eb_page_uptodate() to complain loudly if we're operating on an
non-uptodate page.  Our overnight tests caught this warning earlier this
week

WARNING: CPU: 1 PID: 553508 at fs/btrfs/extent_io.c:6849 assert_eb_page_uptodate+0x3f/0x50
CPU: 1 PID: 553508 Comm: kworker/u4:13 Tainted: G        W         5.17.0-rc3+ #564
Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.13.0-2.fc32 04/01/2014
Workqueue: btrfs-cache btrfs_work_helper
RIP: 0010:assert_eb_page_uptodate+0x3f/0x50
RSP: 0018:ffffa961440a7c68 EFLAGS: 00010246
RAX: 0017ffffc0002112 RBX: ffffe6e74453f9c0 RCX: 0000000000001000
RDX: ffffe6e74467c887 RSI: ffffe6e74453f9c0 RDI: ffff8d4c5efc2fc0
RBP: 0000000000000d56 R08: ffff8d4d4a224000 R09: 0000000000000000
R10: 00015817fa9d1ef0 R11: 000000000000000c R12: 00000000000007b1
R13: ffff8d4c5efc2fc0 R14: 0000000001500000 R15: 0000000001cb1000
FS:  0000000000000000(0000) GS:ffff8d4dbbd00000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007ff31d3448d8 CR3: 0000000118be8004 CR4: 0000000000370ee0
Call Trace:

 extent_buffer_test_bit+0x3f/0x70
 free_space_test_bit+0xa6/0xc0
 load_free_space_tree+0x1f6/0x470
 caching_thread+0x454/0x630
 ? rcu_read_lock_sched_held+0x12/0x60
 ? rcu_read_lock_sched_held+0x12/0x60
 ? rcu_read_lock_sched_held+0x12/0x60
 ? lock_release+0x1f0/0x2d0
 btrfs_work_helper+0xf2/0x3e0
 ? lock_release+0x1f0/0x2d0
 ? finish_task_switch.isra.0+0xf9/0x3a0
 process_one_work+0x26d/0x580
 ? process_one_work+0x580/0x580
 worker_thread+0x55/0x3b0
 ? process_one_work+0x580/0x580
 kthread+0xf0/0x120
 ? kthread_complete_and_exit+0x20/0x20
 ret_from_fork+0x1f/0x30

This was partially fixed by c2e39305299f01 ("btrfs: clear extent buffer
uptodate when we fail to write it"), however all that fix did was keep
us from finding extent buffers after a failed writeout.  It didn't keep
us from continuing to use a buffer that we already had found.

In this case we're searching the commit root to cache the block group,
so we can start committing the transaction and switch the commit root
and then start writing.  After the switch we can look up an extent
buffer that hasn't been written yet and start processing that block
group.  Then we fail to write that block out and clear Uptodate on the
page, and then we start spewing these errors.

Normally we're protected by the tree lock to a certain degree here.  If
we read a block we have that block read locked, and we block the writer
from locking the block before we submit it for the write.  However this
isn't necessarily fool proof because the read could happen before we do
the submit_bio and after we locked and unlocked the extent buffer.

Also in this particular case we have path->skip_locking set, so that
won't save us here.  We'll simply get a block that was valid when we
read it, but became invalid while we were using it.

What we really want is to catch the case where we've "read" a block but
it's not marked Uptodate.  We do not set PageError() on failed reads,
they're just not marked as Uptodate.  We do however set PageError() when
we fail to write.

Fix this by checking !Uptodate && !Error, this way we will not complain
if our buffer gets invalidated while we're using it, and we'll maintain
the spirit of the check which is to make sure we have a fully in-cache
block while we're messing with it.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/extent_io.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index bb3c29984fcd..28b99fecda77 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -6847,14 +6847,25 @@ static void assert_eb_page_uptodate(const struct extent_buffer *eb,
 {
 	struct btrfs_fs_info *fs_info = eb->fs_info;
 
+	/*
+	 * If we are using the commit root we could potentially clear a page
+	 * Uptodate while we're using the extent buffer that we've previously
+	 * looked up.  We don't want to complain in this case, as the page was
+	 * valid before, we just didn't write it out.  Instead we want to catch
+	 * the case where we didn't actually read the block properly, which
+	 * would have !PageUptodate && !PageError, as we clear PageError before
+	 * reading.
+	 */
 	if (fs_info->sectorsize < PAGE_SIZE) {
-		bool uptodate;
+		bool uptodate, error;
 
 		uptodate = btrfs_subpage_test_uptodate(fs_info, page,
 						       eb->start, eb->len);
-		WARN_ON(!uptodate);
+		error = btrfs_subpage_test_error(fs_info, page, eb->start,
+						 eb->len);
+		WARN_ON(!uptodate && !error);
 	} else {
-		WARN_ON(!PageUptodate(page));
+		WARN_ON(!PageUptodate(page) && !PageError(page));
 	}
 }
 
-- 
2.26.3

Re: [PATCH 1/2] btrfs: do not SetPageError on a read error for extent buffers

[Filipe Manana]

On Fri, Feb 11, 2022 at 11:24:38AM -0500, Josef Bacik wrote:
> For reads the page is marked !uptodate and that indicates that there was
> a problem.  We should only be using SetPageError for write errors, and
> in fact do that everywhere except for extent buffer reads.  

We are also doing that for data page reads, at end_page_read() - we clear
the uptodate bit from the page and then set the error bit as well. So it
needs to be fixed as well.

> Fix this so
> we maintain that PageError == write error.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> ---
>  fs/btrfs/extent_io.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index 16b6820c913d..bb3c29984fcd 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -6671,7 +6671,6 @@ int read_extent_buffer_pages(struct extent_buffer *eb, int wait, int mirror_num)
>  				 * i.e unlock page/set error bit.
>  				 */
>  				ret = err;
> -				SetPageError(page);

The comment above also needs to be upodated, and it mentions we
must set the error bit on the page.

Thanks.

>  				unlock_page(page);
>  				atomic_dec(&eb->io_pages);
>  			}
> -- 
> 2.26.3
>