All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/3] btrfs-progs: Handle error properly in btrfs_commit_transaction()
@ 2019-04-16  7:15 Qu Wenruo
  2019-04-16  7:15 ` [PATCH 1/3] btrfs-progs: Remove the dead branch in btrfs_run_delayed_refs() Qu Wenruo
                   ` (3 more replies)
  0 siblings, 4 replies; 8+ messages in thread
From: Qu Wenruo @ 2019-04-16  7:15 UTC (permalink / raw)
  To: linux-btrfs

This patchset can be fetched from github:
https://github.com/adam900710/btrfs-progs/tree/commit_failure_cleanup

This patchset will address the BUG_ON() triggered in fuzz-tests/003 and
fuzz-tests/009.

For proper error handling in btrfs_commit_transaction(), we need a way
to clean up per-transaction data properly.

Currently we only have delayed refs which are per-transaction, so
introduce a new function, btrfs_destroy_delayed_refs() to cleanup
delayed refs.

Now btrfs_commit_transaction() can error out gracefully with proper
cleanup.

Patch 1 and 2 are just some minor cleanups found when crafting the 3rd
patch.

Qu Wenruo (3):
  btrfs-progs: Remove the dead branch in btrfs_run_delayed_refs()
  btrfs-progs: Refactor btrfs_finish_extent_commit()
  btrfs-progs: Handle error properly in btrfs_commit_transaction()

 ctree.h       |  4 +---
 delayed-ref.c | 24 ++++++++++++++++++++++++
 delayed-ref.h |  5 +++++
 extent-tree.c | 22 +++++++++-------------
 transaction.c | 23 +++++++++++++++--------
 5 files changed, 54 insertions(+), 24 deletions(-)

-- 
2.21.0


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 1/3] btrfs-progs: Remove the dead branch in btrfs_run_delayed_refs()
  2019-04-16  7:15 [PATCH 0/3] btrfs-progs: Handle error properly in btrfs_commit_transaction() Qu Wenruo
@ 2019-04-16  7:15 ` Qu Wenruo
  2019-04-16  7:36   ` Nikolay Borisov
  2019-04-16  7:15 ` [PATCH 2/3] btrfs-progs: Refactor btrfs_finish_extent_commit() Qu Wenruo
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 8+ messages in thread
From: Qu Wenruo @ 2019-04-16  7:15 UTC (permalink / raw)
  To: linux-btrfs

cleanup_ref_head() will only return 0 or 1, no way to return minutes
value.

So remove the dead branch.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 extent-tree.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/extent-tree.c b/extent-tree.c
index 8c9cdeff3b02..b7ece0ccc237 100644
--- a/extent-tree.c
+++ b/extent-tree.c
@@ -4235,8 +4235,6 @@ int btrfs_run_delayed_refs(struct btrfs_trans_handle *trans, unsigned long nr)
 				/* We dropped our lock, we need to loop. */
 				ret = 0;
 				continue;
-			} else if (ret) {
-				return ret;
 			}
 			locked_ref = NULL;
 			continue;
-- 
2.21.0


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH 2/3] btrfs-progs: Refactor btrfs_finish_extent_commit()
  2019-04-16  7:15 [PATCH 0/3] btrfs-progs: Handle error properly in btrfs_commit_transaction() Qu Wenruo
  2019-04-16  7:15 ` [PATCH 1/3] btrfs-progs: Remove the dead branch in btrfs_run_delayed_refs() Qu Wenruo
@ 2019-04-16  7:15 ` Qu Wenruo
  2019-04-16  7:39   ` Nikolay Borisov
  2019-04-16  7:15 ` [PATCH 3/3] btrfs-progs: Handle error properly in btrfs_commit_transaction() Qu Wenruo
  2019-05-13 13:55 ` [PATCH 0/3] " David Sterba
  3 siblings, 1 reply; 8+ messages in thread
From: Qu Wenruo @ 2019-04-16  7:15 UTC (permalink / raw)
  To: linux-btrfs

This patch will refactor btrfs_finish_extent_commit() by:
- Make it return void
  There is no failure pattern for btrfs_finish_extent_commit(), thus it
  always return 0. And the caller doesn't care about the return value.
  So no need to return int.

