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

[Su Yue <l@damenly.su>]

On Thu 17 Feb 2022 at 13:02, Filipe Manana <fdmanana@kernel.org> 
wrote:

> On Thu, Feb 17, 2022 at 1:00 PM Qu Wenruo 
> <quwenruo.btrfs@gmx.com> wrote:
>>
>>
>>
>> On 2022/2/17 20:32, Filipe Manana wrote:
>> > On Thu, Feb 17, 2022 at 11:24 AM Qu Wenruo 
>> > <quwenruo.btrfs@gmx.com> wrote:
>> >>
>> >>
>> >>
>> >> On 2022/2/17 19:14, Filipe Manana wrote:
>> >>> 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.
>> >>>
>> >>> This is not correct, what happens is:
>> >>>
>> >>> 1) btrfs_remove_chunk() will remove the device extent items 
>> >>> and the chunk
>> >>>      item;
>> >>>
>> >>> 2) It then calls btrfs_remove_block_group(). This will not 
>> >>> remove the
>> >>>      extent map if the block group was frozen (due to trim, 
>> >>>      and scrub
>> >>>      itself). But it will remove the block group (struct 
>> >>>      btrfs_block_group)
>> >>>      from the red black tree of block groups, and mark the 
>> >>>      block group
>> >>>      as deleted (set struct btrfs_block_group::removed to 
>> >>>      1).
>> >>>
>> >>>      If the extent map was not removed, meaning the block 
>> >>>      group is frozen,
>> >>>      then no one will be able to create a new block group 
>> >>>      with the same logical
>> >>>      address before the block group is unfrozen (by someone 
>> >>>      who is holding
>> >>>      a reference on it). So a lookup on the red black tree 
>> >>>      will return NULL
>> >>>      for the logical address until the block group is 
>> >>>      unfrozen and its
>> >>>      logical address is reused for a new block group.
>> >>>
>> >>> So the ordering of what you are saying is reversed - the 
>> >>> device extent
>> >>> and chunk items are removed before marking the block group 
>> >>> as deleted.
>> >>>
>> >>>>
>> >>>> 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 is also confusing because it gives a wrong idea of the 
>> >>> ordering.
>> >>> The device extents and chunk items are removed by 
>> >>> btrfs_remove_chunk(),
>> >>> which happens before marking a block group as deleted.
>> >>>
>> >>>>
>> >>>> This timing difference leaves a window where we can have a 
>> >>>> mismatch
>> >>>> between block group cache and device extent tree, and 
>> >>>> triggering the
>> >>>> ASSERT().
>> >>>
>> >>> I would like to see more details on this. The sequence of 
>> >>> steps between
>> >>> two tasks that result in this assertion being triggered.
>> >>>
>> >>>>
>> >>>> [FIX]
>> >>>>
>> >>>> - Remove the ASSERT()
>> >>>>
>> >>>> - Add a quick exit if our found bg doesn't match 
>> >>>> @chunk_offset
>> >>>
>> >>> This shouldn't happen. I would like to know the sequence of 
>> >>> steps
>> >>> between 2 (or more) tasks that leads to that.
>> >>>
>> >>> We are getting the block group with 
>> >>> btrfs_lookup_block_group() at
>> >>> scrub_enumerate_chunks(), that calls 
>> >>> block_group_cache_tree_search()
>> >>> with the "contains" argument set to 1, meaning it can 
>> >>> return a
>> >>> block group that contains the given bytenr but does not 
>> >>> start at
>> >>> that bytenr necessarily.
>> >>>
>> >>> This gives me the idea a small block group was deleted and 
>> >>> then a
>> >>> new one was allocated which starts at a lower logical 
>> >>> offset and
>> >>> includes "chunk_offset" (ends beyond that).
>> >>>
>> >>> We should probably be using 
>> >>> btrfs_lookup_first_block_group() at
>> >>> scrub_enumerate_chunks(), so it looks for a block group 
>> >>> that starts
>> >>> exactly at the given logical address.
>> >>>
>> >>> But anyway, as I read this and see the patch's diff, this 
>> >>> feels a lot
>> >>> like a "bail out if something unexpected happens but we 
>> >>> don't know
>> >>> exactly why/how that is possible".
>> >>
>> >> You're completely correct.
>> >>
>> >> Unfortunately I have no way to reproduce the bug on x86_64 
>> >> VMs, and all
>> >> my ARM VMs are too slow to reproduce.
>> >>
>> >> Furthermore, due to some unrelated bugs, v5.17-rc based 
>> >> kernels all
>> >> failed to boot inside the VMs on Apple M1.
>> >>
>> >> So unfortunately I don't have extra details for now.
>> >>
>> >> Will find a way to reproduce first, then update the patch 
>> >> with better
>> >> comments.
>> >
>> > Was it triggered with some specific test case from fstests?
>>
>> Yes, from a regular run on fstests.
>
> Ok, but which specific test(s)?
>

