[PATCH] btrfs-progs: fsck: Fix a false metadata extent warning

[PATCH] btrfs-progs: fsck: Fix a false metadata extent warning

[Qu Wenruo]

At least 2 user from mail list reported btrfsck reported false alert of
"bad metadata [XXXX,YYYY) crossing stripe boundary".

While the reported number are all inside the same 64K boundary.
After some check, all the false alert have the same bytenr feature,
which can be divided by stripe size (64K).

The result seems to be initial 'max_size' can be 0, causing 'start' +
'max_size' - 1, to cross the stripe boundary.

Fix it by always update extent_record->cross_stripe when the
extent_record is updated, to avoid temporary false alert to be reported.

Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
---
 cmds-check.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/cmds-check.c b/cmds-check.c
index d157075..ef23ddb 100644
--- a/cmds-check.c
+++ b/cmds-check.c
@@ -4579,9 +4579,9 @@ static int add_extent_rec(struct cache_tree *extent_cache,
 		 * As now stripe_len is fixed to BTRFS_STRIPE_LEN, just check
 		 * it.
 		 */
-		if (metadata && check_crossing_stripes(rec->start,
-						       rec->max_size))
-				rec->crossing_stripes = 1;
+		if (metadata)
+			rec->crossing_stripes = check_crossing_stripes(
+					rec->start, rec->max_size);
 		check_extent_type(rec);
 		maybe_free_extent_rec(extent_cache, rec);
 		return ret;
@@ -4641,8 +4641,8 @@ static int add_extent_rec(struct cache_tree *extent_cache,
 	}
 
 	if (metadata)
-		if (check_crossing_stripes(rec->start, rec->max_size))
-			rec->crossing_stripes = 1;
+		rec->crossing_stripes = check_crossing_stripes(rec->start,
+				rec->max_size);
 	check_extent_type(rec);
 	return ret;
 }
-- 
2.7.4

Re: [PATCH] btrfs-progs: fsck: Fix a false metadata extent warning

[David Sterba]

On Thu, Mar 31, 2016 at 10:19:34AM +0800, Qu Wenruo wrote:
> At least 2 user from mail list reported btrfsck reported false alert of
> "bad metadata [XXXX,YYYY) crossing stripe boundary".
> 
> While the reported number are all inside the same 64K boundary.
> After some check, all the false alert have the same bytenr feature,
> which can be divided by stripe size (64K).
> 
> The result seems to be initial 'max_size' can be 0, causing 'start' +
> 'max_size' - 1, to cross the stripe boundary.
> 
> Fix it by always update extent_record->cross_stripe when the
> extent_record is updated, to avoid temporary false alert to be reported.
> 
> Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>

Applied, thanks.

Do you have a test image for that?

Re: [PATCH] btrfs-progs: fsck: Fix a false metadata extent warning

[Qu Wenruo]

David Sterba wrote on 2016/03/31 18:30 +0200:
> On Thu, Mar 31, 2016 at 10:19:34AM +0800, Qu Wenruo wrote:
>> At least 2 user from mail list reported btrfsck reported false alert of
>> "bad metadata [XXXX,YYYY) crossing stripe boundary".
>>
>> While the reported number are all inside the same 64K boundary.
>> After some check, all the false alert have the same bytenr feature,
>> which can be divided by stripe size (64K).
>>
>> The result seems to be initial 'max_size' can be 0, causing 'start' +
>> 'max_size' - 1, to cross the stripe boundary.
>>
>> Fix it by always update extent_record->cross_stripe when the
>> extent_record is updated, to avoid temporary false alert to be reported.
>>
>> Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
>
> Applied, thanks.
>
> Do you have a test image for that?
>
>
Unfortunately, no.

Although I figured out the cause the the false alert, I still didn't 
find a image/method to reproduce it, except the images of reporters.

I can dig a little further trying to make a image.

Thanks,
Qu

Re: [PATCH] btrfs-progs: fsck: Fix a false metadata extent warning

[David Sterba]

On Fri, Apr 01, 2016 at 08:28:18AM +0800, Qu Wenruo wrote:
> 
> 
> David Sterba wrote on 2016/03/31 18:30 +0200:
> > On Thu, Mar 31, 2016 at 10:19:34AM +0800, Qu Wenruo wrote:
> >> At least 2 user from mail list reported btrfsck reported false alert of
> >> "bad metadata [XXXX,YYYY) crossing stripe boundary".
> >>
> >> While the reported number are all inside the same 64K boundary.
> >> After some check, all the false alert have the same bytenr feature,
> >> which can be divided by stripe size (64K).
> >>
> >> The result seems to be initial 'max_size' can be 0, causing 'start' +
> >> 'max_size' - 1, to cross the stripe boundary.
> >>
> >> Fix it by always update extent_record->cross_stripe when the
> >> extent_record is updated, to avoid temporary false alert to be reported.
> >>
> >> Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
> >
> > Applied, thanks.
> >
> > Do you have a test image for that?
> >
> >
> Unfortunately, no.
> 
> Although I figured out the cause the the false alert, I still didn't 
> find a image/method to reproduce it, except the images of reporters.
> 
> I can dig a little further trying to make a image.

