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 ?
