On 2019/7/25 下午4:39, Nikolay Borisov wrote:
>
>
> On 25.07.19 г. 9:12 ч., 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 <w...@suse.com>
>
> Changes look simple enough and benign, only thing I wonder is if we
> should force the system in RO mode? In any case:
For the forced RO, it's a must.
Any error here already means extent tree is corrupted, we can't ignore
such serious error.
For the timing of forced RO, it doesn't really matter whether we abort
trans in this function or not.
As in __btrfs_run_delayed_refs() we will abort transaction anyway.
Thanks,
Qu
>
> Reviewed-by: Nikolay Borisov <nbori...@suse.com>
>