After another look, why don't we use nodesize directly? Or stripesize
where applies. With max_size == 0 the test does not make sense, we ought
to know the alignment.

Re: [PATCH] btrfs-progs: fsck: Fix a false metadata extent warning

[Qu Wenruo]

David Sterba wrote on 2016/04/01 10:44 +0200:
> On Fri, Apr 01, 2016 at 08:28:18AM +0800, Qu Wenruo wrote:
>>
>>
>> David Sterba wrote on 2016/03/31 18:30 +0200:
>>> On Thu, Mar 31, 2016 at 10:19:34AM +0800, Qu Wenruo wrote:
>>>> At least 2 user from mail list reported btrfsck reported false alert of
>>>> "bad metadata [XXXX,YYYY) crossing stripe boundary".
>>>>
>>>> While the reported number are all inside the same 64K boundary.
>>>> After some check, all the false alert have the same bytenr feature,
>>>> which can be divided by stripe size (64K).
>>>>
>>>> The result seems to be initial 'max_size' can be 0, causing 'start' +
>>>> 'max_size' - 1, to cross the stripe boundary.
>>>>
>>>> Fix it by always update extent_record->cross_stripe when the
>>>> extent_record is updated, to avoid temporary false alert to be reported.
>>>>
>>>> Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
>>>
>>> Applied, thanks.
>>>
>>> Do you have a test image for that?
>>>
>>>
>> Unfortunately, no.
>>
>> Although I figured out the cause the the false alert, I still didn't
>> find a image/method to reproduce it, except the images of reporters.
>>
>> I can dig a little further trying to make a image.
>
> After another look, why don't we use nodesize directly? Or stripesize
> where applies. With max_size == 0 the test does not make sense, we ought
> to know the alignment.
>
>
Yes, my first though is also to use nodesize directly, which should be 
always correct.

But the problem is, the related function call stack doesn't have any 
member to reach btrfs_root or btrfs_fs_info.

In the very beginning version of such crossing stripe check, I used to 
add a btrfs_root/btrfs_fs_info parameter to the function.

But the code change are too many, so I use 'max_size'.

I can try to re-do such modification, but IIRC it didn't cause a good 
result.

Thanks,
Qu

Re: [PATCH] btrfs-progs: fsck: Fix a false metadata extent warning

[David Sterba]

On Fri, Apr 01, 2016 at 04:50:06PM +0800, Qu Wenruo wrote:
> > After another look, why don't we use nodesize directly? Or stripesize
> > where applies. With max_size == 0 the test does not make sense, we ought
> > to know the alignment.
> >
> Yes, my first though is also to use nodesize directly, which should be 
> always correct.
> 
> But the problem is, the related function call stack doesn't have any 
> member to reach btrfs_root or btrfs_fs_info.
> 
> In the very beginning version of such crossing stripe check, I used to 
> add a btrfs_root/btrfs_fs_info parameter to the function.
> 
> But the code change are too many, so I use 'max_size'.
> 
> I can try to re-do such modification, but IIRC it didn't cause a good 
> result.

Yes it would require refactoring, which would be good on itself, because
add_extent_rec takes 12(!) parameters. Some of its callers would need to
be updated, but it seems doable.

Re: [PATCH] btrfs-progs: fsck: Fix a false metadata extent warning

[Qu Wenruo]

On 04/01/2016 07:39 PM, David Sterba wrote:
> On Fri, Apr 01, 2016 at 04:50:06PM +0800, Qu Wenruo wrote:
>>> After another look, why don't we use nodesize directly? Or stripesize
>>> where applies. With max_size == 0 the test does not make sense, we ought
>>> to know the alignment.
>>>
>> Yes, my first though is also to use nodesize directly, which should be
>> always correct.
>>
>> But the problem is, the related function call stack doesn't have any
>> member to reach btrfs_root or btrfs_fs_info.
>>
>> In the very beginning version of such crossing stripe check, I used to
>> add a btrfs_root/btrfs_fs_info parameter to the function.
>>
>> But the code change are too many, so I use 'max_size'.
>>
>> I can try to re-do such modification, but IIRC it didn't cause a good
>> result.
>
> Yes it would require refactoring, which would be good on itself, because
> add_extent_rec takes 12(!) parameters. Some of its callers would need to
> be updated, but it seems doable.

