btrfs: qgroup: Delay subtree scan to reduce overhead

1073741824.

And to co-operate this, also delayed btrfs_drop_snapshot() call on reloc tree, btrfs_drop_snapshot() call will also be delayed to clean_dirty_subvs().

This patch will increase the size of btrfs_root by 16 bytes.

Signed-off-by Qu Wenruo <[email protected]>

fs/btrfs/relocation.c +12 -0

+static int clean_dirty_subvs(struct reloc_control *rc)
+{
+	struct btrfs_root *root;
+	struct btrfs_root *next;
+	int err = 0;
+	int ret;
+
+	list_for_each_entry_safe(root, next, &rc->dirty_subv_roots,
+				 reloc_dirty_list) {
+		struct btrfs_root *reloc_root = root->reloc_root;
+
+		clear_bit(BTRFS_ROOT_DEAD_RELOC_TREE, &root->state);

+}

Quoted 3 lines · Qu Wenruo

And to co-operate this, also delayed btrfs_drop_snapshot() call on reloc tree, btrfs_drop_snapshot() call will also be delayed to clean_dirty_subvs().

Can you please rephrase this paragraph?

Quoted 21 lines

This patch will increase the size of btrfs_root by 16 bytes.

Signed-off-by Qu Wenruo <[email protected]>

fs/btrfs/relocation.c +17 -0

+static int clean_dirty_subvs(struct reloc_control *rc)
+{
+	struct btrfs_root *root;
+	struct btrfs_root *next;
+	int err = 0;
+	int ret;
+
+	list_for_each_entry_safe(root, next, &rc->dirty_subv_roots,
+				 reloc_dirty_list) {
+		struct btrfs_root *reloc_root = root->reloc_root;
+
+		clear_bit(BTRFS_ROOT_DEAD_RELOC_TREE, &root->state);
+		list_del_init(&root->reloc_dirty_list);
+		root->reloc_root = NULL;
+		if (reloc_root) {
+			ret = btrfs_drop_snapshot(reloc_root, NULL, 0, 1);
+			if (ret < 0 && !err)

fs/btrfs/relocation.c static int find_next_key(struct btrfs_path *path, int level, · line 2176

Quoted 5 lines

fs/btrfs/relocation.c +5 -0

+				err = ret;
+		}
+		btrfs_put_fs_root(root);
+	}
+	return err;

Please dont use the err/ret style but 'ret' that matches function return type and for the temporary return values ret2 etc.

Quoted 2 lines

+}

Refactor btrfs_qgroup_trace_subtree_swap() into qgroup_trace_subtree_swap(), which only needs two extent buffer and some other bool to control the behavior.

This provides the basis for later delayed subtree scan work.

Signed-off-by Qu Wenruo <[email protected]>

---
 fs/btrfs/qgroup.c | 78 ++++++++++++++++++++++++++++++++++-------------
 1 file changed, 57 insertions(+), 21 deletions(-)

fs/btrfs/qgroup.c +15 -0

index ba30adac88a7..8fe6ebe9aef8 100644
@@ -1994,6 +1994,60 @@ static int qgroup_trace_new_subtree_blocks(struct btrfs_trans_handle* trans,
 	return ret;
 }
 
+static int qgroup_trace_subtree_swap(struct btrfs_trans_handle *trans,
+				struct extent_buffer *src_eb,
+				struct extent_buffer *dst_eb,
+				u64 last_snapshot, bool trace_leaf)
+{
+	struct btrfs_fs_info *fs_info = trans->fs_info;
+	struct btrfs_path *dst_path = NULL;
+	int level;
+	int ret;
+
+	if (!test_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags))
+		return 0;
+
+	/* Wrong parameter order */
+	if (btrfs_header_generation(src_eb) > btrfs_header_generation(dst_eb)) {

fs/btrfs/qgroup.c +28 -0

+		goto out;
+	}
+
+	level = btrfs_header_level(dst_eb);
+	dst_path = btrfs_alloc_path();
+	if (!dst_path) {
+		ret = -ENOMEM;
+		goto out;
+	}
+	/* For dst_path */
+	extent_buffer_get(dst_eb);
+	dst_path->nodes[level] = dst_eb;
+	dst_path->slots[level] = 0;
+	dst_path->locks[level] = 0;
+
+	/* Do the generation aware breadth-first search */
+	ret = qgroup_trace_new_subtree_blocks(trans, src_eb, dst_path, level,
+					      level, last_snapshot, trace_leaf);
+	if (ret < 0)
+		goto out;
+	ret = 0;
+
+out:
+	btrfs_free_path(dst_path);
+	if (ret < 0)
+		fs_info->qgroup_flags |= BTRFS_QGROUP_STATUS_FLAG_INCONSISTENT;
+	return ret;
+}

fs/btrfs/ctree.h +5 -0

+};
+
 /*
  * in ram representation of the tree.  extent_root is used for all allocations
  * and for the extent tree extent_root root.

@@ -1339,6 +1350,9 @@ struct btrfs_root {
 	/* Number of active swapfiles */
 	atomic_t nr_swapfiles;
 
