[PATCH 0/6] btrfs: some qgroup fixes and cleanups

[PATCH 0/6] btrfs: some qgroup fixes and cleanups

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

Fix a few a bugs related to qgroups and some cleanups.

Filipe Manana (6):
  btrfs: qgroup: fix swapped blocks existence check when tracing after COW
  btrfs: qgroup: abort transaction on failure to add qgroup relation
  btrfs: qgroup: fix leak of qgroups in rb tree after failure to enable quotas
  btrfs: qgroup: merge error labels in btrfs_quota_enable()
  btrfs: abort transaction on qgroup failures in create_pending_snapshot()
  btrfs: qgroup: fix off-by-one max level check in qgroup_trace_new_subtree_blocks()

 fs/btrfs/qgroup.c      | 55 ++++++++++++++++++++++++------------------
 fs/btrfs/transaction.c | 15 +++++++++---
 2 files changed, 42 insertions(+), 28 deletions(-)

-- 
2.47.2

[PATCH] btrfs: qgroup: fix race between subvolume creation and adding/removing relations

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

When using simple quotas, we can have a race between creating a subvolume
and auto inheriting quotas and the ioctls to add or remove qgroup
relations.

This happens like this:

1) Task A is doing subvolume creation and calls btrfs_qgroup_inherit()
   with the "inherit" parameter as NULL (since BTRFS_SUBVOL_QGROUP_INHERIT
   was not passed in the flags of the subvolume creation ioctl).

2) Task A enters qgroup_auto_inherit() and calls list_count_nodes() to
   get the number of nodes in a qgroup's list, then allocates an array
   with a size matching that number of nodes.

3) Task B enters the BTRFS_IOC_QGROUP_ASSIGN ioctl to add a qgroup
   relation and enters btrfs_add_qgroup_relation() where it locks
   fs_info->qgroup_lock before calling __add_relation_rb() where it
   adds to the list of the same qgroup that is being processed by
   task A.

4) Task A iterates over the qgroup's list elements and assigns to an
   array that has an insufficient size, since the number of elements in
   the list is now larger, by 1, compared to the count it got in step 2,
   resulting in an out of bounds array access.

This all happens because btrfs_qgroup_inherit() does not take the lock
fs_info->qgroup_lock before counting the number of elements in a qgroup's
list and iterating it. So all sorts of other races can happen, like
iterating the list while another task is calling either
btrfs_add_qgroup_relation() or btrfs_del_qgroup_relation() and modifying
the same qgroup list concurrently.

So make btrfs_qgroup_inherit() take fs_info->qgroup_lock.

Fixes: 5343cd9364ea ("btrfs: qgroup: simple quota auto hierarchy for nested subvolumes")
Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/qgroup.c | 27 +++++++++++++++++++--------
 1 file changed, 19 insertions(+), 8 deletions(-)

diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index c07cf6041cb9..195326ef0388 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -3256,26 +3256,34 @@ static int qgroup_auto_inherit(struct btrfs_fs_info *fs_info,
 	struct btrfs_qgroup_inherit *res;
 	size_t struct_sz;
 	u64 *qgids;
+	int ret = 0;
 
 	if (*inherit)
 		return -EEXIST;
 
+	spin_lock(&fs_info->qgroup_lock);
 	inode_qg = find_qgroup_rb(fs_info, inode_rootid);
-	if (!inode_qg)
-		return -ENOENT;
+	if (!inode_qg) {
+		ret = -ENOENT;
+		goto out;
+	}
 
 	num_qgroups = list_count_nodes(&inode_qg->groups);
 
 	if (!num_qgroups)
-		return 0;
+		goto out;
 
 	struct_sz = struct_size(res, qgroups, num_qgroups);
-	if (struct_sz == SIZE_MAX)
-		return -ERANGE;
+	if (struct_sz == SIZE_MAX) {
+		ret = -ERANGE;
+		goto out;
+	}
 
 	res = kzalloc(struct_sz, GFP_NOFS);
-	if (!res)
-		return -ENOMEM;
+	if (!res) {
+		ret = -ENOMEM;
+		goto out;
+	}
 	res->num_qgroups = num_qgroups;
 	qgids = res->qgroups;
 
@@ -3283,7 +3291,10 @@ static int qgroup_auto_inherit(struct btrfs_fs_info *fs_info,
 		qgids[i++] = qg_list->group->qgroupid;
 
 	*inherit = res;
-	return 0;
+out:
+	spin_unlock(&fs_info->qgroup_lock);
+
+	return ret;
 }
 
 /*
-- 
2.47.2

Re: [PATCH] btrfs: qgroup: fix race between subvolume creation and adding/removing relations

[Filipe Manana]

On Fri, Oct 2, 2026 at 7:56 PM <fdmanana@kernel.org> wrote:
>
> From: Filipe Manana <fdmanana@suse.com>
>
> When using simple quotas, we can have a race between creating a subvolume
> and auto inheriting quotas and the ioctls to add or remove qgroup
> relations.
>
> This happens like this:
>
> 1) Task A is doing subvolume creation and calls btrfs_qgroup_inherit()
>    with the "inherit" parameter as NULL (since BTRFS_SUBVOL_QGROUP_INHERIT
>    was not passed in the flags of the subvolume creation ioctl).
>
> 2) Task A enters qgroup_auto_inherit() and calls list_count_nodes() to
>    get the number of nodes in a qgroup's list, then allocates an array
>    with a size matching that number of nodes.
>
> 3) Task B enters the BTRFS_IOC_QGROUP_ASSIGN ioctl to add a qgroup
>    relation and enters btrfs_add_qgroup_relation() where it locks
>    fs_info->qgroup_lock before calling __add_relation_rb() where it
>    adds to the list of the same qgroup that is being processed by
>    task A.
>
> 4) Task A iterates over the qgroup's list elements and assigns to an
>    array that has an insufficient size, since the number of elements in
>    the list is now larger, by 1, compared to the count it got in step 2,
>    resulting in an out of bounds array access.
>
> This all happens because btrfs_qgroup_inherit() does not take the lock
> fs_info->qgroup_lock before counting the number of elements in a qgroup's
> list and iterating it. So all sorts of other races can happen, like
> iterating the list while another task is calling either
> btrfs_add_qgroup_relation() or btrfs_del_qgroup_relation() and modifying
> the same qgroup list concurrently.
>
> So make btrfs_qgroup_inherit() take fs_info->qgroup_lock.
>
> Fixes: 5343cd9364ea ("btrfs: qgroup: simple quota auto hierarchy for nested subvolumes")
> Signed-off-by: Filipe Manana <fdmanana@suse.com>

Ignore this one, it's not valid since we are protected by the
fs_info->qgroup_ioctl_lock mutex.


> ---
>  fs/btrfs/qgroup.c | 27 +++++++++++++++++++--------
>  1 file changed, 19 insertions(+), 8 deletions(-)
>
> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
> index c07cf6041cb9..195326ef0388 100644
> --- a/fs/btrfs/qgroup.c
> +++ b/fs/btrfs/qgroup.c
> @@ -3256,26 +3256,34 @@ static int qgroup_auto_inherit(struct btrfs_fs_info *fs_info,
>         struct btrfs_qgroup_inherit *res;
>         size_t struct_sz;
>         u64 *qgids;
> +       int ret = 0;
>
>         if (*inherit)
>                 return -EEXIST;
>
> +       spin_lock(&fs_info->qgroup_lock);
>         inode_qg = find_qgroup_rb(fs_info, inode_rootid);
> -       if (!inode_qg)
> -               return -ENOENT;
> +       if (!inode_qg) {
> +               ret = -ENOENT;
> +               goto out;
> +       }
>
>         num_qgroups = list_count_nodes(&inode_qg->groups);
>
>         if (!num_qgroups)
> -               return 0;
> +               goto out;
>
>         struct_sz = struct_size(res, qgroups, num_qgroups);
> -       if (struct_sz == SIZE_MAX)
> -               return -ERANGE;
> +       if (struct_sz == SIZE_MAX) {
> +               ret = -ERANGE;
> +               goto out;
> +       }
>
>         res = kzalloc(struct_sz, GFP_NOFS);
> -       if (!res)
> -               return -ENOMEM;
> +       if (!res) {
> +               ret = -ENOMEM;
> +               goto out;
> +       }
>         res->num_qgroups = num_qgroups;
>         qgids = res->qgroups;
>
> @@ -3283,7 +3291,10 @@ static int qgroup_auto_inherit(struct btrfs_fs_info *fs_info,
>                 qgids[i++] = qg_list->group->qgroupid;
>
>         *inherit = res;
> -       return 0;
> +out:
> +       spin_unlock(&fs_info->qgroup_lock);
> +
> +       return ret;
>  }
>
>  /*
> --
> 2.47.2
>
>

[PATCH 1/6] btrfs: qgroup: fix swapped blocks existence check when tracing after COW

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

In btrfs_qgroup_trace_subtree_after_cow(), after we erase the swapped
block, iterate over all possible tree levels to check if we sill have
other swapped blocks. However the check is wrong, it considers that there
are other swapped blocks if any rbtree for any level is empty, instead of
not empty. As a consequence we set blocks->swapped to true basically every
time since we always find an empty rbtree at least at level 7, since in
practice it's nearly impossible to find such a huge btree. This makes
every future call to btrfs_qgroup_trace_subtree_after_cow() do an
unnecessary rbtree search instead of returning immediately.

Fix this by updating the condition to set swapped to true if we find an
rbtree that is not empty.

Fixes: f616f5cd9da7 ("btrfs: qgroup: Use delayed subtree rescan for balance")
Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/qgroup.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index 46de0a5e80d5..798f21608de5 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -4913,7 +4913,7 @@ int btrfs_qgroup_trace_subtree_after_cow(struct btrfs_trans_handle *trans,
 	/* 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])) {
+		if (!RB_EMPTY_ROOT(&blocks->blocks[i])) {
 			swapped = true;
 			break;
 		}
-- 
2.47.2

Re: [PATCH 1/6] btrfs: qgroup: fix swapped blocks existence check when tracing after COW

[Qu Wenruo]

在 2026/10/3 04:25, fdmanana@kernel.org 写道:
> From: Filipe Manana <fdmanana@suse.com>
> 
> In btrfs_qgroup_trace_subtree_after_cow(), after we erase the swapped
> block, iterate over all possible tree levels to check if we sill have
> other swapped blocks. However the check is wrong, it considers that there
> are other swapped blocks if any rbtree for any level is empty, instead of
> not empty. As a consequence we set blocks->swapped to true basically every
> time since we always find an empty rbtree at least at level 7, since in
> practice it's nearly impossible to find such a huge btree. This makes
> every future call to btrfs_qgroup_trace_subtree_after_cow() do an
> unnecessary rbtree search instead of returning immediately.
> 
> Fix this by updating the condition to set swapped to true if we find an
> rbtree that is not empty.
> 
> Fixes: f616f5cd9da7 ("btrfs: qgroup: Use delayed subtree rescan for balance")
> Signed-off-by: Filipe Manana <fdmanana@suse.com>
> ---
>   fs/btrfs/qgroup.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
> index 46de0a5e80d5..798f21608de5 100644
> --- a/fs/btrfs/qgroup.c
> +++ b/fs/btrfs/qgroup.c
> @@ -4913,7 +4913,7 @@ int btrfs_qgroup_trace_subtree_after_cow(struct btrfs_trans_handle *trans,
>   	/* 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])) {
> +		if (!RB_EMPTY_ROOT(&blocks->blocks[i])) {
>   			swapped = true;

You may also want to update the comment of 
btrfs_qgroup_swapped_blocks(), which is also showing the opposite meaning.

Otherwise looks good to me.

Thanks,
Qu>   			break;
>   		}

[PATCH 2/6] btrfs: qgroup: abort transaction on failure to add qgroup relation

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

When adding a qgroup relation, if we fail to add the relation item from
"dst" to "src", we attempt to delete the relation item from "src" to "dst"
that we added just before, however we ignore the deletion result and if we
failed to delete we leave an inconsistency in the quota root. So check the
result of the deletion and if it fails, abort the transaction to avoid
persisting an inconsistent state.

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/qgroup.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index 798f21608de5..4b6c82a24bc5 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -1616,7 +1616,11 @@ int btrfs_add_qgroup_relation(struct btrfs_trans_handle *trans, u64 src, u64 dst
 
 	ret = add_qgroup_relation_item(trans, dst, src);
 	if (ret) {
-		del_qgroup_relation_item(trans, src, dst);
+		int ret2;
+
+		ret2 = del_qgroup_relation_item(trans, src, dst);
+		if (ret2 < 0)
+			btrfs_abort_transaction(trans, ret);
 		goto out;
 	}
 
-- 
2.47.2

Re: [PATCH 2/6] btrfs: qgroup: abort transaction on failure to add qgroup relation

[Qu Wenruo]

在 2026/10/3 04:25, fdmanana@kernel.org 写道:
> From: Filipe Manana <fdmanana@suse.com>
> 
> When adding a qgroup relation, if we fail to add the relation item from
> "dst" to "src", we attempt to delete the relation item from "src" to "dst"
> that we added just before, however we ignore the deletion result and if we
> failed to delete we leave an inconsistency in the quota root. So check the
> result of the deletion and if it fails, abort the transaction to avoid
> persisting an inconsistent state.
> 
> Signed-off-by: Filipe Manana <fdmanana@suse.com>

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu

> ---
>   fs/btrfs/qgroup.c | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
> index 798f21608de5..4b6c82a24bc5 100644
> --- a/fs/btrfs/qgroup.c
> +++ b/fs/btrfs/qgroup.c
> @@ -1616,7 +1616,11 @@ int btrfs_add_qgroup_relation(struct btrfs_trans_handle *trans, u64 src, u64 dst
>   
>   	ret = add_qgroup_relation_item(trans, dst, src);
>   	if (ret) {
> -		del_qgroup_relation_item(trans, src, dst);
> +		int ret2;
> +
> +		ret2 = del_qgroup_relation_item(trans, src, dst);
> +		if (ret2 < 0)
> +			btrfs_abort_transaction(trans, ret);
>   		goto out;
>   	}
>   

[PATCH 3/6] btrfs: qgroup: fix leak of qgroups in rb tree after failure to enable quotas

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

When enabling quotas we add struct btrfs_qroup items to the rbtree
fs_info->qgroup_tree, however if there's an error during the quota enable
operation after adding those items, we exit without removing the items
from the rbtree and freeing them.

Fix this by calling btrfs_free_qgroup_config() on error, which also
deletes all sysfs entries (it calls btrfs_sysfs_del_qgroups()).

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/qgroup.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index 4b6c82a24bc5..9f21b091c545 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -1293,8 +1293,13 @@ int btrfs_quota_enable(struct btrfs_fs_info *fs_info,
 	if (ret)
 		btrfs_put_root(quota_root);
 out:
-	if (ret)
-		btrfs_sysfs_del_qgroups(fs_info);
+	if (ret) {
+		/*
+		 * Free all qgroups previously added with add_qgroup_rb() and
+		 * sysfs entries.
+		 */
+		btrfs_free_qgroup_config(fs_info);
+	}
 	mutex_unlock(&fs_info->qgroup_ioctl_lock);
 	if (ret && trans)
 		btrfs_end_transaction(trans);
