[PATCH 0/3] btrfs: fix a couple sleeps while holding a spinlock

[PATCH 0/3] btrfs: fix a couple sleeps while holding a spinlock

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

After the recent conversions of a couple radix trees to XArrays, we now
can end up attempting to sleep while holding a spinlock. This happens
because if xa_insert() allocates memory (using GFP_NOFS) it may need to
sleep (more likely to happen when under memory pressure). In the old
code this did not happen because we had radix_tree_preload() called
before taking the spinlocks.

Filipe Manana (3):
  btrfs: fix sleep while under a spinlock when allocating delayed inode
  btrfs: fix sleep while under a spinlock when inserting a fs root
  btrfs: free qgroup metadata without holding the fs roots lock

 fs/btrfs/ctree.h         |  6 +++---
 fs/btrfs/delayed-inode.c | 24 ++++++++++++------------
 fs/btrfs/disk-io.c       | 38 +++++++++++++++++++-------------------
 fs/btrfs/inode.c         | 30 ++++++++++++++++--------------
 fs/btrfs/relocation.c    | 11 +++++++----
 fs/btrfs/transaction.c   | 14 +++++++-------
 6 files changed, 64 insertions(+), 59 deletions(-)

-- 
2.35.1

[PATCH 1/3] btrfs: fix sleep while under a spinlock when allocating delayed inode

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

When allocating a delayed inode, at btrfs_get_or_create_delayed_node(),
we are calling xa_insert() while holding the root's inode_lock spinlock.

We are passing GFP_NOFS to xa_insert(), but a memory allocation with
GFP_NOFS can sleep, specially likely to happen if we are under memory
pressure. If that happens we get a splat like the following:

Jul  3 04:08:44 nfsvmf24 kernel: BUG: sleeping function called from invalid context at include/linux/sched/mm.h:274
Jul  3 04:08:44 nfsvmf24 kernel: in_atomic(): 1, irqs_disabled(): 0, non_block: 0, pid: 4308, name: nfsd
Jul  3 04:08:44 nfsvmf24 kernel: preempt_count: 1, expected: 0
Jul  3 04:08:44 nfsvmf24 kernel: RCU nest depth: 0, expected: 0
Jul  3 04:08:44 nfsvmf24 kernel: 4 locks held by nfsd/4308:
Jul  3 04:08:44 nfsvmf24 kernel: #0: ffff8881052d4438 (sb_writers#14){.+.+}-{0:0}, at: nfsd4_setattr+0x17f/0x3c0 [nfsd]
Jul  3 04:08:44 nfsvmf24 kernel: #1: ffff888141e5aa50 (&sb->s_type->i_mutex_key#16){+.+.}-{3:3}, at:  nfsd_setattr+0x56a/0xe00 [nfsd]
Jul  3 04:08:44 nfsvmf24 kernel: #2: ffff8881052d4628 (sb_internal#2){.+.+}-{0:0}, at: btrfs_dirty_inode+0xda/0x1d0
Jul  3 04:08:44 nfsvmf24 kernel: #3: ffff888105ed6628 (&root->inode_lock){+.+.}-{2:2}, at: btrfs_get_or_create_delayed_node+0x27a/0x430

