btrfs-progs: check/lowmem: Fix a false alert caused by hole beyond isize check
diff mbox series

Message ID 20200321010303.124708-1-wqu@suse.com
State New
Headers show
Series
  • btrfs-progs: check/lowmem: Fix a false alert caused by hole beyond isize check
Related show

Commit Message

Qu Wenruo March 21, 2020, 1:03 a.m. UTC
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(-)

Comments

David Sterba March 23, 2020, 7:38 p.m. UTC | #1
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.
Qu Wenruo March 23, 2020, 11:15 p.m. UTC | #2
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
Qu Wenruo March 24, 2020, 7:07 a.m. UTC | #3
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
>
David Sterba March 24, 2020, 4:28 p.m. UTC | #4
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.

Patch
diff mbox series

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;