On 2019/10/10 下午11:06, Nikolay Borisov wrote:
> The code responsible for reading and initilizing tree roots is
> scattered in open_ctree among 2 labels, emulating a loop. This is rather
> confusing to reason about. Instead, factor the code in a new function,
> init_tree_roots which implements the same logical flow.
The refactor itself is great, but maybe we can make it better?
Extra comment inlined below.
>
> Signed-off-by: Nikolay Borisov <nborisov@xxxxxxxx>
> ---
> fs/btrfs/disk-io.c | 136 ++++++++++++++++++++++++++-------------------
> 1 file changed, 80 insertions(+), 56 deletions(-)
>
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index 2c3fa89702e7..b850988023aa 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -2553,6 +2553,82 @@ static int btrfs_validate_write_super(struct btrfs_fs_info *fs_info,
> return ret;
> }
>
> +int __cold init_tree_roots(struct btrfs_fs_info *fs_info)
> +{
> +
> + bool should_retry = btrfs_test_opt(fs_info, USEBACKUPROOT);
> + struct btrfs_super_block *sb = fs_info->super_copy;
> + struct btrfs_root *tree_root = fs_info->tree_root;
> + u64 generation;
> + int level;
> + bool handle_error = false;
> + int num_backups_tried = 0;
> + int backup_index = 0;
> + int ret = 0;
> +
> + while (true) {
> + if (handle_error) {
This two part doesn't look good enough to me.
Can we do something like this?
/*
* change next_root_backup() to:
* - Don't modify backup_index parameter
* - Accept @index as 0, 1, 2, 3, 4.
* 0 is regular sb tree roots, 1~4 is the backup roots, 1 is the best
candiate
* while 4 is the oldest candidate
* - Check if we should try next backup (usebackuproot option)
* - Return proper error value other than -1.
*/
for (backup_index = 0; next_root_backup(fs_info, backup_index) == 0;
backup_index++) {
/*
* do the heavy lifting tree reads here
*/
if (some_thing_went_wrong)
goto next;
/* Everything done correctly */
break;
next:
/* Do the cleanup */
}
To me, that would look more sane other than strange error handling
creeping around.
Thanks,
Qu
> + if (!IS_ERR(tree_root->node))
> + free_extent_buffer(tree_root->node);
> + tree_root->node = NULL;
> +
> + if (!should_retry) {
> + break;
> + }
> + free_root_pointers(fs_info, 0);
> +
> + /* don't use the log in recovery mode, it won't be valid */
> + btrfs_set_super_log_root(sb, 0);
> +
> + /* we can't trust the free space cache either */
> + btrfs_set_opt(fs_info->mount_opt, CLEAR_CACHE);
> +
> + ret = next_root_backup(fs_info, sb, &num_backups_tried,
> + &backup_index);
> + if (ret == -1)
> + return -ESPIPE;
> + }
> + generation = btrfs_super_generation(sb);
> + level = btrfs_super_root_level(sb);
> + tree_root->node = read_tree_block(fs_info, btrfs_super_root(sb),
> + generation, level, NULL);
> + if (IS_ERR(tree_root->node) ||
> + !extent_buffer_uptodate(tree_root->node)) {
> + handle_error = true;
> + btrfs_warn(fs_info, "failed to read tree root");
> + continue;
> + }
> +
> + btrfs_set_root_node(&tree_root->root_item, tree_root->node);
> + tree_root->commit_root = btrfs_root_node(tree_root);
> + btrfs_set_root_refs(&tree_root->root_item, 1);
> +
> + mutex_lock(&tree_root->objectid_mutex);
> + ret = btrfs_find_highest_objectid(tree_root,
> + &tree_root->highest_objectid);
> + if (ret) {
> + mutex_unlock(&tree_root->objectid_mutex);
> + handle_error = true;
> + continue;
> + }
> +
> + ASSERT(tree_root->highest_objectid <= BTRFS_LAST_FREE_OBJECTID);
> + mutex_unlock(&tree_root->objectid_mutex);
> +
> + ret = btrfs_read_roots(fs_info);
> + if (ret) {
> + handle_error = true;
> + continue;
> + }
> +
> + fs_info->generation = generation;
> + fs_info->last_trans_committed = generation;
> + break;
> + }
> +
> + return ret;
> +}
> +
> int __cold open_ctree(struct super_block *sb,
> struct btrfs_fs_devices *fs_devices,
> char *options)
> @@ -2571,8 +2647,6 @@ int __cold open_ctree(struct super_block *sb,
> struct btrfs_root *chunk_root;
> int ret;
> int err = -EINVAL;
> - int num_backups_tried = 0;
> - int backup_index = 0;
> int clear_free_space_tree = 0;
> int level;
>
> @@ -2995,45 +3069,13 @@ int __cold open_ctree(struct super_block *sb,
> goto fail_tree_roots;
> }
>
> -retry_root_backup:
> - generation = btrfs_super_generation(disk_super);
> - level = btrfs_super_root_level(disk_super);
> -
> - tree_root->node = read_tree_block(fs_info,
> - btrfs_super_root(disk_super),
> - generation, level, NULL);
> - if (IS_ERR(tree_root->node) ||
> - !extent_buffer_uptodate(tree_root->node)) {
> - btrfs_warn(fs_info, "failed to read tree root");
> - if (!IS_ERR(tree_root->node))
> - free_extent_buffer(tree_root->node);
> - tree_root->node = NULL;
> - goto recovery_tree_root;
> - }
> -
> - btrfs_set_root_node(&tree_root->root_item, tree_root->node);
> - tree_root->commit_root = btrfs_root_node(tree_root);
> - btrfs_set_root_refs(&tree_root->root_item, 1);
> -
> - mutex_lock(&tree_root->objectid_mutex);
> - ret = btrfs_find_highest_objectid(tree_root,
> - &tree_root->highest_objectid);
> + ret = init_tree_roots(fs_info);
> if (ret) {
> - mutex_unlock(&tree_root->objectid_mutex);
> - goto recovery_tree_root;
> + if (ret == -ESPIPE)
> + goto fail_block_groups;
> + goto fail_tree_roots;
> }
>
> - ASSERT(tree_root->highest_objectid <= BTRFS_LAST_FREE_OBJECTID);
> -
> - mutex_unlock(&tree_root->objectid_mutex);
> -
> - ret = btrfs_read_roots(fs_info);
> - if (ret)
> - goto recovery_tree_root;
> -
> - fs_info->generation = generation;
> - fs_info->last_trans_committed = generation;
> -
> ret = btrfs_verify_dev_extents(fs_info);
> if (ret) {
> btrfs_err(fs_info,
> @@ -3327,24 +3369,6 @@ int __cold open_ctree(struct super_block *sb,
> btrfs_free_stripe_hash_table(fs_info);
> btrfs_close_devices(fs_info->fs_devices);
> return err;
> -
> -recovery_tree_root:
> - if (!btrfs_test_opt(fs_info, USEBACKUPROOT))
> - goto fail_tree_roots;
> -
> - free_root_pointers(fs_info, 0);
> -
> - /* don't use the log in recovery mode, it won't be valid */
> - btrfs_set_super_log_root(disk_super, 0);
> -
> - /* we can't trust the free space cache either */
> - btrfs_set_opt(fs_info->mount_opt, CLEAR_CACHE);
> -
> - ret = next_root_backup(fs_info, fs_info->super_copy,
> - &num_backups_tried, &backup_index);
> - if (ret == -1)
> - goto fail_block_groups;
> - goto retry_root_backup;
> }
> ALLOW_ERROR_INJECTION(open_ctree, ERRNO);
>
>
