Re: bug in btrfs during low memory testing.

[Nikolay Borisov <nborisov@suse.com>]

On 6.07.22 г. 15:09 ч., Matthew Wilcox wrote:
> 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.

Actually I had in mind more along the lines of:

might_sleep_if(gfpflags_allow_blocking(gfp_mask));

but this is essentially what might_alloc does + lockdep annotations, so 
yeah, that would work. What I'd like to achieve with such a modification 
is for new users of xa array to instantly get a splat when xa_insert is 
executed irrespective of whether the allocation happens or not so that 
they have a chance to fix their code sooner rather than later.