[PATCH] btrfs: Rework error detection in init_tree_roots

[PATCH] btrfs: Rework error detection in init_tree_roots

[Nikolay Borisov]

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(-)

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

Re: [PATCH] btrfs: Rework error detection in init_tree_roots

[Johannes Thumshirn]

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> 

Re: [PATCH] btrfs: Rework error detection in init_tree_roots

[Nikolay Borisov]

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

Re: [PATCH] btrfs: Rework error detection in init_tree_roots

[David Sterba]

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 v2] btrfs: Rework error detection in init_tree_roots

[Nikolay Borisov]

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. While at it change the -EUCLEAN to -EIO in case the extent
buffer is not read correctly, this is in line with other sites which
return -EIO when the eb couldn't be read.

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

Changes in v2:
* Return -EIO in case of !extent_buffer_uptodate
* Change the error messages to distinguish both cases albeit they are still
rather similar.

 fs/btrfs/disk-io.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 2f4169231992..2483648f2915 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -2624,18 +2624,17 @@ 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, "Couldn't 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;
-			}
-
-			btrfs_warn(fs_info, "failed to read tree root");
+		} else if (!extent_buffer_uptodate(tree_root->node)) {
+			handle_error = true;
+			ret = -EIO;
+			btrfs_warn(fs_info, "Error while reading tree root");
 			continue;
 		}

--
2.17.1

Re: [PATCH v2] btrfs: Rework error detection in init_tree_roots

[Nikolay Borisov]

On 12.08.20 г. 16:16 ч., Nikolay Borisov wrote:
> 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. While at it change the -EUCLEAN to -EIO in case the extent
> buffer is not read correctly, this is in line with other sites which
> return -EIO when the eb couldn't be read.
> 
> 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>
> ---
> 
> Changes in v2:
> * Return -EIO in case of !extent_buffer_uptodate
> * Change the error messages to distinguish both cases albeit they are still
> rather similar.

Any comments on V2 ?

Re: [PATCH v2] btrfs: Rework error detection in init_tree_roots

[David Sterba]

On Thu, Aug 27, 2020 at 10:02:10AM +0300, Nikolay Borisov wrote:
> 
> 
> On 12.08.20 г. 16:16 ч., Nikolay Borisov wrote:
> > 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. While at it change the -EUCLEAN to -EIO in case the extent
> > buffer is not read correctly, this is in line with other sites which
> > return -EIO when the eb couldn't be read.
> > 
> > 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>
> > ---
> > 
> > Changes in v2:
> > * Return -EIO in case of !extent_buffer_uptodate
> > * Change the error messages to distinguish both cases albeit they are still
> > rather similar.
> 
> Any comments on V2 ?

Code is ok, the messages should not start with an uppercase letter,
fixed.