+	/* Record pairs of swapped blocks for qgroup */
+	struct btrfs_qgroup_swapped_blocks swapped_blocks;
+
 #ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS
 	u64 alloc_bytenr;
 #endif

fs/btrfs/disk-io.c +1 -0

index bfefa1de0455..31b2facdfc1e 100644
@@ -1219,6 +1219,7 @@ static void __setup_root(struct btrfs_root *root, struct btrfs_fs_info *fs_info,
 	root->anon_dev = 0;
 
 	spin_lock_init(&root->root_item_lock);
+	btrfs_qgroup_init_swapped_blocks(&root->swapped_blocks);
 }
 
 static struct btrfs_root *btrfs_alloc_root(struct btrfs_fs_info *fs_info,

fs/btrfs/qgroup.c +44 -0

index 8fe6ebe9aef8..001830328960 100644
@@ -3797,3 +3797,133 @@ void btrfs_qgroup_check_reserved_leak(struct inode *inode)
 	}
 	extent_changeset_release(&changeset);
 }
+
+/*
+ * Delete all swapped blocks record of @root.
+ * Every record here means we skipped a full subtree scan for qgroup.
+ *
+ * Get called when commit one transaction.
+ */
+void btrfs_qgroup_clean_swapped_blocks(struct btrfs_root *root)
+{
+	struct btrfs_qgroup_swapped_blocks *swapped_blocks;
+	int i;
+
+	swapped_blocks = &root->swapped_blocks;
+
+	spin_lock(&swapped_blocks->lock);
+	if (!swapped_blocks->swapped)
+		goto out;
+	for (i = 0; i < BTRFS_MAX_LEVEL; i++) {
+		struct rb_root *cur_root = &swapped_blocks->blocks[i];
+		struct btrfs_qgroup_swapped_block *entry;
+		struct btrfs_qgroup_swapped_block *next;
+
+		rbtree_postorder_for_each_entry_safe(entry, next, cur_root,
+						     node)
+			kfree(entry);
+		swapped_blocks->blocks[i] = RB_ROOT;
+	}
+	swapped_blocks->swapped = false;
+out:
+	spin_unlock(&swapped_blocks->lock);
+}
+
+/*
+ * Adding subtree roots record into @subv_root.
+ *
+ * @subv_root:		tree root of the subvolume tree get swapped
+ * @bg:			block group under balance
+ * @subv_parent/slot:	pointer to the subtree root in file tree
+ * @reloc_parent/slot:	pointer to the subtree root in reloc tree
+ *			BOTH POINTERS ARE BEFORE TREE SWAP
+ * @last_snapshot:	last snapshot generation of the file tree
+ */
+int btrfs_qgroup_add_swapped_blocks(struct btrfs_trans_handle *trans,
+		struct btrfs_root *subv_root,

fs/btrfs/qgroup.c +9 -0

+	struct rb_node *parent = NULL;
+	int ret = 0;
+
+	if (!test_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags))
+		return 0;
+
+	if (btrfs_node_ptr_generation(subv_parent, subv_slot) >
+		btrfs_node_ptr_generation(reloc_parent, reloc_slot)) {
+		btrfs_err_rl(fs_info,

fs/btrfs/qgroup.c +7 -0

+	 */
+	block->subv_bytenr = btrfs_node_blockptr(reloc_parent, reloc_slot);
+	block->subv_generation = btrfs_node_ptr_generation(reloc_parent,
+							   reloc_slot);
+	block->reloc_bytenr = btrfs_node_blockptr(subv_parent, subv_slot);
+	block->reloc_generation = btrfs_node_ptr_generation(subv_parent,
+							    subv_slot);

fs/btrfs/qgroup.c +17 -0

+				ret = -EEXIST;
+			}
+			kfree(block);
+			goto out_unlock;
+		}
+	}
+	rb_link_node(&block->node, parent, p);
+	rb_insert_color(&block->node, &blocks->blocks[level]);
+	blocks->swapped = true;
+out_unlock:
+	spin_unlock(&blocks->lock);
+out:
+	if (ret < 0)
+		fs_info->qgroup_flags |=
+			BTRFS_QGROUP_STATUS_FLAG_INCONSISTENT;
+	return ret;
+}

