Re: bug in btrfs during low memory testing.

[Matthew Wilcox <willy@infradead.org>]

On Wed, Jul 06, 2022 at 09:36:42AM +0300, Nikolay Borisov wrote:
> On 5.07.22 г. 23:29 ч., Matthew Wilcox wrote:
> > On Tue, Jul 05, 2022 at 09:26:47PM +0100, Filipe Manana wrote:
> > > In this case we can actually call xa_insert() without holding that
> > > spinlock, it's safe against other concurrent calls to
> > > btrfs_get_or_create_delayed_node(), btrfs_get_delayed_node(),
> > > btrfs_kill_delayed_inode_items(), etc.
> > > 
> > > However, looking at xa_insert() we have:
> > > 
> > >          xa_lock(xa);
> > >          err = __xa_insert(xa, index, entry, gfp);
> > >          xa_unlock(xa);
> > > 
> > > And xa_lock() is defined as:
> > > 
> > >          #define xa_lock(xa)             spin_lock(&(xa)->xa_lock)
> > > 
> > > So we'll always be under a spinlock even if we change btrfs to not
> > > take the root->inode_lock spinlock.
> > > 
> > > This seems more like a general problem outside btrfs' control.
> > > So CC'ing Willy to double check.
> > 
> > No, the XArray knows about its own spinlock.  It'll drop it if it needs
> > to allocate memory and the GFP flags indicate that the caller can sleep.
> > It doesn't know about your spinlock, so it can't do the same thing for
> > you ;-)
> 
> 
> In order to catch (and prevent) further offensive can we perhaps have
> something like that in xa_insert:
> 
> 
> diff --git a/include/linux/xarray.h b/include/linux/xarray.h
> index c29e11b2c073..63c00b2945a2 100644
> --- a/include/linux/xarray.h
> +++ b/include/linux/xarray.h
> @@ -770,6 +770,9 @@ static inline int __must_check xa_insert(struct xarray
> *xa,
>  {
>         int err;
> 
> +       if (gfpflags_allow_blocking(gfp))
> +               might_sleep()
> +
>         xa_lock(xa);
>         err = __xa_insert(xa, index, entry, gfp);
>         xa_unlock(xa);

I think you mean:

	might_alloc(gfp);

And yes, I think that makes a lot of sense.  Quite a few similar places
to do ... I'll take care of it.