Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 7 Jul 2022 23:18:33 +0800
From:   Zorro Lang <zlang@redhat.com>
To:     Sun Ke <sunke32@huawei.com>
Cc:     fstests@vger.kernel.org, linux-ext4@vger.kernel.org
Subject: Re: [PATCH 2/2] ext4/058: set 256 blocks in a block group Set 256
 blocks in a block group
Message-ID: <20220707151833.72ggvyxjzz2e42kh@zlang-mailbox>
References: <20220707135917.373342-1-sunke32@huawei.com>
 <20220707135917.373342-3-sunke32@huawei.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220707135917.373342-3-sunke32@huawei.com>
Precedence: bulk

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
>