fs/btrfs/qgroup.h +2 -0

index 226956f0bd73..93d7f0998f03 100644
@@ -6,6 +6,8 @@
 #ifndef BTRFS_QGROUP_H
 #define BTRFS_QGROUP_H
 
+#include <linux/spinlock.h>
+#include <linux/rbtree.h>
 #include "ulist.h"
 #include "delayed-ref.h"
 

fs/btrfs/qgroup.h +58 -0

+ *
+ * For balance, we need to swap subtree of file and reloc tree.
+ * In theory, we need to trace all subtree blocks of both file and reloc tree,
+ * since their owner has changed during such swap.
+ *
+ * However since balance has ensured that both subtrees are containing the
+ * same contents and have the same tree structures, such swap won't cause
+ * qgroup number change.
+ *
+ * But there is a race window between subtree swap and transaction commit,
+ * during that window, if we increase/decrease tree level or merge/split tree
+ * blocks, we still needs to trace original subtrees.
+ *
+ * So for balance, we use a delayed subtree trace, whose workflow is:
+ *
+ * 1) Record the subtree root block get swapped.
+ *
+ *    During subtree swap:
+ *    O = Old tree blocks
+ *    N = New tree blocks
+ *          reloc tree                         file tree X
+ *             Root                               Root
+ *            /    \                             /    \
+ *          NA     OB                          OA      OB
+ *        /  |     |  \                      /  |      |  \
+ *      NC  ND     OE  OF                   OC  OD     OE  OF
+ *
+ *   In these case, NA and OA is going to be swapped, record (NA, OA) into
+ *   file tree X.
+ *
+ * 2) After subtree swap.
+ *          reloc tree                         file tree X
+ *             Root                               Root
+ *            /    \                             /    \
+ *          OA     OB                          NA      OB
+ *        /  |     |  \                      /  |      |  \
+ *      OC  OD     OE  OF                   NC  ND     OE  OF
+ *
+ * 3a) CoW happens for OB
+ *     If we are going to CoW tree block OB, we check OB's bytenr against
+ *     tree X's swapped_blocks structure.
+ *     It doesn't fit any one, nothing will happen.
+ *
+ * 3b) CoW happens for NA
+ *     Check NA's bytenr against tree X's swapped_blocks, and get a hit.
+ *     Then we do subtree scan on both subtree OA and NA.
+ *     Resulting 6 tree blocks to be scanned (OA, OC, OD, NA, NC, ND).
+ *
+ *     Then no matter what we do to file tree X, qgroup numbers will
+ *     still be correct.
+ *     Then NA's record get removed from X's swapped_blocks.
+ *
+ * 4)  Transaction commit
+ *     Any record in X's swapped_blocks get removed, since there is no
+ *     modification to swapped subtrees, no need to trigger heavy qgroup
+ *     subtree rescan for them.
+ */
+
 /*
  * Record a dirty extent, and info qgroup to update quota on it
  * TODO: Use kmem cache to alloc it.

fs/btrfs/qgroup.h +13 -0

+
+	/* bytenr/generation of the tree block in subvolume tree after swap */
+	u64 subv_bytenr;
+	u64 subv_generation;
+
+	/* bytenr/generation of the tree block in reloc tree after swap */
+	u64 reloc_bytenr;
+	u64 reloc_generation;
+
+	u64 last_snapshot;
+	struct btrfs_key first_key;
+};
+
 /*
  * Qgroup reservation types:
  *

fs/btrfs/qgroup.h +7 -0

+		struct btrfs_qgroup_swapped_blocks *swapped_blocks)
+{
+	int i;
+
+	spin_lock_init(&swapped_blocks->lock);
+	for (i = 0; i < BTRFS_MAX_LEVEL; i++)
+		swapped_blocks->blocks[i] = RB_ROOT;

fs/btrfs/qgroup.h +0 -0

 #endif

On Tue, Jan 15, 2019 at 04:16:02PM +0800, Qu Wenruo wrote:

fs/btrfs/qgroup.c void btrfs_qgroup_check_reserved_leak(struct inode *inode) · line 3872

Quoted 4 lines

fs/btrfs/qgroup.c +4 -0

+
+	block = kmalloc(sizeof(*block), GFP_NOFS);
+	if (!block) {
+		ret = -ENOMEM;

This goes to 'out' that marks quota as inconsistent, is this intentional?

Ie. if this function does not succed for whatever reason, the quotas are indeed inconsistent.

fs/btrfs/qgroup.c void btrfs_qgroup_check_reserved_leak(struct inode *inode) · line 3907

Quoted 13 lines

fs/btrfs/qgroup.c +12 -0

+	while (*p) {
+		struct btrfs_qgroup_swapped_block *entry;
+
+		parent = *p;
+		entry = rb_entry(parent, struct btrfs_qgroup_swapped_block,
+				 node);
+
+		if (entry->subv_bytenr < block->subv_bytenr)
+			p = &(*p)->rb_left;
+		else if (entry->subv_bytenr > block->subv_bytenr)
+			p = &(*p)->rb_right;
+		else {

I've pointed out missing { } around if/else if/else statemtents in patch 1, would be good if you fix all patches.

fs/btrfs/qgroup.c void btrfs_qgroup_check_reserved_leak(struct inode *inode) · line 3912

Quoted 6 lines

fs/btrfs/qgroup.c +5 -0

+			if (entry->subv_generation != block->subv_generation ||
+			    entry->reloc_bytenr != block->reloc_bytenr ||
+			    entry->reloc_generation !=
+			    block->reloc_generation) {
+				WARN_ON_ONCE(1);

This would need some explanation why you want to see the warning.

_ONCE
is for the whole lifetime of the module but I think you'd be interested on a per-filesystem basis. If this is meant for developers it should go under DEBUG, but otherwise I don't see what would this warning bring to the users.

Quoted 3 lines

+/*
+ * Special performance hack for balance.

Is it really a hack? Hack sounds cool, but isn't it just an optimization?

fs/btrfs/qgroup.h struct btrfs_qgroup_extent_record { · line 128

Quoted 6 lines

fs/btrfs/qgroup.h +5 -0

+struct btrfs_qgroup_swapped_block {
+	struct rb_node node;
+
+	bool trace_leaf;
+	int level;

Please reorder that so int goes before bool, othewise this adds unnecessary padding.

Quoted 1 line · David Sterba

On Tue, Jan 15, 2019 at 04:16:02PM +0800, Qu Wenruo wrote:

<snip>

Quoted 7 lines

fs/btrfs/qgroup.c void btrfs_qgroup_check_reserved_leak(struct inode *inode) · line 3872

Quoted 4 lines

fs/btrfs/qgroup.c +4 -0

+
+	block = kmalloc(sizeof(*block), GFP_NOFS);
+	if (!block) {
+		ret = -ENOMEM;

This goes to 'out' that marks quota as inconsistent, is this intentional?

Yes

Quoted 2 lines

Ie. if this function does not succed for whatever reason, the quotas are indeed inconsistent.

If we hit this ENOMEM case, although it's possible later CoW doesn't touch this subtree, it's also possible later CoW touches this and make qgroup numbers inconsistent.

So we should mark qgroup inconsistent for the possibility of inconsistent qgroup numbers.

[snip]

Quoted 16 lines

fs/btrfs/qgroup.c void btrfs_qgroup_check_reserved_leak(struct inode *inode) · line 3907

Quoted 13 lines

fs/btrfs/qgroup.c +12 -0

+	while (*p) {
+		struct btrfs_qgroup_swapped_block *entry;
+
+		parent = *p;
+		entry = rb_entry(parent, struct btrfs_qgroup_swapped_block,
+				 node);
+
+		if (entry->subv_bytenr < block->subv_bytenr)
+			p = &(*p)->rb_left;
+		else if (entry->subv_bytenr > block->subv_bytenr)
+			p = &(*p)->rb_right;
+		else {

I've pointed out missing { } around if/else if/else statemtents in patch 1, would be good if you fix all patches.

Sorry, it's sometimes hard to apply the same comment to older branches. Normally I'll pay more attention for newly written code, but it's not that easy to spot older code.

Quoted 8 lines

fs/btrfs/qgroup.c void btrfs_qgroup_check_reserved_leak(struct inode *inode) · line 3912

Quoted 6 lines

fs/btrfs/qgroup.c +5 -0

+			if (entry->subv_generation != block->subv_generation ||
+			    entry->reloc_bytenr != block->reloc_bytenr ||
+			    entry->reloc_generation !=
+			    block->reloc_generation) {
+				WARN_ON_ONCE(1);

This would need some explanation why you want to see the warning.

The case here is, I don't want to populate the whole dmesg with tons of possible backtrace from this call sites, but I still hope user to report such error.

So I choose _ONCE() to inform user to report the error, as I don't really see it's possible.

Quoted 5 lines

_ONCE
is for the whole lifetime of the module but I think you'd be interested on a per-filesystem basis. If this is meant for developers it should go under DEBUG, but otherwise I don't see what would this warning bring to the users.

OK, I'll put it under DEBUG and change from ONCE to regular WARN_ON().

[snip]

Quoted 5 lines

Quoted 3 lines

+/*
+ * Special performance hack for balance.

Is it really a hack? Hack sounds cool, but isn't it just an optimization?

It's optimization indeed.
(But hack sounds really cool).

Quoted 9 lines

fs/btrfs/qgroup.h struct btrfs_qgroup_extent_record { · line 128

Quoted 6 lines

fs/btrfs/qgroup.h +5 -0

+struct btrfs_qgroup_swapped_block {
+	struct rb_node node;
+
+	bool trace_leaf;
+	int level;

Please reorder that so int goes before bool, othewise this adds unnecessary padding.

Can we make compiler to do this work?

fs/btrfs/qgroup.c +5 -0

+	bool found = false;
+	bool swapped = false;
+	int level = btrfs_header_level(subv_eb);
+	int ret = 0;
+	int i;

fs/btrfs/qgroup.c +57 -0

+	}
+	n = blocks->blocks[level].rb_node;
+
+	while (n) {
+		block = rb_entry(n, struct btrfs_qgroup_swapped_block, node);
+		if (block->subv_bytenr < subv_eb->start)
+			n = n->rb_left;
+		else if (block->subv_bytenr > subv_eb->start)
+			n = n->rb_right;
+		else {
+			found = true;
+			break;
+		}
+	}
+	if (!found) {
+		spin_unlock(&blocks->lock);
+		goto out;
+	}
+	/* Found one, remove it from @blocks first and update blocks->swapped */
+	rb_erase(&block->node, &blocks->blocks[level]);
+	for (i = 0; i < BTRFS_MAX_LEVEL; i++) {
+		if (RB_EMPTY_ROOT(&blocks->blocks[i])) {
+			swapped = true;
+			break;
+		}
+	}
+	blocks->swapped = swapped;
+	spin_unlock(&blocks->lock);
+
+	/* Read out reloc subtree root */
+	reloc_eb = read_tree_block(fs_info, block->reloc_bytenr,
+				   block->reloc_generation, block->level,
+				   &block->first_key);
+	if (IS_ERR(reloc_eb)) {
+		ret = PTR_ERR(subv_eb);
+		reloc_eb = NULL;
+		goto free_out;
+	}
+	if (!extent_buffer_uptodate(reloc_eb)) {
+		ret = -EIO;
+		goto free_out;
+	}
+
+	ret = qgroup_trace_subtree_swap(trans, reloc_eb, subv_eb,
+			block->last_snapshot, block->trace_leaf);
+free_out:
+	kfree(block);
+	free_extent_buffer(reloc_eb);
+out:
+	if (ret < 0) {
+		btrfs_err_rl(fs_info,
+			     "failed to account subtree at bytenr %llu: %d",
+			     subv_eb->start, ret);
+		fs_info->qgroup_flags |= BTRFS_QGROUP_STATUS_FLAG_INCONSISTENT;
+	}
+	return ret;
+}