It is btrfs/074. It was found during early 16k subpage support 
development
of Qu. Unfortunately dmesg is lost and it can't be reproduced from 
old
branch fetched from `git reflog`.

I'd report back if the issue occurs again.

--
Su
>>
>> But that was on an older branch (some older misc-next, which 
>> has the
>> patch but still based on v5.16).
>>
>> The v5.17-rc based branch will not boot inside the ARM vm at 
>> all, and no
>> early boot messages after the EFI routine.
>>
>> Will attack the boot problem first.
>>
>> Thanks,
>> Qu
>> >
>> >>
>> >> Thanks,
>> >> Qu
>> >>
>> >>>
>> >>>>
>> >>>> - 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>
>> >>>> ---
>> >>>> Changelog:
>> >>>> v2:
>> >>>> - Add a new quick exit with extra comments
>> >>>>
>> >>>> - Add a new comment in the existing safenet in 
>> >>>> scrub_chunk()
>> >>>> ---
>> >>>>    fs/btrfs/scrub.c | 21 ++++++++++++++++++++-
>> >>>>    1 file changed, 20 insertions(+), 1 deletion(-)
>> >>>>
>> >>>> diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c
>> >>>> index 11089568b287..1c3ed4720ddd 100644
>> >>>> --- a/fs/btrfs/scrub.c
>> >>>> +++ b/fs/btrfs/scrub.c
>> >>>> @@ -3578,6 +3578,14 @@ static noinline_for_stack int 
>> >>>> scrub_chunk(struct scrub_ctx *sctx,
>> >>>>               goto out;
>> >>>>
>> >>>>       map = em->map_lookup;
>> >>>> +
>> >>>> +    /*
>> >>>> +     * Due to the delayed modification of device tree, 
>> >>>> this chunk
>> >>>> +     * may not own this dev extent.
>> >>>
>> >>> This is confusing. What delayed modification, what do you 
>> >>> mean by it
>> >>> exactly? Same below, with more details.
>> >>>
>> >>>> +     *
>> >>>> +     * So we need to iterate through all stripes, and 
>> >>>> only scrub
>> >>>> +     * the stripe which matches both device and physical 
>> >>>> offset.
>> >>>> +     */
>> >>>>       for (i = 0; i < map->num_stripes; ++i) {
>> >>>>               if (map->stripes[i].dev->bdev == 
>> >>>>               scrub_dev->bdev &&
>> >>>>                   map->stripes[i].physical == dev_offset) 
>> >>>>                   {
>> >>>> @@ -3699,6 +3707,18 @@ int scrub_enumerate_chunks(struct 
>> >>>> scrub_ctx *sctx,
>> >>>>               if (!cache)
>> >>>>                       goto skip;
>> >>>>
>> >>>> +            /*
>> >>>> +             * Since dev extents modification is delayed, 
>> >>>> it's possible
>> >>>> +             * we got a bg which doesn't really own this 
>> >>>> dev extent.
>> >>>
>> >>> Same as before. Too confusing, what is delayed dev extent 
>> >>> modification?
>> >>>
>> >>> Once added, device extents can only be deleted, they are 
>> >>> never modified.
>> >>> I think you are referring to the deletions and the fact we 
>> >>> use the commit
>> >>> roots to find extents, but that should be a more clear 
>> >>> comment, without
>> >>> such level of ambiguity.
>> >>>
>> >>> Thanks.
>> >>>
>> >>>> +             *
>> >>>> +             * Here just do a quick skip, scrub_chunk() 
>> >>>> has a more
>> >>>> +             * comprehensive check in it.
>> >>>> +             */
>> >>>> +            if (cache->start != chunk_offset) {
>> >>>> +                    btrfs_put_block_group(cache);
>> >>>> +                    goto skip;
>> >>>> +            }
>> >>>> +
>> >>>>               if (sctx->is_dev_replace && 
>> >>>>               btrfs_is_zoned(fs_info)) {
>> >>>>                       spin_lock(&cache->lock);
>> >>>>                       if (!cache->to_copy) {
>> >>>> @@ -3822,7 +3842,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
>> >>>>