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

[Josef Bacik <josef@toxicpanda.com>]

On Tue, Oct 18, 2022 at 02:42:29PM +0200, David Sterba wrote:
> On Mon, Oct 17, 2022 at 02:08:02PM -0400, Josef Bacik wrote:
> > On Mon, Oct 17, 2022 at 04:25:16PM +0200, David Sterba wrote:
> > > > 
> > > > 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,
> 
> I think that bios are the only thing that works with mempools reliably
> because it satisfies the guaranteed forward progress. Otherwise the
> indirect dependenices lead to lockups in the allocation, which is
> equivalent to the potentially infinite looping. The emergency reserve is
> another finite resource so it can get exhausted eventually, and it's
> not scaled to the number of potential requests that could hit the same
> code path and competing for the memory. It's true we'd be dealing with a
> system in a bad state and depending on another subsystems make it less
> predictable. The simplest options are wait or exit with error.
> 

Yup, this is why I didn't do the work now, becuase I don't want to use an
emergency reserve for every extent io tree operation, otherwise we're going to
run into trouble.

I only want to do it in the case of the endio handlers, since that is for
forward progress, and for EXTENT_LOCK, for the same reason.  However IDK how it
would look to have mempools where we allow failures in some cases but use the
emergency reserve for others.  It's an area for future investigation, for now
this is a step in the right direction.  Thanks,

Josef