Re: [PATCH v3 1/5] btrfs: change handle_fs_error in recover_log_trees to aborts
From: Anand Jain <hidden>
Date: 2021-09-28 05:46:27
On 28/09/2021 02:05, Josef Bacik wrote:
During inspection of the return path for replay I noticed that we don't actually abort the transaction if we get a failure during replay. This isn't a problem necessarily, as we properly return the error and will fail to mount. However we still leave this dangling transaction that could conceivably be committed without thinking there was an error. We were using btrfs_handle_fs_error() here, but that pre-dates the transaction abort code. Simply replace the btrfs_handle_fs_error() calls with transaction aborts, so we still know where exactly things went wrong, and add a few in some other un-handled error cases. Reviewed-by: Johannes Thumshirn <redacted> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
After this patch, failure from walk_log_tree() still won't call btrfs_abort_transaction(), is it intentional? Maybe it is time to enhance btrfs_abort_transaction() with an argument to pass those kernel-error-messages in the 3rd argument of btrfs_handle_fs_error() in btrfs_recover_log_trees(). After this patch, it will continue to print function line numbers, but those kernel-error messages are more informative if printed. Thanks, Anand
quoted hunk ↗ jump to hunk
--- fs/btrfs/tree-log.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c index 30590ddd69ac..e0c2d4c7f939 100644 --- a/fs/btrfs/tree-log.c +++ b/fs/btrfs/tree-log.c@@ -6527,8 +6527,7 @@ int btrfs_recover_log_trees(struct btrfs_root *log_root_tree) ret = btrfs_search_slot(NULL, log_root_tree, &key, path, 0, 0); if (ret < 0) { - btrfs_handle_fs_error(fs_info, ret, - "Couldn't find tree log root."); + btrfs_abort_transaction(trans, ret); goto error; } if (ret > 0) {@@ -6545,8 +6544,7 @@ int btrfs_recover_log_trees(struct btrfs_root *log_root_tree) log = btrfs_read_tree_root(log_root_tree, &found_key); if (IS_ERR(log)) { ret = PTR_ERR(log); - btrfs_handle_fs_error(fs_info, ret, - "Couldn't read tree log root."); + btrfs_abort_transaction(trans, ret); goto error; }@@ -6574,8 +6572,7 @@ int btrfs_recover_log_trees(struct btrfs_root *log_root_tree) if (!ret) goto next; - btrfs_handle_fs_error(fs_info, ret, - "Couldn't read target root for tree log recovery."); + btrfs_abort_transaction(trans, ret); goto error; }@@ -6583,14 +6580,15 @@ int btrfs_recover_log_trees(struct btrfs_root *log_root_tree) ret = btrfs_record_root_in_trans(trans, wc.replay_dest); if (ret) /* The loop needs to continue due to the root refs */ - btrfs_handle_fs_error(fs_info, ret, - "failed to record the log root in transaction"); + btrfs_abort_transaction(trans, ret); else ret = walk_log_tree(trans, log, &wc); if (!ret && wc.stage == LOG_WALK_REPLAY_ALL) { ret = fixup_inode_link_counts(trans, wc.replay_dest, path); + if (ret) + btrfs_abort_transaction(trans, ret); } if (!ret && wc.stage == LOG_WALK_REPLAY_ALL) {@@ -6607,6 +6605,8 @@ int btrfs_recover_log_trees(struct btrfs_root *log_root_tree) * could only happen during mount. */ ret = btrfs_init_root_free_objectid(root); + if (ret) + btrfs_abort_transaction(trans, ret); } wc.replay_dest->log_root = NULL;