Re: [PATCH 1/2] btrfs: clear extent buffer uptodate when we fail to write it

[Nikolay Borisov <nborisov@suse.com>]

On 24.11.21 г. 19:37, Josef Bacik wrote:
> I got dmesg errors on generic/281 on our overnight xfstests.  Looking at
> the history this happens occasionally, with errors like this
> 
> ------------[ cut here ]------------
> WARNING: CPU: 0 PID: 673217 at fs/btrfs/extent_io.c:6848 assert_eb_page_uptodate+0x3f/0x50
> CPU: 0 PID: 673217 Comm: kworker/u4:13 Tainted: G        W         5.16.0-rc2+ #469
> 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:ffffae598230bc60 EFLAGS: 00010246
> RAX: 0017ffffc0002112 RBX: ffffebaec4100900 RCX: 0000000000001000
> RDX: ffffebaec45733c7 RSI: ffffebaec4100900 RDI: ffff9fd98919f340
> RBP: 0000000000000d56 R08: ffff9fd98e300000 R09: 0000000000000000
> R10: 0001207370a91c50 R11: 0000000000000000 R12: 00000000000007b0
> R13: ffff9fd98919f340 R14: 0000000001500000 R15: 0000000001cb0000
> FS:  0000000000000000(0000) GS:ffff9fd9fbc00000(0000) knlGS:0000000000000000
> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 00007f549fcf8940 CR3: 0000000114908004 CR4: 0000000000370ef0
> Call Trace:
> 
>  extent_buffer_test_bit+0x3f/0x70
>  free_space_test_bit+0xa6/0xc0
>  load_free_space_tree+0x1d6/0x430
>  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+0x270/0x5a0
>  worker_thread+0x55/0x3c0
>  ? process_one_work+0x5a0/0x5a0
>  kthread+0x174/0x1a0
>  ? set_kthread_struct+0x40/0x40
>  ret_from_fork+0x1f/0x30
> 
> This happens because we're trying to read from a extent buffer page that
> is !PageUptodate.  This happens because we will clear the page uptodate
> when we have an IO error, but we don't clear the extent buffer uptodate.
> If we do a read later and find this extent buffer we'll think its valid
> and not return an error, and then trip over this warning.
> 
> Fix this by also clearing uptodate on the extent buffer when this
> happens, so that we get an error when we do a btrfs_search_slot() and
> find this block later.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> ---
>  fs/btrfs/extent_io.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index b289d26aca0d..3454cac28389 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -4308,6 +4308,12 @@ static void set_btree_ioerr(struct page *page, struct extent_buffer *eb)
>  	if (test_and_set_bit(EXTENT_BUFFER_WRITE_ERR, &eb->bflags))
>  		return;
>  
> +	/*
> +	 * A read may stumble upon this buffer later, make sure that it gets an
> +	 * error and knows there was an error.
> +	 */
> +	clear_bit(EXTENT_BUFFER_UPTODATE, &eb->bflags);


Is it sufficient to set the flag only on the extent buffer, what about
using clear_extent_buffer_uptodate so that constituent pages also get
their UPTODATE cleared?

Also I can't help but think can't we get rid of the BUFFER_WRITE_ERR
because an error during write is signaled by both !UPTODATE and
BUFFER_WRITE_ERR being set.

Looking at the various call sites of set_btree_ioerr they'd call
set_btree_ioerr when the bio has errored out or if
EXTENT_BUFFER_WRITE_ERR is set but in the latter case set_btree_ioerr is
a noop due to the test_and_set_bit() call in set_btree_ioerr.

> +
>  	/*
>  	 * If we error out, we should add back the dirty_metadata_bytes
>  	 * to make it consistent.
>