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

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

[Qu Wenruo]

[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]
Just remove the ASSERT().

In fact there are several checks in scrub_chunk() to skip such device
extents.
In scrub_chunk() we do an chunk mapping search, and handling (!em) or
(em->start != bg->start) case by simply exit with ret = 0.

So the ASSERT() is really not necessary.

Please fold this fix into the patch "btrfs: scrub: cleanup the argument
list of scrub_chunk()".

Reported-by: Su Yue <l@damenly.su>
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/scrub.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c
index 11089568b287..9c6cf1149f08 100644
--- a/fs/btrfs/scrub.c
+++ b/fs/btrfs/scrub.c
@@ -3822,7 +3822,6 @@ int scrub_enumerate_chunks(struct scrub_ctx *sctx,
 		dev_replace->item_needs_writeback = 1;
 		up_write(&dev_replace->rwsem);
 
-		ASSERT(cache->start == chunk_offset);
 		ret = scrub_chunk(sctx, cache, scrub_dev, found_key.offset,
 				  dev_extent_len);
 
-- 
2.35.1

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

[Qu Wenruo]

On 2022/2/15 12:46, 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]
> Just remove the ASSERT().
> 
> In fact there are several checks in scrub_chunk() to skip such device
> extents.
> In scrub_chunk() we do an chunk mapping search, and handling (!em) or
> (em->start != bg->start) case by simply exit with ret = 0.
> 
> So the ASSERT() is really not necessary.
> 
> Please fold this fix into the patch "btrfs: scrub: cleanup the argument
> list of scrub_chunk()".
> 
> Reported-by: Su Yue <l@damenly.su>
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>   fs/btrfs/scrub.c | 1 -
>   1 file changed, 1 deletion(-)
> 
> diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c
> index 11089568b287..9c6cf1149f08 100644
> --- a/fs/btrfs/scrub.c
> +++ b/fs/btrfs/scrub.c
> @@ -3822,7 +3822,6 @@ int scrub_enumerate_chunks(struct scrub_ctx *sctx,
>   		dev_replace->item_needs_writeback = 1;
>   		up_write(&dev_replace->rwsem);
>   
> -		ASSERT(cache->start == chunk_offset);

This check still has some sense, as we expect the bg cache to match with 
the dev extent.

>   		ret = scrub_chunk(sctx, cache, scrub_dev, found_key.offset,
>   				  dev_extent_len);

And since we have removed chunk_offset passed to scrub_chunk(), we have 
an extra check before we call scrub_stripe() against found_key.offset.

So removing the ASSERT() is still OK.

But it would be better to replace the ASSERT() with a full check between 
cache->start and chunk_offset.

I'd better find a way to reproduce the case, which I can not reproduce 
at all under x86_64 vms.

Thanks,
Qu

>