* [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.