- Remove @root and @unpin parameters
  @root is only used to extract fs_info, which can be extracted from
  transaction handler already.
  @unpin is always fs_info->pinned_extents.
  All these parameters can be extracted from @trans, no need to pass
  them.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 ctree.h       |  4 +---
 extent-tree.c | 14 ++++++--------
 transaction.c |  3 +--
 3 files changed, 8 insertions(+), 13 deletions(-)

diff --git a/ctree.h b/ctree.h
index 3357290d7045..76f52b1c9b08 100644
--- a/ctree.h
+++ b/ctree.h
@@ -2516,9 +2516,7 @@ int btrfs_free_extent(struct btrfs_trans_handle *trans,
 		      struct btrfs_root *root,
 		      u64 bytenr, u64 num_bytes, u64 parent,
 		      u64 root_objectid, u64 owner, u64 offset);
-int btrfs_finish_extent_commit(struct btrfs_trans_handle *trans,
-			       struct btrfs_root *root,
-			       struct extent_io_tree *unpin);
+void btrfs_finish_extent_commit(struct btrfs_trans_handle *trans);
 int btrfs_inc_extent_ref(struct btrfs_trans_handle *trans,
 				struct btrfs_root *root,
 				u64 bytenr, u64 num_bytes, u64 parent,
diff --git a/extent-tree.c b/extent-tree.c
index b7ece0ccc237..1140ebfc61ff 100644
--- a/extent-tree.c
+++ b/extent-tree.c
@@ -1977,27 +1977,25 @@ next:
 	return 0;
 }
 
