[PATCH] btrfs: fix false alert caused by legacy btrfs root item

[PATCH] btrfs: fix false alert caused by legacy btrfs root item

[Qu Wenruo]

Commit 259ee7754b67 ("btrfs: tree-checker: Add ROOT_ITEM check")
introduced btrfs root item size check, however btrfs root item has two
format, the legacy one which just ends before generation_v2 member, is
smaller than current btrfs root item size.

This caused btrfs kernel to reject valid but old tree root leaves.

Fix this problem by also allowing legacy root item, since kernel can
already handle them pretty well and upgrade to newer root item format
when needed.

Reported-by: Martin Steigerwald <martin@lichtvoll.de>
Fixes: 259ee7754b67 ("btrfs: tree-checker: Add ROOT_ITEM check")
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/tree-checker.c         | 17 ++++++++++++-----
 include/uapi/linux/btrfs_tree.h |  9 +++++++++
 2 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c
index 7b1fee630f97..6f794aca48d3 100644
--- a/fs/btrfs/tree-checker.c
+++ b/fs/btrfs/tree-checker.c
@@ -1035,7 +1035,7 @@ static int check_root_item(struct extent_buffer *leaf, struct btrfs_key *key,
 			   int slot)
 {
 	struct btrfs_fs_info *fs_info = leaf->fs_info;
-	struct btrfs_root_item ri;
+	struct btrfs_root_item ri = { 0 };
 	const u64 valid_root_flags = BTRFS_ROOT_SUBVOL_RDONLY |
 				     BTRFS_ROOT_SUBVOL_DEAD;
 	int ret;
@@ -1044,14 +1044,21 @@ static int check_root_item(struct extent_buffer *leaf, struct btrfs_key *key,
 	if (ret < 0)
 		return ret;
 
-	if (btrfs_item_size_nr(leaf, slot) != sizeof(ri)) {
+	if (btrfs_item_size_nr(leaf, slot) != sizeof(ri) &&
+	    btrfs_item_size_nr(leaf, slot) != btrfs_legacy_root_item_size()) {
 		generic_err(leaf, slot,
-			    "invalid root item size, have %u expect %zu",
-			    btrfs_item_size_nr(leaf, slot), sizeof(ri));
+			    "invalid root item size, have %u expect %zu or %zu",
+			    btrfs_item_size_nr(leaf, slot), sizeof(ri),
+			    btrfs_legacy_root_item_size());
 	}
 
+	/*
+	 * For legacy root item, the members starting at generation_v2 will be
+	 * all filled with 0.
+	 * And since we allow geneartion_v2 as 0, it will still pass the check.
+	 */
 	read_extent_buffer(leaf, &ri, btrfs_item_ptr_offset(leaf, slot),
-			   sizeof(ri));
+			   btrfs_item_size_nr(leaf, slot));
 
 	/* Generation related */
 	if (btrfs_root_generation(&ri) >
diff --git a/include/uapi/linux/btrfs_tree.h b/include/uapi/linux/btrfs_tree.h
index 9ba64ca6b4ac..464095a28b18 100644
--- a/include/uapi/linux/btrfs_tree.h
+++ b/include/uapi/linux/btrfs_tree.h
@@ -644,6 +644,15 @@ struct btrfs_root_item {
 	__le64 reserved[8]; /* for future */
 } __attribute__ ((__packed__));
 
+/*
+ * Btrfs root item used to be smaller than current size.
+ * The old format ends at where member generation_v2 is.
+ */
+static inline size_t btrfs_legacy_root_item_size(void)
+{
+	return offsetof(struct btrfs_root_item, generation_v2);
+}
+
 /*
  * this is used for both forward and backward root refs
  */
-- 
2.28.0

Re: [PATCH] btrfs: fix false alert caused by legacy btrfs root item

[Martin Steigerwald]

Hi Qu.

Instead of the tool, can I also patch my kernel with the patch below to 
have it automatically fix it?

If so, which approach would you prefer for testing?

I can apply the patch as I compile kernels myself.

Thanks,
Martin

Qu Wenruo - 22.09.20, 04:37:01 CEST:
> Commit 259ee7754b67 ("btrfs: tree-checker: Add ROOT_ITEM check")
> introduced btrfs root item size check, however btrfs root item has two
> format, the legacy one which just ends before generation_v2 member,
> is smaller than current btrfs root item size.
> 
> This caused btrfs kernel to reject valid but old tree root leaves.
> 
> Fix this problem by also allowing legacy root item, since kernel can
> already handle them pretty well and upgrade to newer root item format
> when needed.
> 
> Reported-by: Martin Steigerwald <martin@lichtvoll.de>
> Fixes: 259ee7754b67 ("btrfs: tree-checker: Add ROOT_ITEM check")
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  fs/btrfs/tree-checker.c         | 17 ++++++++++++-----
>  include/uapi/linux/btrfs_tree.h |  9 +++++++++
>  2 files changed, 21 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c
> index 7b1fee630f97..6f794aca48d3 100644
> --- a/fs/btrfs/tree-checker.c
> +++ b/fs/btrfs/tree-checker.c
> @@ -1035,7 +1035,7 @@ static int check_root_item(struct extent_buffer
> *leaf, struct btrfs_key *key, int slot)
>  {
>  	struct btrfs_fs_info *fs_info = leaf->fs_info;
> -	struct btrfs_root_item ri;
> +	struct btrfs_root_item ri = { 0 };
>  	const u64 valid_root_flags = BTRFS_ROOT_SUBVOL_RDONLY |
>  				     BTRFS_ROOT_SUBVOL_DEAD;
>  	int ret;
> @@ -1044,14 +1044,21 @@ static int check_root_item(struct
> extent_buffer *leaf, struct btrfs_key *key, if (ret < 0)
>  		return ret;
> 
> -	if (btrfs_item_size_nr(leaf, slot) != sizeof(ri)) {
> +	if (btrfs_item_size_nr(leaf, slot) != sizeof(ri) &&
> +	    btrfs_item_size_nr(leaf, slot) != 
btrfs_legacy_root_item_size())
> { generic_err(leaf, slot,
> -			    "invalid root item size, have %u expect %zu",
> -			    btrfs_item_size_nr(leaf, slot), sizeof(ri));
> +			    "invalid root item size, have %u expect %zu or 
%zu",
> +			    btrfs_item_size_nr(leaf, slot), sizeof(ri),
> +			    btrfs_legacy_root_item_size());
>  	}
> 
> +	/*
> +	 * For legacy root item, the members starting at generation_v2 
will
> be +	 * all filled with 0.
> +	 * And since we allow geneartion_v2 as 0, it will still pass the
> check. +	 */
>  	read_extent_buffer(leaf, &ri, btrfs_item_ptr_offset(leaf, slot),
> -			   sizeof(ri));
> +			   btrfs_item_size_nr(leaf, slot));
> 
>  	/* Generation related */
>  	if (btrfs_root_generation(&ri) >
> diff --git a/include/uapi/linux/btrfs_tree.h
> b/include/uapi/linux/btrfs_tree.h index 9ba64ca6b4ac..464095a28b18
> 100644
> --- a/include/uapi/linux/btrfs_tree.h
> +++ b/include/uapi/linux/btrfs_tree.h
> @@ -644,6 +644,15 @@ struct btrfs_root_item {
>  	__le64 reserved[8]; /* for future */
>  } __attribute__ ((__packed__));
> 
> +/*
> + * Btrfs root item used to be smaller than current size.
> + * The old format ends at where member generation_v2 is.
> + */
> +static inline size_t btrfs_legacy_root_item_size(void)
> +{
> +	return offsetof(struct btrfs_root_item, generation_v2);
> +}
> +
>  /*
>   * this is used for both forward and backward root refs
>   */


-- 
Martin

Re: [PATCH] btrfs: fix false alert caused by legacy btrfs root item

[Qu Wenruo]

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



On 2020/9/22 下午6:20, Martin Steigerwald wrote:
> Hi Qu.
> 
> Instead of the tool, can I also patch my kernel with the patch below to 
> have it automatically fix it?

Sure, this one is a little safer than the tool.

> 
> If so, which approach would you prefer for testing?
> 
> I can apply the patch as I compile kernels myself.

That's great.

That should solve the problem.

And if you don't like the legacy root item, just do a balance (no matter
data or metadata), and that legacy root item will be converted to
current one, and even affected kernel won't report any error any more.

Thanks,
Qu

> 
> Thanks,
> Martin
> 
> Qu Wenruo - 22.09.20, 04:37:01 CEST:
>> Commit 259ee7754b67 ("btrfs: tree-checker: Add ROOT_ITEM check")
>> introduced btrfs root item size check, however btrfs root item has two
>> format, the legacy one which just ends before generation_v2 member,
>> is smaller than current btrfs root item size.
>>
>> This caused btrfs kernel to reject valid but old tree root leaves.
>>
>> Fix this problem by also allowing legacy root item, since kernel can
>> already handle them pretty well and upgrade to newer root item format
>> when needed.
>>
>> Reported-by: Martin Steigerwald <martin@lichtvoll.de>
>> Fixes: 259ee7754b67 ("btrfs: tree-checker: Add ROOT_ITEM check")
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>  fs/btrfs/tree-checker.c         | 17 ++++++++++++-----
>>  include/uapi/linux/btrfs_tree.h |  9 +++++++++
>>  2 files changed, 21 insertions(+), 5 deletions(-)
>>
>> diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c
>> index 7b1fee630f97..6f794aca48d3 100644
>> --- a/fs/btrfs/tree-checker.c
>> +++ b/fs/btrfs/tree-checker.c
>> @@ -1035,7 +1035,7 @@ static int check_root_item(struct extent_buffer
>> *leaf, struct btrfs_key *key, int slot)
>>  {
>>  	struct btrfs_fs_info *fs_info = leaf->fs_info;
>> -	struct btrfs_root_item ri;
>> +	struct btrfs_root_item ri = { 0 };
>>  	const u64 valid_root_flags = BTRFS_ROOT_SUBVOL_RDONLY |
>>  				     BTRFS_ROOT_SUBVOL_DEAD;
>>  	int ret;
>> @@ -1044,14 +1044,21 @@ static int check_root_item(struct
>> extent_buffer *leaf, struct btrfs_key *key, if (ret < 0)
>>  		return ret;
>>
>> -	if (btrfs_item_size_nr(leaf, slot) != sizeof(ri)) {
>> +	if (btrfs_item_size_nr(leaf, slot) != sizeof(ri) &&
>> +	    btrfs_item_size_nr(leaf, slot) != 
> btrfs_legacy_root_item_size())
>> { generic_err(leaf, slot,
>> -			    "invalid root item size, have %u expect %zu",
>> -			    btrfs_item_size_nr(leaf, slot), sizeof(ri));
>> +			    "invalid root item size, have %u expect %zu or 
> %zu",
>> +			    btrfs_item_size_nr(leaf, slot), sizeof(ri),
>> +			    btrfs_legacy_root_item_size());
>>  	}
>>
>> +	/*
>> +	 * For legacy root item, the members starting at generation_v2 
> will
>> be +	 * all filled with 0.
>> +	 * And since we allow geneartion_v2 as 0, it will still pass the
>> check. +	 */
>>  	read_extent_buffer(leaf, &ri, btrfs_item_ptr_offset(leaf, slot),
>> -			   sizeof(ri));
>> +			   btrfs_item_size_nr(leaf, slot));
>>
>>  	/* Generation related */
>>  	if (btrfs_root_generation(&ri) >
>> diff --git a/include/uapi/linux/btrfs_tree.h
>> b/include/uapi/linux/btrfs_tree.h index 9ba64ca6b4ac..464095a28b18
>> 100644
>> --- a/include/uapi/linux/btrfs_tree.h
>> +++ b/include/uapi/linux/btrfs_tree.h
>> @@ -644,6 +644,15 @@ struct btrfs_root_item {
>>  	__le64 reserved[8]; /* for future */
>>  } __attribute__ ((__packed__));
>>
>> +/*
>> + * Btrfs root item used to be smaller than current size.
>> + * The old format ends at where member generation_v2 is.
>> + */
>> +static inline size_t btrfs_legacy_root_item_size(void)
>> +{
>> +	return offsetof(struct btrfs_root_item, generation_v2);
>> +}
>> +
>>  /*
>>   * this is used for both forward and backward root refs
>>   */
> 
> 


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

Re: [PATCH] btrfs: fix false alert caused by legacy btrfs root item

[Martin Steigerwald]

Qu Wenruo - 22.09.20, 12:34:18 CEST:
> On 2020/9/22 下午6:20, Martin Steigerwald wrote:
> > Instead of the tool, can I also patch my kernel with the patch below
> > to have it automatically fix it?
> 
> Sure, this one is a little safer than the tool.

Thanks.

> > If so, which approach would you prefer for testing?
> > 
> > I can apply the patch as I compile kernels myself.
> 
> That's great.
> 
> That should solve the problem.
> 
> And if you don't like the legacy root item, just do a balance (no
> matter data or metadata), and that legacy root item will be converted
> to current one, and even affected kernel won't report any error any
> more.

Can I get away with a minimal balance? Or does it need to be a full one?

Best,
Martin

> > Qu Wenruo - 22.09.20, 04:37:01 CEST:
> >> Commit 259ee7754b67 ("btrfs: tree-checker: Add ROOT_ITEM check")
> >> introduced btrfs root item size check, however btrfs root item has
> >> two format, the legacy one which just ends before generation_v2
> >> member, is smaller than current btrfs root item size.
> >> 
> >> This caused btrfs kernel to reject valid but old tree root leaves.
> >> 
> >> Fix this problem by also allowing legacy root item, since kernel
> >> can
> >> already handle them pretty well and upgrade to newer root item
> >> format
> >> when needed.
> >> 
> >> Reported-by: Martin Steigerwald <martin@lichtvoll.de>
> >> Fixes: 259ee7754b67 ("btrfs: tree-checker: Add ROOT_ITEM check")
> >> Signed-off-by: Qu Wenruo <wqu@suse.com>
> >> ---
> >> 
> >>  fs/btrfs/tree-checker.c         | 17 ++++++++++++-----
> >>  include/uapi/linux/btrfs_tree.h |  9 +++++++++
> >>  2 files changed, 21 insertions(+), 5 deletions(-)
> >> 
> >> diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c
> >> index 7b1fee630f97..6f794aca48d3 100644
> >> --- a/fs/btrfs/tree-checker.c
> >> +++ b/fs/btrfs/tree-checker.c
> >> @@ -1035,7 +1035,7 @@ static int check_root_item(struct
> >> extent_buffer
> >> *leaf, struct btrfs_key *key, int slot)
> >> 
> >>  {
> >>  
> >>  	struct btrfs_fs_info *fs_info = leaf->fs_info;
> >> 
> >> -	struct btrfs_root_item ri;
> >> +	struct btrfs_root_item ri = { 0 };
> >> 
> >>  	const u64 valid_root_flags = BTRFS_ROOT_SUBVOL_RDONLY |
> >>  	
> >>  				     BTRFS_ROOT_SUBVOL_DEAD;
> >>  	
> >>  	int ret;
> >> 
> >> @@ -1044,14 +1044,21 @@ static int check_root_item(struct
> >> extent_buffer *leaf, struct btrfs_key *key, if (ret < 0)
> >> 
> >>  		return ret;
> >> 
> >> -	if (btrfs_item_size_nr(leaf, slot) != sizeof(ri)) {
> >> +	if (btrfs_item_size_nr(leaf, slot) != sizeof(ri) &&
> >> +	    btrfs_item_size_nr(leaf, slot) !=
> > 
> > btrfs_legacy_root_item_size())
> > 
> >> { generic_err(leaf, slot,
> >> -			    "invalid root item size, have %u expect %zu",
> >> -			    btrfs_item_size_nr(leaf, slot), sizeof(ri));
> >> +			    "invalid root item size, have %u expect %zu or
> > 
> > %zu",
> > 
> >> +			    btrfs_item_size_nr(leaf, slot), sizeof(ri),
> >> +			    btrfs_legacy_root_item_size());
> >> 
> >>  	}
> >> 
> >> +	/*
> >> +	 * For legacy root item, the members starting at generation_v2
> > 
> > will
> > 
> >> be +	 * all filled with 0.
> >> +	 * And since we allow geneartion_v2 as 0, it will still pass the
> >> check. +	 */
> >> 
> >>  	read_extent_buffer(leaf, &ri, btrfs_item_ptr_offset(leaf, slot),
> >> 
> >> -			   sizeof(ri));
> >> +			   btrfs_item_size_nr(leaf, slot));
> >> 
> >>  	/* Generation related */
> >>  	if (btrfs_root_generation(&ri) >
> >> 
> >> diff --git a/include/uapi/linux/btrfs_tree.h
> >> b/include/uapi/linux/btrfs_tree.h index 9ba64ca6b4ac..464095a28b18
> >> 100644
> >> --- a/include/uapi/linux/btrfs_tree.h
> >> +++ b/include/uapi/linux/btrfs_tree.h
> >> @@ -644,6 +644,15 @@ struct btrfs_root_item {
> >> 
> >>  	__le64 reserved[8]; /* for future */
> >>  
> >>  } __attribute__ ((__packed__));
> >> 
> >> +/*
> >> + * Btrfs root item used to be smaller than current size.
> >> + * The old format ends at where member generation_v2 is.
> >> + */
> >> +static inline size_t btrfs_legacy_root_item_size(void)
> >> +{
> >> +	return offsetof(struct btrfs_root_item, generation_v2);
> >> +}
> >> +
> >> 
> >>  /*
> >>  
> >>   * this is used for both forward and backward root refs
> >>   */


-- 
Martin

Re: [PATCH] btrfs: fix false alert caused by legacy btrfs root item

[Qu Wenruo]

On 2020/9/22 下午11:48, Martin Steigerwald wrote:
> Qu Wenruo - 22.09.20, 12:34:18 CEST:
>> On 2020/9/22 下午6:20, Martin Steigerwald wrote:
>>> Instead of the tool, can I also patch my kernel with the patch below
>>> to have it automatically fix it?
>>
>> Sure, this one is a little safer than the tool.
> 
> Thanks.
> 
>>> If so, which approach would you prefer for testing?
>>>
>>> I can apply the patch as I compile kernels myself.
>>
>> That's great.
>>
>> That should solve the problem.
>>
>> And if you don't like the legacy root item, just do a balance (no
>> matter data or metadata), and that legacy root item will be converted
>> to current one, and even affected kernel won't report any error any
>> more.
> 
> Can I get away with a minimal balance? Or does it need to be a full one?

Minimal is enough.
You just need to balance one chunk.

You can confirm it with "btrfs ins dump-tree -t root <device>".
If DATA_RELOC_TREE item size is still 249, it's legacy one.
If it's 429, then it's the current one.

Thanks,
Qu
> 
> Best,
> Martin
> 
>>> Qu Wenruo - 22.09.20, 04:37:01 CEST:
>>>> Commit 259ee7754b67 ("btrfs: tree-checker: Add ROOT_ITEM check")
>>>> introduced btrfs root item size check, however btrfs root item has
>>>> two format, the legacy one which just ends before generation_v2
>>>> member, is smaller than current btrfs root item size.
>>>>
>>>> This caused btrfs kernel to reject valid but old tree root leaves.
>>>>
>>>> Fix this problem by also allowing legacy root item, since kernel
>>>> can
>>>> already handle them pretty well and upgrade to newer root item
>>>> format
>>>> when needed.
>>>>
>>>> Reported-by: Martin Steigerwald <martin@lichtvoll.de>
>>>> Fixes: 259ee7754b67 ("btrfs: tree-checker: Add ROOT_ITEM check")
>>>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>>>> ---
>>>>
>>>>  fs/btrfs/tree-checker.c         | 17 ++++++++++++-----
>>>>  include/uapi/linux/btrfs_tree.h |  9 +++++++++
>>>>  2 files changed, 21 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c
>>>> index 7b1fee630f97..6f794aca48d3 100644
>>>> --- a/fs/btrfs/tree-checker.c
>>>> +++ b/fs/btrfs/tree-checker.c
>>>> @@ -1035,7 +1035,7 @@ static int check_root_item(struct
>>>> extent_buffer
>>>> *leaf, struct btrfs_key *key, int slot)
>>>>
>>>>  {
>>>>  
>>>>  	struct btrfs_fs_info *fs_info = leaf->fs_info;
>>>>
>>>> -	struct btrfs_root_item ri;
>>>> +	struct btrfs_root_item ri = { 0 };
>>>>
>>>>  	const u64 valid_root_flags = BTRFS_ROOT_SUBVOL_RDONLY |
>>>>  	
>>>>  				     BTRFS_ROOT_SUBVOL_DEAD;
>>>>  	
>>>>  	int ret;
>>>>
>>>> @@ -1044,14 +1044,21 @@ static int check_root_item(struct
>>>> extent_buffer *leaf, struct btrfs_key *key, if (ret < 0)
>>>>
>>>>  		return ret;
>>>>
>>>> -	if (btrfs_item_size_nr(leaf, slot) != sizeof(ri)) {
>>>> +	if (btrfs_item_size_nr(leaf, slot) != sizeof(ri) &&
>>>> +	    btrfs_item_size_nr(leaf, slot) !=
>>>
>>> btrfs_legacy_root_item_size())
>>>
>>>> { generic_err(leaf, slot,
>>>> -			    "invalid root item size, have %u expect %zu",
>>>> -			    btrfs_item_size_nr(leaf, slot), sizeof(ri));
>>>> +			    "invalid root item size, have %u expect %zu or
>>>
>>> %zu",
>>>
>>>> +			    btrfs_item_size_nr(leaf, slot), sizeof(ri),
>>>> +			    btrfs_legacy_root_item_size());
>>>>
>>>>  	}
>>>>
>>>> +	/*
>>>> +	 * For legacy root item, the members starting at generation_v2
>>>
>>> will
>>>
>>>> be +	 * all filled with 0.
>>>> +	 * And since we allow geneartion_v2 as 0, it will still pass the
>>>> check. +	 */
>>>>
>>>>  	read_extent_buffer(leaf, &ri, btrfs_item_ptr_offset(leaf, slot),
>>>>
>>>> -			   sizeof(ri));
>>>> +			   btrfs_item_size_nr(leaf, slot));
>>>>
>>>>  	/* Generation related */
>>>>  	if (btrfs_root_generation(&ri) >
>>>>
>>>> diff --git a/include/uapi/linux/btrfs_tree.h
>>>> b/include/uapi/linux/btrfs_tree.h index 9ba64ca6b4ac..464095a28b18
>>>> 100644
>>>> --- a/include/uapi/linux/btrfs_tree.h
>>>> +++ b/include/uapi/linux/btrfs_tree.h
>>>> @@ -644,6 +644,15 @@ struct btrfs_root_item {
>>>>
>>>>  	__le64 reserved[8]; /* for future */
>>>>  
>>>>  } __attribute__ ((__packed__));
>>>>
>>>> +/*
>>>> + * Btrfs root item used to be smaller than current size.
>>>> + * The old format ends at where member generation_v2 is.
>>>> + */
>>>> +static inline size_t btrfs_legacy_root_item_size(void)
>>>> +{
>>>> +	return offsetof(struct btrfs_root_item, generation_v2);
>>>> +}
>>>> +
>>>>
>>>>  /*
>>>>  
>>>>   * this is used for both forward and backward root refs
>>>>   */
> 
> 

Re: [PATCH] btrfs: fix false alert caused by legacy btrfs root item

[Martin Steigerwald]

Qu Wenruo - 23.09.20, 01:17:01 CEST:
> On 2020/9/22 下午11:48, Martin Steigerwald wrote:
> > Qu Wenruo - 22.09.20, 12:34:18 CEST:
> >> On 2020/9/22 下午6:20, Martin Steigerwald wrote:
> >>> Instead of the tool, can I also patch my kernel with the patch
> >>> below
> >>> to have it automatically fix it?
> >> 
> >> Sure, this one is a little safer than the tool.
[…]
> >>> If so, which approach would you prefer for testing?
> >>> I can apply the patch as I compile kernels myself.
> >> 
> >> That's great.
> >> 
> >> That should solve the problem.
> >> 
> >> And if you don't like the legacy root item, just do a balance (no
> >> matter data or metadata), and that legacy root item will be
> >> converted to current one, and even affected kernel won't report
> >> any error any more.
> > 
> > Can I get away with a minimal balance? Or does it need to be a full
> > one?
> Minimal is enough.
> You just need to balance one chunk.
> 
> You can confirm it with "btrfs ins dump-tree -t root <device>".
> If DATA_RELOC_TREE item size is still 249, it's legacy one.
> If it's 429, then it's the current one.

Hmmm its 439. So is that good as well?

        item 178 key (DATA_RELOC_TREE ROOT_ITEM 0) itemoff 8546 itemsize 439
                generation 13246 root_dirid 256 bytenr 896876544 level 0 refs 1
                lastsnap 0 byte_limit 0 bytes_used 16384 flags 0x0(none)
                uuid […]
                drop key (0 UNKNOWN.0 0) level 0

What is this tree used for by the way?

If you like I can test with unpatched kernel whether warning goes away, but
I bet it may not be needed.

[…]
-- 
Martin

Re: [PATCH] btrfs: fix false alert caused by legacy btrfs root item

[Qu Wenruo]

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



On 2020/9/24 上午3:41, Martin Steigerwald wrote:
> Qu Wenruo - 23.09.20, 01:17:01 CEST:
>> On 2020/9/22 下午11:48, Martin Steigerwald wrote:
>>> Qu Wenruo - 22.09.20, 12:34:18 CEST:
>>>> On 2020/9/22 下午6:20, Martin Steigerwald wrote:
>>>>> Instead of the tool, can I also patch my kernel with the patch
>>>>> below
>>>>> to have it automatically fix it?
>>>>
>>>> Sure, this one is a little safer than the tool.
> […]
>>>>> If so, which approach would you prefer for testing?
>>>>> I can apply the patch as I compile kernels myself.
>>>>
>>>> That's great.
>>>>
>>>> That should solve the problem.
>>>>
>>>> And if you don't like the legacy root item, just do a balance (no
>>>> matter data or metadata), and that legacy root item will be
>>>> converted to current one, and even affected kernel won't report
>>>> any error any more.
>>>
>>> Can I get away with a minimal balance? Or does it need to be a full
>>> one?
>> Minimal is enough.
>> You just need to balance one chunk.
>>
>> You can confirm it with "btrfs ins dump-tree -t root <device>".
>> If DATA_RELOC_TREE item size is still 249, it's legacy one.
>> If it's 429, then it's the current one.
> 
> Hmmm its 439. So is that good as well?

Sorry, 439 is the correct size.

> 
>         item 178 key (DATA_RELOC_TREE ROOT_ITEM 0) itemoff 8546 itemsize 439
>                 generation 13246 root_dirid 256 bytenr 896876544 level 0 refs 1
>                 lastsnap 0 byte_limit 0 bytes_used 16384 flags 0x0(none)
>                 uuid […]
>                 drop key (0 UNKNOWN.0 0) level 0
> 
> What is this tree used for by the way?

Used as a tree storing the new relocated chunk data.

> 
> If you like I can test with unpatched kernel whether warning goes away, but
> I bet it may not be needed.

It should be safe now.

Thanks,
Qu

> 
> […]
> 


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

Re: [PATCH] btrfs: fix false alert caused by legacy btrfs root item

[Martin Steigerwald]

Qu Wenruo - 24.09.20, 02:07:01 CEST:
> > If you like I can test with unpatched kernel whether warning goes
> > away, but I bet it may not be needed.
> 
> It should be safe now.

Thanks!

-- 
Martin

Re: [PATCH] btrfs: fix false alert caused by legacy btrfs root item

[Martin Steigerwald]

Qu Wenruo - 22.09.20, 12:34:18 CEST:
> On 2020/9/22 下午6:20, Martin Steigerwald wrote:
> > Instead of the tool, can I also patch my kernel with the patch below
> > to have it automatically fix it?
> 
> Sure, this one is a little safer than the tool.
> 
> > If so, which approach would you prefer for testing?
> > 
> > I can apply the patch as I compile kernels myself.
> 
> That's great.
> 
> That should solve the problem.
> 
> And if you don't like the legacy root item, just do a balance (no
> matter data or metadata), and that legacy root item will be converted
> to current one, and even affected kernel won't report any error any
> more.

Tested with patch. No error message :)

Tested-By: Martin Steigerwald <martin@lichtvoll.de>

Will do balance once I know whether a minimal one is enough. Can test 
with old unpatched kernel then as well. Both 5.9-rc5, as 5.9-rc6 didn't 
compile for some reason.

Thanks,
Martin

> > Qu Wenruo - 22.09.20, 04:37:01 CEST:
> >> Commit 259ee7754b67 ("btrfs: tree-checker: Add ROOT_ITEM check")
> >> introduced btrfs root item size check, however btrfs root item has
> >> two format, the legacy one which just ends before generation_v2
> >> member, is smaller than current btrfs root item size.
> >> 
> >> This caused btrfs kernel to reject valid but old tree root leaves.
> >> 
> >> Fix this problem by also allowing legacy root item, since kernel
> >> can
> >> already handle them pretty well and upgrade to newer root item
> >> format
> >> when needed.
> >> 
> >> Reported-by: Martin Steigerwald <martin@lichtvoll.de>
> >> Fixes: 259ee7754b67 ("btrfs: tree-checker: Add ROOT_ITEM check")
> >> Signed-off-by: Qu Wenruo <wqu@suse.com>
> >> ---
> >> 
> >>  fs/btrfs/tree-checker.c         | 17 ++++++++++++-----
> >>  include/uapi/linux/btrfs_tree.h |  9 +++++++++
> >>  2 files changed, 21 insertions(+), 5 deletions(-)
> >> 
> >> diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c
> >> index 7b1fee630f97..6f794aca48d3 100644
> >> --- a/fs/btrfs/tree-checker.c
> >> +++ b/fs/btrfs/tree-checker.c
> >> @@ -1035,7 +1035,7 @@ static int check_root_item(struct
> >> extent_buffer
> >> *leaf, struct btrfs_key *key, int slot)
> >> 
> >>  {
> >>  
> >>  	struct btrfs_fs_info *fs_info = leaf->fs_info;
> >> 
> >> -	struct btrfs_root_item ri;
> >> +	struct btrfs_root_item ri = { 0 };
> >> 
> >>  	const u64 valid_root_flags = BTRFS_ROOT_SUBVOL_RDONLY |
> >>  	
> >>  				     BTRFS_ROOT_SUBVOL_DEAD;
> >>  	
> >>  	int ret;
> >> 
> >> @@ -1044,14 +1044,21 @@ static int check_root_item(struct
> >> extent_buffer *leaf, struct btrfs_key *key, if (ret < 0)
> >> 
> >>  		return ret;
> >> 
> >> -	if (btrfs_item_size_nr(leaf, slot) != sizeof(ri)) {
> >> +	if (btrfs_item_size_nr(leaf, slot) != sizeof(ri) &&
> >> +	    btrfs_item_size_nr(leaf, slot) !=
> > 
> > btrfs_legacy_root_item_size())
> > 
> >> { generic_err(leaf, slot,
> >> -			    "invalid root item size, have %u expect %zu",
> >> -			    btrfs_item_size_nr(leaf, slot), sizeof(ri));
> >> +			    "invalid root item size, have %u expect %zu or
> > 
> > %zu",
> > 
> >> +			    btrfs_item_size_nr(leaf, slot), sizeof(ri),
> >> +			    btrfs_legacy_root_item_size());
> >> 
> >>  	}
> >> 
> >> +	/*
> >> +	 * For legacy root item, the members starting at generation_v2
> > 
> > will
> > 
> >> be +	 * all filled with 0.
> >> +	 * And since we allow geneartion_v2 as 0, it will still pass the
> >> check. +	 */
> >> 
> >>  	read_extent_buffer(leaf, &ri, btrfs_item_ptr_offset(leaf, slot),
> >> 
> >> -			   sizeof(ri));
> >> +			   btrfs_item_size_nr(leaf, slot));
> >> 
> >>  	/* Generation related */
> >>  	if (btrfs_root_generation(&ri) >
> >> 
> >> diff --git a/include/uapi/linux/btrfs_tree.h
> >> b/include/uapi/linux/btrfs_tree.h index 9ba64ca6b4ac..464095a28b18
> >> 100644
> >> --- a/include/uapi/linux/btrfs_tree.h
> >> +++ b/include/uapi/linux/btrfs_tree.h
> >> @@ -644,6 +644,15 @@ struct btrfs_root_item {
> >> 
> >>  	__le64 reserved[8]; /* for future */
> >>  
> >>  } __attribute__ ((__packed__));
> >> 
> >> +/*
> >> + * Btrfs root item used to be smaller than current size.
> >> + * The old format ends at where member generation_v2 is.
> >> + */
> >> +static inline size_t btrfs_legacy_root_item_size(void)
> >> +{
> >> +	return offsetof(struct btrfs_root_item, generation_v2);
> >> +}
> >> +
> >> 
> >>  /*
> >>  
> >>   * this is used for both forward and backward root refs
> >>   */


-- 
Martin

Re: [PATCH] btrfs: fix false alert caused by legacy btrfs root item

[Qu Wenruo]

On 2020/9/22 下午7:12, kernel test robot wrote:
> Hi Qu,
> 
> Thank you for the patch! Yet something to improve:
> 
> [auto build test ERROR on kdave/for-next]
> [also build test ERROR on v5.9-rc6 next-20200921]
> [cannot apply to btrfs/next]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch]
> 
> url:    https://github.com/0day-ci/linux/commits/Qu-Wenruo/btrfs-fix-false-alert-caused-by-legacy-btrfs-root-item/20200922-103827
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git for-next
> config: x86_64-randconfig-s021-20200920 (attached as .config)
> compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
> reproduce:
>         # apt-get install sparse
>         # sparse version: v0.6.2-201-g24bdaac6-dirty
>         # save the attached .config to linux build tree
>         make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=x86_64 

Well, with my gcc-10.2.0 and C=1, my sparse just segfault on volumes.c...

And no such offsetof() related reports before it crashes.

I'm wondering if this sparse warning really makes any sense, especially
gcc is not complaining by itself.

Thanks,
Qu


> 
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>
> 
> All errors (new ones prefixed by >>):
> 
>    In file included from <command-line>:32:
>>> ./usr/include/linux/btrfs_tree.h:651:19: error: unknown type name 'size_t'
>      651 | static __inline__ size_t btrfs_legacy_root_item_size(void)
>          |                   ^~~~~~
>    ./usr/include/linux/btrfs_tree.h: In function 'btrfs_legacy_root_item_size':
>>> ./usr/include/linux/btrfs_tree.h:653:9: error: implicit declaration of function 'offsetof' [-Werror=implicit-function-declaration]
>      653 |  return offsetof(struct btrfs_root_item, generation_v2);
>          |         ^~~~~~~~
>    ./usr/include/linux/btrfs_tree.h:6:1: note: 'offsetof' is defined in header '<stddef.h>'; did you forget to '#include <stddef.h>'?
>        5 | #include <linux/btrfs.h>
>      +++ |+#include <stddef.h>
>        6 | #include <linux/types.h>
>>> ./usr/include/linux/btrfs_tree.h:653:18: error: expected expression before 'struct'
>      653 |  return offsetof(struct btrfs_root_item, generation_v2);
>          |                  ^~~~~~
>    cc1: some warnings being treated as errors
> 
> ---
> 0-DAY CI Kernel Test Service, Intel Corporation
> https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
> 

Re: [PATCH] btrfs: fix false alert caused by legacy btrfs root item

[Josef Bacik]

On 9/21/20 10:37 PM, Qu Wenruo wrote:
> Commit 259ee7754b67 ("btrfs: tree-checker: Add ROOT_ITEM check")
> introduced btrfs root item size check, however btrfs root item has two
> format, the legacy one which just ends before generation_v2 member, is
> smaller than current btrfs root item size.
> 
> This caused btrfs kernel to reject valid but old tree root leaves.
> 
> Fix this problem by also allowing legacy root item, since kernel can
> already handle them pretty well and upgrade to newer root item format
> when needed.
> 
> Reported-by: Martin Steigerwald <martin@lichtvoll.de>
> Fixes: 259ee7754b67 ("btrfs: tree-checker: Add ROOT_ITEM check")
> Signed-off-by: Qu Wenruo <wqu@suse.com>

Reviewed-by: Josef Bacik <josef@toxicpanda.com>

Thanks,

Josef

Re: [PATCH] btrfs: fix false alert caused by legacy btrfs root item

[kernel test robot]

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

Hi Qu,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on kdave/for-next]
[also build test ERROR on v5.9-rc6 next-20200922]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Qu-Wenruo/btrfs-fix-false-alert-caused-by-legacy-btrfs-root-item/20200922-103827
base:   https://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git for-next
config: x86_64-randconfig-a014-20200920 (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project 6a6b06f5262bb96523eceef4a42fe8e60ae2a630)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install x86_64 cross compiling tool for clang build
        # apt-get install binutils-x86-64-linux-gnu
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from <built-in>:1:
>> ./usr/include/linux/btrfs_tree.h:651:19: error: unknown type name 'size_t'
   static __inline__ size_t btrfs_legacy_root_item_size(void)
                     ^
>> ./usr/include/linux/btrfs_tree.h:653:9: error: implicit declaration of function 'offsetof' [-Werror,-Wimplicit-function-declaration]
           return offsetof(struct btrfs_root_item, generation_v2);
                  ^
>> ./usr/include/linux/btrfs_tree.h:653:18: error: expected expression
           return offsetof(struct btrfs_root_item, generation_v2);
                           ^
>> ./usr/include/linux/btrfs_tree.h:653:42: error: use of undeclared identifier 'generation_v2'
           return offsetof(struct btrfs_root_item, generation_v2);
                                                   ^
   4 errors generated.

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 33825 bytes --]

Re: [PATCH] btrfs: fix false alert caused by legacy btrfs root item

[David Sterba]

On Wed, Sep 23, 2020 at 02:23:18PM +0800, kernel test robot wrote:
> Hi Qu,
> 
> Thank you for the patch! Yet something to improve:
> 
> [auto build test ERROR on kdave/for-next]
> [also build test ERROR on v5.9-rc6 next-20200922]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch]
> 
> url:    https://github.com/0day-ci/linux/commits/Qu-Wenruo/btrfs-fix-false-alert-caused-by-legacy-btrfs-root-item/20200922-103827
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git for-next
> config: x86_64-randconfig-a014-20200920 (attached as .config)
> compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project 6a6b06f5262bb96523eceef4a42fe8e60ae2a630)
> reproduce (this is a W=1 build):
>         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         # install x86_64 cross compiling tool for clang build
>         # apt-get install binutils-x86-64-linux-gnu
>         # save the attached .config to linux build tree
>         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 
> 
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>
> 
> All errors (new ones prefixed by >>):
> 
>    In file included from <built-in>:1:
> >> ./usr/include/linux/btrfs_tree.h:651:19: error: unknown type name 'size_t'
>    static __inline__ size_t btrfs_legacy_root_item_size(void)
>                      ^

