* [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
* [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
* [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
* 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
* 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
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).