I'll try to refactor.
I though current extent-tree rework would change all these mess, but 
considering the case of btrfs-convert, I'd better refactor current code 
other than waiting other reviewers to appear.

Thanks,
Qu

> --
> 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-progs: fsck: Fix a false metadata extent warning

[David Sterba]

On Fri, Apr 01, 2016 at 08:09:56PM +0800, Qu Wenruo wrote:
> 
> 
> On 04/01/2016 07:39 PM, David Sterba wrote:
> > On Fri, Apr 01, 2016 at 04:50:06PM +0800, Qu Wenruo wrote:
> >>> After another look, why don't we use nodesize directly? Or stripesize
> >>> where applies. With max_size == 0 the test does not make sense, we ought
> >>> to know the alignment.
> >>>
> >> Yes, my first though is also to use nodesize directly, which should be
> >> always correct.
> >>
> >> But the problem is, the related function call stack doesn't have any
> >> member to reach btrfs_root or btrfs_fs_info.
> >>
> >> In the very beginning version of such crossing stripe check, I used to
> >> add a btrfs_root/btrfs_fs_info parameter to the function.
> >>
> >> But the code change are too many, so I use 'max_size'.
> >>
> >> I can try to re-do such modification, but IIRC it didn't cause a good
> >> result.
> >
> > Yes it would require refactoring, which would be good on itself, because
> > add_extent_rec takes 12(!) parameters. Some of its callers would need to
> > be updated, but it seems doable.
> 
> I'll try to refactor.

I'm working on it.

Re: [PATCH] btrfs-progs: fsck: Fix a false metadata extent warning

[David Sterba]

On Fri, Apr 01, 2016 at 04:50:06PM +0800, Qu Wenruo wrote:
> > After another look, why don't we use nodesize directly? Or stripesize
> > where applies. With max_size == 0 the test does not make sense, we ought
> > to know the alignment.
> >
> >
> Yes, my first though is also to use nodesize directly, which should be 
> always correct.
> 
> But the problem is, the related function call stack doesn't have any 
> member to reach btrfs_root or btrfs_fs_info.

JFYI, there's global_info avalaible, so it's not necessary to pass
fs_info down the callstacks.

Re: [PATCH] btrfs-progs: fsck: Fix a false metadata extent warning

[Qu Wenruo]

David Sterba wrote on 2016/04/04 13:18 +0200:
> On Fri, Apr 01, 2016 at 04:50:06PM +0800, Qu Wenruo wrote:
>>> After another look, why don't we use nodesize directly? Or stripesize
>>> where applies. With max_size == 0 the test does not make sense, we ought
>>> to know the alignment.
>>>
>>>
>> Yes, my first though is also to use nodesize directly, which should be
>> always correct.
>>
>> But the problem is, the related function call stack doesn't have any
>> member to reach btrfs_root or btrfs_fs_info.
>
> JFYI, there's global_info avalaible, so it's not necessary to pass
> fs_info down the callstacks.
>
>
Oh, that's a good news.

Do I need to re-submit the patch to use fs_info->tree_root->nodesize to 
avoid false alert?
Or wait for your refactor?

Thanks,
Qu

Re: [PATCH] btrfs-progs: fsck: Fix a false metadata extent warning

[David Sterba]

On Tue, Apr 05, 2016 at 09:28:31AM +0800, Qu Wenruo wrote:
> 
> 
> David Sterba wrote on 2016/04/04 13:18 +0200:
> > On Fri, Apr 01, 2016 at 04:50:06PM +0800, Qu Wenruo wrote:
> >>> After another look, why don't we use nodesize directly? Or stripesize
> >>> where applies. With max_size == 0 the test does not make sense, we ought
> >>> to know the alignment.
> >>>
> >>>
> >> Yes, my first though is also to use nodesize directly, which should be
> >> always correct.
> >>
> >> But the problem is, the related function call stack doesn't have any
> >> member to reach btrfs_root or btrfs_fs_info.
> >
> > JFYI, there's global_info avalaible, so it's not necessary to pass
> > fs_info down the callstacks.
> >
> >
> Oh, that's a good news.
> 
> Do I need to re-submit the patch to use fs_info->tree_root->nodesize to 
> avoid false alert?
> Or wait for your refactor?

No need to resend, the refactored code is now in devel.