u32 should be fine here, we use it for all the other item helpers.

Re: [PATCH] btrfs: fix false alert caused by legacy btrfs root item

[Qu Wenruo]

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



On 2020/9/23 下午5:31, David Sterba wrote:
> On Wed, Sep 23, 2020 at 02:23:18PM +0800, kernel test robot wrote:
>> Hi Qu,
>>
>> Thank you for the patch! Yet something to improve:
>>
>> [auto build test ERROR on kdave/for-next]
>> [also build test ERROR on v5.9-rc6 next-20200922]
>> [If your patch is applied to the wrong git tree, kindly drop us a note.
>> And when submitting patch, we suggest to use '--base' as documented in
>> https://git-scm.com/docs/git-format-patch]
>>
>> url:    https://github.com/0day-ci/linux/commits/Qu-Wenruo/btrfs-fix-false-alert-caused-by-legacy-btrfs-root-item/20200922-103827
>> base:   https://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git for-next
>> config: x86_64-randconfig-a014-20200920 (attached as .config)
>> compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project 6a6b06f5262bb96523eceef4a42fe8e60ae2a630)
>> reproduce (this is a W=1 build):
>>         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>>         chmod +x ~/bin/make.cross
>>         # install x86_64 cross compiling tool for clang build
>>         # apt-get install binutils-x86-64-linux-gnu
>>         # save the attached .config to linux build tree
>>         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 
>>
>> If you fix the issue, kindly add following tag as appropriate
>> Reported-by: kernel test robot <lkp@intel.com>
>>
>> All errors (new ones prefixed by >>):
>>
>>    In file included from <built-in>:1:
>>>> ./usr/include/linux/btrfs_tree.h:651:19: error: unknown type name 'size_t'
>>    static __inline__ size_t btrfs_legacy_root_item_size(void)
>>                      ^
> 
> u32 should be fine here, we use it for all the other item helpers.
> 
Sure, but I'm a little interested in why it passes in gcc v10.20...

