Re: [PATCH] btrfs: extent_io: Handle memory allocation failure in __clear_extent_bit()

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

On 2019/4/18 下午7:54, Qu Wenruo wrote:
>
>
> On 2019/4/18 下午7:51, Qu Wenruo wrote:
>>
>>
>> On 2019/4/18 下午7:38, David Sterba wrote:
>>> On Thu, Apr 18, 2019 at 03:30:20PM +0800, Qu Wenruo wrote:
>>>>
>>>>
>>>> On 2019/4/18 下午3:24, Nikolay Borisov wrote:
>>>>>
>>>>>
>>>>> On 18.04.19 г. 10:21 ч., Qu Wenruo wrote:
>>>>>> There is a BUG_ON() in __clear_extent_bit() for memory allocation
>>>>>> failure.
>>>>>>
>>>>>> While comment of __clear_extent_bit() says it can return error, but we
>>>>>> always return 0.
>>>>>>
>>>>>> Some __clear_extent_bit() callers just ignore the return value, while
>>>>>> some still expect error.
>>>>>>
>>>>>> Let's return proper error for this memory allocation anyway, to remove
>>>>>> that BUG_ON() as a first step, so at least we can continue test.
>>>>>
>>>>> I remember Josef did some changes into this code and said that prealloc
>>>>> shouldn't fail because this will cause mayhem down the road i.e. proper
>>>>> error handling is missing. If anything I think it should be added first
>>>>> and then remove the BUG_ONs.
>>>>
>>>> That's true, we could have some strange lockup due to
>>>> lock_extent_bits(), as if some clear_extent_bits() failed due to ENOMEM
>>>> and caller just ignore the error, we could have a lockup.
>>>
>>> Not only lockup but unhandled failed extent range locking totally breaks
>>> assumptions that the following code makes and this would lead to
>>> unpredictable corruptions. Just count how many lock_extent_bits calls
>>> are there. And any caller of __set_extent_bit. There are so many that
>>> the BUG_ON is the measure of last resort to prevent worse problems.
>>>
>>>> I'll try to pre-allocate certain amount of extent_state as the last
>>>> chance of redemption.
>>>
>>> This only lowers the chances to hit the allocation error but there's
>>> always a case when certain amount + 1 would be needed.
>>
>> Lower chance is already good enough (TM) for low possibility (0.001)
>> error injection.
>>
>> And, for real world low memory case, lower chance in btrfs means higher
>> chance in other subsystem, less chance user will blame btrfs. :)
>>
>>>
>>>> Anyway, such BUG_ON() right after kmalloc() is really a blockage for
>>>> error injection test.
>>>
>>> Maybe, but the code is not yet in the state to inject memory allocation
>>> faiulres to that particular path (ie. the state changes).
>>
>> With last-chance reservation, we can make state related memory
>> allocation almost always to success even memory allocation failure
>> injected (if the possibility is low and low concurrency)
>> And the last-chance reservation can be configured at compile/module load
>> time, making it flex enough for most cases.
>
> Forgot to mention, for that method, I'll definitely keep the BUG_ON() on
> @prealloc.
>
> Just make the allocation part fall back to use fs_info->last_chance[] to
> grab a valid memory slot.

This makes no sense in fact, for bcc inject tool, modify the predicts
can filter out the NOFAIL case, so all of my previous last-chance method
just makes no sense anyway.

I'm re-testing using the refined predicates: '(!(gfpflags &
__GFP_NOFAIL)) => submit_one_bio()'

And just now, I hit the same lock_extent_bits() hang with NOFAIL filter.
So this is definitely something wrong in the submit_one_bio() path.

Thanks,
Qu
>
> Thanks,
> Qu
>
>>
>> The main reason I'm doing such error injection test is to ensure write
>> time tree checker is not the cause of the lockup.
>>
>> Of course I can directly inject error into btrfs_check_leaf_full() and
>> btrfs_check_node(), and filter the stack to ensure it only happen in
>> write time, and that's already what I'm crafting, based on the bcc error
>> inject example and kprobe return value overriding.
>>
>> But it will never be a bad idea to explore what can go wrong.
>> And "always BUG_ON()" -> "good enough (TM)" already looks like a
>> improvement to me.
>>
>> Thanks,
>> Qu
>>