| Message ID | 20200804073236.6677-1-nborisov@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: Rework error detection in init_tree_roots | expand |
On 04/08/2020 09:32, Nikolay Borisov wrote: > @@ -2645,17 +2645,16 @@ static int __cold init_tree_roots(struct btrfs_fs_info *fs_info) > 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)) { > + if (IS_ERR(tree_root->node)) { > handle_error = true; > + ret = PTR_ERR(tree_root->node); > + tree_root->node = NULL; > + btrfs_warn(fs_info, "failed to read tree root"); > + continue; [...] > btrfs_warn(fs_info, "failed to read tree root"); > continue; > } Now we're duplicating the warning message. I think it's better to have two distinct messages so we can differentiate which of the two failure cases happened. The 2nd one could be something like "tree root eb not uptodate". Otherwise looks good, Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
On 4.08.20 г. 15:58 ч., Johannes Thumshirn wrote: > On 04/08/2020 09:32, Nikolay Borisov wrote: >> @@ -2645,17 +2645,16 @@ static int __cold init_tree_roots(struct btrfs_fs_info *fs_info) >> 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)) { >> + if (IS_ERR(tree_root->node)) { >> handle_error = true; >> + ret = PTR_ERR(tree_root->node); >> + tree_root->node = NULL; >> + btrfs_warn(fs_info, "failed to read tree root"); >> + continue; > > [...] > >> btrfs_warn(fs_info, "failed to read tree root"); >> continue; >> } > > Now we're duplicating the warning message. I think it's better to have two > distinct messages so we can differentiate which of the two failure cases happened. > > The 2nd one could be something like "tree root eb not uptodate". Sure, I'm happy too replace it with whatever is more informative. Will take another look at the code and see what I can derive. > > Otherwise looks good, > Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> >
On Tue, Aug 04, 2020 at 06:02:58PM +0300, Nikolay Borisov wrote: > > > On 4.08.20 г. 15:58 ч., Johannes Thumshirn wrote: > > On 04/08/2020 09:32, Nikolay Borisov wrote: > >> @@ -2645,17 +2645,16 @@ static int __cold init_tree_roots(struct btrfs_fs_info *fs_info) > >> 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)) { > >> + if (IS_ERR(tree_root->node)) { > >> handle_error = true; > >> + ret = PTR_ERR(tree_root->node); > >> + tree_root->node = NULL; > >> + btrfs_warn(fs_info, "failed to read tree root"); > >> + continue; > > > > [...] > > > >> btrfs_warn(fs_info, "failed to read tree root"); > >> continue; > >> } > > > > Now we're duplicating the warning message. I think it's better to have two > > distinct messages so we can differentiate which of the two failure cases happened. > > > > The 2nd one could be something like "tree root eb not uptodate". > > Sure, I'm happy too replace it with whatever is more informative. Will > take another look at the code and see what I can derive. The errors are different, IS_ERR is because the block was not read at all for some reason, extent_buffer_uptodate is EIO in all other places that do this kind of check. Here it's EUCLEAN and it's been like that since the beginning but I think it should be EIO.
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c index 5fc5f62228f1..ecb8ca53244f 100644 --- a/fs/btrfs/disk-io.c +++ b/fs/btrfs/disk-io.c @@ -2645,17 +2645,16 @@ static int __cold init_tree_roots(struct btrfs_fs_info *fs_info) 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)) { + if (IS_ERR(tree_root->node)) { handle_error = true; + ret = PTR_ERR(tree_root->node); + tree_root->node = NULL; + btrfs_warn(fs_info, "failed to read tree root"); + continue; - if (IS_ERR(tree_root->node)) { - ret = PTR_ERR(tree_root->node); - tree_root->node = NULL; - } else if (!extent_buffer_uptodate(tree_root->node)) { - ret = -EUCLEAN; - } - + } else if (!extent_buffer_uptodate(tree_root->node)) { + handle_error = true; + ret = -EUCLEAN; btrfs_warn(fs_info, "failed to read tree root"); continue; }
To avoid duplicating 3 lines of code the error detection logic in init_tree_roots is somewhat quirky. It first checks for the presence of any error condition, then checks for the specific condition to perform any specific actions. That's spurious because directly checking for each respective error condition and doing the necessary steps is more obvious. Additionally it results in smaller code and the code reads more linearly: add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-95 (-95) Function old new delta open_ctree 17243 17148 -95 Total: Before=113104, After=113009, chg -0.08% Signed-off-by: Nikolay Borisov <nborisov@suse.com> --- fs/btrfs/disk-io.c | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-)