This patchset can be fetched from github:
https://github.com/adam900710/linux/tree/qgroup_delayed_subtree

Which is based on v5.0-rc1.

Quoted 5 lines · Qu Wenruo

This patchset can be fetched from github:
https://github.com/adam900710/linux/tree/qgroup_delayed_subtree

Which is based on v5.0-rc1.

I've been testing these patches hoping to get rid of the qgroup deadlock that these patches are supposed to fix, but instead they make the box completely unusable with 100% cpu usage for minutes at a time at every transaction commit.

Thanks,

Josef

Quoted 10 lines · Josef Bacik

Quoted 5 lines · Qu Wenruo

This patchset can be fetched from github:
https://github.com/adam900710/linux/tree/qgroup_delayed_subtree

Which is based on v5.0-rc1.

I've been testing these patches hoping to get rid of the qgroup deadlock that these patches are supposed to fix, but instead they make the box completely unusable with 100% cpu usage for minutes at a time at every transaction commit.

I'm afraid it's related to the v5.0-rc1 base, not the patchset itself.

Just try to balance metadata with 16 snapshots, you'll see btrfs bumping its generation like crazy, no matter if quota is enabled or not.

And since btrfs is committing transaction like crazy, no wonder it will do tons of qgroup accounting.

My bisect leads to commit 64403612b73a94bc7b02cf8ca126e3b8ced6e921
btrfs: rework btrfs_check_space_for_delayed_refs.