Jul  3 04:08:44 nfsvmf24 kernel: Preemption disabled at:
Jul  3 04:08:44 nfsvmf24 kernel: [<0000000000000000>] 0x0
Jul  3 04:08:44 nfsvmf24 kernel: CPU: 0 PID: 4308 Comm: nfsd Kdump: loaded Not tainted 5.19.0-rc4+ #1
Jul  3 04:08:44 nfsvmf24 kernel: Hardware name: innotek GmbH VirtualBox/VirtualBox, BIOS VirtualBox 12/01/2006
Jul  3 04:08:45 nfsvmf24 kernel: Call Trace:
Jul  3 04:08:45 nfsvmf24 kernel: <TASK>
Jul  3 04:08:45 nfsvmf24 kernel: dump_stack_lvl+0x57/0x7d
Jul  3 04:08:45 nfsvmf24 kernel: __might_resched.cold+0x222/0x26b
Jul  3 04:08:45 nfsvmf24 kernel: kmem_cache_alloc_lru+0x159/0x2c0
Jul  3 04:08:45 nfsvmf24 kernel: __xas_nomem+0x1a5/0x5d0
Jul  3 04:08:45 nfsvmf24 kernel: ? lock_acquire+0x1bb/0x500
Jul  3 04:08:45 nfsvmf24 kernel: __xa_insert+0xff/0x1d0
Jul  3 04:08:45 nfsvmf24 kernel: ? __xa_cmpxchg+0x1f0/0x1f0
Jul  3 04:08:45 nfsvmf24 kernel: ? lockdep_init_map_type+0x2c3/0x7b0
Jul  3 04:08:45 nfsvmf24 kernel: ? rwlock_bug.part.0+0x90/0x90
Jul  3 04:08:45 nfsvmf24 kernel: btrfs_get_or_create_delayed_node+0x295/0x430
Jul  3 04:08:45 nfsvmf24 kernel: btrfs_delayed_update_inode+0x24/0x670
Jul  3 04:08:45 nfsvmf24 kernel: ? btrfs_check_and_init_root_item+0x1f0/0x1f0
Jul  3 04:08:45 nfsvmf24 kernel: ? start_transaction+0x219/0x12d0
Jul  3 04:08:45 nfsvmf24 kernel: btrfs_update_inode+0x1aa/0x430
Jul  3 04:08:45 nfsvmf24 kernel: btrfs_dirty_inode+0xf7/0x1d0
Jul  3 04:08:45 nfsvmf24 kernel: btrfs_setattr+0x928/0x1400
Jul  3 04:08:45 nfsvmf24 kernel: ? mark_held_locks+0x9e/0xe0
Jul  3 04:08:45 nfsvmf24 kernel: ? _raw_spin_unlock+0x29/0x40
Jul  3 04:08:45 nfsvmf24 kernel: ? btrfs_cont_expand+0x9a0/0x9a0
Jul  3 04:08:45 nfsvmf24 kernel: ? fh_update+0x1e0/0x1e0 [nfsd]
Jul  3 04:08:45 nfsvmf24 kernel: ? current_time+0x88/0xd0
Jul  3 04:08:45 nfsvmf24 kernel: ? timestamp_truncate+0x230/0x230
Jul  3 04:08:45 nfsvmf24 kernel: notify_change+0x4b3/0xe40
Jul  3 04:08:45 nfsvmf24 kernel: ? down_write_nested+0xd4/0x130
Jul  3 04:08:45 nfsvmf24 kernel: ? nfsd_setattr+0x984/0xe00 [nfsd]
Jul  3 04:08:45 nfsvmf24 kernel: nfsd_setattr+0x984/0xe00 [nfsd]
Jul  3 04:08:45 nfsvmf24 kernel: ? lock_downgrade+0x680/0x680
Jul  3 04:08:45 nfsvmf24 kernel: ? nfsd_permission+0x310/0x310 [nfsd]
Jul  3 04:08:45 nfsvmf24 kernel: ? __mnt_want_write+0x192/0x270
Jul  3 04:08:45 nfsvmf24 kernel: nfsd4_setattr+0x1df/0x3c0 [nfsd]
Jul  3 04:08:45 nfsvmf24 kernel: ? percpu_counter_add_batch+0x79/0x130
Jul  3 04:08:45 nfsvmf24 kernel: nfsd4_proc_compound+0xce8/0x23e0 [nfsd]
Jul  3 04:08:45 nfsvmf24 kernel: nfsd_dispatch+0x4ed/0xc10 [nfsd]
Jul  3 04:08:45 nfsvmf24 kernel: ? svc_reserve+0xb5/0x130 [sunrpc]
Jul  3 04:08:45 nfsvmf24 kernel: svc_process_common+0xd3f/0x1b00 [sunrpc]
Jul  3 04:08:45 nfsvmf24 kernel: ? svc_generic_rpcbind_set+0x4d0/0x4d0 [sunrpc]
Jul  3 04:08:45 nfsvmf24 kernel: ? nfsd_svc+0xc50/0xc50 [nfsd]
Jul  3 04:08:45 nfsvmf24 kernel: ? svc_sock_secure_port+0x37/0x50 [sunrpc]
Jul  3 04:08:45 nfsvmf24 kernel: ? svc_recv+0x1096/0x2350 [sunrpc]
Jul  3 04:08:45 nfsvmf24 kernel: svc_process+0x361/0x4f0 [sunrpc]
Jul  3 04:08:45 nfsvmf24 kernel: nfsd+0x2d6/0x570 [nfsd]
Jul  3 04:08:45 nfsvmf24 kernel: ? nfsd_shutdown_threads+0x2a0/0x2a0 [nfsd]
Jul  3 04:08:45 nfsvmf24 kernel: kthread+0x2a1/0x340
Jul  3 04:08:45 nfsvmf24 kernel: ? kthread_complete_and_exit+0x20/0x20
Jul  3 04:08:45 nfsvmf24 kernel: ret_from_fork+0x22/0x30
Jul  3 04:08:45 nfsvmf24 kernel: </TASK>
Jul  3 04:08:45 nfsvmf24 kernel:

Fix this by changing root->inode_lock from a spinlock to a mutex.
Mutexes have an optimistic spinning mode (if CONFIG_MUTEX_SPIN_ON_OWNER=y)
and most of the time our critical sections under this lock are very short,
so it should not have a visible impact.

Reported-by: dai.ngo@oracle.com
Link: https://lore.kernel.org/linux-btrfs/c09e1af7-7d1f-1bbf-5562-ead9a4d99562@oracle.com/
Fixes: 253bf57555e451 ("btrfs: turn delayed_nodes_tree into an XArray")
Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/ctree.h         |  4 ++--
 fs/btrfs/delayed-inode.c | 24 ++++++++++++------------
 fs/btrfs/disk-io.c       |  2 +-
 fs/btrfs/inode.c         | 26 ++++++++++++++------------
 fs/btrfs/relocation.c    | 11 +++++++----
 5 files changed, 36 insertions(+), 31 deletions(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index f7afdfd0bae7..7c7d78db27e3 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -1223,13 +1223,13 @@ struct btrfs_root {
 	spinlock_t log_extents_lock[2];
 	struct list_head logged_list[2];
 
-	spinlock_t inode_lock;
+	struct mutex inode_mutex;
 	/* red-black tree that keeps track of in-memory inodes */
 	struct rb_root inode_tree;
 
 	/*
 	 * Xarray that keeps track of delayed nodes of every inode, protected
-	 * by inode_lock
+	 * by @inode_mutex.
 	 */
 	struct xarray delayed_nodes;
 	/*
diff --git a/fs/btrfs/delayed-inode.c b/fs/btrfs/delayed-inode.c
index e7c12fe2fda6..48cdf6bcb341 100644
--- a/fs/btrfs/delayed-inode.c
+++ b/fs/btrfs/delayed-inode.c
@@ -65,14 +65,14 @@ static struct btrfs_delayed_node *btrfs_get_delayed_node(
 		return node;
 	}
 
-	spin_lock(&root->inode_lock);
+	mutex_lock(&root->inode_mutex);
 	node = xa_load(&root->delayed_nodes, ino);
 
 	if (node) {
 		if (btrfs_inode->delayed_node) {
 			refcount_inc(&node->refs);	/* can be accessed */
 			BUG_ON(btrfs_inode->delayed_node != node);
-			spin_unlock(&root->inode_lock);
+			mutex_unlock(&root->inode_mutex);
 			return node;
 		}
 
@@ -99,10 +99,10 @@ static struct btrfs_delayed_node *btrfs_get_delayed_node(
 			node = NULL;
 		}
 
-		spin_unlock(&root->inode_lock);
+		mutex_unlock(&root->inode_mutex);
 		return node;
 	}
