Re: [PATCH] btrfs: fix setting last_trans for reloc roots

[Filipe Manana <fdmanana@gmail.com>]

On Fri, Apr 10, 2020 at 4:44 PM Josef Bacik <josef@toxicpanda.com> wrote:
>
> I made a mistake with my previous fix, I assumed that we didn't need to
> mess with the reloc roots once we were out of the part of relocation
> where we are actually moving the extents.
>
> The subtle thing that I missed is that btrfs_init_reloc_root() also
> updates the last_trans for the reloc root when we do
> btrfs_record_root_in_trans() for the corresponding fs_root.  I've added
> a comment to make sure future me doesn't make this mistake again.
>
> This showed up as a WARN_ON() in btrfs_copy_root() because our
> last_trans didn't == the current transid.  This could happen if we
> snapshotted a fs root with a reloc root after we set
> rc->create_reloc_tree = 0, but before we actually merge the reloc root.
>
> Fixes: 2abc726ab4b8 ("btrfs: do not init a reloc root if we aren't relocating")
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>

Reviewed-by: Filipe Manana <fdmanana@suse.com>

Worth mentioning that the regression produced the following warning
when running snapshot creation and balance in parallel:

85256.545808] BTRFS info (device sdc): relocating block group 30408704
flags metadata|dup
[85256.551956] ------------[ cut here ]------------
[85256.552852] WARNING: CPU: 0 PID: 12823 at fs/btrfs/ctree.c:191
btrfs_copy_root+0x26f/0x430 [btrfs]
[85256.554407] Modules linked in: btrfs dm_log_writes dm_mod
blake2b_generic xor raid6_pq libcrc32c intel_rapl_msr
intel_rapl_common kvm_intel kvm irqbypass bochs_drm drm_vram_helper
crct10dif_pclmul drm_ttm_helper crc32_pclmul ghash_clmulni_intel ttm
drm_kms_helper aesni_intel crypto_simd cryptd glue_helper drm sg
joydev evdev serio_raw qemu_fw_cfg pcspkr button parport_pc ppdev lp
parport ip_tables x_tables autofs4 ext4 crc32c_generic crc16 mbcache
jbd2 sd_mod t10_pi virtio_scsi ata_generic ata_piix libata
crc32c_intel i2c_piix4 scsi_mod virtio_pci psmouse virtio_ring virtio
e1000 [last unloaded: btrfs]
[85256.563623] CPU: 0 PID: 12823 Comm: btrfs Tainted: G        W
  5.6.0-rc7-btrfs-next-58 #1
