btrfs: Rework error detection in init_tree_roots
diff mbox series

Message ID 20200804073236.6677-1-nborisov@suse.com
State New
Headers show
Series
  • btrfs: Rework error detection in init_tree_roots
Related show

Commit Message

Nikolay Borisov Aug. 4, 2020, 7:32 a.m. UTC
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(-)

Comments

Johannes Thumshirn Aug. 4, 2020, 12:58 p.m. UTC | #1
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>
Nikolay Borisov Aug. 4, 2020, 3:02 p.m. UTC | #2
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> 
>
David Sterba Aug. 10, 2020, 3:53 p.m. UTC | #3
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.

Patch
diff mbox series

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;
 		}