Thanks,
Qu

Quoted 4 lines

Thanks,

Josef

Quoted 16 lines · Qu Wenruo

Quoted 10 lines · Josef Bacik

Quoted 5 lines · Qu Wenruo

This patchset can be fetched from github:
https://github.com/adam900710/linux/tree/qgroup_delayed_subtree

Which is based on v5.0-rc1.

I've been testing these patches hoping to get rid of the qgroup deadlock that these patches are supposed to fix, but instead they make the box completely unusable with 100% cpu usage for minutes at a time at every transaction commit.

I'm afraid it's related to the v5.0-rc1 base, not the patchset itself.

Quoted 8 lines

Just try to balance metadata with 16 snapshots, you'll see btrfs bumping its generation like crazy, no matter if quota is enabled or not.

And since btrfs is committing transaction like crazy, no wonder it will do tons of qgroup accounting.

My bisect leads to commit 64403612b73a94bc7b02cf8ca126e3b8ced6e921
btrfs: rework btrfs_check_space_for_delayed_refs.

Furthermore, you could try this RFC test case to see the problem.
https://patchwork.kernel.org/patch/10761715/

It would only take around 100s for v4.20 but over 500 for v5.0-rc1 with tons of unnecessary transaction committed for nothing, no quota enabled.

So I'm afraid that commit is blocking my qgroup patchset.