[85256.565139] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
BIOS rel-1.12.0-59-gc9ba5276e321-prebuilt.qemu.org 04/01/2014
[85256.567134] RIP: 0010:btrfs_copy_root+0x26f/0x430 [btrfs]
[85256.568068] Code: 5f c3 31 d2 4c 89 fe 48 89 ef e8 6c e9 04 00 44
8b 4c 24 08 e9 4d fe ff ff 48 8b 83 60 05 00 00 49 39 04 24 0f 84 e3
fd ff ff <0f> 0b e9 dc fd ff ff 49 8b 86 b0 04 00 00 48 8b 00 48 39 07
0f 84
[85256.571282] RSP: 0018:ffffb96e044279b8 EFLAGS: 00010202
[85256.572193] RAX: 0000000000000009 RBX: ffff9da70bf61000 RCX: ffffb96e04427a48
[85256.573422] RDX: ffff9da733a770c8 RSI: ffff9da70bf61000 RDI: ffff9da694163818
[85256.574657] RBP: ffff9da733a770c8 R08: fffffffffffffff8 R09: 0000000000000002
[85256.575887] R10: ffffb96e044279a0 R11: 0000000000000000 R12: ffff9da694163818
[85256.577122] R13: fffffffffffffff8 R14: ffff9da6d2512000 R15: ffff9da714cdac00
[85256.578352] FS:  00007fdeacf328c0(0000) GS:ffff9da735e00000(0000)
knlGS:0000000000000000
[85256.579741] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[85256.580739] CR2: 000055a2a5b8a118 CR3: 00000001eed78002 CR4: 00000000003606f0
[85256.581971] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[85256.583200] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[85256.584431] Call Trace:
[85256.584887]  ? create_reloc_root+0x49/0x2b0 [btrfs]
[85256.585734]  ? kmem_cache_alloc_trace+0xe5/0x200
[85256.586552]  create_reloc_root+0x8b/0x2b0 [btrfs]
[85256.587387]  btrfs_reloc_post_snapshot+0x96/0x5b0 [btrfs]
[85256.588340]  create_pending_snapshot+0x610/0x1010 [btrfs]
[85256.589293]  create_pending_snapshots+0xa8/0xd0 [btrfs]
[85256.590212]  btrfs_commit_transaction+0x4c7/0xc50 [btrfs]
[85256.591166]  ? btrfs_mksubvol+0x3cd/0x560 [btrfs]
[85256.592006]  btrfs_mksubvol+0x455/0x560 [btrfs]
[85256.592640]  __btrfs_ioctl_snap_create+0x15f/0x190 [btrfs]
[85256.593336]  btrfs_ioctl_snap_create_v2+0xa4/0xf0 [btrfs]
[85256.594008]  ? mem_cgroup_commit_charge+0x6e/0x540
[85256.594615]  btrfs_ioctl+0x12d8/0x3760 [btrfs]
[85256.595171]  ? do_raw_spin_unlock+0x49/0xc0
[85256.595695]  ? _raw_spin_unlock+0x29/0x40
[85256.596198]  ? __handle_mm_fault+0x11b3/0x14b0
[85256.596757]  ? ksys_ioctl+0x92/0xb0
[85256.597194]  ksys_ioctl+0x92/0xb0
[85256.597610]  ? trace_hardirqs_off_thunk+0x1a/0x1c
[85256.598194]  __x64_sys_ioctl+0x16/0x20
[85256.598662]  do_syscall_64+0x5c/0x280
[85256.599120]  entry_SYSCALL_64_after_hwframe+0x49/0xbe
[85256.599752] RIP: 0033:0x7fdeabd3bdd7

This is actually very easy to trigger after the patchset I just sent
for fstests, that fixes btrfs/014.

Thanks.

> ---
>  fs/btrfs/relocation.c | 19 +++++++++++++++++--
>  1 file changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
> index d4734337127a..76bfb524bf3e 100644
> --- a/fs/btrfs/relocation.c
> +++ b/fs/btrfs/relocation.c
> @@ -830,8 +830,7 @@ int btrfs_init_reloc_root(struct btrfs_trans_handle *trans,
>         int clear_rsv = 0;
>         int ret;
>
> -       if (!rc || !rc->create_reloc_tree ||
> -           root->root_key.objectid == BTRFS_TREE_RELOC_OBJECTID)
> +       if (!rc)
>                 return 0;
>
>         /*
> @@ -841,12 +840,28 @@ int btrfs_init_reloc_root(struct btrfs_trans_handle *trans,
>         if (reloc_root_is_dead(root))
>                 return 0;
>
> +       /*
> +        * This is subtle but important.  We do not do
> +        * record_root_in_transaction for reloc roots, instead we record their
> +        * corresponding fs root, and then here we update the last trans for the
> +        * reloc root.  This means that we have to do this for the entire life
> +        * of the reloc root, regardless of which stage of the relocation we are
> +        * in.
> +        */
>         if (root->reloc_root) {
>                 reloc_root = root->reloc_root;
>                 reloc_root->last_trans = trans->transid;
>                 return 0;
>         }
>
> +       /*
> +        * We are merging reloc roots, we do not need new reloc trees.  Also
> +        * reloc trees never need their own reloc tree.
> +        */
> +       if (!rc->create_reloc_tree ||
> +           root->root_key.objectid == BTRFS_TREE_RELOC_OBJECTID)
> +               return 0;
> +
>         if (!trans->reloc_reserved) {
>                 rsv = trans->block_rsv;
>                 trans->block_rsv = rc->block_rsv;
> --
> 2.24.1
>


-- 
Filipe David Manana,

“Whether you think you can, or you think you can't — you're right.”