[PATCH 0/4] Transaction aborts in free-space-tree.c

[PATCH 0/4] Transaction aborts in free-space-tree.c

[David Sterba]

Fix the transaction abort pattern in free-space-tree, it's been there
from the beginning and not conforming to the pattern we use elsewhere.

David Sterba (4):
  btrfs: move transaction aborts to the error site in
    convert_free_space_to_bitmaps()
  btrfs: move transaction aborts to the error site in
    convert_free_space_to_extents()
  btrfs: move transaction aborts to the error site in
    remove_from_free_space_tree()
  btrfs: move transaction aborts to the error site in
    add_to_free_space_tree()

 fs/btrfs/free-space-tree.c | 48 +++++++++++++++++++++++++-------------
 1 file changed, 32 insertions(+), 16 deletions(-)

-- 
2.49.0

[PATCH 1/4] btrfs: move transaction aborts to the error site in convert_free_space_to_bitmaps()

[David Sterba]

Transaction aborts should be done next to the place the error happens,
which was not done in convert_free_space_to_bitmaps(). The DEBUG_WARN()
is removed because we get the abort message.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/free-space-tree.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/fs/btrfs/free-space-tree.c b/fs/btrfs/free-space-tree.c
index ef62f3a69267f3..b5458a71f0b3e8 100644
--- a/fs/btrfs/free-space-tree.c
+++ b/fs/btrfs/free-space-tree.c
@@ -223,6 +223,7 @@ int convert_free_space_to_bitmaps(struct btrfs_trans_handle *trans,
 	bitmap = alloc_bitmap(bitmap_size);
 	if (!bitmap) {
 		ret = -ENOMEM;
+		btrfs_abort_transaction(trans, ret);
 		goto out;
 	}
 
@@ -235,8 +236,10 @@ int convert_free_space_to_bitmaps(struct btrfs_trans_handle *trans,
 
 	while (!done) {
 		ret = btrfs_search_prev_slot(trans, root, &key, path, -1, 1);
-		if (ret)
+		if (ret) {
+			btrfs_abort_transaction(trans, ret);
 			goto out;
+		}
 
 		leaf = path->nodes[0];
 		nr = 0;
@@ -271,14 +274,17 @@ int convert_free_space_to_bitmaps(struct btrfs_trans_handle *trans,
 		}
 
 		ret = btrfs_del_items(trans, root, path, path->slots[0], nr);
-		if (ret)
+		if (ret) {
+			btrfs_abort_transaction(trans, ret);
 			goto out;
+		}
 		btrfs_release_path(path);
 	}
 
 	info = search_free_space_info(trans, block_group, path, 1);
 	if (IS_ERR(info)) {
 		ret = PTR_ERR(info);
+		btrfs_abort_transaction(trans, ret);
 		goto out;
 	}
 	leaf = path->nodes[0];
@@ -293,8 +299,8 @@ int convert_free_space_to_bitmaps(struct btrfs_trans_handle *trans,
 			  "incorrect extent count for %llu; counted %u, expected %u",
 			  block_group->start, extent_count,
 			  expected_extent_count);
-		DEBUG_WARN();
 		ret = -EIO;
+		btrfs_abort_transaction(trans, ret);
 		goto out;
 	}
 
@@ -315,8 +321,10 @@ int convert_free_space_to_bitmaps(struct btrfs_trans_handle *trans,
 
 		ret = btrfs_insert_empty_item(trans, root, path, &key,
 					      data_size);
-		if (ret)
+		if (ret) {
+			btrfs_abort_transaction(trans, ret);
 			goto out;
+		}
 
 		leaf = path->nodes[0];
 		ptr = btrfs_item_ptr_offset(leaf, path->slots[0]);
@@ -331,8 +339,6 @@ int convert_free_space_to_bitmaps(struct btrfs_trans_handle *trans,
 	ret = 0;
 out:
 	kvfree(bitmap);
-	if (ret)
-		btrfs_abort_transaction(trans, ret);
 	return ret;
 }
 
-- 
2.49.0

[PATCH 2/4] btrfs: move transaction aborts to the error site in convert_free_space_to_extents()

[David Sterba]

Transaction aborts should be done next to the place the error happens,
which was not done in convert_free_space_to_extents().

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/free-space-tree.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/fs/btrfs/free-space-tree.c b/fs/btrfs/free-space-tree.c
index b5458a71f0b3e8..ef39d103443524 100644
--- a/fs/btrfs/free-space-tree.c
+++ b/fs/btrfs/free-space-tree.c
@@ -364,6 +364,7 @@ int convert_free_space_to_extents(struct btrfs_trans_handle *trans,
 	bitmap = alloc_bitmap(bitmap_size);
 	if (!bitmap) {
 		ret = -ENOMEM;
+		btrfs_abort_transaction(trans, ret);
 		goto out;
 	}
 
@@ -376,8 +377,10 @@ int convert_free_space_to_extents(struct btrfs_trans_handle *trans,
 
 	while (!done) {
 		ret = btrfs_search_prev_slot(trans, root, &key, path, -1, 1);
-		if (ret)
+		if (ret) {
+			btrfs_abort_transaction(trans, ret);
 			goto out;
+		}
 
 		leaf = path->nodes[0];
 		nr = 0;
@@ -418,14 +421,17 @@ int convert_free_space_to_extents(struct btrfs_trans_handle *trans,
 		}
 
 		ret = btrfs_del_items(trans, root, path, path->slots[0], nr);
-		if (ret)
+		if (ret) {
+			btrfs_abort_transaction(trans, ret);
 			goto out;
+		}
 		btrfs_release_path(path);
 	}
 
 	info = search_free_space_info(trans, block_group, path, 1);
 	if (IS_ERR(info)) {
 		ret = PTR_ERR(info);
+		btrfs_abort_transaction(trans, ret);
 		goto out;
 	}
 	leaf = path->nodes[0];
@@ -447,8 +453,10 @@ int convert_free_space_to_extents(struct btrfs_trans_handle *trans,
 		key.offset = (end_bit - start_bit) * block_group->fs_info->sectorsize;
 
 		ret = btrfs_insert_empty_item(trans, root, path, &key, 0);
-		if (ret)
+		if (ret) {
+			btrfs_abort_transaction(trans, ret);
 			goto out;
+		}
 		btrfs_release_path(path);
 
 		extent_count++;
@@ -461,16 +469,14 @@ int convert_free_space_to_extents(struct btrfs_trans_handle *trans,
 			  "incorrect extent count for %llu; counted %u, expected %u",
 			  block_group->start, extent_count,
 			  expected_extent_count);
-		DEBUG_WARN();
 		ret = -EIO;
+		btrfs_abort_transaction(trans, ret);
 		goto out;
 	}
 
 	ret = 0;
 out:
 	kvfree(bitmap);
-	if (ret)
-		btrfs_abort_transaction(trans, ret);
 	return ret;
 }
 
-- 
2.49.0

[PATCH 3/4] btrfs: move transaction aborts to the error site in remove_from_free_space_tree()

[David Sterba]

Transaction aborts should be done next to the place the error happens,
which was not done in remove_from_free_space_tree().

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/free-space-tree.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/free-space-tree.c b/fs/btrfs/free-space-tree.c
index ef39d103443524..ffe98bea6927f9 100644
--- a/fs/btrfs/free-space-tree.c
+++ b/fs/btrfs/free-space-tree.c
@@ -850,6 +850,7 @@ int remove_from_free_space_tree(struct btrfs_trans_handle *trans,
 	path = btrfs_alloc_path();
 	if (!path) {
 		ret = -ENOMEM;
+		btrfs_abort_transaction(trans, ret);
 		goto out;
 	}
 
@@ -857,6 +858,7 @@ int remove_from_free_space_tree(struct btrfs_trans_handle *trans,
 	if (!block_group) {
 		DEBUG_WARN("no block group found for start=%llu", start);
 		ret = -ENOENT;
+		btrfs_abort_transaction(trans, ret);
 		goto out;
 	}
 
@@ -864,12 +866,12 @@ int remove_from_free_space_tree(struct btrfs_trans_handle *trans,
 	ret = __remove_from_free_space_tree(trans, block_group, path, start,
 					    size);
 	mutex_unlock(&block_group->free_space_lock);
+	if (ret)
+		btrfs_abort_transaction(trans, ret);
 
 	btrfs_put_block_group(block_group);
 out:
 	btrfs_free_path(path);
-	if (ret)
-		btrfs_abort_transaction(trans, ret);
 	return ret;
 }
 
-- 
2.49.0

[PATCH 4/4] btrfs: move transaction aborts to the error site in add_to_free_space_tree()

[David Sterba]

Transaction aborts should be done next to the place the error happens,
which was not done in add_to_free_space_tree().

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/free-space-tree.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/free-space-tree.c b/fs/btrfs/free-space-tree.c
index ffe98bea6927f9..0c573d46639ae9 100644
--- a/fs/btrfs/free-space-tree.c
+++ b/fs/btrfs/free-space-tree.c
@@ -1045,6 +1045,7 @@ int add_to_free_space_tree(struct btrfs_trans_handle *trans,
 	path = btrfs_alloc_path();
 	if (!path) {
 		ret = -ENOMEM;
+		btrfs_abort_transaction(trans, ret);
 		goto out;
 	}
 
@@ -1052,18 +1053,19 @@ int add_to_free_space_tree(struct btrfs_trans_handle *trans,
 	if (!block_group) {
 		DEBUG_WARN("no block group found for start=%llu", start);
 		ret = -ENOENT;
+		btrfs_abort_transaction(trans, ret);
 		goto out;
 	}
 
 	mutex_lock(&block_group->free_space_lock);
 	ret = __add_to_free_space_tree(trans, block_group, path, start, size);
 	mutex_unlock(&block_group->free_space_lock);
+	if (ret)
+		btrfs_abort_transaction(trans, ret);
 
 	btrfs_put_block_group(block_group);
 out:
 	btrfs_free_path(path);
-	if (ret)
-		btrfs_abort_transaction(trans, ret);
 	return ret;
 }
 
-- 
2.49.0

Re: [PATCH 0/4] Transaction aborts in free-space-tree.c

[Daniel Vacek]

On Wed, 30 Apr 2025 at 18:45, David Sterba <dsterba@suse.com> wrote:
>
> Fix the transaction abort pattern in free-space-tree, it's been there
> from the beginning and not conforming to the pattern we use elsewhere.
>
> David Sterba (4):
>   btrfs: move transaction aborts to the error site in
>     convert_free_space_to_bitmaps()
>   btrfs: move transaction aborts to the error site in
>     convert_free_space_to_extents()
>   btrfs: move transaction aborts to the error site in
>     remove_from_free_space_tree()
>   btrfs: move transaction aborts to the error site in
>     add_to_free_space_tree()
>
>  fs/btrfs/free-space-tree.c | 48 +++++++++++++++++++++++++-------------
>  1 file changed, 32 insertions(+), 16 deletions(-)

This looks like unnecessary duplicating the code. Shall we rather go
the other way around?

>
> --
> 2.49.0
>
>

Re: [PATCH 0/4] Transaction aborts in free-space-tree.c

[David Sterba]

On Fri, May 02, 2025 at 03:15:49PM +0200, Daniel Vacek wrote:
> On Wed, 30 Apr 2025 at 18:45, David Sterba <dsterba@suse.com> wrote:
> >
> > Fix the transaction abort pattern in free-space-tree, it's been there
> > from the beginning and not conforming to the pattern we use elsewhere.
> >
> > David Sterba (4):
> >   btrfs: move transaction aborts to the error site in
> >     convert_free_space_to_bitmaps()
> >   btrfs: move transaction aborts to the error site in
> >     convert_free_space_to_extents()
> >   btrfs: move transaction aborts to the error site in
> >     remove_from_free_space_tree()
> >   btrfs: move transaction aborts to the error site in
> >     add_to_free_space_tree()
> >
> >  fs/btrfs/free-space-tree.c | 48 +++++++++++++++++++++++++-------------
> >  1 file changed, 32 insertions(+), 16 deletions(-)
> 
> This looks like unnecessary duplicating the code. Shall we rather go
> the other way around?

What do you mean? There's some smilarity among the functions so yeah
the add/remove pairs can be merged to one, but this is orthogonal to the
transaction abot calls.

Re: [PATCH 0/4] Transaction aborts in free-space-tree.c

[Daniel Vacek]

On Fri, 2 May 2025 at 15:24, David Sterba <dsterba@suse.cz> wrote:
>
> On Fri, May 02, 2025 at 03:15:49PM +0200, Daniel Vacek wrote:
> > On Wed, 30 Apr 2025 at 18:45, David Sterba <dsterba@suse.com> wrote:
> > >
> > > Fix the transaction abort pattern in free-space-tree, it's been there
> > > from the beginning and not conforming to the pattern we use elsewhere.
> > >
> > > David Sterba (4):
> > >   btrfs: move transaction aborts to the error site in
> > >     convert_free_space_to_bitmaps()
> > >   btrfs: move transaction aborts to the error site in
> > >     convert_free_space_to_extents()
> > >   btrfs: move transaction aborts to the error site in
> > >     remove_from_free_space_tree()
> > >   btrfs: move transaction aborts to the error site in
> > >     add_to_free_space_tree()
> > >
> > >  fs/btrfs/free-space-tree.c | 48 +++++++++++++++++++++++++-------------
> > >  1 file changed, 32 insertions(+), 16 deletions(-)
> >
> > This looks like unnecessary duplicating the code. Shall we rather go
> > the other way around?
>
> What do you mean? There's some smilarity among the functions so yeah
> the add/remove pairs can be merged to one, but this is orthogonal to the
> transaction abot calls.

Not that. I meant the code looks simpler without these patches. If
this is the pattern used elsewhere, maybe we should rather change
elsewhere to follow free-space-tree.c?

Re: [PATCH 0/4] Transaction aborts in free-space-tree.c

[David Sterba]

On Fri, May 02, 2025 at 03:32:52PM +0200, Daniel Vacek wrote:
> On Fri, 2 May 2025 at 15:24, David Sterba <dsterba@suse.cz> wrote:
> >
> > On Fri, May 02, 2025 at 03:15:49PM +0200, Daniel Vacek wrote:
> > > On Wed, 30 Apr 2025 at 18:45, David Sterba <dsterba@suse.com> wrote:
> > > >
> > > > Fix the transaction abort pattern in free-space-tree, it's been there
> > > > from the beginning and not conforming to the pattern we use elsewhere.
> > > >
> > > > David Sterba (4):
> > > >   btrfs: move transaction aborts to the error site in
> > > >     convert_free_space_to_bitmaps()
> > > >   btrfs: move transaction aborts to the error site in
> > > >     convert_free_space_to_extents()
> > > >   btrfs: move transaction aborts to the error site in
> > > >     remove_from_free_space_tree()
> > > >   btrfs: move transaction aborts to the error site in
> > > >     add_to_free_space_tree()
> > > >
> > > >  fs/btrfs/free-space-tree.c | 48 +++++++++++++++++++++++++-------------
> > > >  1 file changed, 32 insertions(+), 16 deletions(-)
> > >
> > > This looks like unnecessary duplicating the code. Shall we rather go
> > > the other way around?
> >
> > What do you mean? There's some smilarity among the functions so yeah
> > the add/remove pairs can be merged to one, but this is orthogonal to the
> > transaction abot calls.
> 
> Not that. I meant the code looks simpler without these patches. If
> this is the pattern used elsewhere, maybe we should rather change
> elsewhere to follow free-space-tree.c?

https://btrfs.readthedocs.io/en/latest/dev/Development-notes.html#error-handling-and-transaction-abort

"Please keep all transaction abort exactly at the place where they happen
and do not merge them to one. This pattern should be used everywhere and
is important when debugging because we can pinpoint the line in the code
from the syslog message and do not have to guess which way it got to the
merged call."

Re: [PATCH 0/4] Transaction aborts in free-space-tree.c

[Daniel Vacek]

On Fri, 2 May 2025 at 16:31, David Sterba <dsterba@suse.cz> wrote:
>
> On Fri, May 02, 2025 at 03:32:52PM +0200, Daniel Vacek wrote:
> > On Fri, 2 May 2025 at 15:24, David Sterba <dsterba@suse.cz> wrote:
> > >
> > > On Fri, May 02, 2025 at 03:15:49PM +0200, Daniel Vacek wrote:
> > > > On Wed, 30 Apr 2025 at 18:45, David Sterba <dsterba@suse.com> wrote:
> > > > >
> > > > > Fix the transaction abort pattern in free-space-tree, it's been there
> > > > > from the beginning and not conforming to the pattern we use elsewhere.
> > > > >
> > > > > David Sterba (4):
> > > > >   btrfs: move transaction aborts to the error site in
> > > > >     convert_free_space_to_bitmaps()
> > > > >   btrfs: move transaction aborts to the error site in
> > > > >     convert_free_space_to_extents()
> > > > >   btrfs: move transaction aborts to the error site in
> > > > >     remove_from_free_space_tree()
> > > > >   btrfs: move transaction aborts to the error site in
> > > > >     add_to_free_space_tree()
> > > > >
> > > > >  fs/btrfs/free-space-tree.c | 48 +++++++++++++++++++++++++-------------
> > > > >  1 file changed, 32 insertions(+), 16 deletions(-)
> > > >
> > > > This looks like unnecessary duplicating the code. Shall we rather go
> > > > the other way around?
> > >
> > > What do you mean? There's some smilarity among the functions so yeah
> > > the add/remove pairs can be merged to one, but this is orthogonal to the
> > > transaction abot calls.
> >
> > Not that. I meant the code looks simpler without these patches. If
> > this is the pattern used elsewhere, maybe we should rather change
> > elsewhere to follow free-space-tree.c?
>
> https://btrfs.readthedocs.io/en/latest/dev/Development-notes.html#error-handling-and-transaction-abort
>
> "Please keep all transaction abort exactly at the place where they happen
> and do not merge them to one. This pattern should be used everywhere and
> is important when debugging because we can pinpoint the line in the code
> from the syslog message and do not have to guess which way it got to the
> merged call."

/*
 * Call btrfs_abort_transaction as early as possible when an error condition is
 * detected, that way the exact stack trace is reported for some errors.
 */
#define btrfs_abort_transaction(trans, error)

Indeed, that's clever. It was not clear from the commit message why
this pattern is preferred.

Thanks for the explanation.