Re: [PATCH v2] btrfs: remove an ASSERT() which can cause false alert

David Sterba
2022-04-19T20:38:53Z

On Thu, Feb 17, 2022 at 05:49:34PM +0800, Qu Wenruo wrote:
> [BUG]
> Su reported that with his VM running on an M1 mac, it has a high chance
> to trigger the following ASSERT() with scrub and balance test cases:
>
> ASSERT(cache->start == chunk_offset);
> ret = scrub_chunk(sctx, cache, scrub_dev, found_key.offset,
> dev_extent_len);
>
> [CAUSE]
> The ASSERT() is making sure that the block group cache (@cache) and the
> chunk offset from the device extent match.
>
> But the truth is, block group caches and dev extent items are deleted at
> different timing.
>
> For block group caches and items, after btrfs_remove_block_group() they
> will be marked deleted, but the device extent and the chunk item is
> still in dev tree and chunk tree respectively.
>
> The device extents and chunk items are only deleted in
> btrfs_remove_chunk(), which happens either a btrfs_delete_unused_bgs()
> or btrfs_relocate_chunk().
>
> This timing difference leaves a window where we can have a mismatch
> between block group cache and device extent tree, and triggering the
> ASSERT().
>
> [FIX]
>
> - Remove the ASSERT()
>
> - Add a quick exit if our found bg doesn't match @chunk_offset
>
> - Add a comment on the existing checks in scrub_chunk()
> This is the ultimate safenet, as it will iterate through all the stripes
> of the found chunk.
> And only scrub the stripe if it matches both device and physical
> offset.
>
> So even by some how we got a dev extent which is no longer owned
> by the target chunk, we will just skip this chunk completely, without
> any consequence.
>
> Reported-by: Su Yue <l@damenly.su>
> Signed-off-by: Qu Wenruo <wqu@suse.com>

Filipe sent a fix that's slightly different, as you haven't sent an
update I'll take his patch.