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.
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
