Re: [PATCH 0/3] btrfs: avoid GFP_ATOMIC allocation failures during endio

[Josef Bacik <josef@toxicpanda.com>]

On Mon, Oct 17, 2022 at 04:25:16PM +0200, David Sterba wrote:
> On Fri, Oct 14, 2022 at 10:00:38AM -0400, Josef Bacik wrote:
> > Hello,
> > 
> > As you can imagine we have workloads that don't behave super well sometimes, and
> > they'll OOM the box in a really spectacular fashion.  Sometimes these trip the
> > BUG_ON(!prealloc) things inside of the extent io tree code.
> > 
> > We've talked about switching these allocations to mempools for a while, but
> > that's going to require some extra work.  We can drastically reduce the
> > likelihood of failing these allocations by simply dropping the tree lock and
> > attempting to make the allocation with the original gfp_mask.
> > 
> > The main problem with that approach is we've been using GFP_ATOMIC in the endio
> > path for....reasons?  I *think* the read endio work used to happen in IRQ
> > context, but it hasn't for at least a decade, and in fact if we get read
> > failures we do our failrec allocations with GFP_NOFS, so clearly GFP_ATOMIC
> > isn't really required in this path.
> 
> Up to my possibly dated knowledge endio is done in irq context so we
> need to verify that. I did a quick check in block/ but the bare bio->end_io()
> is not called unser obvious irq protection (spin lock or local_irq
> save/restore), but I could be mistaken due to the maze of block layer.
> 

I went through and read all the code, every path that does a REQ_READ does the
actual endio work in an async worker, only some of the write path happens in IRQ
context.  Additionally we've been allocating failrec's in this context for
years, so if it was actually happening in IRQ context we would have noticed by
now.  I definitely went and looked tho because I was super confused.

> > So kill the GFP_ATOMIC allocations in the endio path, which is where we see
> > these panics, and then change the extent io code to simply do the loop again if
> > it can't allocate the prealloc extent with GFP_ATOMIC so we can make the
> > allocation with the callers gfp_mask.
> > 
> > This is perfectly safe, we'll drop the tree lock and loop around any time we
> > have to re-search the tree after modifying part of our range, we don't need to
> > hold the lock for our entire operation.
> > 
> > The only drawback here is that we could infinite loop if we can't make our
> > allocation.  This is why a mempool would be the proper solution, as we can't
> > fail these allocations without brining the box down, which is what we currently
> > do anyway.
> 
> Aren't the mempools shifting the possibly infinite loop one layer down
> only? With some added bonus of creating indirect dependencies of the
> allocating and freeing threads.

bio's use mempools for the same reason, the emergency reserve exists so that we
always are able to make our allocations.  Clearly we could still end up in a bad
situation if we exhaust the emergency reserve, but the extent states in this
particular case don't get allocated a bunch.  Thanks,

Josef