[PATCH 0/2] btrfs: unfold transaction aborts for bg delete and get rid of a BUG_ON()

[PATCH 0/2] btrfs: unfold transaction aborts for bg delete and get rid of a BUG_ON()

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

Simple changes, details in the change logs.

Filipe Manana (2):
  btrfs: abort transaction on error in btrfs_remove_block_group()
  btrfs: do not BUG_ON() in btrfs_remove_block_group()

 fs/btrfs/block-group.c | 26 ++++++++++++++++++++------
 fs/btrfs/volumes.c     |  7 +++----
 2 files changed, 23 insertions(+), 10 deletions(-)

-- 
2.47.2

[PATCH 1/2] btrfs: abort transaction on error in btrfs_remove_block_group()

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

When btrfs_remove_block_group() fails we abort the transaction in its
single caller (btrfs_remove_chunk()). This makes it harder to find out
where exactly the failure happened, as several steps inside
btrfs_remove_block_group() can fail.

So make btrfs_remove_block_group() abort the transaction whenever an
error happens, instead of aborting in its caller.

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/block-group.c | 19 ++++++++++++++-----
 fs/btrfs/volumes.c     |  7 +++----
 2 files changed, 17 insertions(+), 9 deletions(-)

diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
index 3a0521236ecd..7b723571501e 100644
--- a/fs/btrfs/block-group.c
+++ b/fs/btrfs/block-group.c
@@ -1108,8 +1108,10 @@ int btrfs_remove_block_group(struct btrfs_trans_handle *trans,
 	bool remove_rsv = false;
 
 	block_group = btrfs_lookup_block_group(fs_info, map->start);
-	if (!block_group)
+	if (unlikely(!block_group)) {
+		btrfs_abort_transaction(trans, -ENOENT);
 		return -ENOENT;
+	}
 
 	BUG_ON(!block_group->ro && !(block_group->flags & BTRFS_BLOCK_GROUP_REMAPPED));
 
@@ -1143,8 +1145,9 @@ int btrfs_remove_block_group(struct btrfs_trans_handle *trans,
 	btrfs_clear_data_reloc_bg(block_group);
 
 	path = btrfs_alloc_path();
-	if (!path) {
+	if (unlikely(!path)) {
 		ret = -ENOMEM;
+		btrfs_abort_transaction(trans, ret);
 		goto out;
 	}
 
@@ -1180,8 +1183,10 @@ int btrfs_remove_block_group(struct btrfs_trans_handle *trans,
 	mutex_unlock(&trans->transaction->cache_write_mutex);
 
 	ret = btrfs_remove_free_space_inode(trans, inode, block_group);
-	if (ret)
+	if (unlikely(ret)) {
+		btrfs_abort_transaction(trans, ret);
 		goto out;
+	}
 
 	write_lock(&fs_info->block_group_cache_lock);
 	rb_erase_cached(&block_group->cache_node,
@@ -1268,13 +1273,17 @@ int btrfs_remove_block_group(struct btrfs_trans_handle *trans,
 	 */
 	if (!(block_group->flags & BTRFS_BLOCK_GROUP_REMAPPED)) {
 		ret = btrfs_remove_block_group_free_space(trans, block_group);
-		if (ret)
+		if (unlikely(ret)) {
+			btrfs_abort_transaction(trans, ret);
 			goto out;
+		}
 	}
 
 	ret = remove_block_group_item(trans, path, block_group);
-	if (ret < 0)
+	if (unlikely(ret < 0)) {
+		btrfs_abort_transaction(trans, ret);
 		goto out;
+	}
 
 	spin_lock(&block_group->lock);
 	/*
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index cff2412bc879..d33780082b8d 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -3384,11 +3384,10 @@ int btrfs_remove_chunk(struct btrfs_trans_handle *trans, u64 chunk_offset)
 	 */
 	btrfs_trans_release_chunk_metadata(trans);
 
+	/* On error, btrfs_remove_block_group() aborts the transaction. */
 	ret = btrfs_remove_block_group(trans, map);
-	if (unlikely(ret)) {
-		btrfs_abort_transaction(trans, ret);
-		goto out;
-	}
+	if (unlikely(ret))
+		ASSERT(BTRFS_FS_ERROR(fs_info) != 0);
 
 out:
 	if (trans->removing_chunk) {
-- 
2.47.2

[PATCH 2/2] btrfs: do not BUG_ON() in btrfs_remove_block_group()

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

There's no need to BUG_ON(), we can just abort the transaction and return
an error.

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

diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
index 7b723571501e..3186ed4fd26d 100644
--- a/fs/btrfs/block-group.c
+++ b/fs/btrfs/block-group.c
@@ -1113,7 +1113,12 @@ int btrfs_remove_block_group(struct btrfs_trans_handle *trans,
 		return -ENOENT;
 	}
 
-	BUG_ON(!block_group->ro && !(block_group->flags & BTRFS_BLOCK_GROUP_REMAPPED));
+	if (unlikely(!block_group->ro &&
+		     !(block_group->flags & BTRFS_BLOCK_GROUP_REMAPPED))) {
+		ret = -EUCLEAN;
+		btrfs_abort_transaction(trans, ret);
+		goto out;
+	}
 
 	trace_btrfs_remove_block_group(block_group);
 	/*
-- 
2.47.2

Re: [PATCH 0/2] btrfs: unfold transaction aborts for bg delete and get rid of a BUG_ON()

[Johannes Thumshirn]

On 1/23/26 11:19 AM, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
>
> Simple changes, details in the change logs.
>
> Filipe Manana (2):
>    btrfs: abort transaction on error in btrfs_remove_block_group()
>    btrfs: do not BUG_ON() in btrfs_remove_block_group()
>
>   fs/btrfs/block-group.c | 26 ++++++++++++++++++++------
>   fs/btrfs/volumes.c     |  7 +++----
>   2 files changed, 23 insertions(+), 10 deletions(-)
>

+	if (unlikely(ret))
+		ASSERT(BTRFS_FS_ERROR(fs_info) != 0);

looks strange given ASSERT()s are a config option, but I've compiled it 
with and without and GCC didn't complain.


Anyways looks good,

Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>