Re: [PATCH] ext4/048: Add new regression test

["Chenlong (H)" <chenlongcl.chen@huawei.com>]

On 2021/5/21 0:34:25, Darrick J. Wong wrote:
> On Thu, May 20, 2021 at 11:41:52AM +0800, Chenlong (H) wrote:
>>
>>
>> On 2021/5/20 7:24:35, Darrick J. Wong wrote:
>>> On Wed, May 19, 2021 at 11:10:15AM +0800, chenlong wrote:
>>>> Check the block group zero and prevent initializing reserved inodes.
>>>> But in some special cases, the reserved inode may not all belong to
>>>> the group zero, it may exist into the second group if we format
>>>> filesystem below.
>>>> mkfs.ext4 -b 4096 -g 8192 -N 1024 -I 4096 /dev/sda
>>>> So, it will end up triggering a false positive report of a corrupted
>>>> file system
>>>>
>>>> It's a regression test for commit a149d2a5cabb.
>>>>
>>>> Signed-off-by: Chen Long <chenlongcl.chen@huawei.com>
>>>> ---
>>>>    tests/ext4/048     | 50 ++++++++++++++++++++++++++++++++++++++++++++++
>>>>    tests/ext4/048.out |  5 +++++
>>>>    tests/ext4/group   |  1 +
>>>>    3 files changed, 56 insertions(+)
>>>>    create mode 100755 tests/ext4/048
>>>>    create mode 100644 tests/ext4/048.out
>>>>
>>>> diff --git a/tests/ext4/048 b/tests/ext4/048
>>>> new file mode 100755
>>>> index 00000000..56801579
>>>> --- /dev/null
>>>> +++ b/tests/ext4/048
>>>> @@ -0,0 +1,50 @@
>>>> +#! /bin/bash
>>>> +# SPDX-License-Identifier: GPL-2.0
>>>> +# Copyright (c) 2021 Huawei.  All Rights Reserved.
>>>> +#
>>>> +# FS QA Test 048
>>>> +#
>>>> +# Regression test for commit:
>>>> +# a149d2a5cabb(ext4: fix check to prevent false positive report of incorrect
>>>> +# used inodes)
>>>> +#
>>>> +seq=`basename $0`
>>>> +seqres=$RESULT_DIR/$seq
>>>> +echo "QA output created by $seq"
>>>> +
>>>> +here=`pwd`
>>>> +tmp=/tmp/$$
>>>> +status=1	# failure is the default!
>>>> +trap "_cleanup; exit \$status" 0 1 2 3 15
>>>> +
>>>> +_cleanup()
>>>> +{
>>>> +    cd /
>>>> +    rm -f $tmp.*
>>>> +}
>>>> +
>>>> +# get standard environment, filters and checks
>>>> +. ./common/rc
>>>> +. ./common/filter
>>>> +
>>>> +# remove previous $seqres.full before test
>>>> +rm -f $seqres.full
>>>> +
>>>> +# real QA test starts here
>>>> +_supported_fs ext4
>>>> +_require_scratch
>>>> +
>>>> +echo "+ create scratch fs"
>>>> +_scratch_mkfs_ext4 -b 4096 -g 8192 -N 1024 -I 4096 >> $seqres.full 2>&1
>>>> +
>>>> +echo "+ mount fs"
>>>> +_scratch_mount -o errors=remount-ro
>>>> +sleep 5
>>>
>>> Why do we need to sleep here?  Won't _scratch_mount return nonzero if
>>> the (buggy) kernel thought the fs was corrupt?
>>>
>>> Also, won't the grep below catch that the fs didn't mount?
>>>
>>
>> The default maximum wake-up delay of the lazy init thread of ext4 is
>> EXT4_DEF_LI_MAX_START_DELAY (5s).
>>
>> After mounting the file system, the lazy init thread will be awakened to
>> perform group initialization at 0~5s
>>
>> after the current time. If an error is detected during the initialization
>> process, it will be set to read-only,
>>
>> so the test case needs to delay a little while waiting for the init thread
>> to execute.
> 
> Oh, that's why there's a delay.  Could you paste this explanation into
> the test case as a comment right before the 'sleep 5', please?
> 
> Also, if the delay is configurable you might want to set it to a known
> value at the start of this test.
> 
> --D
>

Thank you，here really need to add this explanation and set the sleep 
time to a configurable value.

>> in this testcase,  _scratch_mount will be return zero, after a short period
>> of time, the file system will be remounted as read-only.
>>
>> In an environment with this bug,  such as:
>>
>> [root@testvm ~]# mkfs.ext4 -b 4096 -g 8192 -N 1024 -I 4096 /dev/sda
>>
>> [root@testvm ~]# mount -o errors=remount-ro /dev/sda /mnt
>>
>> [root@testvm ~]# mount | grep /dev/sda
>>
>> /dev/sda on /mnt type ext4 (rw,relatime,errors=remount-ro)
>>
>> After the file system is mounted，file system status is rw，but a moment
>> later, status will be change to ro.
>>
>> [root@testvm ~]# mount | grep /dev/sda
>>
>> /dev/sda on /mnt type ext4 (ro,relatime,errors=remount-ro)
>>
>>>> +
>>>> +echo "+ check mountpoint status"
>>>> +cat /proc/self/mounts | grep ${SCRATCH_MNT} | \
>>>
>>> Careful here, if someone sets SCRATCH_MNT=/u, this will match another
>>> mount for /usr.  At the bare minimum you want 'grep -w' here, I think.
>>>
>>> --D
>>>
>>
>> There is a risk here and needs to be fixed.
>>
>>>> +    $AWK_PROG '{print $4}' | grep -oE '^rw,' | tee -a $seqres.full
>>>> +
>>>> +# success, all done
>>>> +status=0
>>>> +exit
>>>> diff --git a/tests/ext4/048.out b/tests/ext4/048.out
>>>> new file mode 100644
>>>> index 00000000..16e50e86
>>>> --- /dev/null
>>>> +++ b/tests/ext4/048.out
>>>> @@ -0,0 +1,5 @@
>>>> +QA output created by 048
>>>> ++ create scratch fs
>>>> ++ mount fs
>>>> ++ check mountpoint status
>>>> +rw,
>>>> diff --git a/tests/ext4/group b/tests/ext4/group
>>>> index ceda2ba6..82b77efb 100644
>>>> --- a/tests/ext4/group
>>>> +++ b/tests/ext4/group
>>>> @@ -50,6 +50,7 @@
>>>>    045 auto dir
>>>>    046 auto prealloc quick
>>>>    047 auto quick dax
>>>> +048 auto quick
>>>>    271 auto rw quick
>>>>    301 aio auto ioctl rw stress defrag
>>>>    302 aio auto ioctl rw stress defrag
>>>> -- 
>>>> 2.18.0.huawei.25
>>>>
>>> .
>>>
> .
>