[PATCH] Btrfs: add parent_transid parameter to veirfy_level_key

[PATCH] Btrfs: add parent_transid parameter to veirfy_level_key

[Liu Bo]

@parent_transid could tell whether the eb's generation has been verified
by the caller.

Signed-off-by: Liu Bo <bo.liu@linux.alibaba.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 60caa68c3618..b5d55b0ec19b 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -416,7 +416,7 @@ static int btrfs_check_super_csum(struct btrfs_fs_info *fs_info,
 
 static int verify_level_key(struct btrfs_fs_info *fs_info,
 			    struct extent_buffer *eb, int level,
-			    struct btrfs_key *first_key)
+			    struct btrfs_key *first_key, u64 parent_transid)
 {
 	int found_level;
 	struct btrfs_key found_key;
@@ -454,10 +454,10 @@ static int verify_level_key(struct btrfs_fs_info *fs_info,
 	if (ret) {
 		WARN_ON(1);
 		btrfs_err(fs_info,
-"tree first key mismatch detected, bytenr=%llu key expected=(%llu, %u, %llu) has=(%llu, %u, %llu)",
+"tree first key mismatch detected, bytenr=%llu key expected=(%llu, %u, %llu) has=(%llu, %u, %llu) parent_transid %llu",
 			  eb->start, first_key->objectid, first_key->type,
 			  first_key->offset, found_key.objectid,
-			  found_key.type, found_key.offset);
+			  found_key.type, found_key.offset, parent_transid);
 	}
 #endif
 	return ret;
@@ -493,7 +493,7 @@ static int btree_read_extent_buffer_pages(struct btrfs_fs_info *fs_info,
 						   parent_transid, 0))
 				ret = -EIO;
 			else if (verify_level_key(fs_info, eb, level,
-						  first_key))
+						  first_key, parent_transid))
 				ret = -EUCLEAN;
 			else
 				break;
-- 
1.8.3.1

Re: [PATCH] Btrfs: add parent_transid parameter to veirfy_level_key

[David Sterba]

On Wed, May 16, 2018 at 01:37:37AM +0800, Liu Bo wrote:
> @parent_transid could tell whether the eb's generation has been verified
> by the caller.

Can you please write why you add this change? Like is it an enhancement
or debugging help or someting like that.

Re: [PATCH] Btrfs: add parent_transid parameter to veirfy_level_key

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 1986 bytes --]



On 2018年05月16日 01:37, Liu Bo wrote:
> @parent_transid could tell whether the eb's generation has been verified
> by the caller.
> 
> Signed-off-by: Liu Bo <bo.liu@linux.alibaba.com>

Looks pretty useful to debug the btrfs/124 bug you just fixed.

But a small nitpick inlined below:

> ---
>  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 60caa68c3618..b5d55b0ec19b 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -416,7 +416,7 @@ static int btrfs_check_super_csum(struct btrfs_fs_info *fs_info,
>  
>  static int verify_level_key(struct btrfs_fs_info *fs_info,
>  			    struct extent_buffer *eb, int level,
> -			    struct btrfs_key *first_key)
> +			    struct btrfs_key *first_key, u64 parent_transid)
>  {
>  	int found_level;
>  	struct btrfs_key found_key;
> @@ -454,10 +454,10 @@ static int verify_level_key(struct btrfs_fs_info *fs_info,
>  	if (ret) {
>  		WARN_ON(1);
>  		btrfs_err(fs_info,
> -"tree first key mismatch detected, bytenr=%llu key expected=(%llu, %u, %llu) has=(%llu, %u, %llu)",
> +"tree first key mismatch detected, bytenr=%llu key expected=(%llu, %u, %llu) has=(%llu, %u, %llu) parent_transid %llu",

It would look better if parent transid can be before key output.

Despite that, everything looks good.

Thanks,
Qu

>  			  eb->start, first_key->objectid, first_key->type,
>  			  first_key->offset, found_key.objectid,
> -			  found_key.type, found_key.offset);
> +			  found_key.type, found_key.offset, parent_transid);
>  	}
>  #endif
>  	return ret;
> @@ -493,7 +493,7 @@ static int btree_read_extent_buffer_pages(struct btrfs_fs_info *fs_info,
>  						   parent_transid, 0))
>  				ret = -EIO;
>  			else if (verify_level_key(fs_info, eb, level,
> -						  first_key))
> +						  first_key, parent_transid))
>  				ret = -EUCLEAN;
>  			else
>  				break;
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH v2] Btrfs: add parent_transid parameter to veirfy_level_key

