Re: bug in btrfs during low memory testing.

[Nikolay Borisov <nborisov@suse.com>]

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


Because an alternative route to fix is it to have GFP_ATOMIC in the gfp.

Basically we'd want to bark on xa_insert execution when we are 
in_atomic(), cause by something else apart from xa_lock ?