Thanks,
Qu


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

Re: [PATCH] btrfs: fix false alert caused by legacy btrfs root item

[David Sterba]

On Wed, Sep 23, 2020 at 06:28:41PM +0800, Qu Wenruo wrote:
> On 2020/9/23 下午5:31, David Sterba wrote:
> > On Wed, Sep 23, 2020 at 02:23:18PM +0800, kernel test robot wrote:
> >> Hi Qu,
> >>
> >> Thank you for the patch! Yet something to improve:
> >>
> >> [auto build test ERROR on kdave/for-next]
> >> [also build test ERROR on v5.9-rc6 next-20200922]
> >> [If your patch is applied to the wrong git tree, kindly drop us a note.
> >> And when submitting patch, we suggest to use '--base' as documented in
> >> https://git-scm.com/docs/git-format-patch]
> >>
> >> url:    https://github.com/0day-ci/linux/commits/Qu-Wenruo/btrfs-fix-false-alert-caused-by-legacy-btrfs-root-item/20200922-103827
> >> base:   https://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git for-next
> >> config: x86_64-randconfig-a014-20200920 (attached as .config)
> >> compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project 6a6b06f5262bb96523eceef4a42fe8e60ae2a630)
> >> reproduce (this is a W=1 build):
> >>         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> >>         chmod +x ~/bin/make.cross
> >>         # install x86_64 cross compiling tool for clang build
> >>         # apt-get install binutils-x86-64-linux-gnu
> >>         # save the attached .config to linux build tree
> >>         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 
> >>
> >> If you fix the issue, kindly add following tag as appropriate
> >> Reported-by: kernel test robot <lkp@intel.com>
> >>
> >> All errors (new ones prefixed by >>):
> >>
> >>    In file included from <built-in>:1:
> >>>> ./usr/include/linux/btrfs_tree.h:651:19: error: unknown type name 'size_t'
> >>    static __inline__ size_t btrfs_legacy_root_item_size(void)
> >>                      ^
> > 
> > u32 should be fine here, we use it for all the other item helpers.
> > 
> Sure, but I'm a little interested in why it passes in gcc v10.20...