[Liu Bo]

As verify_level_key() is checked after verify_parent_transid(), i.e.

if (verify_parent_transid())
   ret = -EIO;
else if (verify_level_key())
   ret = -EUCLEAN;

if parent_transid is 0, verify_parent_transid() skips verifying
parent_transid and considers eb as valid, and if verify_level_key()
reports something wrong, we're not going to know if it's caused by
corrupted metadata or non-checkecd eb (e.g. stale eb).

@parent_transid is able to tell whether the eb's generation has been
verified by the caller.

Signed-off-by: Liu Bo <bo.liu@linux.alibaba.com>
---
v2: - more explicit commit log.
    - adjust the position shown in error msg.

 fs/btrfs/disk-io.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 60caa68c3618..ad865176a3ca 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -416,7 +416,7 @@ static int btrfs_check_super_csum(struct btrfs_fs_info *fs_info,
 
 static int verify_level_key(struct btrfs_fs_info *fs_info,
 			    struct extent_buffer *eb, int level,
-			    struct btrfs_key *first_key)
+			    struct btrfs_key *first_key, u64 parent_transid)
 {
 	int found_level;
 	struct btrfs_key found_key;
@@ -454,10 +454,11 @@ static int verify_level_key(struct btrfs_fs_info *fs_info,
 	if (ret) {
 		WARN_ON(1);
 		btrfs_err(fs_info,
-"tree first key mismatch detected, bytenr=%llu key expected=(%llu, %u, %llu) has=(%llu, %u, %llu)",
-			  eb->start, first_key->objectid, first_key->type,
-			  first_key->offset, found_key.objectid,
-			  found_key.type, found_key.offset);
+"tree first key mismatch detected, bytenr=%llu parent_transid=%llu key expected=(%llu, %u, %llu) has=(%llu, %u, %llu)",
+			  eb->start, parent_transid, first_key->objectid,
+			  first_key->type, first_key->offset,
+			  found_key.objectid, found_key.type,
+			  found_key.offset);
 	}
 #endif
 	return ret;
@@ -493,7 +494,7 @@ static int btree_read_extent_buffer_pages(struct btrfs_fs_info *fs_info,
 						   parent_transid, 0))
 				ret = -EIO;
 			else if (verify_level_key(fs_info, eb, level,
-						  first_key))
+						  first_key, parent_transid))
 				ret = -EUCLEAN;
 			else
 				break;
-- 
1.8.3.1

Re: [PATCH v2] Btrfs: add parent_transid parameter to veirfy_level_key

[Nikolay Borisov]

On 18.05.2018 05:59, Liu Bo wrote:
> As verify_level_key() is checked after verify_parent_transid(), i.e.
> 
> if (verify_parent_transid())
>    ret = -EIO;
> else if (verify_level_key())
>    ret = -EUCLEAN;
> 
> if parent_transid is 0, verify_parent_transid() skips verifying
> parent_transid and considers eb as valid, and if verify_level_key()
> reports something wrong, we're not going to know if it's caused by
> corrupted metadata or non-checkecd eb (e.g. stale eb).

It's really unclear (from the changelog) how the stale eb ties with
parent_transid. You should have explained the relationship between
checking parenttransid and stale/non-stale ebs

> 
> @parent_transid is able to tell whether the eb's generation has been
> verified by the caller.

It's really unclear why parent_transid is able to tell whether the
generation is verified by the caller.

