Re: [PATCH 2/2] ext4/058: set 256 blocks in a block group Set 256 blocks in a block group

[Sun Ke <sunke32@huawei.com>]

Thanks for your suggestions, I will improve them in v2.

在 2022/7/7 23:18, Zorro Lang 写道:
> On Thu, Jul 07, 2022 at 09:59:17PM +0800, Sun Ke wrote:
>> Set 256 blocks in a block group, then inject I/O pressure, it will
>> trigger off kernel BUG in ext4_mb_mark_diskspace_used.
>>
>> Regression test for commit a08f789d2ab5 ext4: fix bug_on
>> ext4_mb_use_inode_pa.
>>
>> Signed-off-by: Sun Ke <sunke32@huawei.com>
>> ---
> 
> About the subject:
> "ext4/058: set 256 blocks in a block group Set 256 blocks in a block group"
> 
> Don't use a fixed number for new case, you can use "ext4: ...". And I can't
> understand the meaning of this subject, except you say it's a duplicate :)
> 
> 
>>   tests/ext4/058     | 37 +++++++++++++++++++++++++++++++++++++
>>   tests/ext4/058.out |  2 ++
>>   2 files changed, 39 insertions(+)
>>   create mode 100755 tests/ext4/058
>>   create mode 100644 tests/ext4/058.out
>>
>> diff --git a/tests/ext4/058 b/tests/ext4/058
>> new file mode 100755
>> index 00000000..dc7903b7
>> --- /dev/null
>> +++ b/tests/ext4/058
>> @@ -0,0 +1,37 @@
>> +#! /bin/bash
>> +# SPDX-License-Identifier: GPL-2.0
>> +# Copyright (c) 2022 HUAWEI.  All Rights Reserved.
>> +#
>> +# FS QA Test 058
>> +#
>> +# Set 256 blocks in a block group, then inject I/O pressure,
>> +# it will trigger off kernel BUG in ext4_mb_mark_diskspace_used
>> +#
>> +# Regression test for commit
>> +# a08f789d2ab5 ext4: fix bug_on ext4_mb_use_inode_pa
>> +#
>> +. ./common/preamble
>> +_begin_fstest auto
>> +
>> +# real QA test starts here
>> +
>> +# Modify as appropriate.
>       ^^^
> 
> This's comment can be removed.
> 
>> +_supported_fs generic
> 
> If it's a ext4 specific test case, don't use "generic" at here.
> 
> And _fixed_by_kernel_commit() is recommend.
> 
>> +_require_scratch
>> +_require_command "$KILLALL_PROG" killall
>> +
>> +# set 256 blocks in a block group
>> +MKFS_OPTIONS="-g 256"
>> +_scratch_mkfs >>$seqres.full 2>&1
> 
> I think
>    _scratch_mkfs_ext4 -g 256 >>$seqres.full 2>&1
> is enough. Does other mkfs options will affect this testing?
> 
> Or make sure mkfs passed:
> _scratch_mkfs -g 256 >>$seqres.full 2>&1 || _fail "mkfs failed"
> 
>> +_scratch_mount
>> +
>> +$FSSTRESS_PROG -d $SCRATCH_MNT -n 1000 -p 1 >> $seqres.full 2>&1 &
> 
> Is "-p 1" necessary?
> 
>> +sleep 3
>> +$KILLALL_PROG -q $FSSTRESS_PROG
>> +wait
> 
> Hmm.... one more background fsstress test case again ... if so, you need to make
> sure the fsstress processes be killed in _cleanup(). Please refer to other cases.
> 
> Besides that, I'm wondering if you really need to run fsstress in background?
> Due to from the code logic, you run and kill it directly, then do nothing.
> What special reason cause you have to run fsstress as that?
> 
> Thanks,
> Zorro
> 
>> +
>> +echo "Silence is golden"
>> +
>> +# success, all done
>> +status=0
>> +exit
>> diff --git a/tests/ext4/058.out b/tests/ext4/058.out
>> new file mode 100644
>> index 00000000..fb5ca60b
>> --- /dev/null
>> +++ b/tests/ext4/058.out
>> @@ -0,0 +1,2 @@
>> +QA output created by 058
>> +Silence is golden
>> -- 
>> 2.13.6
>>
> 
> .
>