On Fri, Nov 15, 2019 at 2:11 AM Qu Wenruo <wqu@xxxxxxxx> wrote:
>
> [BUG]
> When running btrfs/072 with only one online CPU, it has a pretty high
> chance to fail:
>
> btrfs/072 12s ... _check_dmesg: something found in dmesg (see xfstests-dev/results//btrfs/072.dmesg)
> - output mismatch (see xfstests-dev/results//btrfs/072.out.bad)
> --- tests/btrfs/072.out 2019-10-22 15:18:14.008965340 +0800
> +++ /xfstests-dev/results//btrfs/072.out.bad 2019-11-14 15:56:45.877152240 +0800
> @@ -1,2 +1,3 @@
> QA output created by 072
> Silence is golden
> +Scrub find errors in "-m dup -d single" test
> ...
>
> And with the following call trace:
> BTRFS info (device dm-5): scrub: started on devid 1
> ------------[ cut here ]------------
> BTRFS: Transaction aborted (error -27)
> WARNING: CPU: 0 PID: 55087 at fs/btrfs/block-group.c:1890 btrfs_create_pending_block_groups+0x3e6/0x470 [btrfs]
> CPU: 0 PID: 55087 Comm: btrfs Tainted: G W O 5.4.0-rc1-custom+ #13
> Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 0.0.0 02/06/2015
> RIP: 0010:btrfs_create_pending_block_groups+0x3e6/0x470 [btrfs]
> Call Trace:
> __btrfs_end_transaction+0xdb/0x310 [btrfs]
> btrfs_end_transaction+0x10/0x20 [btrfs]
> btrfs_inc_block_group_ro+0x1c9/0x210 [btrfs]
> scrub_enumerate_chunks+0x264/0x940 [btrfs]
> btrfs_scrub_dev+0x45c/0x8f0 [btrfs]
> btrfs_ioctl+0x31a1/0x3fb0 [btrfs]
> do_vfs_ioctl+0x636/0xaa0
> ksys_ioctl+0x67/0x90
> __x64_sys_ioctl+0x43/0x50
> do_syscall_64+0x79/0xe0
> entry_SYSCALL_64_after_hwframe+0x49/0xbe
> ---[ end trace 166c865cec7688e7 ]---
>
> [CAUSE]
> The error number -27 is -EFBIG, returned from the following call chain:
> btrfs_end_transaction()
> |- __btrfs_end_transaction()
> |- btrfs_create_pending_block_groups()
> |- btrfs_finish_chunk_alloc()
> |- btrfs_add_system_chunk()
>
> This happens because we have used up all space of
> btrfs_super_block::sys_chunk_array.
>
> The root cause is, we have the following bad loop of creating tons of
> system chunks:
> 1. The only SYSTEM chunk is being scrubbed
> It's very common to have only one SYSTEM chunk.
> 2. New SYSTEM bg will be allocated
> As btrfs_inc_block_group_ro() will check if we have enough space
> after marking current bg RO. If not, then allocate a new chunk.
> 3. New SYSTEM bg is still empty, will be reclaimed
> During the reclaim, we will mark it RO again.
> 4. That newly allocated empty SYSTEM bg get scrubbed
> We go back to step 2, as the bg is already mark RO but still not
> cleaned up yet.
>
> If the cleaner kthread doesn't get executed fast enough (e.g. only one
> CPU), then we will get more and more empty SYSTEM chunks, using up all
> the space of btrfs_super_block::sys_chunk_array.
>
> [FIX]
> Since scrub/dev-replace doesn't always need to allocate new extent,
> especially chunk tree extent, so we don't really need to do chunk
> pre-allocation.
>
> To break above spiral, here we introduce a new parameter to
> btrfs_inc_block_group(), @do_chunk_alloc, which indicates whether we
> need extra chunk pre-allocation.
>
> For relocation, we pass @do_chunk_alloc=true, while for scrub, we pass
> @do_chunk_alloc=false.
> This should keep unnecessary empty chunks from popping up for scrub.
>
> Also, since there are two parameters for btrfs_inc_block_group_ro(),
> add more comment for it.
>
> Signed-off-by: Qu Wenruo <wqu@xxxxxxxx>
Qu,
Strangely, this has caused some unexpected failures on test btrfs/071
(fsstress + device replace + remount followed by scrub).
Since I hadn't seen the issue in my 5.4 (and older) based branches,
and only started to observe the failure in 5.5-rc2+, I left a vm
bisecting since last week after coming back from vacations.
The bisection points out to this change. And going to 5.5-rc5 and
reverting this change, or just doing:
diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c
index 21de630b0730..87478654a3e1 100644
--- a/fs/btrfs/scrub.c
+++ b/fs/btrfs/scrub.c
@@ -3578,7 +3578,7 @@ int scrub_enumerate_chunks(struct scrub_ctx *sctx,
* thread can't be triggered fast enough, and use up all space
* of btrfs_super_block::sys_chunk_array
*/
- ret = btrfs_inc_block_group_ro(cache, false);
+ ret = btrfs_inc_block_group_ro(cache, true);
scrub_pause_off(fs_info);
if (ret == 0) {
which is simpler then reverting due to conflicts, confirms this patch
is what causes btrfs/071 to fail like this:
$ cat results/btrfs/071.out.bad
QA output created by 071
Silence is golden
Scrub find errors in "-m raid0 -d raid0" test
$ cat results/btrfs/071.full
(...)
Test -m raid0 -d raid0
Run fsstress -p 20 -n 100 -d
/home/fdmanana/btrfs-tests/scratch_1/stressdir -f rexchange=0 -f
rwhiteout=0
Start replace worker: 17813
Wait for fsstress to exit and kill all background workers
seed = 1579455326
dev_pool=/dev/sdd /dev/sde /dev/sdf
free_dev=/dev/sdg, src_dev=/dev/sdd
Replacing /dev/sdd with /dev/sdg
Replacing /dev/sde with /dev/sdd
Replacing /dev/sdf with /dev/sde
Replacing /dev/sdg with /dev/sdf
Replacing /dev/sdd with /dev/sdg
Replacing /dev/sde with /dev/sdd
Replacing /dev/sdf with /dev/sde
Replacing /dev/sdg with /dev/sdf
Replacing /dev/sdd with /dev/sdg
Replacing /dev/sde with /dev/sdd
Scrub the filesystem
ERROR: there are uncorrectable errors
scrub done for 0f63c9b5-5618-4484-b6f2-0b7d3294cf05
Scrub started: Fri Jan 17 12:31:35 2020
Status: finished
Duration: 0:00:00
Total to scrub: 5.02GiB
Rate: 0.00B/s
Error summary: csum=1
Corrected: 0
Uncorrectable: 1
Unverified: 0
Scrub find errors in "-m raid0 -d raid0" test
(...)
And in syslog:
(...)
Jan 9 13:14:15 debian5 kernel: [1739740.727952] BTRFS info (device
sdc): dev_replace from /dev/sde (devid 4) to /dev/sdd started
Jan 9 13:14:15 debian5 kernel: [1739740.752226]
scrub_handle_errored_block: 8 callbacks suppressed
Jan 9 13:14:15 debian5 kernel: [1739740.752228] BTRFS warning (device
sdc): checksum error at logical 1129050112 on dev /dev/sde, physical
277803008, root 5, inode 405, offset 1540096, length 4096, links 1
(path: stressdir/pa/d2/d5/fa)
Jan 9 13:14:15 debian5 kernel: [1739740.752230]
btrfs_dev_stat_print_on_error: 8 callbacks suppressed
Jan 9 13:14:15 debian5 kernel: [1739740.758600] BTRFS error (device
sdc): bdev /dev/sde errs: wr 0, rd 0, flush 0, corrupt 1, gen 0
Jan 9 13:14:15 debian5 kernel: [1739740.908263]
scrub_handle_errored_block: 2 callbacks suppressed
Jan 9 13:14:15 debian5 kernel: [1739740.929814] BTRFS error (device
sdc): unable to fixup (regular) error at logical 1129050112 on dev
/dev/sde
Jan 9 13:14:15 debian5 kernel: [1739740.929817] BTRFS info (device
sdc): dev_replace from /dev/sde (devid 4) to /dev/sdd finished
(...)
>From a quick look I don't have an idea how this patch causes such
regression, doesn't seem to make any sense.
I changed btrfs/071 to comment out the remount loop
(https://pastebin.com/X5LiJtpS, the fsstress arguments are to avoid an
infinite loop during fsync, for which there is a fix already) to see
if that had any influence - it does not.
So it seems the problem is device replace and/or scrub only.
I can trigger the failure somewhat easily, 1000 iterations of
btrfs/071 on 5.5-rc5 produced 94 failures like that.
With this patch reverted (or passing true to
btrfs_inc_block_group_ro() from scrub), 20108 iterations succeeded (no
failures at all).
I haven't tried Josef's recent changes to inc_block_group_ro(), so I
can't say yet if they fix the problem.
I can take a closer look next week to see if I can figure out why this
causes such weird failures, unless you already have an idea.
Thanks.
>
> ---
> This version is based on v5.4-rc1, for a commonly known commit hash.
> It would cause conflicts due to
> btrfs_block_group_cache -> btrfs_block_group refactor.
>
> The conflicts should be easy to solve.
>
> Changelog:
> v2:
> - Fix a bug that previous verion never do chunk pre-allocation.
> - Avoid chunk pre-allocation from check_system_chunk()
> - Use extra parameter @do_chunk_alloc other than new function with
> "_force" suffix.
> - Skip unnecessary update_block_group_flags() call completely for
> do_chunk_alloc=false case.
> ---
> fs/btrfs/block-group.c | 49 +++++++++++++++++++++++++++---------------
> fs/btrfs/block-group.h | 3 ++-
> fs/btrfs/relocation.c | 2 +-
> fs/btrfs/scrub.c | 21 +++++++++++++++++-
> 4 files changed, 55 insertions(+), 20 deletions(-)
>
> diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
> index bf7e3f23bba7..ae23319e4233 100644
> --- a/fs/btrfs/block-group.c
> +++ b/fs/btrfs/block-group.c
> @@ -2021,8 +2021,17 @@ static u64 update_block_group_flags(struct btrfs_fs_info *fs_info, u64 flags)
> return flags;
> }
>
> -int btrfs_inc_block_group_ro(struct btrfs_block_group_cache *cache)
> -
> +/*
> + * Mark one block group ro, can be called several times for the same block
> + * group.
> + *
> + * @cache: The destination block group
> + * @do_chunk_alloc: Whether need to do chunk pre-allocation, this is to
> + * ensure we still have some free space after marking this
> + * block group RO.
> + */
> +int btrfs_inc_block_group_ro(struct btrfs_block_group_cache *cache,
> + bool do_chunk_alloc)
> {
> struct btrfs_fs_info *fs_info = cache->fs_info;
> struct btrfs_trans_handle *trans;
> @@ -2052,25 +2061,30 @@ int btrfs_inc_block_group_ro(struct btrfs_block_group_cache *cache)
> goto again;
> }
>
> - /*
> - * if we are changing raid levels, try to allocate a corresponding
> - * block group with the new raid level.
> - */
> - alloc_flags = update_block_group_flags(fs_info, cache->flags);
> - if (alloc_flags != cache->flags) {
> - ret = btrfs_chunk_alloc(trans, alloc_flags, CHUNK_ALLOC_FORCE);
> + if (do_chunk_alloc) {
> /*
> - * ENOSPC is allowed here, we may have enough space
> - * already allocated at the new raid level to
> - * carry on
> + * if we are changing raid levels, try to allocate a
> + * corresponding block group with the new raid level.
> */
> - if (ret == -ENOSPC)
> - ret = 0;
> - if (ret < 0)
> - goto out;
> + alloc_flags = update_block_group_flags(fs_info, cache->flags);
> + if (alloc_flags != cache->flags) {
> + ret = btrfs_chunk_alloc(trans, alloc_flags,
> + CHUNK_ALLOC_FORCE);
> + /*
> + * ENOSPC is allowed here, we may have enough space
> + * already allocated at the new raid level to
> + * carry on
> + */
> + if (ret == -ENOSPC)
> + ret = 0;
> + if (ret < 0)
> + goto out;
> + }
> }
>
> - ret = inc_block_group_ro(cache, 0);
> + ret = inc_block_group_ro(cache, !do_chunk_alloc);
> + if (!do_chunk_alloc)
> + goto unlock_out;
> if (!ret)
> goto out;
> alloc_flags = btrfs_get_alloc_profile(fs_info, cache->space_info->flags);
> @@ -2085,6 +2099,7 @@ int btrfs_inc_block_group_ro(struct btrfs_block_group_cache *cache)
> check_system_chunk(trans, alloc_flags);
> mutex_unlock(&fs_info->chunk_mutex);
> }
> +unlock_out:
> mutex_unlock(&fs_info->ro_block_group_mutex);
>
> btrfs_end_transaction(trans);
> diff --git a/fs/btrfs/block-group.h b/fs/btrfs/block-group.h
> index c391800388dd..0758e6d52acb 100644
> --- a/fs/btrfs/block-group.h
> +++ b/fs/btrfs/block-group.h
> @@ -205,7 +205,8 @@ int btrfs_read_block_groups(struct btrfs_fs_info *info);
> int btrfs_make_block_group(struct btrfs_trans_handle *trans, u64 bytes_used,
> u64 type, u64 chunk_offset, u64 size);
> void btrfs_create_pending_block_groups(struct btrfs_trans_handle *trans);
> -int btrfs_inc_block_group_ro(struct btrfs_block_group_cache *cache);
> +int btrfs_inc_block_group_ro(struct btrfs_block_group_cache *cache,
> + bool do_chunk_alloc);
> void btrfs_dec_block_group_ro(struct btrfs_block_group_cache *cache);
> int btrfs_start_dirty_block_groups(struct btrfs_trans_handle *trans);
> int btrfs_write_dirty_block_groups(struct btrfs_trans_handle *trans);
> diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
> index 00504657b602..fd6df6dd5421 100644
> --- a/fs/btrfs/relocation.c
> +++ b/fs/btrfs/relocation.c
> @@ -4325,7 +4325,7 @@ int btrfs_relocate_block_group(struct btrfs_fs_info *fs_info, u64 group_start)
> rc->extent_root = extent_root;
> rc->block_group = bg;
>
> - ret = btrfs_inc_block_group_ro(rc->block_group);
> + ret = btrfs_inc_block_group_ro(rc->block_group, true);
> if (ret) {
> err = ret;
> goto out;
> diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c
> index f7d4e03f4c5d..7e48d65bbdf6 100644
> --- a/fs/btrfs/scrub.c
> +++ b/fs/btrfs/scrub.c
> @@ -3563,7 +3563,26 @@ int scrub_enumerate_chunks(struct scrub_ctx *sctx,
> * -> btrfs_scrub_pause()
> */
> scrub_pause_on(fs_info);
> - ret = btrfs_inc_block_group_ro(cache);
> +
> + /*
> + * Don't do chunk preallocation for scrub.
> + *
> + * This is especially important for SYSTEM bgs, or we can hit
> + * -EFBIG from btrfs_finish_chunk_alloc() like:
> + * 1. The only SYSTEM bg is marked RO.
> + * Since SYSTEM bg is small, that's pretty common.
> + * 2. New SYSTEM bg will be allocated
> + * Due to regular version will allocate new chunk.
> + * 3. New SYSTEM bg is empty and will get cleaned up
> + * Before cleanup really happens, it's marked RO again.
> + * 4. Empty SYSTEM bg get scrubbed
> + * We go back to 2.
> + *
> + * This can easily boost the amount of SYSTEM chunks if cleaner
> + * thread can't be triggered fast enough, and use up all space
> + * of btrfs_super_block::sys_chunk_array
> + */
> + ret = btrfs_inc_block_group_ro(cache, false);
> if (!ret && sctx->is_dev_replace) {
> /*
> * If we are doing a device replace wait for any tasks
> --
> 2.24.0
>
--
Filipe David Manana,
“Whether you think you can, or you think you can't — you're right.”