It depends on the config options. It passed for me too, but Josef
noticed it does not build and pointed to the CONFIG_UAPI_HEADER_TEST
and CONFIG_HEADERS_INSTALL options.

This is a valid report and needs to be fixed, I looked around to the
UAPI headers that use offset of how this is solved and it's enough to
include linux/stddef.h (or stddef.h for non-kernel build). Updated
misc-next pushed. Thanks.

Re: [PATCH] btrfs: fix false alert caused by legacy btrfs root item

[David Sterba]

On Tue, Sep 22, 2020 at 10:37:01AM +0800, Qu Wenruo wrote:
> Commit 259ee7754b67 ("btrfs: tree-checker: Add ROOT_ITEM check")
> introduced btrfs root item size check, however btrfs root item has two
> format, the legacy one which just ends before generation_v2 member, is
> smaller than current btrfs root item size.
> 
> This caused btrfs kernel to reject valid but old tree root leaves.
> 
> Fix this problem by also allowing legacy root item, since kernel can
> already handle them pretty well and upgrade to newer root item format
> when needed.
> 
> Reported-by: Martin Steigerwald <martin@lichtvoll.de>
> Fixes: 259ee7754b67 ("btrfs: tree-checker: Add ROOT_ITEM check")
> Signed-off-by: Qu Wenruo <wqu@suse.com>

