Re: [PATCH v3 3/5] btrfs: Detect unbalanced tree with empty leaf before crashing btree operations

[Qu Wenruo <wqu@suse.com>]

On 2020/8/12 上午9:50, Josef Bacik wrote:
> On 8/11/20 8:29 PM, Qu Wenruo wrote:
>>
>>
>> On 2020/8/12 上午8:23, Josef Bacik wrote:
>>> On 8/11/20 7:04 PM, Qu Wenruo wrote:
>>>>
>>>>
>> [...]
>>>>> Which I assume is the problem?  The generation is 19, is that >
>>>>> last_trans_committed?  Seems like this check just needs to be moved
>>>>> lower, right?  Thanks,
>>>>
>>>> Nope, that generation 19 is valid. That fs has a higher generation, so
>>>> that's completely valid.
>>>>
>>>> The generation 19 is there because there is another csum leaf whose
>>>> generation is 19.
>>>>
>>>
>>> Then this patch does nothing, because we already have this check lower,
>>> so how exactly did it make the panic go away?  Thanks,
>>>
>>> Josef
>>
>> Sorry, I don't get your point.
>>
>> The generation 19 isn't larger than last_trans_committed, so that check
>> has nothing to do with this case.
>>
>> And then it goes to the header_nritems() check, which is 0, and with
>> first_key present, which is invalid and we error out, rejecting the
>> corrupted leaf.
>>
>> What's the problem then?
> 
> 
>         /* We have @first_key, so this @eb must have at least one item */
>         if (btrfs_header_nritems(eb) == 0) {
>                 btrfs_err(fs_info,
>                 "invalid tree nritems, bytenr=%llu nritems=0 expect >0",
>                           eb->start);
>                 WARN_ON(IS_ENABLED(CONFIG_BTRFS_DEBUG));
>                 return -EUCLEAN;
>         }
> 
>         /*
>          * For live tree block (new tree blocks in current transaction),
>          * we need proper lock context to avoid race, which is
> impossible here.
>          * So we only checks tree blocks which is read from disk, whose
>          * generation <= fs_info->last_trans_committed.
>          */
>         if (btrfs_header_generation(eb) > fs_info->last_trans_committed)
>                 return 0;
> 
>         /* We have @first_key, so this @eb must have at least one item */
>         if (btrfs_header_nritems(eb) == 0) {
>                 btrfs_err(fs_info,
>                 "invalid tree nritems, bytenr=%llu nritems=0 expect >0",
>                           eb->start);
>                 WARN_ON(IS_ENABLED(CONFIG_BTRFS_DEBUG));
>                 return -EUCLEAN;
>         }
> 
> 
> This is the code, you have the exact same check above the header
> generation thing, and that's not the problem, so I don't understand why
> you have added this check.

Oh, you're right.

A bad auto merge, which we have merged the same patch before...

Thanks for spotting this!
Qu
> 
> Josef
>