| Message ID | 20191010150647.20940-2-nborisov@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Cleanups in mount path | expand |
On 10/10/2019 17:06, Nikolay Borisov wrote: > -recovery_tree_root: > - if (!btrfs_test_opt(fs_info, USEBACKUPROOT)) > - goto fail_tree_roots; What happened to this test in your refactoring?
On 11.10.19 г. 10:50 ч., Johannes Thumshirn wrote: > On 10/10/2019 17:06, Nikolay Borisov wrote: >> -recovery_tree_root: >> - if (!btrfs_test_opt(fs_info, USEBACKUPROOT)) >> - goto fail_tree_roots; > > What happened to this test in your refactoring? It went to the top: + bool should_retry = btrfs_test_opt(fs_info, USEBACKUPROOT); > >
On 11/10/2019 10:21, Nikolay Borisov wrote: > It went to the top: > + bool should_retry = btrfs_test_opt(fs_info, USEBACKUPROOT); > Args, /me is blind Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
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@suse.com> > --- > 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); > >
On 11.10.19 г. 12:31 ч., Qu Wenruo wrote: > > > 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@suse.com> >> --- >> 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. Patch 3 actually makes next_root_backup always return EINVAL. > */ > 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 */ > } I agree that the interface of next_root_backup could be cleaned further in the spirit you suggested but I don't think the net result will be any cleaner. We will still have a loop and a goto inside the loop which I don't think is any cleaner. I'm not a big fan of the if (handle_error) either but I think it's way more explicit. > > To me, that would look more sane other than strange error handling > creeping around. Let's see what other people think about the two approaches before deciding which way to go . > > Thanks, > Qu <snip>
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) { + 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);
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. Signed-off-by: Nikolay Borisov <nborisov@suse.com> --- fs/btrfs/disk-io.c | 136 ++++++++++++++++++++++++++------------------- 1 file changed, 80 insertions(+), 56 deletions(-)