-	spin_unlock(&root->inode_lock);
+	mutex_unlock(&root->inode_mutex);
 
 	return NULL;
 }
@@ -129,17 +129,17 @@ static struct btrfs_delayed_node *btrfs_get_or_create_delayed_node(
 		/* Cached in the inode and can be accessed */
 		refcount_set(&node->refs, 2);
 
-		spin_lock(&root->inode_lock);
+		mutex_lock(&root->inode_mutex);
 		ret = xa_insert(&root->delayed_nodes, ino, node, GFP_NOFS);
 		if (ret) {
-			spin_unlock(&root->inode_lock);
+			mutex_unlock(&root->inode_mutex);
 			kmem_cache_free(delayed_node_cache, node);
 			if (ret != -EBUSY)
 				return ERR_PTR(ret);
 		}
 	} while (ret);
 	btrfs_inode->delayed_node = node;
-	spin_unlock(&root->inode_lock);
+	mutex_unlock(&root->inode_mutex);
 
 	return node;
 }
@@ -252,14 +252,14 @@ static void __btrfs_release_delayed_node(
 	if (refcount_dec_and_test(&delayed_node->refs)) {
 		struct btrfs_root *root = delayed_node->root;
 
-		spin_lock(&root->inode_lock);
+		mutex_lock(&root->inode_mutex);
 		/*
 		 * Once our refcount goes to zero, nobody is allowed to bump it
 		 * back up.  We can delete it now.
 		 */
 		ASSERT(refcount_read(&delayed_node->refs) == 0);
 		xa_erase(&root->delayed_nodes, delayed_node->inode_id);
-		spin_unlock(&root->inode_lock);
+		mutex_unlock(&root->inode_mutex);
 		kmem_cache_free(delayed_node_cache, delayed_node);
 	}
 }
@@ -1954,9 +1954,9 @@ void btrfs_kill_all_delayed_nodes(struct btrfs_root *root)
 	while (1) {
 		int n = 0;
 
-		spin_lock(&root->inode_lock);
+		mutex_lock(&root->inode_mutex);
 		if (xa_empty(&root->delayed_nodes)) {
-			spin_unlock(&root->inode_lock);
+			mutex_unlock(&root->inode_mutex);
 			return;
 		}
 
@@ -1973,7 +1973,7 @@ void btrfs_kill_all_delayed_nodes(struct btrfs_root *root)
 				break;
 		}
 		index++;
-		spin_unlock(&root->inode_lock);
+		mutex_unlock(&root->inode_mutex);
 
 		for (int i = 0; i < n; i++) {
 			__btrfs_kill_delayed_node(delayed_nodes[i]);
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 800ad3a9c68e..79aeb5795d72 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -1079,7 +1079,7 @@ static void __setup_root(struct btrfs_root *root, struct btrfs_fs_info *fs_info,
 	INIT_LIST_HEAD(&root->reloc_dirty_list);
 	INIT_LIST_HEAD(&root->logged_list[0]);
 	INIT_LIST_HEAD(&root->logged_list[1]);
-	spin_lock_init(&root->inode_lock);
+	mutex_init(&root->inode_mutex);
 	spin_lock_init(&root->delalloc_lock);
 	spin_lock_init(&root->ordered_extent_lock);
 	spin_lock_init(&root->accounting_lock);
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index b86be4c3513d..b510fd917424 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -4496,8 +4496,8 @@ static void btrfs_prune_dentries(struct btrfs_root *root)
 	if (!BTRFS_FS_ERROR(fs_info))
 		WARN_ON(btrfs_root_refs(&root->root_item) != 0);
 
-	spin_lock(&root->inode_lock);
 again:
+	mutex_lock(&root->inode_mutex);
 	node = root->inode_tree.rb_node;
 	prev = NULL;
 	while (node) {
@@ -4526,7 +4526,7 @@ static void btrfs_prune_dentries(struct btrfs_root *root)
 		objectid = btrfs_ino(entry) + 1;
 		inode = igrab(&entry->vfs_inode);
 		if (inode) {
-			spin_unlock(&root->inode_lock);
+			mutex_unlock(&root->inode_mutex);
 			if (atomic_read(&inode->i_count) > 1)
 				d_prune_aliases(inode);
 			/*
@@ -4535,16 +4535,18 @@ static void btrfs_prune_dentries(struct btrfs_root *root)
 			 */
 			iput(inode);
 			cond_resched();
-			spin_lock(&root->inode_lock);
 			goto again;
 		}
 
-		if (cond_resched_lock(&root->inode_lock))
+		if (need_resched()) {
+			mutex_unlock(&root->inode_mutex);
+			cond_resched();
 			goto again;
+		}
 
 		node = rb_next(node);
 	}
-	spin_unlock(&root->inode_lock);
+	mutex_unlock(&root->inode_mutex);
 }
 
 int btrfs_delete_subvolume(struct inode *dir, struct dentry *dentry)
@@ -5551,7 +5553,7 @@ static void inode_tree_add(struct inode *inode)
 	if (inode_unhashed(inode))
 		return;
 	parent = NULL;
-	spin_lock(&root->inode_lock);
+	mutex_lock(&root->inode_mutex);
 	p = &root->inode_tree.rb_node;
 	while (*p) {
 		parent = *p;
@@ -5566,13 +5568,13 @@ static void inode_tree_add(struct inode *inode)
 				  (I_WILL_FREE | I_FREEING)));
 			rb_replace_node(parent, new, &root->inode_tree);
 			RB_CLEAR_NODE(parent);
