[PATCH] btrfs-progs: check/lowmem: Fix a false alert caused by hole beyond isize check

[PATCH] btrfs-progs: check/lowmem: Fix a false alert caused by hole beyond isize check

[Qu Wenruo]

Commit 91a12c0ddb00 ("btrfs-progs: fix lowmem check's handling of
holes") makes lowmem mode check to skip hole detection after isize.

However it also skipped the extent end update if the extent ends just at
isize.

This caused fsck-test/001 to fail with false hole error report.

Fix it by updating the @end parameter if we have an extent ends at inode
size.

Fixes: 91a12c0ddb00 ("btrfs-progs: fix lowmem check's handling of holes")
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
David, please fold the fix into the original patch.
---
 check/mode-lowmem.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/check/mode-lowmem.c b/check/mode-lowmem.c
index 7f734f974d02..1d2df349a9bf 100644
--- a/check/mode-lowmem.c
+++ b/check/mode-lowmem.c
@@ -2165,8 +2165,12 @@ static int check_file_extent(struct btrfs_root *root, struct btrfs_path *path,
 		}
 	}
 
-	if (fkey.offset + extent_num_bytes < isize)
-		*end = fkey.offset + extent_num_bytes;
+	/*
+	 * Don't update extent end beyond rounded up isize. As holes
+	 * after isize is not considered as missing holes.
+	 */
+	*end = min(round_up(isize, root->fs_info->sectorsize),
+		   fkey.offset + extent_num_bytes);
 	if (!is_hole)
 		*size += extent_num_bytes;
 
-- 
2.25.1

Re: [PATCH] btrfs-progs: check/lowmem: Fix a false alert caused by hole beyond isize check

[David Sterba]

On Sat, Mar 21, 2020 at 09:03:03AM +0800, Qu Wenruo wrote:
> Commit 91a12c0ddb00 ("btrfs-progs: fix lowmem check's handling of
> holes") makes lowmem mode check to skip hole detection after isize.
> 
> However it also skipped the extent end update if the extent ends just at
> isize.
> 
> This caused fsck-test/001 to fail with false hole error report.
> 
> Fix it by updating the @end parameter if we have an extent ends at inode
> size.
> 
> Fixes: 91a12c0ddb00 ("btrfs-progs: fix lowmem check's handling of holes")
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
> David, please fold the fix into the original patch.

Folded, thanks for the fix. The lowmem mode tests still don't pass for
me, have been failing since 5.1. I've now added a make target for it so
it's easier to run them without setting up the env variables.

Re: [PATCH] btrfs-progs: check/lowmem: Fix a false alert caused by hole beyond isize check

[Qu Wenruo]

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



On 2020/3/24 上午3:38, David Sterba wrote:
> On Sat, Mar 21, 2020 at 09:03:03AM +0800, Qu Wenruo wrote:
>> Commit 91a12c0ddb00 ("btrfs-progs: fix lowmem check's handling of
>> holes") makes lowmem mode check to skip hole detection after isize.
>>
>> However it also skipped the extent end update if the extent ends just at
>> isize.
>>
>> This caused fsck-test/001 to fail with false hole error report.
>>
>> Fix it by updating the @end parameter if we have an extent ends at inode
>> size.
>>
>> Fixes: 91a12c0ddb00 ("btrfs-progs: fix lowmem check's handling of holes")
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>> David, please fold the fix into the original patch.
> 
> Folded, thanks for the fix. The lowmem mode tests still don't pass for
> me, have been failing since 5.1. I've now added a make target for it so
> it's easier to run them without setting up the env variables.
> 

Mind to provide the fsck-test-result.txt for me to further investigate
the problem?

Thanks,
Qu


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

Re: [PATCH] btrfs-progs: check/lowmem: Fix a false alert caused by hole beyond isize check

[Qu Wenruo]

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



On 2020/3/24 上午7:15, Qu Wenruo wrote:
> 
> 
> On 2020/3/24 上午3:38, David Sterba wrote:
>> On Sat, Mar 21, 2020 at 09:03:03AM +0800, Qu Wenruo wrote:
>>> Commit 91a12c0ddb00 ("btrfs-progs: fix lowmem check's handling of
>>> holes") makes lowmem mode check to skip hole detection after isize.
>>>
>>> However it also skipped the extent end update if the extent ends just at
>>> isize.
>>>
>>> This caused fsck-test/001 to fail with false hole error report.
>>>
>>> Fix it by updating the @end parameter if we have an extent ends at inode
>>> size.
>>>
>>> Fixes: 91a12c0ddb00 ("btrfs-progs: fix lowmem check's handling of holes")
>>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>>> ---
>>> David, please fold the fix into the original patch.
>>
>> Folded, thanks for the fix. The lowmem mode tests still don't pass for
>> me, have been failing since 5.1. I've now added a make target for it so
>> it's easier to run them without setting up the env variables.
>>
> 
> Mind to provide the fsck-test-result.txt for me to further investigate
> the problem?

My bad, I didn't catch the problem in the pull request, and my test
environments all failed to catch the problem.

The lack of diversity makes it pretty hard to detect it, as it looks
like all machines tends to zero the unintialized stack structure.

Anyway, the proper fix for that long existing v5.1 bug is here:
https://patchwork.kernel.org/patch/11454395/

And I'll look into the possibility to auto use valgrind in selftests to
avoid similar bug to sneak in.

Thanks,
Qu
> 
> Thanks,
> Qu
> 


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

Re: [PATCH] btrfs-progs: check/lowmem: Fix a false alert caused by hole beyond isize check

[David Sterba]

On Tue, Mar 24, 2020 at 03:07:28PM +0800, Qu Wenruo wrote:
> On 2020/3/24 上午7:15, Qu Wenruo wrote:
> > On 2020/3/24 上午3:38, David Sterba wrote:
> >> On Sat, Mar 21, 2020 at 09:03:03AM +0800, Qu Wenruo wrote:
> >>> Commit 91a12c0ddb00 ("btrfs-progs: fix lowmem check's handling of
> >>> holes") makes lowmem mode check to skip hole detection after isize.
> >>>
> >>> However it also skipped the extent end update if the extent ends just at
> >>> isize.
> >>>
> >>> This caused fsck-test/001 to fail with false hole error report.
> >>>
> >>> Fix it by updating the @end parameter if we have an extent ends at inode
> >>> size.
> >>>
> >>> Fixes: 91a12c0ddb00 ("btrfs-progs: fix lowmem check's handling of holes")
> >>> Signed-off-by: Qu Wenruo <wqu@suse.com>
> >>> ---
> >>> David, please fold the fix into the original patch.
> >>
> >> Folded, thanks for the fix. The lowmem mode tests still don't pass for
> >> me, have been failing since 5.1. I've now added a make target for it so
> >> it's easier to run them without setting up the env variables.
> >>
> > 
> > Mind to provide the fsck-test-result.txt for me to further investigate
> > the problem?
> 
> My bad, I didn't catch the problem in the pull request, and my test
> environments all failed to catch the problem.
> 
> The lack of diversity makes it pretty hard to detect it, as it looks
> like all machines tends to zero the unintialized stack structure.
> 
> Anyway, the proper fix for that long existing v5.1 bug is here:
> https://patchwork.kernel.org/patch/11454395/

Thank you, that crash was probably the last big thing to do before a
release.

> And I'll look into the possibility to auto use valgrind in selftests to
> avoid similar bug to sneak in.

As you've found out, the valgrind or other instrumentation is there but
there are some problems. For that the GCCs sanitizers are more
lightweight option, but they make the test pass.