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.