-			spin_unlock(&root->inode_lock);
+			mutex_unlock(&root->inode_mutex);
 			return;
 		}
 	}
 	rb_link_node(new, parent, p);
 	rb_insert_color(new, &root->inode_tree);
-	spin_unlock(&root->inode_lock);
+	mutex_unlock(&root->inode_mutex);
 }
 
 static void inode_tree_del(struct btrfs_inode *inode)
@@ -5580,18 +5582,18 @@ static void inode_tree_del(struct btrfs_inode *inode)
 	struct btrfs_root *root = inode->root;
 	int empty = 0;
 
-	spin_lock(&root->inode_lock);
+	mutex_lock(&root->inode_mutex);
 	if (!RB_EMPTY_NODE(&inode->rb_node)) {
 		rb_erase(&inode->rb_node, &root->inode_tree);
 		RB_CLEAR_NODE(&inode->rb_node);
 		empty = RB_EMPTY_ROOT(&root->inode_tree);
 	}
-	spin_unlock(&root->inode_lock);
+	mutex_unlock(&root->inode_mutex);
 
 	if (empty && btrfs_root_refs(&root->root_item) == 0) {
-		spin_lock(&root->inode_lock);
+		mutex_lock(&root->inode_mutex);
 		empty = RB_EMPTY_ROOT(&root->inode_tree);
-		spin_unlock(&root->inode_lock);
+		mutex_unlock(&root->inode_mutex);
 		if (empty)
 			btrfs_add_dead_root(root);
 	}
diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index a6dc827e75af..e3336a3c8f47 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -956,8 +956,8 @@ static struct inode *find_next_inode(struct btrfs_root *root, u64 objectid)
 	struct btrfs_inode *entry;
 	struct inode *inode;
 
-	spin_lock(&root->inode_lock);
 again:
+	mutex_lock(&root->inode_mutex);
 	node = root->inode_tree.rb_node;
 	prev = NULL;
 	while (node) {
@@ -985,17 +985,20 @@ static struct inode *find_next_inode(struct btrfs_root *root, u64 objectid)
 		entry = rb_entry(node, struct btrfs_inode, rb_node);
 		inode = igrab(&entry->vfs_inode);
 		if (inode) {
-			spin_unlock(&root->inode_lock);
+			mutex_unlock(&root->inode_mutex);
 			return inode;
 		}
 
 		objectid = btrfs_ino(entry) + 1;
-		if (cond_resched_lock(&root->inode_lock))
+		if (need_resched()) {
+			mutex_unlock(&root->inode_mutex);
+			cond_resched();
 			goto again;
+		}
 
 		node = rb_next(node);
 	}
-	spin_unlock(&root->inode_lock);
+	mutex_unlock(&root->inode_mutex);
 	return NULL;
 }
 
-- 
2.35.1

[PATCH 2/3] btrfs: fix sleep while under a spinlock when inserting a fs root

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

When inserting a fs root, at btrfs_insert_fs_root(), we are calling
xa_insert() while holding the spinlock fs_info->fs_roots_lock.

We are passing GFP_NOFS to xa_insert(), but a memory allocation with
GFP_NOFS can sleep, specially likely to happen if we are under memory
pressure. If that happens we will get a splat like the one reported in
a similar case for delayed inodes (see the Link tag below).

Fix this by changing fs_info->fs_roots_lock from a spinlock to a mutex.
Mutexes have an optimistic spinning mode (if CONFIG_MUTEX_SPIN_ON_OWNER=y)
and most of the time our critical sections under this lock are very short,
so it should not have a visible impact.