> 
> Signed-off-by: Liu Bo <bo.liu@linux.alibaba.com>
> ---
> v2: - more explicit commit log.
>     - adjust the position shown in error msg.
> 
>  fs/btrfs/disk-io.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index 60caa68c3618..ad865176a3ca 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -416,7 +416,7 @@ static int btrfs_check_super_csum(struct btrfs_fs_info *fs_info,
>  
>  static int verify_level_key(struct btrfs_fs_info *fs_info,
>  			    struct extent_buffer *eb, int level,
> -			    struct btrfs_key *first_key)
> +			    struct btrfs_key *first_key, u64 parent_transid)
>  {
>  	int found_level;
>  	struct btrfs_key found_key;
> @@ -454,10 +454,11 @@ static int verify_level_key(struct btrfs_fs_info *fs_info,
>  	if (ret) {
>  		WARN_ON(1);
>  		btrfs_err(fs_info,
> -"tree first key mismatch detected, bytenr=%llu key expected=(%llu, %u, %llu) has=(%llu, %u, %llu)",
> -			  eb->start, first_key->objectid, first_key->type,
> -			  first_key->offset, found_key.objectid,
> -			  found_key.type, found_key.offset);
> +"tree first key mismatch detected, bytenr=%llu parent_transid=%llu key expected=(%llu, %u, %llu) has=(%llu, %u, %llu)",
> +			  eb->start, parent_transid, first_key->objectid,
> +			  first_key->type, first_key->offset,
> +			  found_key.objectid, found_key.type,
> +			  found_key.offset);
>  	}
>  #endif
>  	return ret;
> @@ -493,7 +494,7 @@ static int btree_read_extent_buffer_pages(struct btrfs_fs_info *fs_info,
>  						   parent_transid, 0))
>  				ret = -EIO;
>  			else if (verify_level_key(fs_info, eb, level,
> -						  first_key))
> +						  first_key, parent_transid))
>  				ret = -EUCLEAN;
>  			else
>  				break;
> 

Re: [PATCH v2] Btrfs: add parent_transid parameter to veirfy_level_key

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 2481 bytes --]



On 2018年05月18日 10:59, Liu Bo wrote:
> As verify_level_key() is checked after verify_parent_transid(), i.e.
> 
> if (verify_parent_transid())
>    ret = -EIO;
> else if (verify_level_key())
>    ret = -EUCLEAN;
> 
> if parent_transid is 0, verify_parent_transid() skips verifying
> parent_transid and considers eb as valid, and if verify_level_key()
> reports something wrong, we're not going to know if it's caused by
> corrupted metadata or non-checkecd eb (e.g. stale eb).
> 
> @parent_transid is able to tell whether the eb's generation has been
> verified by the caller.
> 
> Signed-off-by: Liu Bo <bo.liu@linux.alibaba.com>

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu

> ---
> v2: - more explicit commit log.
>     - adjust the position shown in error msg.
> 
>  fs/btrfs/disk-io.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index 60caa68c3618..ad865176a3ca 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -416,7 +416,7 @@ static int btrfs_check_super_csum(struct btrfs_fs_info *fs_info,
>  
>  static int verify_level_key(struct btrfs_fs_info *fs_info,
>  			    struct extent_buffer *eb, int level,
> -			    struct btrfs_key *first_key)
> +			    struct btrfs_key *first_key, u64 parent_transid)
>  {
>  	int found_level;
>  	struct btrfs_key found_key;
> @@ -454,10 +454,11 @@ static int verify_level_key(struct btrfs_fs_info *fs_info,
>  	if (ret) {
>  		WARN_ON(1);
>  		btrfs_err(fs_info,
> -"tree first key mismatch detected, bytenr=%llu key expected=(%llu, %u, %llu) has=(%llu, %u, %llu)",
> -			  eb->start, first_key->objectid, first_key->type,
> -			  first_key->offset, found_key.objectid,
> -			  found_key.type, found_key.offset);
> +"tree first key mismatch detected, bytenr=%llu parent_transid=%llu key expected=(%llu, %u, %llu) has=(%llu, %u, %llu)",
> +			  eb->start, parent_transid, first_key->objectid,
> +			  first_key->type, first_key->offset,
> +			  found_key.objectid, found_key.type,
> +			  found_key.offset);
>  	}
>  #endif
>  	return ret;
> @@ -493,7 +494,7 @@ static int btree_read_extent_buffer_pages(struct btrfs_fs_info *fs_info,
>  						   parent_transid, 0))
>  				ret = -EIO;
>  			else if (verify_level_key(fs_info, eb, level,
> -						  first_key))
> +						  first_key, parent_transid))
>  				ret = -EUCLEAN;
>  			else
>  				break;
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2] Btrfs: add parent_transid parameter to veirfy_level_key

[Qu Wenruo]

On 2018年05月18日 13:02, Nikolay Borisov wrote:
> 
> 
> On 18.05.2018 05:59, Liu Bo wrote:
>> As verify_level_key() is checked after verify_parent_transid(), i.e.
>>
>> if (verify_parent_transid())
>>    ret = -EIO;
>> else if (verify_level_key())
>>    ret = -EUCLEAN;
>>
>> if parent_transid is 0, verify_parent_transid() skips verifying
>> parent_transid and considers eb as valid, and if verify_level_key()
>> reports something wrong, we're not going to know if it's caused by
>> corrupted metadata or non-checkecd eb (e.g. stale eb).
> 
> It's really unclear (from the changelog) how the stale eb ties with
> parent_transid. You should have explained the relationship between
> checking parenttransid and stale/non-stale ebs

