Re: [PATCH 2/5] btrfs: extent-tree: Kill BUG_ON() in __btrfs_free_extent() and do better comment

[WenRuo Qu <wqu@suse.com>]

On 2019/7/10 下午6:48, Nikolay Borisov wrote:
> 
> 
> On 10.07.19 г. 11:02 ч., Qu Wenruo wrote:
>> __btrfs_free_extent() is one of the best cases to show how optimization
>> could make a function hard to read.
>>
>> In fact __btrfs_free_extent() is only doing two major works:
>> 1. Reduce the refs number of an extent backref
>>    Either it's an inlined extent backref (inside EXTENT/METADATA item) or
>>    a keyed extent backref (SHARED_* item).
>>    We only need to locate that backref line, either reduce the number or
>>    remove the backref line completely.
>>
>> 2. Update the refs count in EXTENT/METADATA_ITEM
>>
>> But in real world, we do it in a complex but somewhat efficient way.
>> During step 1), we will try to locate the EXTENT/METADATA_ITEM without
>> triggering another btrfs_search_slot() as fast path.
>>
>> Only when we failed to locate that item, we will trigger another
>> btrfs_search_slot() to get that EXTENT/METADATA_ITEM after we
>> updated/deleted the backref line.
>>
>> And we have a lot of restrict check on things like refs_to_drop against
>> extent refs and special case check for single ref extent.
>>
>> All of these results:
>> - 7 BUG_ON()s in a single function
>>   Although all these BUG_ON() are doing correct check, they're super
>>   easy to get triggered for fuzzed images.
>>   It's never a good idea to piss the end user.
>>
>> - Near 300 lines without much useful comments but a lot of hidden
>>   conditions
>>   I believe even the author needs several minutes to recall what the
>>   code is doing
>>   Not to mention a lot of BUG_ON() conditions needs to go back tens of
>>   lines to find out why.
>>
>> This patch address all these problems by:
>> - Introduce two examples to show what __btrfs_free_extent() is doing
>>   One inlined backref case and one keyed case.
>>   Should cover most cases.
>>
>> - Kill all BUG_ON()s with proper error message and optional leaf dump
>>
>> - Add comment to show the overall workflow
>>
>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=202819
>> [ The report triggers one BUG_ON() in __btrfs_free_extent() ]
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
> 
> <snip>
> 
>> @@ -6997,19 +7068,24 @@ static int __btrfs_free_extent(struct btrfs_trans_handle *trans,
>>  	if (owner_objectid < BTRFS_FIRST_FREE_OBJECTID &&
>>  	    key.type == BTRFS_EXTENT_ITEM_KEY) {
>>  		struct btrfs_tree_block_info *bi;
>> -		BUG_ON(item_size < sizeof(*ei) + sizeof(*bi));
>> +		if (unlikely(item_size < sizeof(*ei) + sizeof(*bi))) {
>> +			btrfs_crit(info,
>> +"invalid extent item size for key (%llu, %u, %llu) owner %llu, has %u expect >%lu",
> nit: stray '>'

With values filled, it would be:
"invalid extent item size for key (1048575, 168, 4096) owner 7, has 10
expect >33"

Now you'd see it's not a stray '>'.

Thanks,
Qu
> 
> <snip>
>