Re: [PATCH v3 1/5] btrfs: change handle_fs_error in recover_log_trees to aborts

[Anand Jain <anand.jain@oracle.com>]

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 <johannes.thumshirn@wdc.com>
> 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

> ---
>   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;
>