Quoted 28 lines · Qu Wenruo

Quoted 16 lines · Qu Wenruo

Quoted 10 lines · Josef Bacik

Quoted 5 lines · Qu Wenruo

This patchset can be fetched from github:
https://github.com/adam900710/linux/tree/qgroup_delayed_subtree

Which is based on v5.0-rc1.

I've been testing these patches hoping to get rid of the qgroup deadlock that these patches are supposed to fix, but instead they make the box completely unusable with 100% cpu usage for minutes at a time at every transaction commit.

I'm afraid it's related to the v5.0-rc1 base, not the patchset itself.

Quoted 8 lines

Just try to balance metadata with 16 snapshots, you'll see btrfs bumping its generation like crazy, no matter if quota is enabled or not.

And since btrfs is committing transaction like crazy, no wonder it will do tons of qgroup accounting.

My bisect leads to commit 64403612b73a94bc7b02cf8ca126e3b8ced6e921
btrfs: rework btrfs_check_space_for_delayed_refs.

Quoted 8 lines

Furthermore, you could try this RFC test case to see the problem.
https://patchwork.kernel.org/patch/10761715/

It would only take around 100s for v4.20 but over 500 for v5.0-rc1 with tons of unnecessary transaction committed for nothing, no quota enabled.

So I'm afraid that commit is blocking my qgroup patchset.

I've fixed the balance problem, it took 2 seconds to figure out, I'm just waiting on xfstests to finish running.