-int btrfs_finish_extent_commit(struct btrfs_trans_handle *trans,
-			       struct btrfs_root *root,
-			       struct extent_io_tree *unpin)
+void btrfs_finish_extent_commit(struct btrfs_trans_handle *trans)
 {
 	u64 start;
 	u64 end;
 	int ret;
-	struct extent_io_tree *free_space_cache;
-	free_space_cache = &root->fs_info->free_space_cache;
+	struct btrfs_fs_info *fs_info = trans->fs_info;
+	struct extent_io_tree *free_space_cache = &fs_info->free_space_cache;
+	struct extent_io_tree *pinned_extents = &fs_info->pinned_extents;
 
 	while(1) {
-		ret = find_first_extent_bit(unpin, 0, &start, &end,
+		ret = find_first_extent_bit(pinned_extents, 0, &start, &end,
 					    EXTENT_DIRTY);
 		if (ret)
 			break;
 		update_pinned_extents(trans->fs_info, start, end + 1 - start,
 				      0);
-		clear_extent_dirty(unpin, start, end);
+		clear_extent_dirty(pinned_extents, start, end);
 		set_extent_dirty(free_space_cache, start, end);
 	}
-	return 0;
 }
 
 static int pin_down_bytes(struct btrfs_trans_handle *trans, u64 bytenr,
diff --git a/transaction.c b/transaction.c
index 3a63988b0969..8d15eb11c7b5 100644
--- a/transaction.c
+++ b/transaction.c
@@ -198,8 +198,7 @@ commit_tree:
 	if (ret < 0)
 		goto out;
 	ret = write_ctree_super(trans);
-	btrfs_finish_extent_commit(trans, fs_info->extent_root,
-			           &fs_info->pinned_extents);
+	btrfs_finish_extent_commit(trans);
 	kfree(trans);
 	free_extent_buffer(root->commit_root);
 	root->commit_root = NULL;
-- 
2.21.0


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH 3/3] btrfs-progs: Handle error properly in btrfs_commit_transaction()
  2019-04-16  7:15 [PATCH 0/3] btrfs-progs: Handle error properly in btrfs_commit_transaction() Qu Wenruo
  2019-04-16  7:15 ` [PATCH 1/3] btrfs-progs: Remove the dead branch in btrfs_run_delayed_refs() Qu Wenruo
  2019-04-16  7:15 ` [PATCH 2/3] btrfs-progs: Refactor btrfs_finish_extent_commit() Qu Wenruo
@ 2019-04-16  7:15 ` Qu Wenruo
  2019-04-16  7:48   ` Nikolay Borisov
  2019-05-13 13:55 ` [PATCH 0/3] " David Sterba
  3 siblings, 1 reply; 8+ messages in thread
From: Qu Wenruo @ 2019-04-16  7:15 UTC (permalink / raw)
  To: linux-btrfs

[BUG]
When running fuzz-tests/003 and fuzz-tests/009, btrfs-progs will crash
due to BUG_ON().

[CAUSE]
We abused BUG_ON() in btrfs_commit_transaction(), which is one of the
most error prone function for fuzzed images.

Currently to cleanup the aborted transaction, we only need to clean up
the only per-transaction data: delayed refs.

This patch will introduce a new function, btrfs_destroy_delayed_refs()
to cleanup delayed refs when we failed to commit transaction.

With that function, we will gently destroy per-trans delayed ref, and
remove the BUG_ON()s in btrfs_commit_transaction().

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 delayed-ref.c | 24 ++++++++++++++++++++++++
 delayed-ref.h |  5 +++++
 extent-tree.c |  6 +++---
 transaction.c | 20 ++++++++++++++------
 4 files changed, 46 insertions(+), 9 deletions(-)

diff --git a/delayed-ref.c b/delayed-ref.c
index 9974dbbd4c6b..69f8be34fe18 100644
--- a/delayed-ref.c
+++ b/delayed-ref.c
@@ -605,3 +605,27 @@ free_ref:
 
 	return -ENOMEM;
 }
+
+void btrfs_destroy_delayed_refs(struct btrfs_trans_handle *trans)
+{
+	struct btrfs_fs_info *fs_info = trans->fs_info;
+	struct rb_node *node;
+	struct btrfs_delayed_ref_root *delayed_refs;
+
+	delayed_refs = &trans->delayed_refs;
+	if (RB_EMPTY_ROOT(&delayed_refs->href_root))
+		return;
+	while ((node = rb_first(&delayed_refs->href_root)) != NULL) {
+		struct btrfs_delayed_ref_head *head;
+		struct btrfs_delayed_ref_node *ref;
+		struct rb_node *n;
+
+		head = rb_entry(node, struct btrfs_delayed_ref_head, href_node);
+		while ((n = rb_first(&head->ref_tree)) != NULL) {
+			ref = rb_entry(n, struct btrfs_delayed_ref_node,
+				       ref_node);
+			drop_delayed_ref(trans, delayed_refs, head, ref);
+		}
+		ASSERT(cleanup_ref_head(trans, fs_info, head) == 0);
+	}
+}
diff --git a/delayed-ref.h b/delayed-ref.h
index 30a68b2a278c..4ff3eaa929ba 100644
--- a/delayed-ref.h
+++ b/delayed-ref.h
@@ -205,4 +205,9 @@ btrfs_delayed_node_to_tree_ref(struct btrfs_delayed_ref_node *node)
 {
 	return container_of(node, struct btrfs_delayed_tree_ref, node);
 }
+
+int cleanup_ref_head(struct btrfs_trans_handle *trans,
+		     struct btrfs_fs_info *fs_info,
+		     struct btrfs_delayed_ref_head *head);
+void btrfs_destroy_delayed_refs(struct btrfs_trans_handle *trans);
 #endif
diff --git a/extent-tree.c b/extent-tree.c
index 1140ebfc61ff..e62ee8c2ba13 100644
--- a/extent-tree.c
+++ b/extent-tree.c
@@ -4085,9 +4085,9 @@ static void unselect_delayed_ref_head(struct btrfs_delayed_ref_root *delayed_ref
 	delayed_refs->num_heads_ready++;
 }
 
-static int cleanup_ref_head(struct btrfs_trans_handle *trans,
-			    struct btrfs_fs_info *fs_info,
-			    struct btrfs_delayed_ref_head *head)
+int cleanup_ref_head(struct btrfs_trans_handle *trans,
+		     struct btrfs_fs_info *fs_info,
+		     struct btrfs_delayed_ref_head *head)
 {
 	struct btrfs_delayed_ref_root *delayed_refs;
 
diff --git a/transaction.c b/transaction.c
index 8d15eb11c7b5..138e10f0d6cc 100644
--- a/transaction.c
+++ b/transaction.c
@@ -17,6 +17,7 @@
 #include "kerncompat.h"
 #include "disk-io.h"
 #include "transaction.h"
+#include "delayed-ref.h"
 
 #include "messages.h"
 
@@ -165,7 +166,8 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans,
 	 * consistent
 	 */
 	ret = btrfs_run_delayed_refs(trans, -1);
-	BUG_ON(ret);
+	if (ret < 0)
+		goto error;
 
 	if (root->commit_root == root->node)
 		goto commit_tree;
@@ -182,21 +184,24 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans,
 	root->root_item.level = btrfs_header_level(root->node);
 	ret = btrfs_update_root(trans, root->fs_info->tree_root,
 				&root->root_key, &root->root_item);
-	BUG_ON(ret);
+	if (ret < 0)
+		goto error;
 
 commit_tree:
 	ret = commit_tree_roots(trans, fs_info);
-	BUG_ON(ret);
+	if (ret < 0)
+		goto error;
 	/*
 	 * Ensure that all committed roots are properly accounted in the
 	 * extent tree
 	 */
 	ret = btrfs_run_delayed_refs(trans, -1);
-	BUG_ON(ret);
+	if (ret < 0)
+		goto error;
 	btrfs_write_dirty_block_groups(trans);
 	__commit_transaction(trans, root);
 	if (ret < 0)
-		goto out;
+		goto error;
 	ret = write_ctree_super(trans);
 	btrfs_finish_extent_commit(trans);
 	kfree(trans);
@@ -204,7 +209,10 @@ commit_tree:
 	root->commit_root = NULL;
 	fs_info->running_transaction = NULL;
 	fs_info->last_trans_committed = transid;
-out:
+	return ret;
+error:
+	btrfs_destroy_delayed_refs(trans);
+	free(trans);
 	return ret;
 }
 
-- 
2.21.0


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/3] btrfs-progs: Remove the dead branch in btrfs_run_delayed_refs()
  2019-04-16  7:15 ` [PATCH 1/3] btrfs-progs: Remove the dead branch in btrfs_run_delayed_refs() Qu Wenruo
@ 2019-04-16  7:36   ` Nikolay Borisov
  0 siblings, 0 replies; 8+ messages in thread
From: Nikolay Borisov @ 2019-04-16  7:36 UTC (permalink / raw)
  To: Qu Wenruo, linux-btrfs



On 16.04.19 г. 10:15 ч., Qu Wenruo wrote:
> cleanup_ref_head() will only return 0 or 1, no way to return minutes
> value.
> 
> So remove the dead branch.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>

This is a left-over from the kernel copy, due to the kernel version able
to return < 0 from, say, run_and_cleanup_extent_op.

Reviewed-by: Nikolay Borisov <nborisov@suse.com>
> ---
>  extent-tree.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/extent-tree.c b/extent-tree.c
> index 8c9cdeff3b02..b7ece0ccc237 100644
> --- a/extent-tree.c
> +++ b/extent-tree.c
> @@ -4235,8 +4235,6 @@ int btrfs_run_delayed_refs(struct btrfs_trans_handle *trans, unsigned long nr)
>  				/* We dropped our lock, we need to loop. */
>  				ret = 0;
>  				continue;
> -			} else if (ret) {
> -				return ret;
>  			}
>  			locked_ref = NULL;
>  			continue;
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/3] btrfs-progs: Refactor btrfs_finish_extent_commit()
  2019-04-16  7:15 ` [PATCH 2/3] btrfs-progs: Refactor btrfs_finish_extent_commit() Qu Wenruo
@ 2019-04-16  7:39   ` Nikolay Borisov
  0 siblings, 0 replies; 8+ messages in thread
From: Nikolay Borisov @ 2019-04-16  7:39 UTC (permalink / raw)
  To: Qu Wenruo, linux-btrfs



On 16.04.19 г. 10:15 ч., Qu Wenruo wrote:
> This patch will refactor btrfs_finish_extent_commit() by:
> - Make it return void
>   There is no failure pattern for btrfs_finish_extent_commit(), thus it
>   always return 0. And the caller doesn't care about the return value.
>   So no need to return int.
> 
> - Remove @root and @unpin parameters
>   @root is only used to extract fs_info, which can be extracted from
>   transaction handler already.
>   @unpin is always fs_info->pinned_extents.
>   All these parameters can be extracted from @trans, no need to pass
>   them.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>

This also makes the function's signature identical to its kernel
counterpart.


Reviewed-by: Nikolay Borisov <nborisov@suse.com>

> ---
>  ctree.h       |  4 +---
>  extent-tree.c | 14 ++++++--------
>  transaction.c |  3 +--
>  3 files changed, 8 insertions(+), 13 deletions(-)
> 
> diff --git a/ctree.h b/ctree.h
> index 3357290d7045..76f52b1c9b08 100644
> --- a/ctree.h
> +++ b/ctree.h
> @@ -2516,9 +2516,7 @@ int btrfs_free_extent(struct btrfs_trans_handle *trans,
>  		      struct btrfs_root *root,
>  		      u64 bytenr, u64 num_bytes, u64 parent,
>  		      u64 root_objectid, u64 owner, u64 offset);
> -int btrfs_finish_extent_commit(struct btrfs_trans_handle *trans,
> -			       struct btrfs_root *root,
> -			       struct extent_io_tree *unpin);
> +void btrfs_finish_extent_commit(struct btrfs_trans_handle *trans);
>  int btrfs_inc_extent_ref(struct btrfs_trans_handle *trans,
>  				struct btrfs_root *root,
>  				u64 bytenr, u64 num_bytes, u64 parent,
> diff --git a/extent-tree.c b/extent-tree.c
> index b7ece0ccc237..1140ebfc61ff 100644
> --- a/extent-tree.c
> +++ b/extent-tree.c
> @@ -1977,27 +1977,25 @@ next:
>  	return 0;
>  }
>  
> -int btrfs_finish_extent_commit(struct btrfs_trans_handle *trans,
> -			       struct btrfs_root *root,
> -			       struct extent_io_tree *unpin)
> +void btrfs_finish_extent_commit(struct btrfs_trans_handle *trans)
>  {
>  	u64 start;
>  	u64 end;
>  	int ret;
> -	struct extent_io_tree *free_space_cache;
> -	free_space_cache = &root->fs_info->free_space_cache;
> +	struct btrfs_fs_info *fs_info = trans->fs_info;
> +	struct extent_io_tree *free_space_cache = &fs_info->free_space_cache;
> +	struct extent_io_tree *pinned_extents = &fs_info->pinned_extents;
>  
>  	while(1) {
> -		ret = find_first_extent_bit(unpin, 0, &start, &end,
> +		ret = find_first_extent_bit(pinned_extents, 0, &start, &end,
>  					    EXTENT_DIRTY);
>  		if (ret)
>  			break;
>  		update_pinned_extents(trans->fs_info, start, end + 1 - start,
>  				      0);
> -		clear_extent_dirty(unpin, start, end);
> +		clear_extent_dirty(pinned_extents, start, end);
>  		set_extent_dirty(free_space_cache, start, end);
>  	}
> -	return 0;
>  }
>  
>  static int pin_down_bytes(struct btrfs_trans_handle *trans, u64 bytenr,
> diff --git a/transaction.c b/transaction.c
> index 3a63988b0969..8d15eb11c7b5 100644
> --- a/transaction.c
> +++ b/transaction.c
> @@ -198,8 +198,7 @@ commit_tree:
>  	if (ret < 0)
>  		goto out;
>  	ret = write_ctree_super(trans);
> -	btrfs_finish_extent_commit(trans, fs_info->extent_root,
> -			           &fs_info->pinned_extents);
> +	btrfs_finish_extent_commit(trans);
>  	kfree(trans);
>  	free_extent_buffer(root->commit_root);
>  	root->commit_root = NULL;
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 3/3] btrfs-progs: Handle error properly in btrfs_commit_transaction()
  2019-04-16  7:15 ` [PATCH 3/3] btrfs-progs: Handle error properly in btrfs_commit_transaction() Qu Wenruo
@ 2019-04-16  7:48   ` Nikolay Borisov
  0 siblings, 0 replies; 8+ messages in thread
From: Nikolay Borisov @ 2019-04-16  7:48 UTC (permalink / raw)
  To: Qu Wenruo, linux-btrfs



On 16.04.19 г. 10:15 ч., Qu Wenruo wrote:
> [BUG]
> When running fuzz-tests/003 and fuzz-tests/009, btrfs-progs will crash
> due to BUG_ON().
> 
> [CAUSE]
> We abused BUG_ON() in btrfs_commit_transaction(), which is one of the
> most error prone function for fuzzed images.
> 
> Currently to cleanup the aborted transaction, we only need to clean up
> the only per-transaction data: delayed refs.
> 
> This patch will introduce a new function, btrfs_destroy_delayed_refs()
> to cleanup delayed refs when we failed to commit transaction.
> 
> With that function, we will gently destroy per-trans delayed ref, and
> remove the BUG_ON()s in btrfs_commit_transaction().
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>


Reviewed-by: Nikolay Borisov <nborisov@suse.com>

Though see below for a suggestion of further cleanup

> ---
>  delayed-ref.c | 24 ++++++++++++++++++++++++
>  delayed-ref.h |  5 +++++
>  extent-tree.c |  6 +++---
>  transaction.c | 20 ++++++++++++++------
>  4 files changed, 46 insertions(+), 9 deletions(-)
> 
> diff --git a/delayed-ref.c b/delayed-ref.c
> index 9974dbbd4c6b..69f8be34fe18 100644
> --- a/delayed-ref.c
> +++ b/delayed-ref.c
> @@ -605,3 +605,27 @@ free_ref:
>  
>  	return -ENOMEM;
>  }
> +
> +void btrfs_destroy_delayed_refs(struct btrfs_trans_handle *trans)
> +{
> +	struct btrfs_fs_info *fs_info = trans->fs_info;
> +	struct rb_node *node;
> +	struct btrfs_delayed_ref_root *delayed_refs;
> +
> +	delayed_refs = &trans->delayed_refs;
> +	if (RB_EMPTY_ROOT(&delayed_refs->href_root))
> +		return;
> +	while ((node = rb_first(&delayed_refs->href_root)) != NULL) {
> +		struct btrfs_delayed_ref_head *head;
> +		struct btrfs_delayed_ref_node *ref;
> +		struct rb_node *n;
> +
> +		head = rb_entry(node, struct btrfs_delayed_ref_head, href_node);
> +		while ((n = rb_first(&head->ref_tree)) != NULL) {
> +			ref = rb_entry(n, struct btrfs_delayed_ref_node,
> +				       ref_node);
> +			drop_delayed_ref(trans, delayed_refs, head, ref);
> +		}
> +		ASSERT(cleanup_ref_head(trans, fs_info, head) == 0);
> +	}
> +}
> diff --git a/delayed-ref.h b/delayed-ref.h
> index 30a68b2a278c..4ff3eaa929ba 100644
> --- a/delayed-ref.h
> +++ b/delayed-ref.h
> @@ -205,4 +205,9 @@ btrfs_delayed_node_to_tree_ref(struct btrfs_delayed_ref_node *node)
>  {
>  	return container_of(node, struct btrfs_delayed_tree_ref, node);
>  }
> +
> +int cleanup_ref_head(struct btrfs_trans_handle *trans,
> +		     struct btrfs_fs_info *fs_info,
> +		     struct btrfs_delayed_ref_head *head);
> +void btrfs_destroy_delayed_refs(struct btrfs_trans_handle *trans);
>  #endif
> diff --git a/extent-tree.c b/extent-tree.c
> index 1140ebfc61ff..e62ee8c2ba13 100644
> --- a/extent-tree.c
> +++ b/extent-tree.c
> @@ -4085,9 +4085,9 @@ static void unselect_delayed_ref_head(struct btrfs_delayed_ref_root *delayed_ref
>  	delayed_refs->num_heads_ready++;
>  }
>  
> -static int cleanup_ref_head(struct btrfs_trans_handle *trans,
> -			    struct btrfs_fs_info *fs_info,
> -			    struct btrfs_delayed_ref_head *head)
> +int cleanup_ref_head(struct btrfs_trans_handle *trans,
> +		     struct btrfs_fs_info *fs_info,
> +		     struct btrfs_delayed_ref_head *head)

You can create a followup patch that removes the fs_info argument from
cleanup_ref_head to make it identical to the kernel version.

>  {
>  	struct btrfs_delayed_ref_root *delayed_refs;
>  
> diff --git a/transaction.c b/transaction.c
> index 8d15eb11c7b5..138e10f0d6cc 100644
> --- a/transaction.c
> +++ b/transaction.c
> @@ -17,6 +17,7 @@
>  #include "kerncompat.h"
>  #include "disk-io.h"
>  #include "transaction.h"
> +#include "delayed-ref.h"
>  
>  #include "messages.h"
>  
> @@ -165,7 +166,8 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans,
>  	 * consistent
>  	 */
>  	ret = btrfs_run_delayed_refs(trans, -1);
> -	BUG_ON(ret);
> +	if (ret < 0)
> +		goto error;
>  
>  	if (root->commit_root == root->node)
>  		goto commit_tree;
> @@ -182,21 +184,24 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans,
>  	root->root_item.level = btrfs_header_level(root->node);
>  	ret = btrfs_update_root(trans, root->fs_info->tree_root,
>  				&root->root_key, &root->root_item);
> -	BUG_ON(ret);
> +	if (ret < 0)
> +		goto error;
>  
>  commit_tree:
>  	ret = commit_tree_roots(trans, fs_info);
> -	BUG_ON(ret);
> +	if (ret < 0)
> +		goto error;
>  	/*
>  	 * Ensure that all committed roots are properly accounted in the
>  	 * extent tree
>  	 */
>  	ret = btrfs_run_delayed_refs(trans, -1);
> -	BUG_ON(ret);
> +	if (ret < 0)
> +		goto error;
>  	btrfs_write_dirty_block_groups(trans);
>  	__commit_transaction(trans, root);
>  	if (ret < 0)
> -		goto out;
> +		goto error;
>  	ret = write_ctree_super(trans);
>  	btrfs_finish_extent_commit(trans);
>  	kfree(trans);
> @@ -204,7 +209,10 @@ commit_tree:
>  	root->commit_root = NULL;
>  	fs_info->running_transaction = NULL;
>  	fs_info->last_trans_committed = transid;
> -out:
> +	return ret;
> +error:
> +	btrfs_destroy_delayed_refs(trans);
> +	free(trans);
>  	return ret;
>  }
>  
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 0/3] btrfs-progs: Handle error properly in btrfs_commit_transaction()
  2019-04-16  7:15 [PATCH 0/3] btrfs-progs: Handle error properly in btrfs_commit_transaction() Qu Wenruo
                   ` (2 preceding siblings ...)
  2019-04-16  7:15 ` [PATCH 3/3] btrfs-progs: Handle error properly in btrfs_commit_transaction() Qu Wenruo
@ 2019-05-13 13:55 ` David Sterba
  3 siblings, 0 replies; 8+ messages in thread
From: David Sterba @ 2019-05-13 13:55 UTC (permalink / raw)
  To: Qu Wenruo; +Cc: linux-btrfs

On Tue, Apr 16, 2019 at 03:15:23PM +0800, Qu Wenruo wrote:
> This patchset can be fetched from github:
> https://github.com/adam900710/btrfs-progs/tree/commit_failure_cleanup
> 
> This patchset will address the BUG_ON() triggered in fuzz-tests/003 and
> fuzz-tests/009.
> 
> For proper error handling in btrfs_commit_transaction(), we need a way
> to clean up per-transaction data properly.
> 
> Currently we only have delayed refs which are per-transaction, so
> introduce a new function, btrfs_destroy_delayed_refs() to cleanup
> delayed refs.
> 
> Now btrfs_commit_transaction() can error out gracefully with proper
> cleanup.
> 
> Patch 1 and 2 are just some minor cleanups found when crafting the 3rd
> patch.
> 
> Qu Wenruo (3):
>   btrfs-progs: Remove the dead branch in btrfs_run_delayed_refs()
>   btrfs-progs: Refactor btrfs_finish_extent_commit()
>   btrfs-progs: Handle error properly in btrfs_commit_transaction()

Added to devel, thanks.

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2019-05-13 13:54 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-16  7:15 [PATCH 0/3] btrfs-progs: Handle error properly in btrfs_commit_transaction() Qu Wenruo
2019-04-16  7:15 ` [PATCH 1/3] btrfs-progs: Remove the dead branch in btrfs_run_delayed_refs() Qu Wenruo
2019-04-16  7:36   ` Nikolay Borisov
2019-04-16  7:15 ` [PATCH 2/3] btrfs-progs: Refactor btrfs_finish_extent_commit() Qu Wenruo
2019-04-16  7:39   ` Nikolay Borisov
2019-04-16  7:15 ` [PATCH 3/3] btrfs-progs: Handle error properly in btrfs_commit_transaction() Qu Wenruo
2019-04-16  7:48   ` Nikolay Borisov
2019-05-13 13:55 ` [PATCH 0/3] " David Sterba

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.