* [PATCH 1/3] btrfs: fix failure to rebuild free space tree using multiple transactions
2025-06-08 22:43 [PATCH 0/3] btrfs: free space tree fixes and cleanups fdmanana
@ 2025-06-08 22:43 ` fdmanana
2025-06-08 22:43 ` [PATCH 2/3] btrfs: always abort transaction on failure to add block group to free space tree fdmanana
` (2 subsequent siblings)
3 siblings, 0 replies; 9+ messages in thread
From: fdmanana @ 2025-06-08 22:43 UTC (permalink / raw)
To: linux-btrfs
From: Filipe Manana <fdmanana@suse.com>
If we are rebuilding a free space tree, while modifying the free space
tree we may need to allocate a new metadata block group.
If we end up using multiple transactions for the rebuild, when we call
btrfs_end_transaction() we enter btrfs_create_pending_block_groups()
which calls add_block_group_free_space() to add items to the free space
tree for the block group.
Then later during the free space tree rebuild, at
btrfs_rebuild_free_space_tree(), we may find such new block groups
and call populate_free_space_tree() for them, which fails with -EEXIST
because there are already items in the free space tree. Then we abort the
transaction with -EEXIST at btrfs_rebuild_free_space_tree().
Notice that we say "may find" the new block groups because a new block
group may be inserted in the block groups rbtree, which is being iterated
by the rebuild process, before or after the current node where the rebuild
process is currently at.
Syzbot recently reported such case which produces a trace like the
following:
------------[ cut here ]------------
BTRFS: Transaction aborted (error -17)
WARNING: CPU: 1 PID: 7626 at fs/btrfs/free-space-tree.c:1341 btrfs_rebuild_free_space_tree+0x470/0x54c fs/btrfs/free-space-tree.c:1341
Modules linked in:
CPU: 1 UID: 0 PID: 7626 Comm: syz.2.25 Not tainted 6.15.0-rc7-syzkaller-00085-gd7fa1af5b33e-dirty #0 PREEMPT
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 05/07/2025
pstate: 60400005 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
pc : btrfs_rebuild_free_space_tree+0x470/0x54c fs/btrfs/free-space-tree.c:1341
lr : btrfs_rebuild_free_space_tree+0x470/0x54c fs/btrfs/free-space-tree.c:1341
sp : ffff80009c4f7740
x29: ffff80009c4f77b0 x28: ffff0000d4c3f400 x27: 0000000000000000
x26: dfff800000000000 x25: ffff70001389eee8 x24: 0000000000000003
x23: 1fffe000182b6e7b x22: 0000000000000000 x21: ffff0000c15b73d8
x20: 00000000ffffffef x19: ffff0000c15b7378 x18: 1fffe0003386f276
x17: ffff80008f31e000 x16: ffff80008adbe98c x15: 0000000000000001
x14: 1fffe0001b281550 x13: 0000000000000000 x12: 0000000000000000
x11: ffff60001b281551 x10: 0000000000000003 x9 : 1c8922000a902c00
x8 : 1c8922000a902c00 x7 : ffff800080485878 x6 : 0000000000000000
x5 : 0000000000000001 x4 : 0000000000000001 x3 : ffff80008047843c
x2 : 0000000000000001 x1 : ffff80008b3ebc40 x0 : 0000000000000001
Call trace:
btrfs_rebuild_free_space_tree+0x470/0x54c fs/btrfs/free-space-tree.c:1341 (P)
btrfs_start_pre_rw_mount+0xa78/0xe10 fs/btrfs/disk-io.c:3074
btrfs_remount_rw fs/btrfs/super.c:1319 [inline]
btrfs_reconfigure+0x828/0x2418 fs/btrfs/super.c:1543
reconfigure_super+0x1d4/0x6f0 fs/super.c:1083
do_remount fs/namespace.c:3365 [inline]
path_mount+0xb34/0xde0 fs/namespace.c:4200
do_mount fs/namespace.c:4221 [inline]
__do_sys_mount fs/namespace.c:4432 [inline]
__se_sys_mount fs/namespace.c:4409 [inline]
__arm64_sys_mount+0x3e8/0x468 fs/namespace.c:4409
__invoke_syscall arch/arm64/kernel/syscall.c:35 [inline]
invoke_syscall+0x98/0x2b8 arch/arm64/kernel/syscall.c:49
el0_svc_common+0x130/0x23c arch/arm64/kernel/syscall.c:132
do_el0_svc+0x48/0x58 arch/arm64/kernel/syscall.c:151
el0_svc+0x58/0x17c arch/arm64/kernel/entry-common.c:767
el0t_64_sync_handler+0x78/0x108 arch/arm64/kernel/entry-common.c:786
el0t_64_sync+0x198/0x19c arch/arm64/kernel/entry.S:600
irq event stamp: 330
hardirqs last enabled at (329): [<ffff80008048590c>] raw_spin_rq_unlock_irq kernel/sched/sched.h:1525 [inline]
hardirqs last enabled at (329): [<ffff80008048590c>] finish_lock_switch+0xb0/0x1c0 kernel/sched/core.c:5130
hardirqs last disabled at (330): [<ffff80008adb9e60>] el1_dbg+0x24/0x80 arch/arm64/kernel/entry-common.c:511
softirqs last enabled at (10): [<ffff8000801fbf10>] local_bh_enable+0x10/0x34 include/linux/bottom_half.h:32
softirqs last disabled at (8): [<ffff8000801fbedc>] local_bh_disable+0x10/0x34 include/linux/bottom_half.h:19
---[ end trace 0000000000000000 ]---
Fix this by flagging new block groups which had their free space tree
entries already added and then skip them in the rebuild process. Also,
since the rebuild may be triggered when doing a remount, make sure that
when we clear an existing free space tree that we clear such flag from
every existing block group, otherwise we would skip those block groups
during the rebuild.
Reported-by: syzbot+d0014fb0fc39c5487ae5@syzkaller.appspotmail.com
Link: https://lore.kernel.org/linux-btrfs/68460a54.050a0220.daf97.0af5.GAE@google.com/
Fixes: 882af9f13e83 ("btrfs: handle free space tree rebuild in multiple transactions")
Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
fs/btrfs/block-group.h | 2 ++
fs/btrfs/free-space-tree.c | 40 ++++++++++++++++++++++++++++++++++++++
2 files changed, 42 insertions(+)
diff --git a/fs/btrfs/block-group.h b/fs/btrfs/block-group.h
index 9de356bcb411..aa176cc9a324 100644
--- a/fs/btrfs/block-group.h
+++ b/fs/btrfs/block-group.h
@@ -83,6 +83,8 @@ enum btrfs_block_group_flags {
BLOCK_GROUP_FLAG_ZONED_DATA_RELOC,
/* Does the block group need to be added to the free space tree? */
BLOCK_GROUP_FLAG_NEEDS_FREE_SPACE,
+ /* Set after we add a new block group to the free space tree. */
+ BLOCK_GROUP_FLAG_FREE_SPACE_ADDED,
/* Indicate that the block group is placed on a sequential zone */
BLOCK_GROUP_FLAG_SEQUENTIAL_ZONE,
/*
diff --git a/fs/btrfs/free-space-tree.c b/fs/btrfs/free-space-tree.c
index 15721b9bbfe2..9eb9858e8e99 100644
--- a/fs/btrfs/free-space-tree.c
+++ b/fs/btrfs/free-space-tree.c
@@ -1241,6 +1241,7 @@ static int clear_free_space_tree(struct btrfs_trans_handle *trans,
{
BTRFS_PATH_AUTO_FREE(path);
struct btrfs_key key;
+ struct rb_node *node;
int nr;
int ret;
@@ -1269,6 +1270,16 @@ static int clear_free_space_tree(struct btrfs_trans_handle *trans,
btrfs_release_path(path);
}
+ node = rb_first_cached(&trans->fs_info->block_group_cache_tree);
+ while (node) {
+ struct btrfs_block_group *bg;
+
+ bg = rb_entry(node, struct btrfs_block_group, cache_node);
+ clear_bit(BLOCK_GROUP_FLAG_FREE_SPACE_ADDED, &bg->runtime_flags);
+ node = rb_next(node);
+ cond_resched();
+ }
+
return 0;
}
@@ -1358,12 +1369,18 @@ int btrfs_rebuild_free_space_tree(struct btrfs_fs_info *fs_info)
block_group = rb_entry(node, struct btrfs_block_group,
cache_node);
+
+ if (test_bit(BLOCK_GROUP_FLAG_FREE_SPACE_ADDED,
+ &block_group->runtime_flags))
+ goto next;
+
ret = populate_free_space_tree(trans, block_group);
if (ret) {
btrfs_abort_transaction(trans, ret);
btrfs_end_transaction(trans);
return ret;
}
+next:
if (btrfs_should_end_transaction(trans)) {
btrfs_end_transaction(trans);
trans = btrfs_start_transaction(free_space_root, 1);
@@ -1390,6 +1407,29 @@ static int __add_block_group_free_space(struct btrfs_trans_handle *trans,
clear_bit(BLOCK_GROUP_FLAG_NEEDS_FREE_SPACE, &block_group->runtime_flags);
+ /*
+ * While rebuilding the free space tree we may allocate new metadata
+ * block groups while modifying the free space tree.
+ *
+ * Because during the rebuild (at btrfs_rebuild_free_space_tree()) we
+ * can use multiple transactions, every time btrfs_end_transaction() is
+ * called at btrfs_rebuild_free_space_tree() we finish the creation of
+ * new block groups by calling btrfs_create_pending_block_groups(), and
+ * that in turn calls us, through add_block_group_free_space(), to add
+ * a free space info item and a free space extent item for the block
+ * group.
+ *
+ * Then later btrfs_rebuild_free_space_tree() may find such new block
+ * groups and processes them with populate_free_space_tree(), which can
+ * fail with EEXIST since there are already items for the block group in
+ * the free space tree. Notice that we say "may find" because a new
+ * block group may be added to the block groups rbtree in a node before
+ * or after the block group currently being processed by the rebuild
+ * process. So signal the rebuild process to skip such new block groups
+ * if it finds them.
+ */
+ set_bit(BLOCK_GROUP_FLAG_FREE_SPACE_ADDED, &block_group->runtime_flags);
+
ret = add_new_free_space_info(trans, block_group, path);
if (ret)
return ret;
--
2.47.2
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH 2/3] btrfs: always abort transaction on failure to add block group to free space tree
2025-06-08 22:43 [PATCH 0/3] btrfs: free space tree fixes and cleanups fdmanana
2025-06-08 22:43 ` [PATCH 1/3] btrfs: fix failure to rebuild free space tree using multiple transactions fdmanana
@ 2025-06-08 22:43 ` fdmanana
2025-06-08 22:43 ` [PATCH 3/3] btrfs: check BLOCK_GROUP_FLAG_NEEDS_FREE_SPACE at __add_block_group_free_space() fdmanana
2025-06-16 15:45 ` [PATCH 0/3] btrfs: free space tree fixes and cleanups Boris Burkov
3 siblings, 0 replies; 9+ messages in thread
From: fdmanana @ 2025-06-08 22:43 UTC (permalink / raw)
To: linux-btrfs
From: Filipe Manana <fdmanana@suse.com>
Only one of the callers of __add_block_group_free_space() aborts the
transaction if the call fails, while the others don't do it and it's
either never done up the call chain or much higher in the call chain.
So make sure we abort the transaction at __add_block_group_free_space()
if it fails, which brings a couple benefits:
1) If some call chain never aborts the transaction, we avoid having some
metadata inconsistency because BLOCK_GROUP_FLAG_NEEDS_FREE_SPACE is
cleared when we enter __add_block_group_free_space() and therefore
__add_block_group_free_space() is never called again to add the block
group items to the free space tree, since the function is only called
when that flag is set in a block group;
2) If the call chain already aborts the transaction, then we get a better
trace that points to the exact step from __add_block_group_free_space()
which failed, which is better for analysis.
So abort the transaction at __add_block_group_free_space() if any of its
steps fails.
Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
fs/btrfs/free-space-tree.c | 16 +++++++++-------
1 file changed, 9 insertions(+), 7 deletions(-)
diff --git a/fs/btrfs/free-space-tree.c b/fs/btrfs/free-space-tree.c
index 9eb9858e8e99..af005fb4b676 100644
--- a/fs/btrfs/free-space-tree.c
+++ b/fs/btrfs/free-space-tree.c
@@ -1431,12 +1431,17 @@ static int __add_block_group_free_space(struct btrfs_trans_handle *trans,
set_bit(BLOCK_GROUP_FLAG_FREE_SPACE_ADDED, &block_group->runtime_flags);
ret = add_new_free_space_info(trans, block_group, path);
- if (ret)
+ if (ret) {
+ btrfs_abort_transaction(trans, ret);
return ret;
+ }
- return __add_to_free_space_tree(trans, block_group, path,
- block_group->start,
- block_group->length);
+ ret = __add_to_free_space_tree(trans, block_group, path,
+ block_group->start, block_group->length);
+ if (ret)
+ btrfs_abort_transaction(trans, ret);
+
+ return 0;
}
int add_block_group_free_space(struct btrfs_trans_handle *trans,
@@ -1461,9 +1466,6 @@ int add_block_group_free_space(struct btrfs_trans_handle *trans,
}
ret = __add_block_group_free_space(trans, block_group, path);
- if (ret)
- btrfs_abort_transaction(trans, ret);
-
out:
btrfs_free_path(path);
mutex_unlock(&block_group->free_space_lock);
--
2.47.2
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH 3/3] btrfs: check BLOCK_GROUP_FLAG_NEEDS_FREE_SPACE at __add_block_group_free_space()
2025-06-08 22:43 [PATCH 0/3] btrfs: free space tree fixes and cleanups fdmanana
2025-06-08 22:43 ` [PATCH 1/3] btrfs: fix failure to rebuild free space tree using multiple transactions fdmanana
2025-06-08 22:43 ` [PATCH 2/3] btrfs: always abort transaction on failure to add block group to free space tree fdmanana
@ 2025-06-08 22:43 ` fdmanana
2025-06-16 15:44 ` Boris Burkov
2025-06-16 15:45 ` [PATCH 0/3] btrfs: free space tree fixes and cleanups Boris Burkov
3 siblings, 1 reply; 9+ messages in thread
From: fdmanana @ 2025-06-08 22:43 UTC (permalink / raw)
To: linux-btrfs
From: Filipe Manana <fdmanana@suse.com>
Every caller of __add_block_group_free_space() is checking if the flag
BLOCK_GROUP_FLAG_NEEDS_FREE_SPACE is set before calling it. This is
duplicate code and it's prone to some mistake in case we add more callers
in the future. So move the check for that flag into the start of
__add_block_group_free_space().
Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
fs/btrfs/free-space-tree.c | 58 ++++++++++++++++++--------------------
1 file changed, 28 insertions(+), 30 deletions(-)
diff --git a/fs/btrfs/free-space-tree.c b/fs/btrfs/free-space-tree.c
index af005fb4b676..f03f3610b713 100644
--- a/fs/btrfs/free-space-tree.c
+++ b/fs/btrfs/free-space-tree.c
@@ -816,11 +816,9 @@ int __remove_from_free_space_tree(struct btrfs_trans_handle *trans,
u32 flags;
int ret;
- if (test_bit(BLOCK_GROUP_FLAG_NEEDS_FREE_SPACE, &block_group->runtime_flags)) {
- ret = __add_block_group_free_space(trans, block_group, path);
- if (ret)
- return ret;
- }
+ ret = __add_block_group_free_space(trans, block_group, path);
+ if (ret)
+ return ret;
info = search_free_space_info(NULL, block_group, path, 0);
if (IS_ERR(info))
@@ -1011,11 +1009,9 @@ int __add_to_free_space_tree(struct btrfs_trans_handle *trans,
u32 flags;
int ret;
- if (test_bit(BLOCK_GROUP_FLAG_NEEDS_FREE_SPACE, &block_group->runtime_flags)) {
- ret = __add_block_group_free_space(trans, block_group, path);
- if (ret)
- return ret;
- }
+ ret = __add_block_group_free_space(trans, block_group, path);
+ if (ret)
+ return ret;
info = search_free_space_info(NULL, block_group, path, 0);
if (IS_ERR(info))
@@ -1403,9 +1399,12 @@ static int __add_block_group_free_space(struct btrfs_trans_handle *trans,
struct btrfs_block_group *block_group,
struct btrfs_path *path)
{
+ bool own_path = false;
int ret;
- clear_bit(BLOCK_GROUP_FLAG_NEEDS_FREE_SPACE, &block_group->runtime_flags);
+ if (!test_and_clear_bit(BLOCK_GROUP_FLAG_NEEDS_FREE_SPACE,
+ &block_group->runtime_flags))
+ return 0;
/*
* While rebuilding the free space tree we may allocate new metadata
@@ -1430,10 +1429,19 @@ static int __add_block_group_free_space(struct btrfs_trans_handle *trans,
*/
set_bit(BLOCK_GROUP_FLAG_FREE_SPACE_ADDED, &block_group->runtime_flags);
+ if (!path) {
+ path = btrfs_alloc_path();
+ if (!path) {
+ btrfs_abort_transaction(trans, -ENOMEM);
+ return -ENOMEM;
+ }
+ own_path = true;
+ }
+
ret = add_new_free_space_info(trans, block_group, path);
if (ret) {
btrfs_abort_transaction(trans, ret);
- return ret;
+ goto out;
}
ret = __add_to_free_space_tree(trans, block_group, path,
@@ -1441,33 +1449,23 @@ static int __add_block_group_free_space(struct btrfs_trans_handle *trans,
if (ret)
btrfs_abort_transaction(trans, ret);
- return 0;
+out:
+ if (own_path)
+ btrfs_free_path(path);
+
+ return ret;
}
int add_block_group_free_space(struct btrfs_trans_handle *trans,
struct btrfs_block_group *block_group)
{
- struct btrfs_fs_info *fs_info = trans->fs_info;
- struct btrfs_path *path = NULL;
- int ret = 0;
+ int ret;
- if (!btrfs_fs_compat_ro(fs_info, FREE_SPACE_TREE))
+ if (!btrfs_fs_compat_ro(trans->fs_info, FREE_SPACE_TREE))
return 0;
mutex_lock(&block_group->free_space_lock);
- if (!test_bit(BLOCK_GROUP_FLAG_NEEDS_FREE_SPACE, &block_group->runtime_flags))
- goto out;
-
- path = btrfs_alloc_path();
- if (!path) {
- ret = -ENOMEM;
- btrfs_abort_transaction(trans, ret);
- goto out;
- }
-
- ret = __add_block_group_free_space(trans, block_group, path);
-out:
- btrfs_free_path(path);
+ ret = __add_block_group_free_space(trans, block_group, NULL);
mutex_unlock(&block_group->free_space_lock);
return ret;
}
--
2.47.2
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH 3/3] btrfs: check BLOCK_GROUP_FLAG_NEEDS_FREE_SPACE at __add_block_group_free_space()
2025-06-08 22:43 ` [PATCH 3/3] btrfs: check BLOCK_GROUP_FLAG_NEEDS_FREE_SPACE at __add_block_group_free_space() fdmanana
@ 2025-06-16 15:44 ` Boris Burkov
2025-06-16 15:48 ` Filipe Manana
0 siblings, 1 reply; 9+ messages in thread
From: Boris Burkov @ 2025-06-16 15:44 UTC (permalink / raw)
To: fdmanana; +Cc: linux-btrfs
On Sun, Jun 08, 2025 at 11:43:34PM +0100, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
>
> Every caller of __add_block_group_free_space() is checking if the flag
> BLOCK_GROUP_FLAG_NEEDS_FREE_SPACE is set before calling it. This is
> duplicate code and it's prone to some mistake in case we add more callers
> in the future. So move the check for that flag into the start of
> __add_block_group_free_space().
>
> Signed-off-by: Filipe Manana <fdmanana@suse.com>
> ---
> fs/btrfs/free-space-tree.c | 58 ++++++++++++++++++--------------------
> 1 file changed, 28 insertions(+), 30 deletions(-)
>
> diff --git a/fs/btrfs/free-space-tree.c b/fs/btrfs/free-space-tree.c
> index af005fb4b676..f03f3610b713 100644
> --- a/fs/btrfs/free-space-tree.c
> +++ b/fs/btrfs/free-space-tree.c
> @@ -816,11 +816,9 @@ int __remove_from_free_space_tree(struct btrfs_trans_handle *trans,
> u32 flags;
> int ret;
>
> - if (test_bit(BLOCK_GROUP_FLAG_NEEDS_FREE_SPACE, &block_group->runtime_flags)) {
> - ret = __add_block_group_free_space(trans, block_group, path);
> - if (ret)
> - return ret;
> - }
> + ret = __add_block_group_free_space(trans, block_group, path);
> + if (ret)
> + return ret;
>
> info = search_free_space_info(NULL, block_group, path, 0);
> if (IS_ERR(info))
> @@ -1011,11 +1009,9 @@ int __add_to_free_space_tree(struct btrfs_trans_handle *trans,
> u32 flags;
> int ret;
>
> - if (test_bit(BLOCK_GROUP_FLAG_NEEDS_FREE_SPACE, &block_group->runtime_flags)) {
> - ret = __add_block_group_free_space(trans, block_group, path);
> - if (ret)
> - return ret;
> - }
> + ret = __add_block_group_free_space(trans, block_group, path);
> + if (ret)
> + return ret;
>
> info = search_free_space_info(NULL, block_group, path, 0);
> if (IS_ERR(info))
> @@ -1403,9 +1399,12 @@ static int __add_block_group_free_space(struct btrfs_trans_handle *trans,
> struct btrfs_block_group *block_group,
> struct btrfs_path *path)
> {
> + bool own_path = false;
> int ret;
>
> - clear_bit(BLOCK_GROUP_FLAG_NEEDS_FREE_SPACE, &block_group->runtime_flags);
> + if (!test_and_clear_bit(BLOCK_GROUP_FLAG_NEEDS_FREE_SPACE,
> + &block_group->runtime_flags))
> + return 0;
>
> /*
> * While rebuilding the free space tree we may allocate new metadata
> @@ -1430,10 +1429,19 @@ static int __add_block_group_free_space(struct btrfs_trans_handle *trans,
> */
> set_bit(BLOCK_GROUP_FLAG_FREE_SPACE_ADDED, &block_group->runtime_flags);
>
> + if (!path) {
> + path = btrfs_alloc_path();
> + if (!path) {
> + btrfs_abort_transaction(trans, -ENOMEM);
> + return -ENOMEM;
> + }
> + own_path = true;
> + }
> +
Is the "own_path" change intended to be bundled with this one? If so,
can you mention it in the commit message as well?
> ret = add_new_free_space_info(trans, block_group, path);
> if (ret) {
> btrfs_abort_transaction(trans, ret);
> - return ret;
> + goto out;
> }
>
> ret = __add_to_free_space_tree(trans, block_group, path,
> @@ -1441,33 +1449,23 @@ static int __add_block_group_free_space(struct btrfs_trans_handle *trans,
> if (ret)
> btrfs_abort_transaction(trans, ret);
>
> - return 0;
> +out:
> + if (own_path)
> + btrfs_free_path(path);
> +
> + return ret;
> }
>
> int add_block_group_free_space(struct btrfs_trans_handle *trans,
> struct btrfs_block_group *block_group)
> {
> - struct btrfs_fs_info *fs_info = trans->fs_info;
> - struct btrfs_path *path = NULL;
> - int ret = 0;
> + int ret;
>
> - if (!btrfs_fs_compat_ro(fs_info, FREE_SPACE_TREE))
> + if (!btrfs_fs_compat_ro(trans->fs_info, FREE_SPACE_TREE))
> return 0;
>
> mutex_lock(&block_group->free_space_lock);
> - if (!test_bit(BLOCK_GROUP_FLAG_NEEDS_FREE_SPACE, &block_group->runtime_flags))
> - goto out;
> -
> - path = btrfs_alloc_path();
> - if (!path) {
> - ret = -ENOMEM;
> - btrfs_abort_transaction(trans, ret);
> - goto out;
> - }
> -
> - ret = __add_block_group_free_space(trans, block_group, path);
> -out:
> - btrfs_free_path(path);
> + ret = __add_block_group_free_space(trans, block_group, NULL);
> mutex_unlock(&block_group->free_space_lock);
> return ret;
> }
> --
> 2.47.2
>
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH 3/3] btrfs: check BLOCK_GROUP_FLAG_NEEDS_FREE_SPACE at __add_block_group_free_space()
2025-06-16 15:44 ` Boris Burkov
@ 2025-06-16 15:48 ` Filipe Manana
2025-06-16 16:49 ` Boris Burkov
0 siblings, 1 reply; 9+ messages in thread
From: Filipe Manana @ 2025-06-16 15:48 UTC (permalink / raw)
To: Boris Burkov; +Cc: linux-btrfs
On Mon, Jun 16, 2025 at 4:45 PM Boris Burkov <boris@bur.io> wrote:
>
> On Sun, Jun 08, 2025 at 11:43:34PM +0100, fdmanana@kernel.org wrote:
> > From: Filipe Manana <fdmanana@suse.com>
> >
> > Every caller of __add_block_group_free_space() is checking if the flag
> > BLOCK_GROUP_FLAG_NEEDS_FREE_SPACE is set before calling it. This is
> > duplicate code and it's prone to some mistake in case we add more callers
> > in the future. So move the check for that flag into the start of
> > __add_block_group_free_space().
> >
> > Signed-off-by: Filipe Manana <fdmanana@suse.com>
> > ---
> > fs/btrfs/free-space-tree.c | 58 ++++++++++++++++++--------------------
> > 1 file changed, 28 insertions(+), 30 deletions(-)
> >
> > diff --git a/fs/btrfs/free-space-tree.c b/fs/btrfs/free-space-tree.c
> > index af005fb4b676..f03f3610b713 100644
> > --- a/fs/btrfs/free-space-tree.c
> > +++ b/fs/btrfs/free-space-tree.c
> > @@ -816,11 +816,9 @@ int __remove_from_free_space_tree(struct btrfs_trans_handle *trans,
> > u32 flags;
> > int ret;
> >
> > - if (test_bit(BLOCK_GROUP_FLAG_NEEDS_FREE_SPACE, &block_group->runtime_flags)) {
> > - ret = __add_block_group_free_space(trans, block_group, path);
> > - if (ret)
> > - return ret;
> > - }
> > + ret = __add_block_group_free_space(trans, block_group, path);
> > + if (ret)
> > + return ret;
> >
> > info = search_free_space_info(NULL, block_group, path, 0);
> > if (IS_ERR(info))
> > @@ -1011,11 +1009,9 @@ int __add_to_free_space_tree(struct btrfs_trans_handle *trans,
> > u32 flags;
> > int ret;
> >
> > - if (test_bit(BLOCK_GROUP_FLAG_NEEDS_FREE_SPACE, &block_group->runtime_flags)) {
> > - ret = __add_block_group_free_space(trans, block_group, path);
> > - if (ret)
> > - return ret;
> > - }
> > + ret = __add_block_group_free_space(trans, block_group, path);
> > + if (ret)
> > + return ret;
> >
> > info = search_free_space_info(NULL, block_group, path, 0);
> > if (IS_ERR(info))
> > @@ -1403,9 +1399,12 @@ static int __add_block_group_free_space(struct btrfs_trans_handle *trans,
> > struct btrfs_block_group *block_group,
> > struct btrfs_path *path)
> > {
> > + bool own_path = false;
> > int ret;
> >
> > - clear_bit(BLOCK_GROUP_FLAG_NEEDS_FREE_SPACE, &block_group->runtime_flags);
> > + if (!test_and_clear_bit(BLOCK_GROUP_FLAG_NEEDS_FREE_SPACE,
> > + &block_group->runtime_flags))
> > + return 0;
> >
> > /*
> > * While rebuilding the free space tree we may allocate new metadata
> > @@ -1430,10 +1429,19 @@ static int __add_block_group_free_space(struct btrfs_trans_handle *trans,
> > */
> > set_bit(BLOCK_GROUP_FLAG_FREE_SPACE_ADDED, &block_group->runtime_flags);
> >
> > + if (!path) {
> > + path = btrfs_alloc_path();
> > + if (!path) {
> > + btrfs_abort_transaction(trans, -ENOMEM);
> > + return -ENOMEM;
> > + }
> > + own_path = true;
> > + }
> > +
>
> Is the "own_path" change intended to be bundled with this one? If so,
> can you mention it in the commit message as well?
Yes it's supposed, why wouldn't it?
This is because the path allocation from add_block_group_free_space()
has to be gone and done in this function now if it receives a NULL
path.
I would think this is obvious since the diff for
add_block_group_free_space() removes the path allocation.
Thanks.
>
> > ret = add_new_free_space_info(trans, block_group, path);
> > if (ret) {
> > btrfs_abort_transaction(trans, ret);
> > - return ret;
> > + goto out;
> > }
> >
> > ret = __add_to_free_space_tree(trans, block_group, path,
> > @@ -1441,33 +1449,23 @@ static int __add_block_group_free_space(struct btrfs_trans_handle *trans,
> > if (ret)
> > btrfs_abort_transaction(trans, ret);
> >
> > - return 0;
> > +out:
> > + if (own_path)
> > + btrfs_free_path(path);
> > +
> > + return ret;
> > }
> >
> > int add_block_group_free_space(struct btrfs_trans_handle *trans,
> > struct btrfs_block_group *block_group)
> > {
> > - struct btrfs_fs_info *fs_info = trans->fs_info;
> > - struct btrfs_path *path = NULL;
> > - int ret = 0;
> > + int ret;
> >
> > - if (!btrfs_fs_compat_ro(fs_info, FREE_SPACE_TREE))
> > + if (!btrfs_fs_compat_ro(trans->fs_info, FREE_SPACE_TREE))
> > return 0;
> >
> > mutex_lock(&block_group->free_space_lock);
> > - if (!test_bit(BLOCK_GROUP_FLAG_NEEDS_FREE_SPACE, &block_group->runtime_flags))
> > - goto out;
> > -
> > - path = btrfs_alloc_path();
> > - if (!path) {
> > - ret = -ENOMEM;
> > - btrfs_abort_transaction(trans, ret);
> > - goto out;
> > - }
> > -
> > - ret = __add_block_group_free_space(trans, block_group, path);
> > -out:
> > - btrfs_free_path(path);
> > + ret = __add_block_group_free_space(trans, block_group, NULL);
> > mutex_unlock(&block_group->free_space_lock);
> > return ret;
> > }
> > --
> > 2.47.2
> >
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH 3/3] btrfs: check BLOCK_GROUP_FLAG_NEEDS_FREE_SPACE at __add_block_group_free_space()
2025-06-16 15:48 ` Filipe Manana
@ 2025-06-16 16:49 ` Boris Burkov
2025-06-16 21:47 ` Filipe Manana
0 siblings, 1 reply; 9+ messages in thread
From: Boris Burkov @ 2025-06-16 16:49 UTC (permalink / raw)
To: Filipe Manana; +Cc: linux-btrfs
On Mon, Jun 16, 2025 at 04:48:34PM +0100, Filipe Manana wrote:
> On Mon, Jun 16, 2025 at 4:45 PM Boris Burkov <boris@bur.io> wrote:
> >
> > On Sun, Jun 08, 2025 at 11:43:34PM +0100, fdmanana@kernel.org wrote:
> > > From: Filipe Manana <fdmanana@suse.com>
> > >
> > > Every caller of __add_block_group_free_space() is checking if the flag
> > > BLOCK_GROUP_FLAG_NEEDS_FREE_SPACE is set before calling it. This is
> > > duplicate code and it's prone to some mistake in case we add more callers
> > > in the future. So move the check for that flag into the start of
> > > __add_block_group_free_space().
> > >
> > > Signed-off-by: Filipe Manana <fdmanana@suse.com>
> > > ---
> > > fs/btrfs/free-space-tree.c | 58 ++++++++++++++++++--------------------
> > > 1 file changed, 28 insertions(+), 30 deletions(-)
> > >
> > > diff --git a/fs/btrfs/free-space-tree.c b/fs/btrfs/free-space-tree.c
> > > index af005fb4b676..f03f3610b713 100644
> > > --- a/fs/btrfs/free-space-tree.c
> > > +++ b/fs/btrfs/free-space-tree.c
> > > @@ -816,11 +816,9 @@ int __remove_from_free_space_tree(struct btrfs_trans_handle *trans,
> > > u32 flags;
> > > int ret;
> > >
> > > - if (test_bit(BLOCK_GROUP_FLAG_NEEDS_FREE_SPACE, &block_group->runtime_flags)) {
> > > - ret = __add_block_group_free_space(trans, block_group, path);
> > > - if (ret)
> > > - return ret;
> > > - }
> > > + ret = __add_block_group_free_space(trans, block_group, path);
> > > + if (ret)
> > > + return ret;
> > >
> > > info = search_free_space_info(NULL, block_group, path, 0);
> > > if (IS_ERR(info))
> > > @@ -1011,11 +1009,9 @@ int __add_to_free_space_tree(struct btrfs_trans_handle *trans,
> > > u32 flags;
> > > int ret;
> > >
> > > - if (test_bit(BLOCK_GROUP_FLAG_NEEDS_FREE_SPACE, &block_group->runtime_flags)) {
> > > - ret = __add_block_group_free_space(trans, block_group, path);
> > > - if (ret)
> > > - return ret;
> > > - }
> > > + ret = __add_block_group_free_space(trans, block_group, path);
> > > + if (ret)
> > > + return ret;
> > >
> > > info = search_free_space_info(NULL, block_group, path, 0);
> > > if (IS_ERR(info))
> > > @@ -1403,9 +1399,12 @@ static int __add_block_group_free_space(struct btrfs_trans_handle *trans,
> > > struct btrfs_block_group *block_group,
> > > struct btrfs_path *path)
> > > {
> > > + bool own_path = false;
> > > int ret;
> > >
> > > - clear_bit(BLOCK_GROUP_FLAG_NEEDS_FREE_SPACE, &block_group->runtime_flags);
> > > + if (!test_and_clear_bit(BLOCK_GROUP_FLAG_NEEDS_FREE_SPACE,
> > > + &block_group->runtime_flags))
> > > + return 0;
> > >
> > > /*
> > > * While rebuilding the free space tree we may allocate new metadata
> > > @@ -1430,10 +1429,19 @@ static int __add_block_group_free_space(struct btrfs_trans_handle *trans,
> > > */
> > > set_bit(BLOCK_GROUP_FLAG_FREE_SPACE_ADDED, &block_group->runtime_flags);
> > >
> > > + if (!path) {
> > > + path = btrfs_alloc_path();
> > > + if (!path) {
> > > + btrfs_abort_transaction(trans, -ENOMEM);
> > > + return -ENOMEM;
> > > + }
> > > + own_path = true;
> > > + }
> > > +
> >
> > Is the "own_path" change intended to be bundled with this one? If so,
> > can you mention it in the commit message as well?
>
> Yes it's supposed, why wouldn't it?
> This is because the path allocation from add_block_group_free_space()
> has to be gone and done in this function now if it receives a NULL
> path.
>
> I would think this is obvious since the diff for
> add_block_group_free_space() removes the path allocation.
That totally makes sense. All I'm asking for is a
"Since add_block_group_free_space() conditionally allocated the path
based on the check, move that allocation into
__add_block_group_free_space() as well" in the commit message.
(or whatever equivalent you like)
>
> Thanks.
>
> >
> > > ret = add_new_free_space_info(trans, block_group, path);
> > > if (ret) {
> > > btrfs_abort_transaction(trans, ret);
> > > - return ret;
> > > + goto out;
> > > }
> > >
> > > ret = __add_to_free_space_tree(trans, block_group, path,
> > > @@ -1441,33 +1449,23 @@ static int __add_block_group_free_space(struct btrfs_trans_handle *trans,
> > > if (ret)
> > > btrfs_abort_transaction(trans, ret);
> > >
> > > - return 0;
> > > +out:
> > > + if (own_path)
> > > + btrfs_free_path(path);
> > > +
> > > + return ret;
> > > }
> > >
> > > int add_block_group_free_space(struct btrfs_trans_handle *trans,
> > > struct btrfs_block_group *block_group)
> > > {
> > > - struct btrfs_fs_info *fs_info = trans->fs_info;
> > > - struct btrfs_path *path = NULL;
> > > - int ret = 0;
> > > + int ret;
> > >
> > > - if (!btrfs_fs_compat_ro(fs_info, FREE_SPACE_TREE))
> > > + if (!btrfs_fs_compat_ro(trans->fs_info, FREE_SPACE_TREE))
> > > return 0;
> > >
> > > mutex_lock(&block_group->free_space_lock);
> > > - if (!test_bit(BLOCK_GROUP_FLAG_NEEDS_FREE_SPACE, &block_group->runtime_flags))
> > > - goto out;
> > > -
> > > - path = btrfs_alloc_path();
> > > - if (!path) {
> > > - ret = -ENOMEM;
> > > - btrfs_abort_transaction(trans, ret);
> > > - goto out;
> > > - }
> > > -
> > > - ret = __add_block_group_free_space(trans, block_group, path);
> > > -out:
> > > - btrfs_free_path(path);
> > > + ret = __add_block_group_free_space(trans, block_group, NULL);
> > > mutex_unlock(&block_group->free_space_lock);
> > > return ret;
> > > }
> > > --
> > > 2.47.2
> > >
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH 3/3] btrfs: check BLOCK_GROUP_FLAG_NEEDS_FREE_SPACE at __add_block_group_free_space()
2025-06-16 16:49 ` Boris Burkov
@ 2025-06-16 21:47 ` Filipe Manana
0 siblings, 0 replies; 9+ messages in thread
From: Filipe Manana @ 2025-06-16 21:47 UTC (permalink / raw)
To: Boris Burkov; +Cc: linux-btrfs
On Mon, Jun 16, 2025 at 5:49 PM Boris Burkov <boris@bur.io> wrote:
>
> On Mon, Jun 16, 2025 at 04:48:34PM +0100, Filipe Manana wrote:
> > On Mon, Jun 16, 2025 at 4:45 PM Boris Burkov <boris@bur.io> wrote:
> > >
> > > On Sun, Jun 08, 2025 at 11:43:34PM +0100, fdmanana@kernel.org wrote:
> > > > From: Filipe Manana <fdmanana@suse.com>
> > > >
> > > > Every caller of __add_block_group_free_space() is checking if the flag
> > > > BLOCK_GROUP_FLAG_NEEDS_FREE_SPACE is set before calling it. This is
> > > > duplicate code and it's prone to some mistake in case we add more callers
> > > > in the future. So move the check for that flag into the start of
> > > > __add_block_group_free_space().
> > > >
> > > > Signed-off-by: Filipe Manana <fdmanana@suse.com>
> > > > ---
> > > > fs/btrfs/free-space-tree.c | 58 ++++++++++++++++++--------------------
> > > > 1 file changed, 28 insertions(+), 30 deletions(-)
> > > >
> > > > diff --git a/fs/btrfs/free-space-tree.c b/fs/btrfs/free-space-tree.c
> > > > index af005fb4b676..f03f3610b713 100644
> > > > --- a/fs/btrfs/free-space-tree.c
> > > > +++ b/fs/btrfs/free-space-tree.c
> > > > @@ -816,11 +816,9 @@ int __remove_from_free_space_tree(struct btrfs_trans_handle *trans,
> > > > u32 flags;
> > > > int ret;
> > > >
> > > > - if (test_bit(BLOCK_GROUP_FLAG_NEEDS_FREE_SPACE, &block_group->runtime_flags)) {
> > > > - ret = __add_block_group_free_space(trans, block_group, path);
> > > > - if (ret)
> > > > - return ret;
> > > > - }
> > > > + ret = __add_block_group_free_space(trans, block_group, path);
> > > > + if (ret)
> > > > + return ret;
> > > >
> > > > info = search_free_space_info(NULL, block_group, path, 0);
> > > > if (IS_ERR(info))
> > > > @@ -1011,11 +1009,9 @@ int __add_to_free_space_tree(struct btrfs_trans_handle *trans,
> > > > u32 flags;
> > > > int ret;
> > > >
> > > > - if (test_bit(BLOCK_GROUP_FLAG_NEEDS_FREE_SPACE, &block_group->runtime_flags)) {
> > > > - ret = __add_block_group_free_space(trans, block_group, path);
> > > > - if (ret)
> > > > - return ret;
> > > > - }
> > > > + ret = __add_block_group_free_space(trans, block_group, path);
> > > > + if (ret)
> > > > + return ret;
> > > >
> > > > info = search_free_space_info(NULL, block_group, path, 0);
> > > > if (IS_ERR(info))
> > > > @@ -1403,9 +1399,12 @@ static int __add_block_group_free_space(struct btrfs_trans_handle *trans,
> > > > struct btrfs_block_group *block_group,
> > > > struct btrfs_path *path)
> > > > {
> > > > + bool own_path = false;
> > > > int ret;
> > > >
> > > > - clear_bit(BLOCK_GROUP_FLAG_NEEDS_FREE_SPACE, &block_group->runtime_flags);
> > > > + if (!test_and_clear_bit(BLOCK_GROUP_FLAG_NEEDS_FREE_SPACE,
> > > > + &block_group->runtime_flags))
> > > > + return 0;
> > > >
> > > > /*
> > > > * While rebuilding the free space tree we may allocate new metadata
> > > > @@ -1430,10 +1429,19 @@ static int __add_block_group_free_space(struct btrfs_trans_handle *trans,
> > > > */
> > > > set_bit(BLOCK_GROUP_FLAG_FREE_SPACE_ADDED, &block_group->runtime_flags);
> > > >
> > > > + if (!path) {
> > > > + path = btrfs_alloc_path();
> > > > + if (!path) {
> > > > + btrfs_abort_transaction(trans, -ENOMEM);
> > > > + return -ENOMEM;
> > > > + }
> > > > + own_path = true;
> > > > + }
> > > > +
> > >
> > > Is the "own_path" change intended to be bundled with this one? If so,
> > > can you mention it in the commit message as well?
> >
> > Yes it's supposed, why wouldn't it?
> > This is because the path allocation from add_block_group_free_space()
> > has to be gone and done in this function now if it receives a NULL
> > path.
> >
> > I would think this is obvious since the diff for
> > add_block_group_free_space() removes the path allocation.
>
> That totally makes sense. All I'm asking for is a
> "Since add_block_group_free_space() conditionally allocated the path
> based on the check, move that allocation into
> __add_block_group_free_space() as well" in the commit message.
Ok, I'll add a note to the commit message before pushing to for-next.
Thanks.
>
> (or whatever equivalent you like)
>
> >
> > Thanks.
> >
> > >
> > > > ret = add_new_free_space_info(trans, block_group, path);
> > > > if (ret) {
> > > > btrfs_abort_transaction(trans, ret);
> > > > - return ret;
> > > > + goto out;
> > > > }
> > > >
> > > > ret = __add_to_free_space_tree(trans, block_group, path,
> > > > @@ -1441,33 +1449,23 @@ static int __add_block_group_free_space(struct btrfs_trans_handle *trans,
> > > > if (ret)
> > > > btrfs_abort_transaction(trans, ret);
> > > >
> > > > - return 0;
> > > > +out:
> > > > + if (own_path)
> > > > + btrfs_free_path(path);
> > > > +
> > > > + return ret;
> > > > }
> > > >
> > > > int add_block_group_free_space(struct btrfs_trans_handle *trans,
> > > > struct btrfs_block_group *block_group)
> > > > {
> > > > - struct btrfs_fs_info *fs_info = trans->fs_info;
> > > > - struct btrfs_path *path = NULL;
> > > > - int ret = 0;
> > > > + int ret;
> > > >
> > > > - if (!btrfs_fs_compat_ro(fs_info, FREE_SPACE_TREE))
> > > > + if (!btrfs_fs_compat_ro(trans->fs_info, FREE_SPACE_TREE))
> > > > return 0;
> > > >
> > > > mutex_lock(&block_group->free_space_lock);
> > > > - if (!test_bit(BLOCK_GROUP_FLAG_NEEDS_FREE_SPACE, &block_group->runtime_flags))
> > > > - goto out;
> > > > -
> > > > - path = btrfs_alloc_path();
> > > > - if (!path) {
> > > > - ret = -ENOMEM;
> > > > - btrfs_abort_transaction(trans, ret);
> > > > - goto out;
> > > > - }
> > > > -
> > > > - ret = __add_block_group_free_space(trans, block_group, path);
> > > > -out:
> > > > - btrfs_free_path(path);
> > > > + ret = __add_block_group_free_space(trans, block_group, NULL);
> > > > mutex_unlock(&block_group->free_space_lock);
> > > > return ret;
> > > > }
> > > > --
> > > > 2.47.2
> > > >
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 0/3] btrfs: free space tree fixes and cleanups
2025-06-08 22:43 [PATCH 0/3] btrfs: free space tree fixes and cleanups fdmanana
` (2 preceding siblings ...)
2025-06-08 22:43 ` [PATCH 3/3] btrfs: check BLOCK_GROUP_FLAG_NEEDS_FREE_SPACE at __add_block_group_free_space() fdmanana
@ 2025-06-16 15:45 ` Boris Burkov
3 siblings, 0 replies; 9+ messages in thread
From: Boris Burkov @ 2025-06-16 15:45 UTC (permalink / raw)
To: fdmanana; +Cc: linux-btrfs
On Sun, Jun 08, 2025 at 11:43:31PM +0100, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
>
> Fix a regression when rebuilding a free space tree, reported by syzbot,
> ensure transaction aborts where critical when adding a new block group to
> the free space tree, and a cleanup. Details in the changelogs.
This series LGTM, you can add:
Reviewed-by: Boris Burkov <boris@bur.io>
>
> Filipe Manana (3):
> btrfs: fix failure to rebuild free space tree using multiple transactions
> btrfs: always abort transaction on failure to add block group to free space tree
> btrfs: check BLOCK_GROUP_FLAG_NEEDS_FREE_SPACE at __add_block_group_free_space()
>
> fs/btrfs/block-group.h | 2 +
> fs/btrfs/free-space-tree.c | 110 +++++++++++++++++++++++++------------
> 2 files changed, 77 insertions(+), 35 deletions(-)
>
> --
> 2.47.2
>
^ permalink raw reply [flat|nested] 9+ messages in thread