And your patch making things worse has nothing to do with that problem. Our test doesn't run balance, so the issue you reported has nothing to do with the fact that your patch makes our boxes unusable with qgroups on.

The problem is with your deadlock avoidence patch we're now making a lot more dirty extents to run in the critical section of the transaction commit. Also because we're no longer pre-fetching the old roots, we're doing the old roots and new roots lookup inside the critical section, so now each dirty extent takes 2x as long. With my basic test it was taking 5 minutes to do the qgroup accounting, which keeps the box from booting essentially.

Without your patch it's still awful because btrfs-cleaner just sits there at 100% while deleting snapshots, but at least it's not making the whole system stop running while it does all that work in the transaction commit.

And if you had done the proper root cause analysis you would have noticed that we're taking tree locks in the find_parent_nodes() case when we're searching the commit root, something we shouldn't be doing. So all that really needs to be done is to check if (!path->skip_locking) btrfs_tree_read_lock(eb); in those cases and the deadlock goes away. Because no matter what we shouldn't be taking locks when we're not given a trans in the backref lookup code.

So the fact that
we were doing just that and thus deadlocking should have been a red flag.

I will be sending these patches in the morning, once all of the various testing that should take place on patches is complete. The balance patches you have for qgroups don't appear to be a problem, but that deadlock one is bogus and needs to be dropped. Thanks,

Josef

Quoted 56 lines · Josef Bacik

Quoted 28 lines · Qu Wenruo

Quoted 16 lines · Qu Wenruo

Quoted 10 lines · Josef Bacik

Quoted 5 lines · Qu Wenruo

This patchset can be fetched from github:
https://github.com/adam900710/linux/tree/qgroup_delayed_subtree

Which is based on v5.0-rc1.

I've been testing these patches hoping to get rid of the qgroup deadlock that these patches are supposed to fix, but instead they make the box completely unusable with 100% cpu usage for minutes at a time at every transaction commit.

I'm afraid it's related to the v5.0-rc1 base, not the patchset itself.

Quoted 8 lines

Just try to balance metadata with 16 snapshots, you'll see btrfs bumping its generation like crazy, no matter if quota is enabled or not.

And since btrfs is committing transaction like crazy, no wonder it will do tons of qgroup accounting.

My bisect leads to commit 64403612b73a94bc7b02cf8ca126e3b8ced6e921
btrfs: rework btrfs_check_space_for_delayed_refs.

Quoted 8 lines

Furthermore, you could try this RFC test case to see the problem.
https://patchwork.kernel.org/patch/10761715/

It would only take around 100s for v4.20 but over 500 for v5.0-rc1 with tons of unnecessary transaction committed for nothing, no quota enabled.

So I'm afraid that commit is blocking my qgroup patchset.

I've fixed the balance problem, it took 2 seconds to figure out, I'm just waiting on xfstests to finish running.

And your patch making things worse has nothing to do with that problem. Our test doesn't run balance, so the issue you reported has nothing to do with the fact that your patch makes our boxes unusable with qgroups on.

The problem is with your deadlock avoidence patch we're now making a lot more dirty extents to run in the critical section of the transaction commit. Also because we're no longer pre-fetching the old roots, we're doing the old roots and new roots lookup inside the critical section, so now each dirty extent takes 2x as long. With my basic test it was taking 5 minutes to do the qgroup accounting, which keeps the box from booting essentially.

Without your patch it's still awful because btrfs-cleaner just sits there at 100% while deleting snapshots, but at least it's not making the whole system stop running while it does all that work in the transaction commit.

Quoted 6 lines

And if you had done the proper root cause analysis you would have noticed that we're taking tree locks in the find_parent_nodes() case when we're searching the commit root, something we shouldn't be doing. So all that really needs to be done is to check if (!path->skip_locking) btrfs_tree_read_lock(eb); in those cases and the deadlock goes away. Because no matter what we shouldn't be taking locks when we're not given a trans in the backref lookup code.

That indeed looks much better than my current solution.

Although I'm not 100% sure for cases like tree blocks shared between commit and current root (tree block not modified yet).

I'll definitely invest more time to try to fix this bug.

Quoted 66 lines · Qu Wenruo

Quoted 56 lines · Josef Bacik

Quoted 28 lines · Qu Wenruo

