[PATCH] btrfs: Add more details while checking tree block

[PATCH] btrfs: Add more details while checking tree block

[Su Yue]

For easier debug, print eb->start if level is invalid.
Also make print clear if bytenr found is not expected.

Signed-off-by: Su Yue <suy.fnst@cn.fujitsu.com>
---
 fs/btrfs/disk-io.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index c3504b4d281b..a90dab84f41b 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -615,8 +615,8 @@ static int btree_readpage_end_io_hook(struct btrfs_io_bio *io_bio,
 
 	found_start = btrfs_header_bytenr(eb);
 	if (found_start != eb->start) {
-		btrfs_err_rl(fs_info, "bad tree block start %llu %llu",
-			     found_start, eb->start);
+		btrfs_err_rl(fs_info, "bad tree block start want %llu have %llu",
+			     eb->start, found_start);
 		ret = -EIO;
 		goto err;
 	}
@@ -628,8 +628,8 @@ static int btree_readpage_end_io_hook(struct btrfs_io_bio *io_bio,
 	}
 	found_level = btrfs_header_level(eb);
 	if (found_level >= BTRFS_MAX_LEVEL) {
-		btrfs_err(fs_info, "bad tree block level %d",
-			  (int)btrfs_header_level(eb));
+		btrfs_err(fs_info, "bad tree block level %d on %llu",
+			  (int)btrfs_header_level(eb), eb->start);
 		ret = -EIO;
 		goto err;
 	}
-- 
2.17.1

Re: [PATCH] btrfs: Add more details while checking tree block

[David Sterba]

On Fri, Jun 22, 2018 at 09:52:15AM +0800, Su Yue wrote:
> For easier debug, print eb->start if level is invalid.
> Also make print clear if bytenr found is not expected.
> 
> Signed-off-by: Su Yue <suy.fnst@cn.fujitsu.com>

Reviewed-by: David Sterba <dsterba@suse.com>

Re: [PATCH] btrfs: Add more details while checking tree block

[Nikolay Borisov]

On 22.06.2018 04:52, Su Yue wrote:
> For easier debug, print eb->start if level is invalid.
> Also make print clear if bytenr found is not expected.
> 
> Signed-off-by: Su Yue <suy.fnst@cn.fujitsu.com>
> ---
>  fs/btrfs/disk-io.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index c3504b4d281b..a90dab84f41b 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -615,8 +615,8 @@ static int btree_readpage_end_io_hook(struct btrfs_io_bio *io_bio,
>  
>  	found_start = btrfs_header_bytenr(eb);
>  	if (found_start != eb->start) {
> -		btrfs_err_rl(fs_info, "bad tree block start %llu %llu",
> -			     found_start, eb->start);
> +		btrfs_err_rl(fs_info, "bad tree block start want %llu have %llu",

nit: I'd rather have the want/have in brackets (want %llu have% llu)

> +			     eb->start, found_start);
>  		ret = -EIO;
>  		goto err;
>  	}
> @@ -628,8 +628,8 @@ static int btree_readpage_end_io_hook(struct btrfs_io_bio *io_bio,
>  	}
>  	found_level = btrfs_header_level(eb);
>  	if (found_level >= BTRFS_MAX_LEVEL) {
> -		btrfs_err(fs_info, "bad tree block level %d",
> -			  (int)btrfs_header_level(eb));
> +		btrfs_err(fs_info, "bad tree block level %d on %llu",
> +			  (int)btrfs_header_level(eb), eb->start);
>  		ret = -EIO;
>  		goto err;
>  	}
> 

Re: Fwd: [PATCH] btrfs: Add more details while checking tree block

[Su Yue]

> From: Nikolay Borisov <nborisov@suse.com>
> Date: Fri, Jun 22, 2018 at 7:49 PM
> Subject: Re: [PATCH] btrfs: Add more details while checking tree block
> To: Su Yue <suy.fnst@cn.fujitsu.com>, <linux-btrfs@vger.kernel.org>
> 
> 
> 
> 
> On 22.06.2018 04:52, Su Yue wrote:
> > For easier debug, print eb->start if level is invalid.
> > Also make print clear if bytenr found is not expected.
> >
> > Signed-off-by: Su Yue <suy.fnst@cn.fujitsu.com>
> > ---
> >  fs/btrfs/disk-io.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> > index c3504b4d281b..a90dab84f41b 100644
> > --- a/fs/btrfs/disk-io.c
> > +++ b/fs/btrfs/disk-io.c
> > @@ -615,8 +615,8 @@ static int btree_readpage_end_io_hook(struct btrfs_io_bio *io_bio,
> >
> >       found_start = btrfs_header_bytenr(eb);
> >       if (found_start != eb->start) {
> > -             btrfs_err_rl(fs_info, "bad tree block start %llu %llu",
> > -                          found_start, eb->start);
> > +             btrfs_err_rl(fs_info, "bad tree block start want %llu have %llu",
> 
> nit: I'd rather have the want/have in brackets (want %llu have% llu)
> 
Hmmm... Here the output message is short enough, so I think there is no
need for brackets. 
And searching for word "want" in disk_io.c , there are no brackets too.

Thanks,
Su
> > +                          eb->start, found_start);
> >               ret = -EIO;
> >               goto err;
> >       }
> > @@ -628,8 +628,8 @@ static int btree_readpage_end_io_hook(struct btrfs_io_bio *io_bio,
> >       }
> >       found_level = btrfs_header_level(eb);
> >       if (found_level >= BTRFS_MAX_LEVEL) {
> > -             btrfs_err(fs_info, "bad tree block level %d",
> > -                       (int)btrfs_header_level(eb));
> > +             btrfs_err(fs_info, "bad tree block level %d on %llu",
> > +                       (int)btrfs_header_level(eb), eb->start);
> >               ret = -EIO;
> >               goto err;
> >       }
> >
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH] btrfs: Add more details while checking tree block

[Hans van Kranenburg]

On 06/22/2018 01:48 PM, Nikolay Borisov wrote:
> 
> 
> On 22.06.2018 04:52, Su Yue wrote:
>> For easier debug, print eb->start if level is invalid.
>> Also make print clear if bytenr found is not expected.
>>
>> Signed-off-by: Su Yue <suy.fnst@cn.fujitsu.com>
>> ---
>>  fs/btrfs/disk-io.c | 8 ++++----
>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
>> index c3504b4d281b..a90dab84f41b 100644
>> --- a/fs/btrfs/disk-io.c
>> +++ b/fs/btrfs/disk-io.c
>> @@ -615,8 +615,8 @@ static int btree_readpage_end_io_hook(struct btrfs_io_bio *io_bio,
>>  
>>  	found_start = btrfs_header_bytenr(eb);
>>  	if (found_start != eb->start) {
>> -		btrfs_err_rl(fs_info, "bad tree block start %llu %llu",
>> -			     found_start, eb->start);
>> +		btrfs_err_rl(fs_info, "bad tree block start want %llu have %llu",
> 
> nit: I'd rather have the want/have in brackets (want %llu have% llu)

>From a user support point of view, this text should really be improved.
There are a few places where 'want' and 'have' are reported in error
strings, and it's totally unclear what they mean.

Intuitively I'd say when checking a csum, the "want" would be what's on
disk now, since you want that to be correct, and the "have" would be
what you have calculated, but it's actually the other way round, or
wasn't it? Or was it?

Every time someone pastes such a message when we help on IRC for
example, there's confusion, and I have to look up the source again,
because I always forget.

What about (%llu stored on disk, %llu calculated now) or something similar?

> 
>> +			     eb->start, found_start);
>>  		ret = -EIO;
>>  		goto err;
>>  	}
>> @@ -628,8 +628,8 @@ static int btree_readpage_end_io_hook(struct btrfs_io_bio *io_bio,
>>  	}
>>  	found_level = btrfs_header_level(eb);
>>  	if (found_level >= BTRFS_MAX_LEVEL) {
>> -		btrfs_err(fs_info, "bad tree block level %d",
>> -			  (int)btrfs_header_level(eb));
>> +		btrfs_err(fs_info, "bad tree block level %d on %llu",
>> +			  (int)btrfs_header_level(eb), eb->start);
>>  		ret = -EIO;
>>  		goto err;
>>  	}
>>

-- 
Hans van Kranenburg

Re: [PATCH] btrfs: Add more details while checking tree block

[Hugo Mills]

[-- Attachment #1: Type: text/plain, Size: 2761 bytes --]

On Fri, Jun 22, 2018 at 05:26:02PM +0200, Hans van Kranenburg wrote:
> On 06/22/2018 01:48 PM, Nikolay Borisov wrote:
> > 
> > 
> > On 22.06.2018 04:52, Su Yue wrote:
> >> For easier debug, print eb->start if level is invalid.
> >> Also make print clear if bytenr found is not expected.
> >>
> >> Signed-off-by: Su Yue <suy.fnst@cn.fujitsu.com>
> >> ---
> >>  fs/btrfs/disk-io.c | 8 ++++----
> >>  1 file changed, 4 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> >> index c3504b4d281b..a90dab84f41b 100644
> >> --- a/fs/btrfs/disk-io.c
> >> +++ b/fs/btrfs/disk-io.c
> >> @@ -615,8 +615,8 @@ static int btree_readpage_end_io_hook(struct btrfs_io_bio *io_bio,
> >>  
> >>  	found_start = btrfs_header_bytenr(eb);
> >>  	if (found_start != eb->start) {
> >> -		btrfs_err_rl(fs_info, "bad tree block start %llu %llu",
> >> -			     found_start, eb->start);
> >> +		btrfs_err_rl(fs_info, "bad tree block start want %llu have %llu",
> > 
> > nit: I'd rather have the want/have in brackets (want %llu have% llu)
> 
> From a user support point of view, this text should really be improved.
> There are a few places where 'want' and 'have' are reported in error
> strings, and it's totally unclear what they mean.
> 
> Intuitively I'd say when checking a csum, the "want" would be what's on
> disk now, since you want that to be correct, and the "have" would be
> what you have calculated, but it's actually the other way round, or
> wasn't it? Or was it?
> 
> Every time someone pastes such a message when we help on IRC for
> example, there's confusion, and I have to look up the source again,
> because I always forget.
> 
> What about (%llu stored on disk, %llu calculated now) or something similar?

   Yes, definitely this. I experience the same confusion as Hans, and
I think a lot of other people do, too. I usually read "want" and
"have" the wrong way round, so more clarity would be really helpful.

   Hugo.

> >> +			     eb->start, found_start);
> >>  		ret = -EIO;
> >>  		goto err;
> >>  	}
> >> @@ -628,8 +628,8 @@ static int btree_readpage_end_io_hook(struct btrfs_io_bio *io_bio,
> >>  	}
> >>  	found_level = btrfs_header_level(eb);
> >>  	if (found_level >= BTRFS_MAX_LEVEL) {
> >> -		btrfs_err(fs_info, "bad tree block level %d",
> >> -			  (int)btrfs_header_level(eb));
> >> +		btrfs_err(fs_info, "bad tree block level %d on %llu",
> >> +			  (int)btrfs_header_level(eb), eb->start);
> >>  		ret = -EIO;
> >>  		goto err;
> >>  	}
> >>
> 

-- 
Hugo Mills             | "There's a Martian war machine outside -- they want
hugo@... carfax.org.uk | to talk to you about a cure for the common cold."
http://carfax.org.uk/  |
PGP: E2AB1DE4          |                           Stephen Franklin, Babylon 5

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] btrfs: Add more details while checking tree block

[Su Yue]

> Sent: Friday, June 22, 2018 at 11:40 PM
> From: "Hugo Mills" <hugo@carfax.org.uk>
> To: "Hans van Kranenburg" <hans.van.kranenburg@mendix.com>
> Cc: "Nikolay Borisov" <nborisov@suse.com>, "Su Yue" <suy.fnst@cn.fujitsu.com>, linux-btrfs@vger.kernel.org
> Subject: Re: [PATCH] btrfs: Add more details while checking tree block
>
> On Fri, Jun 22, 2018 at 05:26:02PM +0200, Hans van Kranenburg wrote:
> > On 06/22/2018 01:48 PM, Nikolay Borisov wrote:
> > > 
> > > 
> > > On 22.06.2018 04:52, Su Yue wrote:
> > >> For easier debug, print eb->start if level is invalid.
> > >> Also make print clear if bytenr found is not expected.
> > >>
> > >> Signed-off-by: Su Yue <suy.fnst@cn.fujitsu.com>
> > >> ---
> > >>  fs/btrfs/disk-io.c | 8 ++++----
> > >>  1 file changed, 4 insertions(+), 4 deletions(-)
> > >>
> > >> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> > >> index c3504b4d281b..a90dab84f41b 100644
> > >> --- a/fs/btrfs/disk-io.c
> > >> +++ b/fs/btrfs/disk-io.c
> > >> @@ -615,8 +615,8 @@ static int btree_readpage_end_io_hook(struct btrfs_io_bio *io_bio,
> > >>  
> > >>  	found_start = btrfs_header_bytenr(eb);
> > >>  	if (found_start != eb->start) {
> > >> -		btrfs_err_rl(fs_info, "bad tree block start %llu %llu",
> > >> -			     found_start, eb->start);
> > >> +		btrfs_err_rl(fs_info, "bad tree block start want %llu have %llu",
> > > 
> > > nit: I'd rather have the want/have in brackets (want %llu have% llu)
> > 
> > From a user support point of view, this text should really be improved.
> > There are a few places where 'want' and 'have' are reported in error
> > strings, and it's totally unclear what they mean.
> > 
> > Intuitively I'd say when checking a csum, the "want" would be what's on
> > disk now, since you want that to be correct, and the "have" would be
> > what you have calculated, but it's actually the other way round, or
> > wasn't it? Or was it?
> > 
> > Every time someone pastes such a message when we help on IRC for
> > example, there's confusion, and I have to look up the source again,
> > because I always forget.
> > 
> > What about (%llu stored on disk, %llu calculated now) or something similar?
> 
>    Yes, definitely this. I experience the same confusion as Hans, and
> I think a lot of other people do, too. I usually read "want" and
> "have" the wrong way round, so more clarity would be really helpful.
> 
Thanks for the response. I will try to think up a "proper" description.

Su
>    Hugo.
> 
> > >> +			     eb->start, found_start);
> > >>  		ret = -EIO;
> > >>  		goto err;
> > >>  	}
> > >> @@ -628,8 +628,8 @@ static int btree_readpage_end_io_hook(struct btrfs_io_bio *io_bio,
> > >>  	}
> > >>  	found_level = btrfs_header_level(eb);
> > >>  	if (found_level >= BTRFS_MAX_LEVEL) {
> > >> -		btrfs_err(fs_info, "bad tree block level %d",
> > >> -			  (int)btrfs_header_level(eb));
> > >> +		btrfs_err(fs_info, "bad tree block level %d on %llu",
> > >> +			  (int)btrfs_header_level(eb), eb->start);
> > >>  		ret = -EIO;
> > >>  		goto err;
> > >>  	}
> > >>
> > 
> 
> -- 
> Hugo Mills             | "There's a Martian war machine outside -- they want
> hugo@... carfax.org.uk | to talk to you about a cure for the common cold."
> http://carfax.org.uk/  |
> PGP: E2AB1DE4          |                           Stephen Franklin, Babylon 5
> 

Re: [PATCH] btrfs: Add more details while checking tree block

[Su Yue]

> Sent: Friday, June 22, 2018 at 11:26 PM
> From: "Hans van Kranenburg" <hans.van.kranenburg@mendix.com>
> To: "Nikolay Borisov" <nborisov@suse.com>, "Su Yue" <suy.fnst@cn.fujitsu.com>, linux-btrfs@vger.kernel.org
> Subject: Re: [PATCH] btrfs: Add more details while checking tree block
>
> On 06/22/2018 01:48 PM, Nikolay Borisov wrote:
> > 
> > 
> > On 22.06.2018 04:52, Su Yue wrote:
> >> For easier debug, print eb->start if level is invalid.
> >> Also make print clear if bytenr found is not expected.
> >>
> >> Signed-off-by: Su Yue <suy.fnst@cn.fujitsu.com>
> >> ---
> >>  fs/btrfs/disk-io.c | 8 ++++----
> >>  1 file changed, 4 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> >> index c3504b4d281b..a90dab84f41b 100644
> >> --- a/fs/btrfs/disk-io.c
> >> +++ b/fs/btrfs/disk-io.c
> >> @@ -615,8 +615,8 @@ static int btree_readpage_end_io_hook(struct btrfs_io_bio *io_bio,
> >>  
> >>  	found_start = btrfs_header_bytenr(eb);
> >>  	if (found_start != eb->start) {
> >> -		btrfs_err_rl(fs_info, "bad tree block start %llu %llu",
> >> -			     found_start, eb->start);
> >> +		btrfs_err_rl(fs_info, "bad tree block start want %llu have %llu",
> > 
> > nit: I'd rather have the want/have in brackets (want %llu have% llu)
> 
> From a user support point of view, this text should really be improved.
> There are a few places where 'want' and 'have' are reported in error
> strings, and it's totally unclear what they mean.
> 
Yes. The strings are too concise for users to understand errors.
Developers always prefer to avoid "unnecessary" words in logs.

> Intuitively I'd say when checking a csum, the "want" would be what's on
> disk now, since you want that to be correct, and the "have" would be
> what you have calculated, but it's actually the other way round, or
> wasn't it? Or was it?
For csum, you are right at all.

In this situation, IIRC bytenr of "have" is read from disk.
Bytenr of "want" is assigned while constructing eb, from parent ptr
, root item and other things which are also read from disk.
> 
> Every time someone pastes such a message when we help on IRC for
> example, there's confusion, and I have to look up the source again,
> because I always forget.

Me too.
> 
> What about (%llu stored on disk, %llu calculated now) or something similar?
> 
"(%llu stored on disk " part sounds good to me. But I don't know how to 
describe the second part. May the good sleep help me.

Thanks,
Su
> > 
> >> +			     eb->start, found_start);
> >>  		ret = -EIO;
> >>  		goto err;
> >>  	}
> >> @@ -628,8 +628,8 @@ static int btree_readpage_end_io_hook(struct btrfs_io_bio *io_bio,
> >>  	}
> >>  	found_level = btrfs_header_level(eb);
> >>  	if (found_level >= BTRFS_MAX_LEVEL) {
> >> -		btrfs_err(fs_info, "bad tree block level %d",
> >> -			  (int)btrfs_header_level(eb));
> >> +		btrfs_err(fs_info, "bad tree block level %d on %llu",
> >> +			  (int)btrfs_header_level(eb), eb->start);
> >>  		ret = -EIO;
> >>  		goto err;
> >>  	}
> >>
> 
> -- 
> Hans van Kranenburg
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH] btrfs: Add more details while checking tree block

[Nikolay Borisov]

On 22.06.2018 19:17, Su Yue wrote:
> 
> 
> 
>> Sent: Friday, June 22, 2018 at 11:26 PM
>> From: "Hans van Kranenburg" <hans.van.kranenburg@mendix.com>
>> To: "Nikolay Borisov" <nborisov@suse.com>, "Su Yue" <suy.fnst@cn.fujitsu.com>, linux-btrfs@vger.kernel.org
>> Subject: Re: [PATCH] btrfs: Add more details while checking tree block
>>
>> On 06/22/2018 01:48 PM, Nikolay Borisov wrote:
>>>
>>>
>>> On 22.06.2018 04:52, Su Yue wrote:
>>>> For easier debug, print eb->start if level is invalid.
>>>> Also make print clear if bytenr found is not expected.
>>>>
>>>> Signed-off-by: Su Yue <suy.fnst@cn.fujitsu.com>
>>>> ---
>>>>  fs/btrfs/disk-io.c | 8 ++++----
>>>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
>>>> index c3504b4d281b..a90dab84f41b 100644
>>>> --- a/fs/btrfs/disk-io.c
>>>> +++ b/fs/btrfs/disk-io.c
>>>> @@ -615,8 +615,8 @@ static int btree_readpage_end_io_hook(struct btrfs_io_bio *io_bio,
>>>>  
>>>>  	found_start = btrfs_header_bytenr(eb);
>>>>  	if (found_start != eb->start) {
>>>> -		btrfs_err_rl(fs_info, "bad tree block start %llu %llu",
>>>> -			     found_start, eb->start);
>>>> +		btrfs_err_rl(fs_info, "bad tree block start want %llu have %llu",
>>>
>>> nit: I'd rather have the want/have in brackets (want %llu have% llu)
>>
>> From a user support point of view, this text should really be improved.
>> There are a few places where 'want' and 'have' are reported in error
>> strings, and it's totally unclear what they mean.
>>
> Yes. The strings are too concise for users to understand errors.
> Developers always prefer to avoid "unnecessary" words in logs.
> 
>> Intuitively I'd say when checking a csum, the "want" would be what's on
>> disk now, since you want that to be correct, and the "have" would be
>> what you have calculated, but it's actually the other way round, or
>> wasn't it? Or was it?
> For csum, you are right at all.
> 
> In this situation, IIRC bytenr of "have" is read from disk.
> Bytenr of "want" is assigned while constructing eb, from parent ptr
> , root item and other things which are also read from disk.
>>
>> Every time someone pastes such a message when we help on IRC for
>> example, there's confusion, and I have to look up the source again,
>> because I always forget.
> 
> Me too.
>>
>> What about (%llu stored on disk, %llu calculated now) or something similar?

Wha tabout expected got

>>
> "(%llu stored on disk " part sounds good to me. But I don't know how to 
> describe the second part. May the good sleep help me.
> 
> Thanks,
> Su
>>>
>>>> +			     eb->start, found_start);
>>>>  		ret = -EIO;
>>>>  		goto err;
>>>>  	}
>>>> @@ -628,8 +628,8 @@ static int btree_readpage_end_io_hook(struct btrfs_io_bio *io_bio,
>>>>  	}
>>>>  	found_level = btrfs_header_level(eb);
>>>>  	if (found_level >= BTRFS_MAX_LEVEL) {
>>>> -		btrfs_err(fs_info, "bad tree block level %d",
>>>> -			  (int)btrfs_header_level(eb));
>>>> +		btrfs_err(fs_info, "bad tree block level %d on %llu",
>>>> +			  (int)btrfs_header_level(eb), eb->start);
>>>>  		ret = -EIO;
>>>>  		goto err;
>>>>  	}
>>>>
>>
>> -- 
>> Hans van Kranenburg
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
> 

Re: [PATCH] btrfs: Add more details while checking tree block

[Hans van Kranenburg]

On 06/22/2018 06:25 PM, Nikolay Borisov wrote:
> 
> 
> On 22.06.2018 19:17, Su Yue wrote:
>>
>>
>>
>>> Sent: Friday, June 22, 2018 at 11:26 PM
>>> From: "Hans van Kranenburg" <hans.van.kranenburg@mendix.com>
>>> To: "Nikolay Borisov" <nborisov@suse.com>, "Su Yue" <suy.fnst@cn.fujitsu.com>, linux-btrfs@vger.kernel.org
>>> Subject: Re: [PATCH] btrfs: Add more details while checking tree block
>>>
>>> On 06/22/2018 01:48 PM, Nikolay Borisov wrote:
>>>>
>>>>
>>>> On 22.06.2018 04:52, Su Yue wrote:
>>>>> For easier debug, print eb->start if level is invalid.
>>>>> Also make print clear if bytenr found is not expected.
>>>>>
>>>>> Signed-off-by: Su Yue <suy.fnst@cn.fujitsu.com>
>>>>> ---
>>>>>  fs/btrfs/disk-io.c | 8 ++++----
>>>>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
>>>>> index c3504b4d281b..a90dab84f41b 100644
>>>>> --- a/fs/btrfs/disk-io.c
>>>>> +++ b/fs/btrfs/disk-io.c
>>>>> @@ -615,8 +615,8 @@ static int btree_readpage_end_io_hook(struct btrfs_io_bio *io_bio,
>>>>>  
>>>>>  	found_start = btrfs_header_bytenr(eb);
>>>>>  	if (found_start != eb->start) {
>>>>> -		btrfs_err_rl(fs_info, "bad tree block start %llu %llu",
>>>>> -			     found_start, eb->start);
>>>>> +		btrfs_err_rl(fs_info, "bad tree block start want %llu have %llu",
>>>>
>>>> nit: I'd rather have the want/have in brackets (want %llu have% llu)
>>>
>>> From a user support point of view, this text should really be improved.
>>> There are a few places where 'want' and 'have' are reported in error
>>> strings, and it's totally unclear what they mean.
>>>
>> Yes. The strings are too concise for users to understand errors.
>> Developers always prefer to avoid "unnecessary" words in logs.
>>
>>> Intuitively I'd say when checking a csum, the "want" would be what's on
>>> disk now, since you want that to be correct, and the "have" would be
>>> what you have calculated, but it's actually the other way round, or
>>> wasn't it? Or was it?
>> For csum, you are right at all.
>>
>> In this situation, IIRC bytenr of "have" is read from disk.
>> Bytenr of "want" is assigned while constructing eb, from parent ptr
>> , root item and other things which are also read from disk.
>>>
>>> Every time someone pastes such a message when we help on IRC for
>>> example, there's confusion, and I have to look up the source again,
>>> because I always forget.
>>
>> Me too.
>>>
>>> What about (%llu stored on disk, %llu calculated now) or something similar?
> 
> Wha tabout expected got

No, that's just as horrible as want and found.

Did you 'expect' the same checksum to be on disk disk as what you 'get'
when calculating one? Or did you 'expect' the same thing after
calculation as what you 'got' when reading from disk?

/o\

>>>
>> "(%llu stored on disk " part sounds good to me. But I don't know how to 
>> describe the second part. May the good sleep help me.
>>
>> Thanks,
>> Su
>>>>
>>>>> +			     eb->start, found_start);
>>>>>  		ret = -EIO;
>>>>>  		goto err;
>>>>>  	}
>>>>> @@ -628,8 +628,8 @@ static int btree_readpage_end_io_hook(struct btrfs_io_bio *io_bio,
>>>>>  	}
>>>>>  	found_level = btrfs_header_level(eb);
>>>>>  	if (found_level >= BTRFS_MAX_LEVEL) {
>>>>> -		btrfs_err(fs_info, "bad tree block level %d",
>>>>> -			  (int)btrfs_header_level(eb));
>>>>> +		btrfs_err(fs_info, "bad tree block level %d on %llu",
>>>>> +			  (int)btrfs_header_level(eb), eb->start);
>>>>>  		ret = -EIO;
>>>>>  		goto err;
>>>>>  	}
>>>>>
>>>
>>> -- 
>>> Hans van Kranenburg
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>
>>


-- 
Hans van Kranenburg

Re: [PATCH] btrfs: Add more details while checking tree block

[Noah Massey]

On Fri, Jun 22, 2018 at 12:32 PM Hans van Kranenburg
<hans.van.kranenburg@mendix.com> wrote:
>
> On 06/22/2018 06:25 PM, Nikolay Borisov wrote:
> >
> >
> > On 22.06.2018 19:17, Su Yue wrote:
> >>
> >>
> >>
> >>> Sent: Friday, June 22, 2018 at 11:26 PM
> >>> From: "Hans van Kranenburg" <hans.van.kranenburg@mendix.com>
> >>> To: "Nikolay Borisov" <nborisov@suse.com>, "Su Yue" <suy.fnst@cn.fujitsu.com>, linux-btrfs@vger.kernel.org
> >>> Subject: Re: [PATCH] btrfs: Add more details while checking tree block
> >>>
> >>> On 06/22/2018 01:48 PM, Nikolay Borisov wrote:
> >>>>
> >>>>
> >>>> On 22.06.2018 04:52, Su Yue wrote:
> >>>>> For easier debug, print eb->start if level is invalid.
> >>>>> Also make print clear if bytenr found is not expected.
> >>>>>
> >>>>> Signed-off-by: Su Yue <suy.fnst@cn.fujitsu.com>
> >>>>> ---
> >>>>>  fs/btrfs/disk-io.c | 8 ++++----
> >>>>>  1 file changed, 4 insertions(+), 4 deletions(-)
> >>>>>
> >>>>> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> >>>>> index c3504b4d281b..a90dab84f41b 100644
> >>>>> --- a/fs/btrfs/disk-io.c
> >>>>> +++ b/fs/btrfs/disk-io.c
> >>>>> @@ -615,8 +615,8 @@ static int btree_readpage_end_io_hook(struct btrfs_io_bio *io_bio,
> >>>>>
> >>>>>   found_start = btrfs_header_bytenr(eb);
> >>>>>   if (found_start != eb->start) {
> >>>>> -         btrfs_err_rl(fs_info, "bad tree block start %llu %llu",
> >>>>> -                      found_start, eb->start);
> >>>>> +         btrfs_err_rl(fs_info, "bad tree block start want %llu have %llu",
> >>>>
> >>>> nit: I'd rather have the want/have in brackets (want %llu have% llu)
> >>>
> >>> From a user support point of view, this text should really be improved.
> >>> There are a few places where 'want' and 'have' are reported in error
> >>> strings, and it's totally unclear what they mean.
> >>>
> >> Yes. The strings are too concise for users to understand errors.
> >> Developers always prefer to avoid "unnecessary" words in logs.
> >>
> >>> Intuitively I'd say when checking a csum, the "want" would be what's on
> >>> disk now, since you want that to be correct, and the "have" would be
> >>> what you have calculated, but it's actually the other way round, or
> >>> wasn't it? Or was it?
> >> For csum, you are right at all.
> >>
> >> In this situation, IIRC bytenr of "have" is read from disk.
> >> Bytenr of "want" is assigned while constructing eb, from parent ptr
> >> , root item and other things which are also read from disk.
> >>>
> >>> Every time someone pastes such a message when we help on IRC for
> >>> example, there's confusion, and I have to look up the source again,
> >>> because I always forget.
> >>
> >> Me too.
> >>>
> >>> What about (%llu stored on disk, %llu calculated now) or something similar?
> >
> > Wha tabout expected got
>
> No, that's just as horrible as want and found.
>
> Did you 'expect' the same checksum to be on disk disk as what you 'get'
> when calculating one? Or did you 'expect' the same thing after
> calculation as what you 'got' when reading from disk?
>
> /o\
>

So it's "read | archived | recorded" versus "calculated | generated |
computed" ?

If we want to be succinct, it's "then" versus "now", but that's not as
descriptive.

~ Noah