It's in fact related to the fixing patch:
Btrfs: fix the corruption by reading stale btree blocks

But whatever, extra mention won't hurt.

> 
>>
>> @parent_transid is able to tell whether the eb's generation has been
>> verified by the caller.
> 
> It's really unclear why parent_transid is able to tell whether the
> generation is verified by the caller.

At least this looks pretty straightforward to me.
@parent_transid = 0 means no generation check, and @parent_transid != 0
means already checked.

So at least for this part I didn't get any confusion.

Thanks,
Qu

> 
>>
>> Signed-off-by: Liu Bo <bo.liu@linux.alibaba.com>
>> ---
>> v2: - more explicit commit log.
>>     - adjust the position shown in error msg.
>>
>>  fs/btrfs/disk-io.c | 13 +++++++------
>>  1 file changed, 7 insertions(+), 6 deletions(-)
>>
>> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
>> index 60caa68c3618..ad865176a3ca 100644
>> --- a/fs/btrfs/disk-io.c
>> +++ b/fs/btrfs/disk-io.c
>> @@ -416,7 +416,7 @@ static int btrfs_check_super_csum(struct btrfs_fs_info *fs_info,
>>  
>>  static int verify_level_key(struct btrfs_fs_info *fs_info,
>>  			    struct extent_buffer *eb, int level,
>> -			    struct btrfs_key *first_key)
>> +			    struct btrfs_key *first_key, u64 parent_transid)
>>  {
>>  	int found_level;
>>  	struct btrfs_key found_key;
>> @@ -454,10 +454,11 @@ static int verify_level_key(struct btrfs_fs_info *fs_info,
>>  	if (ret) {
>>  		WARN_ON(1);
>>  		btrfs_err(fs_info,
>> -"tree first key mismatch detected, bytenr=%llu key expected=(%llu, %u, %llu) has=(%llu, %u, %llu)",
>> -			  eb->start, first_key->objectid, first_key->type,
>> -			  first_key->offset, found_key.objectid,
>> -			  found_key.type, found_key.offset);
>> +"tree first key mismatch detected, bytenr=%llu parent_transid=%llu key expected=(%llu, %u, %llu) has=(%llu, %u, %llu)",
>> +			  eb->start, parent_transid, first_key->objectid,
>> +			  first_key->type, first_key->offset,
>> +			  found_key.objectid, found_key.type,
>> +			  found_key.offset);
>>  	}
>>  #endif
>>  	return ret;
>> @@ -493,7 +494,7 @@ static int btree_read_extent_buffer_pages(struct btrfs_fs_info *fs_info,
>>  						   parent_transid, 0))
>>  				ret = -EIO;
>>  			else if (verify_level_key(fs_info, eb, level,
>> -						  first_key))
>> +						  first_key, parent_transid))
>>  				ret = -EUCLEAN;
>>  			else
>>  				break;
>>
> --
> 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 v2] Btrfs: add parent_transid parameter to veirfy_level_key

[David Sterba]

On Fri, May 18, 2018 at 01:10:59PM +0800, Qu Wenruo wrote:
> 
> 
> On 2018年05月18日 13:02, Nikolay Borisov wrote:
> > 
> > 
> > On 18.05.2018 05:59, Liu Bo wrote:
> >> As verify_level_key() is checked after verify_parent_transid(), i.e.
> >>
> >> if (verify_parent_transid())
> >>    ret = -EIO;
> >> else if (verify_level_key())
> >>    ret = -EUCLEAN;
> >>
> >> if parent_transid is 0, verify_parent_transid() skips verifying
> >> parent_transid and considers eb as valid, and if verify_level_key()
> >> reports something wrong, we're not going to know if it's caused by
> >> corrupted metadata or non-checkecd eb (e.g. stale eb).
> > 
> > It's really unclear (from the changelog) how the stale eb ties with
> > parent_transid. You should have explained the relationship between
> > checking parenttransid and stale/non-stale ebs
> 
> It's in fact related to the fixing patch:
> Btrfs: fix the corruption by reading stale btree blocks
> 
> But whatever, extra mention won't hurt.

I've added reference to the patch as it has more details so we don't
have to repeat it in the changelog.

Patch added to misc-next, thanks.