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

[Qu Wenruo <wqu@suse.de>]

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.

I'll try to pre-allocate certain amount of extent_state as the last
chance of redemption.

Anyway, such BUG_ON() right after kmalloc() is really a blockage for
error injection test.

Thanks,
Qu

> 
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>  fs/btrfs/extent_io.c | 12 ++++++++++--
>>  1 file changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
>> index 828708f6510c..f1bfc632ef7b 100644
>> --- a/fs/btrfs/extent_io.c
>> +++ b/fs/btrfs/extent_io.c
>> @@ -743,7 +743,10 @@ int __clear_extent_bit(struct extent_io_tree *tree, u64 start, u64 end,
>>  
>>  	if (state->start < start) {
>>  		prealloc = alloc_extent_state_atomic(prealloc);
>> -		BUG_ON(!prealloc);
>> +		if (!prealloc) {
>> +			err = -ENOMEM;
>> +			goto out;
>> +		}
>>  		err = split_state(tree, state, prealloc, start);
>>  		if (err)
>>  			extent_io_tree_panic(tree, err);
>> @@ -766,7 +769,10 @@ int __clear_extent_bit(struct extent_io_tree *tree, u64 start, u64 end,
>>  	 */
>>  	if (state->start <= end && state->end > end) {
>>  		prealloc = alloc_extent_state_atomic(prealloc);
>> -		BUG_ON(!prealloc);
>> +		if (!prealloc) {
>> +			err = -ENOMEM;
>> +			goto out;
>> +		}
>>  		err = split_state(tree, state, prealloc, end + 1);
>>  		if (err)
>>  			extent_io_tree_panic(tree, err);
>> @@ -801,6 +807,8 @@ int __clear_extent_bit(struct extent_io_tree *tree, u64 start, u64 end,
>>  	if (prealloc)
>>  		free_extent_state(prealloc);
>>  
>> +	if (err)
>> +		return err;
>>  	return 0;
>>  
>>  }
>>