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

[David Sterba <dsterba@suse.cz>]

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.

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