Link: https://lore.kernel.org/linux-btrfs/c09e1af7-7d1f-1bbf-5562-ead9a4d99562@oracle.com/
Fixes: 48b36a602a335c ("btrfs: turn fs_roots_radix in btrfs_fs_info into an XArray")
Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/ctree.h       |  2 +-
 fs/btrfs/disk-io.c     | 36 ++++++++++++++++++------------------
 fs/btrfs/inode.c       |  4 ++--
 fs/btrfs/transaction.c | 14 +++++++-------
 4 files changed, 28 insertions(+), 28 deletions(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 7c7d78db27e3..9d8b9badbca7 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -680,7 +680,7 @@ struct btrfs_fs_info {
 	struct rb_root global_root_tree;
 
 	/* The xarray that holds all the FS roots */
-	spinlock_t fs_roots_lock;
+	struct mutex fs_roots_mutex;
 	struct xarray fs_roots;
 
 	/* block group cache stuff */
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 79aeb5795d72..0da86fba370e 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -1118,9 +1118,9 @@ static void __setup_root(struct btrfs_root *root, struct btrfs_fs_info *fs_info,
 	btrfs_qgroup_init_swapped_blocks(&root->swapped_blocks);
 #ifdef CONFIG_BTRFS_DEBUG
 	INIT_LIST_HEAD(&root->leak_list);
-	spin_lock(&fs_info->fs_roots_lock);
+	mutex_lock(&fs_info->fs_roots_mutex);
 	list_add_tail(&root->leak_list, &fs_info->allocated_roots);
-	spin_unlock(&fs_info->fs_roots_lock);
+	mutex_unlock(&fs_info->fs_roots_mutex);
 #endif
 }
 
@@ -1567,11 +1567,11 @@ static struct btrfs_root *btrfs_lookup_fs_root(struct btrfs_fs_info *fs_info,
 {
 	struct btrfs_root *root;
 
-	spin_lock(&fs_info->fs_roots_lock);
+	mutex_lock(&fs_info->fs_roots_mutex);
 	root = xa_load(&fs_info->fs_roots, (unsigned long)root_id);
 	if (root)
 		root = btrfs_grab_root(root);
-	spin_unlock(&fs_info->fs_roots_lock);
+	mutex_unlock(&fs_info->fs_roots_mutex);
 	return root;
 }
 
@@ -1613,14 +1613,14 @@ int btrfs_insert_fs_root(struct btrfs_fs_info *fs_info,
 {
 	int ret;
 
-	spin_lock(&fs_info->fs_roots_lock);
+	mutex_lock(&fs_info->fs_roots_mutex);
 	ret = xa_insert(&fs_info->fs_roots, (unsigned long)root->root_key.objectid,
 			root, GFP_NOFS);
 	if (ret == 0) {
 		btrfs_grab_root(root);
 		set_bit(BTRFS_ROOT_REGISTERED, &root->state);
 	}
-	spin_unlock(&fs_info->fs_roots_lock);
+	mutex_unlock(&fs_info->fs_roots_mutex);
 
 	return ret;
 }
@@ -2235,9 +2235,9 @@ void btrfs_put_root(struct btrfs_root *root)
 		btrfs_drew_lock_destroy(&root->snapshot_lock);
 		free_root_extent_buffers(root);
 #ifdef CONFIG_BTRFS_DEBUG
-		spin_lock(&root->fs_info->fs_roots_lock);
+		mutex_lock(&root->fs_info->fs_roots_mutex);
 		list_del_init(&root->leak_list);
-		spin_unlock(&root->fs_info->fs_roots_lock);
+		mutex_unlock(&root->fs_info->fs_roots_mutex);
 #endif
 		kfree(root);
 	}
@@ -3029,7 +3029,7 @@ void btrfs_init_fs_info(struct btrfs_fs_info *fs_info)
 	INIT_LIST_HEAD(&fs_info->caching_block_groups);
 	spin_lock_init(&fs_info->delalloc_root_lock);
 	spin_lock_init(&fs_info->trans_lock);
-	spin_lock_init(&fs_info->fs_roots_lock);
+	mutex_init(&fs_info->fs_roots_mutex);
 	spin_lock_init(&fs_info->delayed_iput_lock);
 	spin_lock_init(&fs_info->defrag_inodes_lock);
 	spin_lock_init(&fs_info->super_lock);
@@ -4399,11 +4399,11 @@ void btrfs_drop_and_free_fs_root(struct btrfs_fs_info *fs_info,
 {
 	bool drop_ref = false;
 
-	spin_lock(&fs_info->fs_roots_lock);
+	mutex_lock(&fs_info->fs_roots_mutex);
 	xa_erase(&fs_info->fs_roots, (unsigned long)root->root_key.objectid);
 	if (test_and_clear_bit(BTRFS_ROOT_REGISTERED, &root->state))
 		drop_ref = true;
-	spin_unlock(&fs_info->fs_roots_lock);
+	mutex_unlock(&fs_info->fs_roots_mutex);
 
 	if (BTRFS_FS_ERROR(fs_info)) {
 		ASSERT(root->log_root == NULL);
@@ -4428,9 +4428,9 @@ int btrfs_cleanup_fs_roots(struct btrfs_fs_info *fs_info)
 	while (1) {
 		struct btrfs_root *root;
 
-		spin_lock(&fs_info->fs_roots_lock);
+		mutex_lock(&fs_info->fs_roots_mutex);
 		if (!xa_find(&fs_info->fs_roots, &index, ULONG_MAX, XA_PRESENT)) {
-			spin_unlock(&fs_info->fs_roots_lock);
+			mutex_unlock(&fs_info->fs_roots_mutex);
 			return err;
 		}
 
@@ -4442,7 +4442,7 @@ int btrfs_cleanup_fs_roots(struct btrfs_fs_info *fs_info)
 			if (grabbed >= ARRAY_SIZE(roots))
 				break;
 		}
-		spin_unlock(&fs_info->fs_roots_lock);
+		mutex_unlock(&fs_info->fs_roots_mutex);
 
 		for (i = 0; i < grabbed; i++) {
 			if (!roots[i])
@@ -4783,12 +4783,12 @@ static void btrfs_drop_all_logs(struct btrfs_fs_info *fs_info)
 	int grabbed = 0;
 	struct btrfs_root *roots[8];
 
-	spin_lock(&fs_info->fs_roots_lock);
+	mutex_lock(&fs_info->fs_roots_mutex);
 	while ((grabbed = xa_extract(&fs_info->fs_roots, (void **)roots, index,
 				     ULONG_MAX, 8, XA_PRESENT))) {
 		for (int i = 0; i < grabbed; i++)
 			roots[i] = btrfs_grab_root(roots[i]);
-		spin_unlock(&fs_info->fs_roots_lock);
+		mutex_unlock(&fs_info->fs_roots_mutex);
 
 		for (int i = 0; i < grabbed; i++) {
 			if (!roots[i])
@@ -4798,9 +4798,9 @@ static void btrfs_drop_all_logs(struct btrfs_fs_info *fs_info)
 			btrfs_put_root(roots[i]);
 		}
 		index++;
-		spin_lock(&fs_info->fs_roots_lock);
+		mutex_lock(&fs_info->fs_roots_mutex);
 	}
-	spin_unlock(&fs_info->fs_roots_lock);
+	mutex_unlock(&fs_info->fs_roots_mutex);
 	btrfs_free_log_root_tree(NULL, fs_info);
 }
 
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index b510fd917424..3e10a2d1b4a5 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -3665,12 +3665,12 @@ int btrfs_orphan_cleanup(struct btrfs_root *root)
 			 * up the root from that xarray.
 			 */
 
-			spin_lock(&fs_info->fs_roots_lock);
+			mutex_lock(&fs_info->fs_roots_mutex);
 			dead_root = xa_load(&fs_info->fs_roots,
 					    (unsigned long)found_key.objectid);
 			if (dead_root && btrfs_root_refs(&dead_root->root_item) == 0)
 				is_dead_root = 1;
-			spin_unlock(&fs_info->fs_roots_lock);
+			mutex_unlock(&fs_info->fs_roots_mutex);
 
 			if (is_dead_root) {
 				/* prevent this orphan from being found again */
diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
index 06c0a958d114..1283be132776 100644
--- a/fs/btrfs/transaction.c
+++ b/fs/btrfs/transaction.c
@@ -437,15 +437,15 @@ static int record_root_in_trans(struct btrfs_trans_handle *trans,
 		 */
 		smp_wmb();
 
-		spin_lock(&fs_info->fs_roots_lock);
+		mutex_lock(&fs_info->fs_roots_mutex);
 		if (root->last_trans == trans->transid && !force) {
-			spin_unlock(&fs_info->fs_roots_lock);
+			mutex_unlock(&fs_info->fs_roots_mutex);
 			return 0;
 		}
 		xa_set_mark(&fs_info->fs_roots,
 			    (unsigned long)root->root_key.objectid,
 			    BTRFS_ROOT_TRANS_TAG);
-		spin_unlock(&fs_info->fs_roots_lock);
+		mutex_unlock(&fs_info->fs_roots_mutex);
 		root->last_trans = trans->transid;
 
 		/* this is pretty tricky.  We don't want to
@@ -1411,7 +1411,7 @@ static noinline int commit_fs_roots(struct btrfs_trans_handle *trans)
 	 */
 	ASSERT(trans->transaction->state == TRANS_STATE_COMMIT_DOING);
 
-	spin_lock(&fs_info->fs_roots_lock);
+	mutex_lock(&fs_info->fs_roots_mutex);
 	xa_for_each_marked(&fs_info->fs_roots, index, root, BTRFS_ROOT_TRANS_TAG) {
 		int ret;
 
@@ -1426,7 +1426,7 @@ static noinline int commit_fs_roots(struct btrfs_trans_handle *trans)
 		xa_clear_mark(&fs_info->fs_roots,
 			      (unsigned long)root->root_key.objectid,
 			      BTRFS_ROOT_TRANS_TAG);
-		spin_unlock(&fs_info->fs_roots_lock);
+		mutex_unlock(&fs_info->fs_roots_mutex);
 
 		btrfs_free_log(trans, root);
 		ret = btrfs_update_reloc_root(trans, root);
@@ -1447,10 +1447,10 @@ static noinline int commit_fs_roots(struct btrfs_trans_handle *trans)
 					&root->root_key, &root->root_item);
 		if (ret)
 			return ret;
-		spin_lock(&fs_info->fs_roots_lock);
+		mutex_lock(&fs_info->fs_roots_mutex);
 		btrfs_qgroup_free_meta_all_pertrans(root);
 	}
-	spin_unlock(&fs_info->fs_roots_lock);
+	mutex_unlock(&fs_info->fs_roots_mutex);
 	return 0;
 }
 
-- 
2.35.1

[PATCH 3/3] btrfs: free qgroup metadata without holding the fs roots lock

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

At commit_fs_roots(), we are freeing the per transaction qgroup metadata
while holding the fs roots lock, which is not necessary as it does not
change anything in the root itself or the fs roots XArray. So change
that to be done without holding that lock.

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/transaction.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
index 1283be132776..e014b5d8a813 100644
--- a/fs/btrfs/transaction.c
+++ b/fs/btrfs/transaction.c
@@ -1447,8 +1447,8 @@ static noinline int commit_fs_roots(struct btrfs_trans_handle *trans)
 					&root->root_key, &root->root_item);
 		if (ret)
 			return ret;
-		mutex_lock(&fs_info->fs_roots_mutex);
 		btrfs_qgroup_free_meta_all_pertrans(root);
+		mutex_lock(&fs_info->fs_roots_mutex);
 	}
 	mutex_unlock(&fs_info->fs_roots_mutex);
 	return 0;
-- 
2.35.1

Re: [PATCH 0/3] btrfs: fix a couple sleeps while holding a spinlock

[David Sterba]

Adding Matthew to CC

On Wed, Jul 06, 2022 at 10:09:44AM +0100, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> After the recent conversions of a couple radix trees to XArrays, we now
> can end up attempting to sleep while holding a spinlock.

Ouch, I worked on the asumption that the old preload API is
transparently provided by xarray and that sleeping under spinlock won't
happen, otherwise the conversion from radix to xarray is not just an API
rename. Note that for some time the radix_tree structure was just an
alias for xarray, so this is not a new behaviour.

Re: [PATCH 0/3] btrfs: fix a couple sleeps while holding a spinlock

[Matthew Wilcox]

On Thu, Jul 07, 2022 at 06:31:44PM +0200, David Sterba wrote:
> Adding Matthew to CC
> 
> On Wed, Jul 06, 2022 at 10:09:44AM +0100, fdmanana@kernel.org wrote:
> > From: Filipe Manana <fdmanana@suse.com>
> > 
> > After the recent conversions of a couple radix trees to XArrays, we now
> > can end up attempting to sleep while holding a spinlock.
> 
> Ouch, I worked on the asumption that the old preload API is
> transparently provided by xarray and that sleeping under spinlock won't
> happen, otherwise the conversion from radix to xarray is not just an API
> rename. Note that for some time the radix_tree structure was just an
> alias for xarray, so this is not a new behaviour.

Umm ... the *structure* is identical, but the *API* is not.  Or the
conversion would have happened long ago.  There is no preload API
for the XArray because it's one of the worst features of the radix
tree API.

Re: [PATCH 0/3] btrfs: fix a couple sleeps while holding a spinlock

[Nikolay Borisov]

On 6.07.22 г. 12:09 ч., fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> After the recent conversions of a couple radix trees to XArrays, we now
> can end up attempting to sleep while holding a spinlock. This happens
> because if xa_insert() allocates memory (using GFP_NOFS) it may need to
> sleep (more likely to happen when under memory pressure). In the old
> code this did not happen because we had radix_tree_preload() called
> before taking the spinlocks.
> 
> Filipe Manana (3):
>    btrfs: fix sleep while under a spinlock when allocating delayed inode
>    btrfs: fix sleep while under a spinlock when inserting a fs root
>    btrfs: free qgroup metadata without holding the fs roots lock
> 
>   fs/btrfs/ctree.h         |  6 +++---
>   fs/btrfs/delayed-inode.c | 24 ++++++++++++------------
>   fs/btrfs/disk-io.c       | 38 +++++++++++++++++++-------------------
>   fs/btrfs/inode.c         | 30 ++++++++++++++++--------------
>   fs/btrfs/relocation.c    | 11 +++++++----
>   fs/btrfs/transaction.c   | 14 +++++++-------
>   6 files changed, 64 insertions(+), 59 deletions(-)
> 


Reviewed-by: Nikolay Borisov <nborisov@suse.com>

Re: [PATCH 0/3] btrfs: fix a couple sleeps while holding a spinlock

[Filipe Manana]

On Wed, Jul 06, 2022 at 10:09:44AM +0100, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> After the recent conversions of a couple radix trees to XArrays, we now
> can end up attempting to sleep while holding a spinlock. This happens
> because if xa_insert() allocates memory (using GFP_NOFS) it may need to
> sleep (more likely to happen when under memory pressure). In the old
> code this did not happen because we had radix_tree_preload() called
> before taking the spinlocks.
> 
> Filipe Manana (3):
>   btrfs: fix sleep while under a spinlock when allocating delayed inode
>   btrfs: fix sleep while under a spinlock when inserting a fs root
>   btrfs: free qgroup metadata without holding the fs roots lock

David, are you going to pick these up or revert the patches that did the
radix tree to xarray conversion?

> 
>  fs/btrfs/ctree.h         |  6 +++---
>  fs/btrfs/delayed-inode.c | 24 ++++++++++++------------
>  fs/btrfs/disk-io.c       | 38 +++++++++++++++++++-------------------
>  fs/btrfs/inode.c         | 30 ++++++++++++++++--------------
>  fs/btrfs/relocation.c    | 11 +++++++----
>  fs/btrfs/transaction.c   | 14 +++++++-------
>  6 files changed, 64 insertions(+), 59 deletions(-)
> 
> -- 
> 2.35.1
> 

Re: [PATCH 0/3] btrfs: fix a couple sleeps while holding a spinlock

[David Sterba]

On Wed, Jul 13, 2022 at 02:59:55PM +0100, Filipe Manana wrote:
> On Wed, Jul 06, 2022 at 10:09:44AM +0100, fdmanana@kernel.org wrote:
> > From: Filipe Manana <fdmanana@suse.com>
> > 
> > After the recent conversions of a couple radix trees to XArrays, we now
> > can end up attempting to sleep while holding a spinlock. This happens
> > because if xa_insert() allocates memory (using GFP_NOFS) it may need to
> > sleep (more likely to happen when under memory pressure). In the old
> > code this did not happen because we had radix_tree_preload() called
> > before taking the spinlocks.
> > 
> > Filipe Manana (3):
> >   btrfs: fix sleep while under a spinlock when allocating delayed inode
> >   btrfs: fix sleep while under a spinlock when inserting a fs root
> >   btrfs: free qgroup metadata without holding the fs roots lock
> 
> David, are you going to pick these up or revert the patches that did the
> radix tree to xarray conversion?

Switching sping lock to mutex seems quite heavy weight, and reverting
xarray conversion is intrusive, so it's choosing from two bad options,
also that we haven't identified the problems earlier. Doing such changes
in rc6 is quite unpleasant, I'll explore the options.

Re: [PATCH 0/3] btrfs: fix a couple sleeps while holding a spinlock

[Nikolay Borisov]

On 15.07.22 г. 15:01 ч., David Sterba wrote:
> On Wed, Jul 13, 2022 at 02:59:55PM +0100, Filipe Manana wrote:
>> On Wed, Jul 06, 2022 at 10:09:44AM +0100, fdmanana@kernel.org wrote:
>>> From: Filipe Manana <fdmanana@suse.com>
>>>
>>> After the recent conversions of a couple radix trees to XArrays, we now
>>> can end up attempting to sleep while holding a spinlock. This happens
>>> because if xa_insert() allocates memory (using GFP_NOFS) it may need to
>>> sleep (more likely to happen when under memory pressure). In the old
>>> code this did not happen because we had radix_tree_preload() called
>>> before taking the spinlocks.
>>>
>>> Filipe Manana (3):
>>>    btrfs: fix sleep while under a spinlock when allocating delayed inode
>>>    btrfs: fix sleep while under a spinlock when inserting a fs root
>>>    btrfs: free qgroup metadata without holding the fs roots lock
>>
>> David, are you going to pick these up or revert the patches that did the
>> radix tree to xarray conversion?
> 
> Switching sping lock to mutex seems quite heavy weight, and reverting
> xarray conversion is intrusive, so it's choosing from two bad options,
> also that we haven't identified the problems earlier. Doing such changes
> in rc6 is quite unpleasant, I'll explore the options.


I'm actually in favor of using the mutexes. For example looking at the 
users of root->inode_lock:

btrfs_prune_dentries - we hold the inode_lock and iterate the root's 
in-memory inode list until we find a starting node to start pruning. 
Pointer chasing isn't terribly performant.

fin_next_inode iin relocation - basically the same as btrfs_prune_dentries.

inode_tree_add - again, pointer chasing searching for the correct place 
to add an inode.

inode_tree_del - thiis is indeed a short critsec, but given that mutexes 
have an optimistic spin mode i don't think it'd be a problem here.


btrfs_kill_all_delayed_nodes - rather long critsec where all delayed 
inodes for a root are being iterated -> slow.


btrfs_get_delayed_node - rather short critsec, possibly handled by the 
optimistic spin mode of the mutex

btrfs_get_or_create_delayed_node - xa_insert called under it, in case of 
memory allocation - slow

__btrfs_release_delayed_node - short critsec, handled by optimistic spin

All in all to me it seems that spinlock is the wrong lock type for the 
kind of critical section being protected by it.


btrfs_fs_info::fs_roots_lock - this one indeed seems to protect mostly 
short-lived critsec but the mutexe's optimistic spin mode should handle 
it. But also looking at it I think some of the uses can be eliminated 
altogether and rely simply on the internal locking of the xarray.

Re: [PATCH 0/3] btrfs: fix a couple sleeps while holding a spinlock

[David Sterba]

On Fri, Jul 15, 2022 at 03:47:34PM +0300, Nikolay Borisov wrote:
> 
> 
> On 15.07.22 г. 15:01 ч., David Sterba wrote:
> > On Wed, Jul 13, 2022 at 02:59:55PM +0100, Filipe Manana wrote:
> >> On Wed, Jul 06, 2022 at 10:09:44AM +0100, fdmanana@kernel.org wrote:
> >>> From: Filipe Manana <fdmanana@suse.com>
> >>>
> >>> After the recent conversions of a couple radix trees to XArrays, we now
> >>> can end up attempting to sleep while holding a spinlock. This happens
> >>> because if xa_insert() allocates memory (using GFP_NOFS) it may need to
> >>> sleep (more likely to happen when under memory pressure). In the old
> >>> code this did not happen because we had radix_tree_preload() called
> >>> before taking the spinlocks.
> >>>
> >>> Filipe Manana (3):
> >>>    btrfs: fix sleep while under a spinlock when allocating delayed inode
> >>>    btrfs: fix sleep while under a spinlock when inserting a fs root
> >>>    btrfs: free qgroup metadata without holding the fs roots lock
> >>
> >> David, are you going to pick these up or revert the patches that did the
> >> radix tree to xarray conversion?
> > 
> > Switching sping lock to mutex seems quite heavy weight, and reverting
> > xarray conversion is intrusive, so it's choosing from two bad options,
> > also that we haven't identified the problems earlier. Doing such changes
> > in rc6 is quite unpleasant, I'll explore the options.
> 
> 
> I'm actually in favor of using the mutexes. For example looking at the 
> users of root->inode_lock:

We want to do the xarray conversion eventually but this would be better
done in the whole cycle, so I'm going to send the revert.