Quoted 16 lines · Qu Wenruo

Quoted 10 lines · Josef Bacik

Quoted 5 lines · Qu Wenruo

This patchset can be fetched from github:
https://github.com/adam900710/linux/tree/qgroup_delayed_subtree

Which is based on v5.0-rc1.

I've been testing these patches hoping to get rid of the qgroup deadlock that these patches are supposed to fix, but instead they make the box completely unusable with 100% cpu usage for minutes at a time at every transaction commit.

I'm afraid it's related to the v5.0-rc1 base, not the patchset itself.

Quoted 8 lines

Just try to balance metadata with 16 snapshots, you'll see btrfs bumping its generation like crazy, no matter if quota is enabled or not.

And since btrfs is committing transaction like crazy, no wonder it will do tons of qgroup accounting.

My bisect leads to commit 64403612b73a94bc7b02cf8ca126e3b8ced6e921
btrfs: rework btrfs_check_space_for_delayed_refs.

Quoted 8 lines

Furthermore, you could try this RFC test case to see the problem.
https://patchwork.kernel.org/patch/10761715/

It would only take around 100s for v4.20 but over 500 for v5.0-rc1 with tons of unnecessary transaction committed for nothing, no quota enabled.

So I'm afraid that commit is blocking my qgroup patchset.

I've fixed the balance problem, it took 2 seconds to figure out, I'm just waiting on xfstests to finish running.

And your patch making things worse has nothing to do with that problem. Our test doesn't run balance, so the issue you reported has nothing to do with the fact that your patch makes our boxes unusable with qgroups on.

The problem is with your deadlock avoidence patch we're now making a lot more dirty extents to run in the critical section of the transaction commit. Also because we're no longer pre-fetching the old roots, we're doing the old roots and new roots lookup inside the critical section, so now each dirty extent takes 2x as long. With my basic test it was taking 5 minutes to do the qgroup accounting, which keeps the box from booting essentially.

Without your patch it's still awful because btrfs-cleaner just sits there at 100% while deleting snapshots, but at least it's not making the whole system stop running while it does all that work in the transaction commit.

Quoted 6 lines

And if you had done the proper root cause analysis you would have noticed that we're taking tree locks in the find_parent_nodes() case when we're searching the commit root, something we shouldn't be doing. So all that really needs to be done is to check if (!path->skip_locking) btrfs_tree_read_lock(eb); in those cases and the deadlock goes away. Because no matter what we shouldn't be taking locks when we're not given a trans in the backref lookup code.

Quoted 4 lines

That indeed looks much better than my current solution.

Although I'm not 100% sure for cases like tree blocks shared between commit and current root (tree block not modified yet).

Doesn't matter, the block can't be modified so we don't need the locking, If the current root needs to change it then it cow's it and messes with the new eb, not the one attached to the commit root.

Quoted 1 line

Quoted 2 lines

I'll definitely invest more time to try to fix this bug.

Don't worry about it, I'm already running the patch through my tests, I'll send them in the morning when the tests are finished. Thanks,

Josef

This patchset can be fetched from github:
https://github.com/adam900710/linux/tree/qgroup_delayed_subtree

Which is based on v5.0-rc1.

This patch address the heavy load subtree scan, but delaying it until we're going to modify the swapped tree block.

v4:
- Renaming members from "file_*" to "subv_*".
Members like "file_bytenr" is pretty confusing, renaming it to "subv_bytenr" avoid the confusion.

Quoted 7 lines · Qu Wenruo

This patchset can be fetched from github:
https://github.com/adam900710/linux/tree/qgroup_delayed_subtree

Which is based on v5.0-rc1.

This patch address the heavy load subtree scan, but delaying it until we're going to modify the swapped tree block.

I'd like to merge this patchset, the overall logic is fine but I've found lots of coding style issues, too many to fix them just myself.

Quoted 4 lines

v4:
- Renaming members from "file_*" to "subv_*".
Members like "file_bytenr" is pretty confusing, renaming it to "subv_bytenr" avoid the confusion.

The naming change is incomplete, there are many more references to 'file tree' in comments and changelogs, the naming should be unified everywhere. Also I'd prefer to use 'subvol' though it's a bit longer than 'subv', the former seems to be the most common form.

I'll comment the patches with the rest.