* [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 an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.