* [PATCH 0/3] Cleanups in mount path @ 2019-10-10 15:06 Nikolay Borisov 2019-10-10 15:06 ` [PATCH 1/3] btrfs: Factor out tree roots initialization during mount Nikolay Borisov ` (2 more replies) 0 siblings, 3 replies; 10+ messages in thread From: Nikolay Borisov @ 2019-10-10 15:06 UTC (permalink / raw) To: linux-btrfs; +Cc: Nikolay Borisov Following the misunderstanding around the 2nd argument of free_root_pointers it became apparent that the relevant code in open_ctre is not entirely clear. That's mainly due it being split among 2 labels, emulating a loop. This series cleans that up by factoring it out in a discrete function, init_root_trees in patch 1 and then subsequent patches implement minor cleanups that became apparent while working with the code. This has survived full xfstest run. Nikolay Borisov (3): btrfs: Factor out tree roots initialization during mount btrfs: Don't use objectid_mutex during mount btrfs: Jump to correct label on init_root_trees failure fs/btrfs/disk-io.c | 138 +++++++++++++++++++++++++-------------------- 1 file changed, 78 insertions(+), 60 deletions(-) -- 2.17.1 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/3] btrfs: Factor out tree roots initialization during mount 2019-10-10 15:06 [PATCH 0/3] Cleanups in mount path Nikolay Borisov @ 2019-10-10 15:06 ` Nikolay Borisov 2019-10-11 7:50 ` Johannes Thumshirn 2019-10-11 9:31 ` Qu Wenruo 2019-10-10 15:06 ` [PATCH 2/3] btrfs: Don't use objectid_mutex " Nikolay Borisov 2019-10-10 15:06 ` [PATCH 3/3] btrfs: Jump to correct label on init_root_trees failure Nikolay Borisov 2 siblings, 2 replies; 10+ messages in thread From: Nikolay Borisov @ 2019-10-10 15:06 UTC (permalink / raw) To: linux-btrfs; +Cc: Nikolay Borisov 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(-) 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); -- 2.17.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 1/3] btrfs: Factor out tree roots initialization during mount 2019-10-10 15:06 ` [PATCH 1/3] btrfs: Factor out tree roots initialization during mount Nikolay Borisov @ 2019-10-11 7:50 ` Johannes Thumshirn 2019-10-11 8:21 ` Nikolay Borisov 2019-10-11 9:31 ` Qu Wenruo 1 sibling, 1 reply; 10+ messages in thread From: Johannes Thumshirn @ 2019-10-11 7:50 UTC (permalink / raw) To: Nikolay Borisov, linux-btrfs 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? -- Johannes Thumshirn SUSE Labs Filesystems jthumshirn@suse.de +49 911 74053 689 SUSE Software Solutions Germany GmbH Maxfeldstr. 5 90409 Nürnberg Germany (HRB 247165, AG München) Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/3] btrfs: Factor out tree roots initialization during mount 2019-10-11 7:50 ` Johannes Thumshirn @ 2019-10-11 8:21 ` Nikolay Borisov 2019-10-11 8:23 ` Johannes Thumshirn 0 siblings, 1 reply; 10+ messages in thread From: Nikolay Borisov @ 2019-10-11 8:21 UTC (permalink / raw) To: Johannes Thumshirn, linux-btrfs 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); > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/3] btrfs: Factor out tree roots initialization during mount 2019-10-11 8:21 ` Nikolay Borisov @ 2019-10-11 8:23 ` Johannes Thumshirn 0 siblings, 0 replies; 10+ messages in thread From: Johannes Thumshirn @ 2019-10-11 8:23 UTC (permalink / raw) To: Nikolay Borisov, linux-btrfs 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> -- Johannes Thumshirn SUSE Labs Filesystems jthumshirn@suse.de +49 911 74053 689 SUSE Software Solutions Germany GmbH Maxfeldstr. 5 90409 Nürnberg Germany (HRB 247165, AG München) Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/3] btrfs: Factor out tree roots initialization during mount 2019-10-10 15:06 ` [PATCH 1/3] btrfs: Factor out tree roots initialization during mount Nikolay Borisov 2019-10-11 7:50 ` Johannes Thumshirn @ 2019-10-11 9:31 ` Qu Wenruo 2019-10-11 10:12 ` Nikolay Borisov 1 sibling, 1 reply; 10+ messages in thread From: Qu Wenruo @ 2019-10-11 9:31 UTC (permalink / raw) To: Nikolay Borisov, linux-btrfs 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); > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/3] btrfs: Factor out tree roots initialization during mount 2019-10-11 9:31 ` Qu Wenruo @ 2019-10-11 10:12 ` Nikolay Borisov 0 siblings, 0 replies; 10+ messages in thread From: Nikolay Borisov @ 2019-10-11 10:12 UTC (permalink / raw) To: Qu Wenruo, linux-btrfs 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> ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 2/3] btrfs: Don't use objectid_mutex during mount 2019-10-10 15:06 [PATCH 0/3] Cleanups in mount path Nikolay Borisov 2019-10-10 15:06 ` [PATCH 1/3] btrfs: Factor out tree roots initialization during mount Nikolay Borisov @ 2019-10-10 15:06 ` Nikolay Borisov 2019-10-10 16:05 ` David Sterba 2019-10-10 15:06 ` [PATCH 3/3] btrfs: Jump to correct label on init_root_trees failure Nikolay Borisov 2 siblings, 1 reply; 10+ messages in thread From: Nikolay Borisov @ 2019-10-10 15:06 UTC (permalink / raw) To: linux-btrfs; +Cc: Nikolay Borisov Since the filesystem is not well formed and no trees are loaded it's pointless holding the objectid_mutex. Just remove its usage. Signed-off-by: Nikolay Borisov <nborisov@suse.com> --- fs/btrfs/disk-io.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c index b850988023aa..72580eb6b706 100644 --- a/fs/btrfs/disk-io.c +++ b/fs/btrfs/disk-io.c @@ -2603,17 +2603,14 @@ int __cold init_tree_roots(struct btrfs_fs_info *fs_info) 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) { -- 2.17.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 2/3] btrfs: Don't use objectid_mutex during mount 2019-10-10 15:06 ` [PATCH 2/3] btrfs: Don't use objectid_mutex " Nikolay Borisov @ 2019-10-10 16:05 ` David Sterba 0 siblings, 0 replies; 10+ messages in thread From: David Sterba @ 2019-10-10 16:05 UTC (permalink / raw) To: Nikolay Borisov; +Cc: linux-btrfs On Thu, Oct 10, 2019 at 06:06:46PM +0300, Nikolay Borisov wrote: > Since the filesystem is not well formed and no trees are loaded it's > pointless holding the objectid_mutex. Just remove its usage. Yes. There's a case when the lock should be kept even in that phase, so we could add a lockdep assertion, but I found btrfs_recover_log_trees with a comment why the lock is not needed. Given that btrfs_find_highest_objectid is used only in a hadnful places, I think we're fine with just the comments. > Signed-off-by: Nikolay Borisov <nborisov@suse.com> > --- > fs/btrfs/disk-io.c | 3 --- > 1 file changed, 3 deletions(-) > > diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c > index b850988023aa..72580eb6b706 100644 > --- a/fs/btrfs/disk-io.c > +++ b/fs/btrfs/disk-io.c > @@ -2603,17 +2603,14 @@ int __cold init_tree_roots(struct btrfs_fs_info *fs_info) > tree_root->commit_root = btrfs_root_node(tree_root); > btrfs_set_root_refs(&tree_root->root_item, 1); > > - mutex_lock(&tree_root->objectid_mutex); /* * We don't need to hold objectid_mutex because ... */ > ret = btrfs_find_highest_objectid(tree_root, > &tree_root->highest_objectid); > if (ret) { > - mutex_unlock(&tree_root->objectid_mutex); > handle_error = true; > continue; > } ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 3/3] btrfs: Jump to correct label on init_root_trees failure 2019-10-10 15:06 [PATCH 0/3] Cleanups in mount path Nikolay Borisov 2019-10-10 15:06 ` [PATCH 1/3] btrfs: Factor out tree roots initialization during mount Nikolay Borisov 2019-10-10 15:06 ` [PATCH 2/3] btrfs: Don't use objectid_mutex " Nikolay Borisov @ 2019-10-10 15:06 ` Nikolay Borisov 2 siblings, 0 replies; 10+ messages in thread From: Nikolay Borisov @ 2019-10-10 15:06 UTC (permalink / raw) To: linux-btrfs; +Cc: Nikolay Borisov During the refactoring and introduction of init_root_trees the code retained the special handling of 'next_root_backup' failure by returning -ESPIPE from init_root_trees and jumping to fail_block_groups. This is wrong because next_root_backup doesn't tinker with blockgroups at all and so any failure in it should be handled the same way as failures from its sole caller (init_root_trees) - jumping to fail_tree_roots. This was introduced in the original commit which implemented backup roots af31f5e5b84b ("Btrfs: add a log of past tree roots"). Fix it by always jumping to fail_tree_roots. While at it put some sanity into next_root_backup by returning -EINVAL. Signed-off-by: Nikolay Borisov <nborisov@suse.com> --- fs/btrfs/disk-io.c | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c index 72580eb6b706..1eed0de4020f 100644 --- a/fs/btrfs/disk-io.c +++ b/fs/btrfs/disk-io.c @@ -1913,7 +1913,7 @@ static void backup_super_roots(struct btrfs_fs_info *info) * the array, so you send it the number of backups you've already * tried and the last backup index you used. * - * this returns -1 when it has tried all the backups + * Returns EINVAL when it has tried all the backups */ static noinline int next_root_backup(struct btrfs_fs_info *info, struct btrfs_super_block *super, @@ -1927,13 +1927,13 @@ static noinline int next_root_backup(struct btrfs_fs_info *info, newest = find_newest_super_backup(info, gen); if (newest == -1) - return -1; + return -EINVAL; *backup_index = newest; *num_backups_tried = 1; } else if (*num_backups_tried == BTRFS_NUM_BACKUP_ROOTS) { /* we've tried all the backups, all done */ - return -1; + return -EINVAL; } else { /* jump to the next oldest backup */ newest = (*backup_index + BTRFS_NUM_BACKUP_ROOTS - 1) % @@ -2585,8 +2585,8 @@ int __cold init_tree_roots(struct btrfs_fs_info *fs_info) ret = next_root_backup(fs_info, sb, &num_backups_tried, &backup_index); - if (ret == -1) - return -ESPIPE; + if (ret) + return ret; } generation = btrfs_super_generation(sb); level = btrfs_super_root_level(sb); @@ -3067,11 +3067,8 @@ int __cold open_ctree(struct super_block *sb, } ret = init_tree_roots(fs_info); - if (ret) { - if (ret == -ESPIPE) - goto fail_block_groups; + if (ret) goto fail_tree_roots; - } ret = btrfs_verify_dev_extents(fs_info); if (ret) { -- 2.17.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
end of thread, other threads:[~2019-10-11 10:12 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-10-10 15:06 [PATCH 0/3] Cleanups in mount path Nikolay Borisov 2019-10-10 15:06 ` [PATCH 1/3] btrfs: Factor out tree roots initialization during mount Nikolay Borisov 2019-10-11 7:50 ` Johannes Thumshirn 2019-10-11 8:21 ` Nikolay Borisov 2019-10-11 8:23 ` Johannes Thumshirn 2019-10-11 9:31 ` Qu Wenruo 2019-10-11 10:12 ` Nikolay Borisov 2019-10-10 15:06 ` [PATCH 2/3] btrfs: Don't use objectid_mutex " Nikolay Borisov 2019-10-10 16:05 ` David Sterba 2019-10-10 15:06 ` [PATCH 3/3] btrfs: Jump to correct label on init_root_trees failure Nikolay Borisov
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).