-- 
2.47.2

[PATCH 4/6] btrfs: qgroup: merge error labels in btrfs_quota_enable()

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

There is no need to have 3 different labels to which we goto on error.
Simplify this and have a single one, named 'out', where we always free
the path and put the root, as btrfs_free_path() and btrfs_put_root()
accept NULL arguments and do nothing in that case.

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/qgroup.c | 36 +++++++++++++++++-------------------
 1 file changed, 17 insertions(+), 19 deletions(-)

diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index 9f21b091c545..c9fc3b6a5132 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -999,7 +999,7 @@ static int btrfs_clean_quota_tree(struct btrfs_trans_handle *trans,
 int btrfs_quota_enable(struct btrfs_fs_info *fs_info,
 		       struct btrfs_ioctl_quota_ctl_args *quota_ctl_args)
 {
-	struct btrfs_root *quota_root;
+	struct btrfs_root *quota_root = NULL;
 	struct btrfs_root *tree_root = fs_info->tree_root;
 	struct btrfs_path *path = NULL;
 	struct btrfs_qgroup_status_item *ptr;
@@ -1076,6 +1076,7 @@ int btrfs_quota_enable(struct btrfs_fs_info *fs_info,
 	quota_root = btrfs_create_tree(trans, BTRFS_QUOTA_TREE_OBJECTID);
 	if (IS_ERR(quota_root)) {
 		ret =  PTR_ERR(quota_root);
+		quota_root = NULL;
 		btrfs_abort_transaction(trans, ret);
 		goto out;
 	}
@@ -1084,7 +1085,7 @@ int btrfs_quota_enable(struct btrfs_fs_info *fs_info,
 	if (unlikely(!path)) {
 		ret = -ENOMEM;
 		btrfs_abort_transaction(trans, ret);
-		goto out_free_root;
+		goto out;
 	}
 
 	key.objectid = 0;
@@ -1095,7 +1096,7 @@ int btrfs_quota_enable(struct btrfs_fs_info *fs_info,
 				      sizeof(*ptr));
 	if (unlikely(ret)) {
 		btrfs_abort_transaction(trans, ret);
-		goto out_free_path;
+		goto out;
 	}
 
 	leaf = path->nodes[0];
@@ -1131,7 +1132,7 @@ int btrfs_quota_enable(struct btrfs_fs_info *fs_info,
 		goto out_add_root;
 	if (unlikely(ret < 0)) {
 		btrfs_abort_transaction(trans, ret);
-		goto out_free_path;
+		goto out;
 	}
 
 	while (1) {
@@ -1150,14 +1151,14 @@ int btrfs_quota_enable(struct btrfs_fs_info *fs_info,
 			if (unlikely(!prealloc)) {
 				ret = -ENOMEM;
 				btrfs_abort_transaction(trans, ret);
-				goto out_free_path;
+				goto out;
 			}
 
 			ret = add_qgroup_item(trans, quota_root,
 					      found_key.offset);
 			if (unlikely(ret)) {
 				btrfs_abort_transaction(trans, ret);
-				goto out_free_path;
+				goto out;
 			}
 
 			qgroup = add_qgroup_rb(fs_info, prealloc, found_key.offset);
@@ -1165,13 +1166,13 @@ int btrfs_quota_enable(struct btrfs_fs_info *fs_info,
 			ret = btrfs_sysfs_add_one_qgroup(fs_info, qgroup);
 			if (unlikely(ret < 0)) {
 				btrfs_abort_transaction(trans, ret);
-				goto out_free_path;
+				goto out;
 			}
 			ret = btrfs_search_slot_for_read(tree_root, &found_key,
 							 path, 1, 0);
 			if (unlikely(ret < 0)) {
 				btrfs_abort_transaction(trans, ret);
-				goto out_free_path;
+				goto out;
 			}
 			if (ret > 0) {
 				/*
@@ -1188,7 +1189,7 @@ int btrfs_quota_enable(struct btrfs_fs_info *fs_info,
 		ret = btrfs_next_item(tree_root, path);
 		if (unlikely(ret < 0)) {
 			btrfs_abort_transaction(trans, ret);
-			goto out_free_path;
+			goto out;
 		}
 		if (ret)
 			break;
@@ -1199,21 +1200,21 @@ int btrfs_quota_enable(struct btrfs_fs_info *fs_info,
 	ret = add_qgroup_item(trans, quota_root, BTRFS_FS_TREE_OBJECTID);
 	if (unlikely(ret)) {
 		btrfs_abort_transaction(trans, ret);
-		goto out_free_path;
+		goto out;
 	}
 
 	ASSERT(prealloc == NULL);
 	prealloc = kzalloc_obj(*prealloc, GFP_NOFS);
 	if (!prealloc) {
 		ret = -ENOMEM;
-		goto out_free_path;
+		goto out;
 	}
 	qgroup = add_qgroup_rb(fs_info, prealloc, BTRFS_FS_TREE_OBJECTID);
 	prealloc = NULL;
 	ret = btrfs_sysfs_add_one_qgroup(fs_info, qgroup);
 	if (unlikely(ret < 0)) {
 		btrfs_abort_transaction(trans, ret);
-		goto out_free_path;
+		goto out;
 	}
 
 	/*
@@ -1245,7 +1246,7 @@ int btrfs_quota_enable(struct btrfs_fs_info *fs_info,
 			clear_bit(BTRFS_FS_SQUOTA_ENABLING, &fs_info->flags);
 			fs_info->qgroup_enable_gen = 0;
 		}
-		goto out_free_path;
+		goto out;
 	}
 
 	/*
@@ -1262,7 +1263,7 @@ int btrfs_quota_enable(struct btrfs_fs_info *fs_info,
 
 	/* Skip rescan for simple qgroups. */
 	if (btrfs_qgroup_mode(fs_info) == BTRFS_QGROUP_MODE_SIMPLE)
-		goto out_free_path;
+		goto out;
 
 	ret = qgroup_rescan_init(fs_info, 0, 1);
 	if (!ret) {
@@ -1287,18 +1288,15 @@ int btrfs_quota_enable(struct btrfs_fs_info *fs_info,
 		ret = 0;
 	}
 
-out_free_path:
-	btrfs_free_path(path);
-out_free_root:
-	if (ret)
-		btrfs_put_root(quota_root);
 out:
+	btrfs_free_path(path);
 	if (ret) {
 		/*
 		 * Free all qgroups previously added with add_qgroup_rb() and
 		 * sysfs entries.
 		 */
 		btrfs_free_qgroup_config(fs_info);
+		btrfs_put_root(quota_root);
 	}
 	mutex_unlock(&fs_info->qgroup_ioctl_lock);
 	if (ret && trans)
-- 
2.47.2

Re: [PATCH 4/6] btrfs: qgroup: merge error labels in btrfs_quota_enable()

[Qu Wenruo]

在 2026/10/3 04:25, fdmanana@kernel.org 写道:
> From: Filipe Manana <fdmanana@suse.com>
> 
> There is no need to have 3 different labels to which we goto on error.
> Simplify this and have a single one, named 'out', where we always free
> the path and put the root, as btrfs_free_path() and btrfs_put_root()
> accept NULL arguments and do nothing in that case.
> 
> Signed-off-by: Filipe Manana <fdmanana@suse.com>

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu
> ---
>   fs/btrfs/qgroup.c | 36 +++++++++++++++++-------------------
>   1 file changed, 17 insertions(+), 19 deletions(-)
> 
> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
> index 9f21b091c545..c9fc3b6a5132 100644
> --- a/fs/btrfs/qgroup.c
> +++ b/fs/btrfs/qgroup.c
> @@ -999,7 +999,7 @@ static int btrfs_clean_quota_tree(struct btrfs_trans_handle *trans,
>   int btrfs_quota_enable(struct btrfs_fs_info *fs_info,
>   		       struct btrfs_ioctl_quota_ctl_args *quota_ctl_args)
>   {
> -	struct btrfs_root *quota_root;
> +	struct btrfs_root *quota_root = NULL;
>   	struct btrfs_root *tree_root = fs_info->tree_root;
>   	struct btrfs_path *path = NULL;
>   	struct btrfs_qgroup_status_item *ptr;
> @@ -1076,6 +1076,7 @@ int btrfs_quota_enable(struct btrfs_fs_info *fs_info,
>   	quota_root = btrfs_create_tree(trans, BTRFS_QUOTA_TREE_OBJECTID);
>   	if (IS_ERR(quota_root)) {
>   		ret =  PTR_ERR(quota_root);
> +		quota_root = NULL;
>   		btrfs_abort_transaction(trans, ret);
>   		goto out;
>   	}
> @@ -1084,7 +1085,7 @@ int btrfs_quota_enable(struct btrfs_fs_info *fs_info,
>   	if (unlikely(!path)) {
>   		ret = -ENOMEM;
>   		btrfs_abort_transaction(trans, ret);
> -		goto out_free_root;
> +		goto out;
>   	}
>   
>   	key.objectid = 0;
> @@ -1095,7 +1096,7 @@ int btrfs_quota_enable(struct btrfs_fs_info *fs_info,
>   				      sizeof(*ptr));
>   	if (unlikely(ret)) {
>   		btrfs_abort_transaction(trans, ret);
> -		goto out_free_path;
> +		goto out;
>   	}
>   
>   	leaf = path->nodes[0];
> @@ -1131,7 +1132,7 @@ int btrfs_quota_enable(struct btrfs_fs_info *fs_info,
>   		goto out_add_root;
>   	if (unlikely(ret < 0)) {
>   		btrfs_abort_transaction(trans, ret);
> -		goto out_free_path;
> +		goto out;
>   	}
>   
>   	while (1) {
> @@ -1150,14 +1151,14 @@ int btrfs_quota_enable(struct btrfs_fs_info *fs_info,
>   			if (unlikely(!prealloc)) {
>   				ret = -ENOMEM;
>   				btrfs_abort_transaction(trans, ret);
> -				goto out_free_path;
> +				goto out;
>   			}
>   
>   			ret = add_qgroup_item(trans, quota_root,
>   					      found_key.offset);
>   			if (unlikely(ret)) {
>   				btrfs_abort_transaction(trans, ret);
> -				goto out_free_path;
> +				goto out;
>   			}
>   
>   			qgroup = add_qgroup_rb(fs_info, prealloc, found_key.offset);
> @@ -1165,13 +1166,13 @@ int btrfs_quota_enable(struct btrfs_fs_info *fs_info,
>   			ret = btrfs_sysfs_add_one_qgroup(fs_info, qgroup);
>   			if (unlikely(ret < 0)) {
>   				btrfs_abort_transaction(trans, ret);
> -				goto out_free_path;
> +				goto out;
>   			}
>   			ret = btrfs_search_slot_for_read(tree_root, &found_key,
>   							 path, 1, 0);
>   			if (unlikely(ret < 0)) {
>   				btrfs_abort_transaction(trans, ret);
> -				goto out_free_path;
> +				goto out;
>   			}
>   			if (ret > 0) {
>   				/*
> @@ -1188,7 +1189,7 @@ int btrfs_quota_enable(struct btrfs_fs_info *fs_info,
>   		ret = btrfs_next_item(tree_root, path);
>   		if (unlikely(ret < 0)) {
>   			btrfs_abort_transaction(trans, ret);
> -			goto out_free_path;
> +			goto out;
>   		}
>   		if (ret)
>   			break;
> @@ -1199,21 +1200,21 @@ int btrfs_quota_enable(struct btrfs_fs_info *fs_info,
>   	ret = add_qgroup_item(trans, quota_root, BTRFS_FS_TREE_OBJECTID);
>   	if (unlikely(ret)) {
>   		btrfs_abort_transaction(trans, ret);
> -		goto out_free_path;
> +		goto out;
>   	}
>   
>   	ASSERT(prealloc == NULL);
>   	prealloc = kzalloc_obj(*prealloc, GFP_NOFS);
>   	if (!prealloc) {
>   		ret = -ENOMEM;
> -		goto out_free_path;
> +		goto out;
>   	}
>   	qgroup = add_qgroup_rb(fs_info, prealloc, BTRFS_FS_TREE_OBJECTID);
>   	prealloc = NULL;
>   	ret = btrfs_sysfs_add_one_qgroup(fs_info, qgroup);
>   	if (unlikely(ret < 0)) {
>   		btrfs_abort_transaction(trans, ret);
> -		goto out_free_path;
> +		goto out;
>   	}
>   
>   	/*
> @@ -1245,7 +1246,7 @@ int btrfs_quota_enable(struct btrfs_fs_info *fs_info,
>   			clear_bit(BTRFS_FS_SQUOTA_ENABLING, &fs_info->flags);
>   			fs_info->qgroup_enable_gen = 0;
>   		}
> -		goto out_free_path;
> +		goto out;
>   	}
>   
>   	/*
> @@ -1262,7 +1263,7 @@ int btrfs_quota_enable(struct btrfs_fs_info *fs_info,
>   
>   	/* Skip rescan for simple qgroups. */
>   	if (btrfs_qgroup_mode(fs_info) == BTRFS_QGROUP_MODE_SIMPLE)
> -		goto out_free_path;
> +		goto out;
>   
>   	ret = qgroup_rescan_init(fs_info, 0, 1);
>   	if (!ret) {
> @@ -1287,18 +1288,15 @@ int btrfs_quota_enable(struct btrfs_fs_info *fs_info,
>   		ret = 0;
>   	}
>   
> -out_free_path:
> -	btrfs_free_path(path);
> -out_free_root:
> -	if (ret)
> -		btrfs_put_root(quota_root);
>   out:
> +	btrfs_free_path(path);
>   	if (ret) {
>   		/*
>   		 * Free all qgroups previously added with add_qgroup_rb() and
>   		 * sysfs entries.
>   		 */
>   		btrfs_free_qgroup_config(fs_info);
> +		btrfs_put_root(quota_root);
>   	}
>   	mutex_unlock(&fs_info->qgroup_ioctl_lock);
>   	if (ret && trans)

[PATCH 5/6] btrfs: abort transaction on qgroup failures in create_pending_snapshot()

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

If we get an error when calling one of the qgroup functions, we jump to
the 'fail' label without aborting the transaction. This is not a bug as up
the call chain (transaction commit path), we will end up aborting the
transaction. However having the explicit transaction abort in
create_pending_snapshot() allows us to get a stack trace and log message
that tells us exactly where we failed, useful for troubleshooting, and
also adds consistency since in that function we abort the transaction in
every other error path.

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/transaction.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
index da4ffa7bda00..fcbcb4a208be 100644
--- a/fs/btrfs/transaction.c
+++ b/fs/btrfs/transaction.c
@@ -1920,14 +1920,21 @@ static noinline int create_pending_snapshot(struct btrfs_trans_handle *trans,
 	 * To co-operate with that hack, we do hack again.
 	 * Or snapshot will be greatly slowed down by a subtree qgroup rescan
 	 */
-	if (btrfs_qgroup_mode(fs_info) == BTRFS_QGROUP_MODE_FULL)
+	if (btrfs_qgroup_mode(fs_info) == BTRFS_QGROUP_MODE_FULL) {
 		ret = qgroup_account_snapshot(trans, root, parent_root,
 					      pending->inherit, objectid);
-	else if (btrfs_qgroup_mode(fs_info) == BTRFS_QGROUP_MODE_SIMPLE)
+		if (unlikely(ret < 0)) {
+			btrfs_abort_transaction(trans, ret);
+			goto fail;
+		}
+	} else if (btrfs_qgroup_mode(fs_info) == BTRFS_QGROUP_MODE_SIMPLE) {
 		ret = btrfs_qgroup_inherit(trans, btrfs_root_id(root), objectid,
 					   btrfs_root_id(parent_root), pending->inherit);
-	if (unlikely(ret < 0))
-		goto fail;
+		if (unlikely(ret < 0)) {
+			btrfs_abort_transaction(trans, ret);
+			goto fail;
+		}
+	}
 
 	ret = btrfs_insert_dir_item(trans, &fname.disk_name,
 				    parent_inode, &key, BTRFS_FT_DIR, index, NULL);
-- 
2.47.2

Re: [PATCH 5/6] btrfs: abort transaction on qgroup failures in create_pending_snapshot()

[Qu Wenruo]

在 2026/10/3 04:25, fdmanana@kernel.org 写道:
> From: Filipe Manana <fdmanana@suse.com>
> 
> If we get an error when calling one of the qgroup functions, we jump to
> the 'fail' label without aborting the transaction. This is not a bug as up
> the call chain (transaction commit path), we will end up aborting the
> transaction. However having the explicit transaction abort in
> create_pending_snapshot() allows us to get a stack trace and log message
> that tells us exactly where we failed, useful for troubleshooting, and
> also adds consistency since in that function we abort the transaction in
> every other error path.
> 
> Signed-off-by: Filipe Manana <fdmanana@suse.com>

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu

> ---
>   fs/btrfs/transaction.c | 15 +++++++++++----
>   1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
> index da4ffa7bda00..fcbcb4a208be 100644
> --- a/fs/btrfs/transaction.c
> +++ b/fs/btrfs/transaction.c
> @@ -1920,14 +1920,21 @@ static noinline int create_pending_snapshot(struct btrfs_trans_handle *trans,
>   	 * To co-operate with that hack, we do hack again.
>   	 * Or snapshot will be greatly slowed down by a subtree qgroup rescan
>   	 */
> -	if (btrfs_qgroup_mode(fs_info) == BTRFS_QGROUP_MODE_FULL)
> +	if (btrfs_qgroup_mode(fs_info) == BTRFS_QGROUP_MODE_FULL) {
>   		ret = qgroup_account_snapshot(trans, root, parent_root,
>   					      pending->inherit, objectid);
> -	else if (btrfs_qgroup_mode(fs_info) == BTRFS_QGROUP_MODE_SIMPLE)
> +		if (unlikely(ret < 0)) {
> +			btrfs_abort_transaction(trans, ret);
> +			goto fail;
> +		}
> +	} else if (btrfs_qgroup_mode(fs_info) == BTRFS_QGROUP_MODE_SIMPLE) {
>   		ret = btrfs_qgroup_inherit(trans, btrfs_root_id(root), objectid,
>   					   btrfs_root_id(parent_root), pending->inherit);
> -	if (unlikely(ret < 0))
> -		goto fail;
> +		if (unlikely(ret < 0)) {
> +			btrfs_abort_transaction(trans, ret);
> +			goto fail;
> +		}
> +	}
>   
>   	ret = btrfs_insert_dir_item(trans, &fname.disk_name,
>   				    parent_inode, &key, BTRFS_FT_DIR, index, NULL);

[PATCH 6/6] btrfs: qgroup: fix off-by-one max level check in qgroup_trace_new_subtree_blocks()

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

We have maximum of 8 (BTRFS_MAX_LEVEL) levels in a btree, going from 0 to
7, so the upper bound checks should be:

  >= BTRFS_MAX_LEVEL

and not:

  >= BTRFS_MAX_LEVEL - 1

In practice this will hardly ever be a problem since having a btree with
8 levels would imply a massively huge fs.

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/qgroup.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index c9fc3b6a5132..54d82d6fd4bb 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -2491,8 +2491,8 @@ static int qgroup_trace_new_subtree_blocks(struct btrfs_trans_handle* trans,
 	int i;
 
 	/* Level sanity check */
-	if (unlikely(cur_level < 0 || cur_level >= BTRFS_MAX_LEVEL - 1 ||
-		     root_level < 0 || root_level >= BTRFS_MAX_LEVEL - 1 ||
+	if (unlikely(cur_level < 0 || cur_level >= BTRFS_MAX_LEVEL ||
+		     root_level < 0 || root_level >= BTRFS_MAX_LEVEL ||
 		     root_level < cur_level)) {
 		btrfs_err_rl(fs_info,
 			"%s: bad levels, cur_level=%d root_level=%d",
-- 
2.47.2

[PATCH v2 0/6] btrfs: some qgroup fixes and cleanups

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

Fix a few a bugs related to qgroups and some cleanups.

V2: Updated a comment in patch 1/6.
    Added reviewed-by tags to reviewed patches.

Filipe Manana (6):
  btrfs: qgroup: fix swapped blocks existence check when tracing after COW
  btrfs: qgroup: abort transaction on failure to add qgroup relation
  btrfs: qgroup: fix leak of qgroups in rb tree after failure to enable quotas
  btrfs: qgroup: merge error labels in btrfs_quota_enable()
  btrfs: abort transaction on qgroup failures in create_pending_snapshot()
  btrfs: qgroup: fix off-by-one max level check in qgroup_trace_new_subtree_blocks()

 fs/btrfs/ctree.h       |  2 +-
 fs/btrfs/qgroup.c      | 55 ++++++++++++++++++++++++------------------
 fs/btrfs/transaction.c | 15 +++++++++---
 3 files changed, 43 insertions(+), 29 deletions(-)

-- 
2.47.2

[PATCH v2 1/6] btrfs: qgroup: fix swapped blocks existence check when tracing after COW

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

In btrfs_qgroup_trace_subtree_after_cow(), after we erase the swapped
block, iterate over all possible tree levels to check if we sill have
other swapped blocks. However the check is wrong, it considers that there
are other swapped blocks if any rbtree for any level is empty, instead of
not empty. As a consequence we set blocks->swapped to true basically every
time since we always find an empty rbtree at least at level 7, since in
practice it's nearly impossible to find such a huge btree. This makes
every future call to btrfs_qgroup_trace_subtree_after_cow() do an
unnecessary rbtree search instead of returning immediately.

Fix this by updating the condition to set swapped to true if we find an
rbtree that is not empty.

Fixes: f616f5cd9da7 ("btrfs: qgroup: Use delayed subtree rescan for balance")
Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/ctree.h  | 2 +-
 fs/btrfs/qgroup.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 22ba2b4505b3..00d4c4184e76 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -160,7 +160,7 @@ enum {
  */
 struct btrfs_qgroup_swapped_blocks {
 	spinlock_t lock;
-	/* RM_EMPTY_ROOT() of above blocks[] */
+	/* True if any of the rbtrees in the blocks array below is not empty. */
 	bool swapped;
 	struct rb_root blocks[BTRFS_MAX_LEVEL];
 };
diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index 46de0a5e80d5..798f21608de5 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -4913,7 +4913,7 @@ int btrfs_qgroup_trace_subtree_after_cow(struct btrfs_trans_handle *trans,
 	/* 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])) {
+		if (!RB_EMPTY_ROOT(&blocks->blocks[i])) {
 			swapped = true;
 			break;
 		}
-- 
2.47.2

Re: [PATCH v2 1/6] btrfs: qgroup: fix swapped blocks existence check when tracing after COW

[Qu Wenruo]

在 2026/10/4 03:30, fdmanana@kernel.org 写道:
> From: Filipe Manana <fdmanana@suse.com>
> 
> In btrfs_qgroup_trace_subtree_after_cow(), after we erase the swapped
> block, iterate over all possible tree levels to check if we sill have
> other swapped blocks. However the check is wrong, it considers that there
> are other swapped blocks if any rbtree for any level is empty, instead of
> not empty. As a consequence we set blocks->swapped to true basically every
> time since we always find an empty rbtree at least at level 7, since in
> practice it's nearly impossible to find such a huge btree. This makes
> every future call to btrfs_qgroup_trace_subtree_after_cow() do an
> unnecessary rbtree search instead of returning immediately.
> 
> Fix this by updating the condition to set swapped to true if we find an
> rbtree that is not empty.
> 
> Fixes: f616f5cd9da7 ("btrfs: qgroup: Use delayed subtree rescan for balance")
> Signed-off-by: Filipe Manana <fdmanana@suse.com>

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu

> ---
>   fs/btrfs/ctree.h  | 2 +-
>   fs/btrfs/qgroup.c | 2 +-
>   2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> index 22ba2b4505b3..00d4c4184e76 100644
> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -160,7 +160,7 @@ enum {
>    */
>   struct btrfs_qgroup_swapped_blocks {
>   	spinlock_t lock;
> -	/* RM_EMPTY_ROOT() of above blocks[] */
> +	/* True if any of the rbtrees in the blocks array below is not empty. */
>   	bool swapped;
>   	struct rb_root blocks[BTRFS_MAX_LEVEL];
>   };
> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
> index 46de0a5e80d5..798f21608de5 100644
> --- a/fs/btrfs/qgroup.c
> +++ b/fs/btrfs/qgroup.c
> @@ -4913,7 +4913,7 @@ int btrfs_qgroup_trace_subtree_after_cow(struct btrfs_trans_handle *trans,
>   	/* 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])) {
> +		if (!RB_EMPTY_ROOT(&blocks->blocks[i])) {
>   			swapped = true;
>   			break;
>   		}

[PATCH v2 2/6] btrfs: qgroup: abort transaction on failure to add qgroup relation

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

When adding a qgroup relation, if we fail to add the relation item from
"dst" to "src", we attempt to delete the relation item from "src" to "dst"
that we added just before, however we ignore the deletion result and if we
failed to delete we leave an inconsistency in the quota root. So check the
result of the deletion and if it fails, abort the transaction to avoid
persisting an inconsistent state.

Reviewed-by: Qu Wenruo <wqu@suse.com>
Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/qgroup.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index 798f21608de5..4b6c82a24bc5 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -1616,7 +1616,11 @@ int btrfs_add_qgroup_relation(struct btrfs_trans_handle *trans, u64 src, u64 dst
 
 	ret = add_qgroup_relation_item(trans, dst, src);
 	if (ret) {
-		del_qgroup_relation_item(trans, src, dst);
+		int ret2;
+
+		ret2 = del_qgroup_relation_item(trans, src, dst);
+		if (ret2 < 0)
+			btrfs_abort_transaction(trans, ret);
 		goto out;
 	}
 
-- 
2.47.2

[PATCH v2 3/6] btrfs: qgroup: fix leak of qgroups in rb tree after failure to enable quotas

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

When enabling quotas we add struct btrfs_qroup items to the rbtree
fs_info->qgroup_tree, however if there's an error during the quota enable
operation after adding those items, we exit without removing the items
from the rbtree and freeing them.

Fix this by calling btrfs_free_qgroup_config() on error, which also
deletes all sysfs entries (it calls btrfs_sysfs_del_qgroups()).

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/qgroup.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index 4b6c82a24bc5..9f21b091c545 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -1293,8 +1293,13 @@ int btrfs_quota_enable(struct btrfs_fs_info *fs_info,
 	if (ret)
 		btrfs_put_root(quota_root);
 out:
-	if (ret)
-		btrfs_sysfs_del_qgroups(fs_info);
+	if (ret) {
+		/*
+		 * Free all qgroups previously added with add_qgroup_rb() and
+		 * sysfs entries.
+		 */
+		btrfs_free_qgroup_config(fs_info);
+	}
 	mutex_unlock(&fs_info->qgroup_ioctl_lock);
 	if (ret && trans)
 		btrfs_end_transaction(trans);
-- 
2.47.2

Re: [PATCH v2 3/6] btrfs: qgroup: fix leak of qgroups in rb tree after failure to enable quotas

[Qu Wenruo]

在 2026/10/4 03:30, fdmanana@kernel.org 写道:
> From: Filipe Manana <fdmanana@suse.com>
> 
> When enabling quotas we add struct btrfs_qroup items to the rbtree
> fs_info->qgroup_tree, however if there's an error during the quota enable
> operation after adding those items, we exit without removing the items
> from the rbtree and freeing them.
> 
> Fix this by calling btrfs_free_qgroup_config() on error, which also
> deletes all sysfs entries (it calls btrfs_sysfs_del_qgroups()).
> 
> Signed-off-by: Filipe Manana <fdmanana@suse.com>

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu> ---
>   fs/btrfs/qgroup.c | 9 +++++++--
>   1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
> index 4b6c82a24bc5..9f21b091c545 100644
> --- a/fs/btrfs/qgroup.c
> +++ b/fs/btrfs/qgroup.c
> @@ -1293,8 +1293,13 @@ int btrfs_quota_enable(struct btrfs_fs_info *fs_info,
>   	if (ret)
>   		btrfs_put_root(quota_root);
>   out:
> -	if (ret)
> -		btrfs_sysfs_del_qgroups(fs_info);
> +	if (ret) {
> +		/*
> +		 * Free all qgroups previously added with add_qgroup_rb() and
> +		 * sysfs entries.
> +		 */
> +		btrfs_free_qgroup_config(fs_info);
> +	}
>   	mutex_unlock(&fs_info->qgroup_ioctl_lock);
>   	if (ret && trans)
>   		btrfs_end_transaction(trans);

[PATCH v2 4/6] btrfs: qgroup: merge error labels in btrfs_quota_enable()

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

There is no need to have 3 different labels to which we goto on error.
Simplify this and have a single one, named 'out', where we always free
the path and put the root, as btrfs_free_path() and btrfs_put_root()
accept NULL arguments and do nothing in that case.

Reviewed-by: Qu Wenruo <wqu@suse.com>
Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/qgroup.c | 36 +++++++++++++++++-------------------
 1 file changed, 17 insertions(+), 19 deletions(-)

diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index 9f21b091c545..c9fc3b6a5132 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -999,7 +999,7 @@ static int btrfs_clean_quota_tree(struct btrfs_trans_handle *trans,
 int btrfs_quota_enable(struct btrfs_fs_info *fs_info,
 		       struct btrfs_ioctl_quota_ctl_args *quota_ctl_args)
 {
-	struct btrfs_root *quota_root;
+	struct btrfs_root *quota_root = NULL;
 	struct btrfs_root *tree_root = fs_info->tree_root;
 	struct btrfs_path *path = NULL;
 	struct btrfs_qgroup_status_item *ptr;
@@ -1076,6 +1076,7 @@ int btrfs_quota_enable(struct btrfs_fs_info *fs_info,
 	quota_root = btrfs_create_tree(trans, BTRFS_QUOTA_TREE_OBJECTID);
 	if (IS_ERR(quota_root)) {
 		ret =  PTR_ERR(quota_root);
+		quota_root = NULL;
 		btrfs_abort_transaction(trans, ret);
 		goto out;
 	}
@@ -1084,7 +1085,7 @@ int btrfs_quota_enable(struct btrfs_fs_info *fs_info,
 	if (unlikely(!path)) {
 		ret = -ENOMEM;
 		btrfs_abort_transaction(trans, ret);
-		goto out_free_root;
+		goto out;
 	}
 
 	key.objectid = 0;
@@ -1095,7 +1096,7 @@ int btrfs_quota_enable(struct btrfs_fs_info *fs_info,
 				      sizeof(*ptr));
 	if (unlikely(ret)) {
 		btrfs_abort_transaction(trans, ret);
-		goto out_free_path;
+		goto out;
 	}
 
 	leaf = path->nodes[0];
@@ -1131,7 +1132,7 @@ int btrfs_quota_enable(struct btrfs_fs_info *fs_info,
 		goto out_add_root;
 	if (unlikely(ret < 0)) {
 		btrfs_abort_transaction(trans, ret);
-		goto out_free_path;
+		goto out;
 	}
 
 	while (1) {
@@ -1150,14 +1151,14 @@ int btrfs_quota_enable(struct btrfs_fs_info *fs_info,
 			if (unlikely(!prealloc)) {
 				ret = -ENOMEM;
 				btrfs_abort_transaction(trans, ret);
-				goto out_free_path;
+				goto out;
 			}
 
 			ret = add_qgroup_item(trans, quota_root,
 					      found_key.offset);
 			if (unlikely(ret)) {
 				btrfs_abort_transaction(trans, ret);
-				goto out_free_path;
+				goto out;
 			}
 
 			qgroup = add_qgroup_rb(fs_info, prealloc, found_key.offset);
@@ -1165,13 +1166,13 @@ int btrfs_quota_enable(struct btrfs_fs_info *fs_info,
 			ret = btrfs_sysfs_add_one_qgroup(fs_info, qgroup);
 			if (unlikely(ret < 0)) {
 				btrfs_abort_transaction(trans, ret);
-				goto out_free_path;
+				goto out;
 			}
 			ret = btrfs_search_slot_for_read(tree_root, &found_key,
 							 path, 1, 0);
 			if (unlikely(ret < 0)) {
 				btrfs_abort_transaction(trans, ret);
-				goto out_free_path;
+				goto out;
 			}
 			if (ret > 0) {
 				/*
@@ -1188,7 +1189,7 @@ int btrfs_quota_enable(struct btrfs_fs_info *fs_info,
 		ret = btrfs_next_item(tree_root, path);
 		if (unlikely(ret < 0)) {
 			btrfs_abort_transaction(trans, ret);
-			goto out_free_path;
+			goto out;
 		}
 		if (ret)
 			break;
@@ -1199,21 +1200,21 @@ int btrfs_quota_enable(struct btrfs_fs_info *fs_info,
 	ret = add_qgroup_item(trans, quota_root, BTRFS_FS_TREE_OBJECTID);
 	if (unlikely(ret)) {
 		btrfs_abort_transaction(trans, ret);
-		goto out_free_path;
+		goto out;
 	}
 
 	ASSERT(prealloc == NULL);
 	prealloc = kzalloc_obj(*prealloc, GFP_NOFS);
 	if (!prealloc) {
 		ret = -ENOMEM;
-		goto out_free_path;
+		goto out;
 	}
 	qgroup = add_qgroup_rb(fs_info, prealloc, BTRFS_FS_TREE_OBJECTID);
 	prealloc = NULL;
 	ret = btrfs_sysfs_add_one_qgroup(fs_info, qgroup);
 	if (unlikely(ret < 0)) {
 		btrfs_abort_transaction(trans, ret);
-		goto out_free_path;
+		goto out;
 	}
 
 	/*
@@ -1245,7 +1246,7 @@ int btrfs_quota_enable(struct btrfs_fs_info *fs_info,
 			clear_bit(BTRFS_FS_SQUOTA_ENABLING, &fs_info->flags);
 			fs_info->qgroup_enable_gen = 0;
 		}
-		goto out_free_path;
+		goto out;
 	}
 
 	/*
@@ -1262,7 +1263,7 @@ int btrfs_quota_enable(struct btrfs_fs_info *fs_info,
 
 	/* Skip rescan for simple qgroups. */
 	if (btrfs_qgroup_mode(fs_info) == BTRFS_QGROUP_MODE_SIMPLE)
-		goto out_free_path;
+		goto out;
 
 	ret = qgroup_rescan_init(fs_info, 0, 1);
 	if (!ret) {
@@ -1287,18 +1288,15 @@ int btrfs_quota_enable(struct btrfs_fs_info *fs_info,
 		ret = 0;
 	}
 
-out_free_path:
-	btrfs_free_path(path);
-out_free_root:
-	if (ret)
-		btrfs_put_root(quota_root);
 out:
+	btrfs_free_path(path);
 	if (ret) {
 		/*
 		 * Free all qgroups previously added with add_qgroup_rb() and
 		 * sysfs entries.
 		 */
 		btrfs_free_qgroup_config(fs_info);
+		btrfs_put_root(quota_root);
 	}
 	mutex_unlock(&fs_info->qgroup_ioctl_lock);
 	if (ret && trans)
-- 
2.47.2

[PATCH v2 5/6] btrfs: abort transaction on qgroup failures in create_pending_snapshot()

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

If we get an error when calling one of the qgroup functions, we jump to
the 'fail' label without aborting the transaction. This is not a bug as up
the call chain (transaction commit path), we will end up aborting the
transaction. However having the explicit transaction abort in
create_pending_snapshot() allows us to get a stack trace and log message
that tells us exactly where we failed, useful for troubleshooting, and
also adds consistency since in that function we abort the transaction in
every other error path.

Reviewed-by: Qu Wenruo <wqu@suse.com>
Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/transaction.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
index da4ffa7bda00..fcbcb4a208be 100644
--- a/fs/btrfs/transaction.c
+++ b/fs/btrfs/transaction.c
@@ -1920,14 +1920,21 @@ static noinline int create_pending_snapshot(struct btrfs_trans_handle *trans,
 	 * To co-operate with that hack, we do hack again.
 	 * Or snapshot will be greatly slowed down by a subtree qgroup rescan
 	 */
-	if (btrfs_qgroup_mode(fs_info) == BTRFS_QGROUP_MODE_FULL)
+	if (btrfs_qgroup_mode(fs_info) == BTRFS_QGROUP_MODE_FULL) {
 		ret = qgroup_account_snapshot(trans, root, parent_root,
 					      pending->inherit, objectid);
-	else if (btrfs_qgroup_mode(fs_info) == BTRFS_QGROUP_MODE_SIMPLE)
+		if (unlikely(ret < 0)) {
+			btrfs_abort_transaction(trans, ret);
+			goto fail;
+		}
+	} else if (btrfs_qgroup_mode(fs_info) == BTRFS_QGROUP_MODE_SIMPLE) {
 		ret = btrfs_qgroup_inherit(trans, btrfs_root_id(root), objectid,
 					   btrfs_root_id(parent_root), pending->inherit);
-	if (unlikely(ret < 0))
-		goto fail;
+		if (unlikely(ret < 0)) {
+			btrfs_abort_transaction(trans, ret);
+			goto fail;
+		}
+	}
 
 	ret = btrfs_insert_dir_item(trans, &fname.disk_name,
 				    parent_inode, &key, BTRFS_FT_DIR, index, NULL);
-- 
2.47.2

[PATCH v2 6/6] btrfs: qgroup: fix off-by-one max level check in qgroup_trace_new_subtree_blocks()

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

We have maximum of 8 (BTRFS_MAX_LEVEL) levels in a btree, going from 0 to
7, so the upper bound checks should be:

  >= BTRFS_MAX_LEVEL

and not:

  >= BTRFS_MAX_LEVEL - 1

In practice this will hardly ever be a problem since having a btree with
8 levels would imply a massively huge fs.

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/qgroup.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index c9fc3b6a5132..54d82d6fd4bb 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -2491,8 +2491,8 @@ static int qgroup_trace_new_subtree_blocks(struct btrfs_trans_handle* trans,
 	int i;
 
 	/* Level sanity check */
-	if (unlikely(cur_level < 0 || cur_level >= BTRFS_MAX_LEVEL - 1 ||
-		     root_level < 0 || root_level >= BTRFS_MAX_LEVEL - 1 ||
+	if (unlikely(cur_level < 0 || cur_level >= BTRFS_MAX_LEVEL ||
+		     root_level < 0 || root_level >= BTRFS_MAX_LEVEL ||
 		     root_level < cur_level)) {
 		btrfs_err_rl(fs_info,
 			"%s: bad levels, cur_level=%d root_level=%d",
-- 
2.47.2

Re: [PATCH v2 6/6] btrfs: qgroup: fix off-by-one max level check in qgroup_trace_new_subtree_blocks()

[Qu Wenruo]

在 2026/10/4 03:30, fdmanana@kernel.org 写道:
> From: Filipe Manana <fdmanana@suse.com>
> 
> We have maximum of 8 (BTRFS_MAX_LEVEL) levels in a btree, going from 0 to
> 7, so the upper bound checks should be:
> 
>    >= BTRFS_MAX_LEVEL
> 
> and not:
> 
>    >= BTRFS_MAX_LEVEL - 1
> 
> In practice this will hardly ever be a problem since having a btree with
> 8 levels would imply a massively huge fs.
> 
> Signed-off-by: Filipe Manana <fdmanana@suse.com>

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu

> ---
>   fs/btrfs/qgroup.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
> index c9fc3b6a5132..54d82d6fd4bb 100644
> --- a/fs/btrfs/qgroup.c
> +++ b/fs/btrfs/qgroup.c
> @@ -2491,8 +2491,8 @@ static int qgroup_trace_new_subtree_blocks(struct btrfs_trans_handle* trans,
>   	int i;
>   
>   	/* Level sanity check */
> -	if (unlikely(cur_level < 0 || cur_level >= BTRFS_MAX_LEVEL - 1 ||
> -		     root_level < 0 || root_level >= BTRFS_MAX_LEVEL - 1 ||
> +	if (unlikely(cur_level < 0 || cur_level >= BTRFS_MAX_LEVEL ||
> +		     root_level < 0 || root_level >= BTRFS_MAX_LEVEL ||
>   		     root_level < cur_level)) {
>   		btrfs_err_rl(fs_info,
>   			"%s: bad levels, cur_level=%d root_level=%d",