Added to misc-next, thanks.

Re: [PATCH] btrfs: fix false alert caused by legacy btrfs root item

[Martin Steigerwald]

Hi Qu!

Qu Wenruo - 22.09.20, 04:37:01 CEST:
> Commit 259ee7754b67 ("btrfs: tree-checker: Add ROOT_ITEM check")
> introduced btrfs root item size check, however btrfs root item has two
> format, the legacy one which just ends before generation_v2 member,
> is smaller than current btrfs root item size.
>
> This caused btrfs kernel to reject valid but old tree root leaves.
> 
> Fix this problem by also allowing legacy root item, since kernel can
> already handle them pretty well and upgrade to newer root item format
> when needed.

Is this going into 5.9? Asking cause it is not in 5.9-rc8.

Of course I can keep the patch and as the external disk has been fixed, I 
would not even need it anymore.

Best,
Martin

> Reported-by: Martin Steigerwald <martin@lichtvoll.de>
> Fixes: 259ee7754b67 ("btrfs: tree-checker: Add ROOT_ITEM check")
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  fs/btrfs/tree-checker.c         | 17 ++++++++++++-----
>  include/uapi/linux/btrfs_tree.h |  9 +++++++++
>  2 files changed, 21 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c
> index 7b1fee630f97..6f794aca48d3 100644
> --- a/fs/btrfs/tree-checker.c
> +++ b/fs/btrfs/tree-checker.c
> @@ -1035,7 +1035,7 @@ static int check_root_item(struct extent_buffer
> *leaf, struct btrfs_key *key, int slot)
>  {
>  	struct btrfs_fs_info *fs_info = leaf->fs_info;
> -	struct btrfs_root_item ri;
> +	struct btrfs_root_item ri = { 0 };
>  	const u64 valid_root_flags = BTRFS_ROOT_SUBVOL_RDONLY |
>  				     BTRFS_ROOT_SUBVOL_DEAD;
>  	int ret;
> @@ -1044,14 +1044,21 @@ static int check_root_item(struct
> extent_buffer *leaf, struct btrfs_key *key, if (ret < 0)
>  		return ret;
> 
> -	if (btrfs_item_size_nr(leaf, slot) != sizeof(ri)) {
> +	if (btrfs_item_size_nr(leaf, slot) != sizeof(ri) &&
> +	    btrfs_item_size_nr(leaf, slot) != 
btrfs_legacy_root_item_size())
> { generic_err(leaf, slot,
> -			    "invalid root item size, have %u expect %zu",
> -			    btrfs_item_size_nr(leaf, slot), sizeof(ri));
> +			    "invalid root item size, have %u expect %zu or 
%zu",
> +			    btrfs_item_size_nr(leaf, slot), sizeof(ri),
> +			    btrfs_legacy_root_item_size());
>  	}
> 
> +	/*
> +	 * For legacy root item, the members starting at generation_v2 
will
> be +	 * all filled with 0.
> +	 * And since we allow geneartion_v2 as 0, it will still pass the
> check. +	 */
>  	read_extent_buffer(leaf, &ri, btrfs_item_ptr_offset(leaf, slot),
> -			   sizeof(ri));
> +			   btrfs_item_size_nr(leaf, slot));
> 
>  	/* Generation related */
>  	if (btrfs_root_generation(&ri) >
> diff --git a/include/uapi/linux/btrfs_tree.h
> b/include/uapi/linux/btrfs_tree.h index 9ba64ca6b4ac..464095a28b18
> 100644
> --- a/include/uapi/linux/btrfs_tree.h
> +++ b/include/uapi/linux/btrfs_tree.h
> @@ -644,6 +644,15 @@ struct btrfs_root_item {
>  	__le64 reserved[8]; /* for future */
>  } __attribute__ ((__packed__));
> 
> +/*
> + * Btrfs root item used to be smaller than current size.
> + * The old format ends at where member generation_v2 is.
> + */
> +static inline size_t btrfs_legacy_root_item_size(void)
> +{
> +	return offsetof(struct btrfs_root_item, generation_v2);
> +}
> +
>  /*
>   * this is used for both forward and backward root refs
>   */


-- 
Martin

Re: [PATCH] btrfs: fix false alert caused by legacy btrfs root item

[Qu Wenruo]

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



On 2020/10/5 下午11:29, Martin Steigerwald wrote:
> Hi Qu!
> 
> Qu Wenruo - 22.09.20, 04:37:01 CEST:
>> Commit 259ee7754b67 ("btrfs: tree-checker: Add ROOT_ITEM check")
>> introduced btrfs root item size check, however btrfs root item has two
>> format, the legacy one which just ends before generation_v2 member,
>> is smaller than current btrfs root item size.
>>
>> This caused btrfs kernel to reject valid but old tree root leaves.
>>
>> Fix this problem by also allowing legacy root item, since kernel can
>> already handle them pretty well and upgrade to newer root item format
>> when needed.
> 
> Is this going into 5.9? Asking cause it is not in 5.9-rc8.

David has the final say on whether to go into a late rc.

But it's already in David's misc-next branch, which means v5.10 would
have it at least.

> 
> Of course I can keep the patch and as the external disk has been fixed, I 
> would not even need it anymore.

That's true, so you don't need to bother the problem any more.

Thanks,
Qu

> 
> Best,
> Martin
> 
>> Reported-by: Martin Steigerwald <martin@lichtvoll.de>
>> Fixes: 259ee7754b67 ("btrfs: tree-checker: Add ROOT_ITEM check")
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>  fs/btrfs/tree-checker.c         | 17 ++++++++++++-----
>>  include/uapi/linux/btrfs_tree.h |  9 +++++++++
>>  2 files changed, 21 insertions(+), 5 deletions(-)
>>
>> diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c
>> index 7b1fee630f97..6f794aca48d3 100644
>> --- a/fs/btrfs/tree-checker.c
>> +++ b/fs/btrfs/tree-checker.c
>> @@ -1035,7 +1035,7 @@ static int check_root_item(struct extent_buffer
>> *leaf, struct btrfs_key *key, int slot)
>>  {
>>  	struct btrfs_fs_info *fs_info = leaf->fs_info;
>> -	struct btrfs_root_item ri;
>> +	struct btrfs_root_item ri = { 0 };
>>  	const u64 valid_root_flags = BTRFS_ROOT_SUBVOL_RDONLY |
>>  				     BTRFS_ROOT_SUBVOL_DEAD;
>>  	int ret;
>> @@ -1044,14 +1044,21 @@ static int check_root_item(struct
>> extent_buffer *leaf, struct btrfs_key *key, if (ret < 0)
>>  		return ret;
>>
>> -	if (btrfs_item_size_nr(leaf, slot) != sizeof(ri)) {
>> +	if (btrfs_item_size_nr(leaf, slot) != sizeof(ri) &&
>> +	    btrfs_item_size_nr(leaf, slot) != 
> btrfs_legacy_root_item_size())
>> { generic_err(leaf, slot,
>> -			    "invalid root item size, have %u expect %zu",
>> -			    btrfs_item_size_nr(leaf, slot), sizeof(ri));
>> +			    "invalid root item size, have %u expect %zu or 
> %zu",
>> +			    btrfs_item_size_nr(leaf, slot), sizeof(ri),
>> +			    btrfs_legacy_root_item_size());
>>  	}
>>
>> +	/*
>> +	 * For legacy root item, the members starting at generation_v2 
> will
>> be +	 * all filled with 0.
>> +	 * And since we allow geneartion_v2 as 0, it will still pass the
>> check. +	 */
>>  	read_extent_buffer(leaf, &ri, btrfs_item_ptr_offset(leaf, slot),
>> -			   sizeof(ri));
>> +			   btrfs_item_size_nr(leaf, slot));
>>
>>  	/* Generation related */
>>  	if (btrfs_root_generation(&ri) >
>> diff --git a/include/uapi/linux/btrfs_tree.h
>> b/include/uapi/linux/btrfs_tree.h index 9ba64ca6b4ac..464095a28b18
>> 100644
>> --- a/include/uapi/linux/btrfs_tree.h
>> +++ b/include/uapi/linux/btrfs_tree.h
>> @@ -644,6 +644,15 @@ struct btrfs_root_item {
>>  	__le64 reserved[8]; /* for future */
>>  } __attribute__ ((__packed__));
>>
>> +/*
>> + * Btrfs root item used to be smaller than current size.
>> + * The old format ends at where member generation_v2 is.
>> + */
>> +static inline size_t btrfs_legacy_root_item_size(void)
>> +{
>> +	return offsetof(struct btrfs_root_item, generation_v2);
>> +}
>> +
>>  /*
>>   * this is used for both forward and backward root refs
>>   */
> 
> 


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