2022-07-07 13:48:59

by Sun Ke

[permalink] [raw]
Subject: [PATCH 0/2] two regression tests for ext4

two regression tests for ext4

Sun Ke (2):
ext4/057: resize fs after resize_inode without e2fsck
ext4/058: set 256 blocks in a block group Set 256 blocks in a block
group

tests/ext4/057 | 41 +++++++++++++++++++++++++++++++++++++++++
tests/ext4/057.out | 2 ++
tests/ext4/058 | 37 +++++++++++++++++++++++++++++++++++++
tests/ext4/058.out | 2 ++
4 files changed, 82 insertions(+)
create mode 100755 tests/ext4/057
create mode 100644 tests/ext4/057.out
create mode 100755 tests/ext4/058
create mode 100644 tests/ext4/058.out

--
2.13.6


2022-07-07 13:49:05

by Sun Ke

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

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 <[email protected]>
---
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.
+_supported_fs generic
+_require_scratch
+_require_command "$KILLALL_PROG" killall
+
+# set 256 blocks in a block group
+MKFS_OPTIONS="-g 256"
+_scratch_mkfs >>$seqres.full 2>&1
+_scratch_mount
+
+$FSSTRESS_PROG -d $SCRATCH_MNT -n 1000 -p 1 >> $seqres.full 2>&1 &
+sleep 3
+$KILLALL_PROG -q $FSSTRESS_PROG
+wait
+
+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

2022-07-07 13:49:08

by Sun Ke

[permalink] [raw]
Subject: [PATCH 1/2] ext4/057: resize fs after resize_inode without e2fsck

Forget to run requested e2fsck after resize_inode, then resize fs, it
will trigger off null pointer.

Regression test for commit b55c3cd102a6 ext4: add reserved GDT blocks
check

Signed-off-by: Sun Ke <[email protected]>
---
tests/ext4/057 | 41 +++++++++++++++++++++++++++++++++++++++++
tests/ext4/057.out | 2 ++
2 files changed, 43 insertions(+)
create mode 100755 tests/ext4/057
create mode 100644 tests/ext4/057.out

diff --git a/tests/ext4/057 b/tests/ext4/057
new file mode 100755
index 00000000..dacc14be
--- /dev/null
+++ b/tests/ext4/057
@@ -0,0 +1,41 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) 2022 HUAWEI. All Rights Reserved.
+#
+# FS QA Test 057
+#
+# Forget to run requested e2fsck after resize_inode, then resize fs,
+# it will trigger off null pointer.
+#
+# Regression test for commit
+# b55c3cd102a6 ext4: add reserved GDT blocks check
+#
+. ./common/preamble
+_begin_fstest auto
+
+# real QA test starts here
+
+# Modify as appropriate.
+_supported_fs ext4
+_require_scratch
+_require_command "$TUNE2FS_PROG" tune2fs
+_require_command "$RESIZE2FS_PROG" resize2
+
+
+# set fs size 3G
+dev_size=$((3 * 1024 * 1024 * 1024))
+_scratch_mkfs_sized $dev_size >/dev/null 2>&1
+
+# forget to run requested e2fsck after resize_inode
+$TUNE2FS_PROG -O ^resize_inode $SCRATCH_DEV >/dev/null 2>&1
+
+_scratch_mount
+
+# resize fs from 3G to 8G
+$RESIZE2FS_PROG $SCRATCH_DEV 8G >/dev/null 2>&1
+
+echo "Silence is golden"
+
+# success, all done
+status=0
+exit
diff --git a/tests/ext4/057.out b/tests/ext4/057.out
new file mode 100644
index 00000000..185023c7
--- /dev/null
+++ b/tests/ext4/057.out
@@ -0,0 +1,2 @@
+QA output created by 057
+Silence is golden
--
2.13.6

2022-07-07 15:03:16

by Zorro Lang

[permalink] [raw]
Subject: Re: [PATCH 1/2] ext4/057: resize fs after resize_inode without e2fsck

On Thu, Jul 07, 2022 at 09:59:16PM +0800, Sun Ke wrote:
> Forget to run requested e2fsck after resize_inode, then resize fs, it
> will trigger off null pointer.
>
> Regression test for commit b55c3cd102a6 ext4: add reserved GDT blocks
> check
>
> Signed-off-by: Sun Ke <[email protected]>
> ---

Don't use a fixed case number for a new case. It might not be "057" when
I merge it.

> tests/ext4/057 | 41 +++++++++++++++++++++++++++++++++++++++++
> tests/ext4/057.out | 2 ++
> 2 files changed, 43 insertions(+)
> create mode 100755 tests/ext4/057
> create mode 100644 tests/ext4/057.out
>
> diff --git a/tests/ext4/057 b/tests/ext4/057
> new file mode 100755
> index 00000000..dacc14be
> --- /dev/null
> +++ b/tests/ext4/057
> @@ -0,0 +1,41 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (c) 2022 HUAWEI. All Rights Reserved.
> +#
> +# FS QA Test 057
> +#
> +# Forget to run requested e2fsck after resize_inode, then resize fs,
> +# it will trigger off null pointer.
> +#
> +# Regression test for commit
> +# b55c3cd102a6 ext4: add reserved GDT blocks check
> +#
> +. ./common/preamble
> +_begin_fstest auto

This's a "resize" related test, and I think it'll be "quick" enough if you
use smaller fs size to test.

> +
> +# real QA test starts here
> +
> +# Modify as appropriate.
> +_supported_fs ext4
> +_require_scratch

_fixed_by_kernel_commit() is recommended, if this's a known regression test.

> +_require_command "$TUNE2FS_PROG" tune2fs
> +_require_command "$RESIZE2FS_PROG" resize2
^^
resize2fs ?

> +
> +
> +# set fs size 3G
> +dev_size=$((3 * 1024 * 1024 * 1024))
> +_scratch_mkfs_sized $dev_size >/dev/null 2>&1
> +
> +# forget to run requested e2fsck after resize_inode
> +$TUNE2FS_PROG -O ^resize_inode $SCRATCH_DEV >/dev/null 2>&1
> +
> +_scratch_mount
> +
> +# resize fs from 3G to 8G

This comment is useless. You can describe what's expected, and what
kind of bug might be trigger at here.

> +$RESIZE2FS_PROG $SCRATCH_DEV 8G >/dev/null 2>&1

Better to print to $seqres.full, to help debug if need.

Better use _require_scratch_size at beginning, to make sure you have enough
space. BTW, do you really need such big size to trigger this bug? Better to
figure out if you can use smaller size (e.g. 512m to 1g) to help this
case always can be run, even with small test devices.

> +
> +echo "Silence is golden"
> +
> +# success, all done
> +status=0
> +exit
> diff --git a/tests/ext4/057.out b/tests/ext4/057.out
> new file mode 100644
> index 00000000..185023c7
> --- /dev/null
> +++ b/tests/ext4/057.out
> @@ -0,0 +1,2 @@
> +QA output created by 057
> +Silence is golden
> --
> 2.13.6
>

2022-07-07 15:32:10

by Zorro Lang

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

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 <[email protected]>
> ---

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
>

2022-07-08 11:04:37

by Sun Ke

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

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 <[email protected]>
>> ---
>
> 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
>>
>
> .
>