On 2023/11/17 22:24, Zorro Lang wrote:
Hi Baokun,
Thanks for this new coverage, some review points as below ...
Hi Zorro,
Thank you for your careful review！
tests/ext4/062 | 53 ++++++++++++++++++++++++++++++++++++++++++++++
tests/ext4/062.out | 2 ++
2 files changed, 55 insertions(+)
create mode 100755 tests/ext4/062
create mode 100644 tests/ext4/062.out
diff --git a/tests/ext4/062 b/tests/ext4/062
new file mode 100755
index 00000000..0ae4ed80
--- /dev/null
+++ b/tests/ext4/062
@@ -0,0 +1,53 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) 2023 HUAWEI. All Rights Reserved.
+#
+# FS QA Test No. 062
+#
+# Append writes to a file approaching 16T and observe if a kernel crash is
+# caused by ext4_lblk_t overflow triggering BUG_ON at ext4_mb_new_inode_pa().
+# This is a regression test for commit
+# bc056e7163ac ("ext4: fix BUG in ext4_mb_new_inode_pa() due to overflow")
+#
+. ./common/preamble
+_begin_fstest auto quick insert
^^^
prealloc
I'll add this.
+
+# real QA test starts here
+_supported_fs ext4
+_fixed_by_kernel_commit bc056e7163ac \
+ "ext4: fix BUG in ext4_mb_new_inode_pa() due to overflow"
+
+_require_xfs_io_command "falloc"
+_require_xfs_io_command "open"
+_require_xfs_io_command "pwrite"
I think we don't need to check the "open" and "pwrite" operations,
if you don't need special option for them.
OK!
+_require_scratch_size_nocheck $((100 * 1024)) # 100MB in KB
I think 100MB is small enough, don't need this check.
All right!
+
+# Initalize a 4k 100M ext4 fs
+dev_size=$((100 * 1024 * 1024))
+MKFS_OPTIONS="-b 4096 $MKFS_OPTIONS" _scratch_mkfs_sized $dev_size \
+ >>$seqres.full 2>&1 || _fail "mkfs failed"
Can we use bsize=$(_get_file_block_size ....) to get the real block size,
to avoid use a specific mkfs option?
If so, my second question is if this case can be a generic case. The test
steps look common enough, I'm glad to see what will happen on other
filesystems through this test.
The reason for specifying 4096 for formatting here is that mkfs.ext4
specifies a
default block size of 1024 when the disk image is 100M.
Although these operations appear to be generic scenarios, the
reproduction of
this problem relies on ext4's pre-allocation mechanism. Block allocation
requests
are scaled up in ext4_mb_normalize_request(), and the extra blocks are
used for
preallocation to reduce fragmentation. Therefore, this test case is
placed only in
the ext4 test suite.
+
+_scratch_mount
+
+# Reserve 1M space
+$XFS_IO_PROG -f -c "falloc 0 1M" "${SCRATCH_MNT}/tmp" >> $seqres.full 2>&1
+
+# Create a file with logical block numbers close to overflow
+$XFS_IO_PROG -f -c "falloc 0 10M" "${SCRATCH_MNT}/file" >> $seqres.full 2>&1
+$XFS_IO_PROG -f -c "finsert 1M 16777203M" "${SCRATCH_MNT}/file" >> $seqres.full 2>&1
_require_xfs_io_command "finsert"
Okay.
+
+# Filling up the free space ensures that the pre-allocated space is the reserved space.
+$XFS_IO_PROG -f -c "falloc 0 80388096" "${SCRATCH_MNT}/fill" >> $seqres.full 2>&1
Can we make sure this step fill the whole free space? There's a helper in
common/populate named _fill_fs, please check if it's what you want?
The remaining space for a 100M 4k ext4 image after the previous
operations is
80388096, so we can confirm that we can fill the entire free space here.
Of course, using _fill_fs is also workable, but it's faster to do it in
a single fallocate.
+
+# Remove reserved space to gain free space for allocation
+rm -f ${SCRATCH_MNT}/tmp
+
+# Trying to allocate two blocks triggers BUG_ON.
+$XFS_IO_PROG -c "open -ad ${SCRATCH_MNT}/file" -c "pwrite -S 0xff 0 8192" >> $seqres.full 2>&1
Oh, you need open -d option, that means you need direct IO, so you need:
_require_odirect
Indeed!
And all above xfs_io commands, include falloc, finsert, open and pwrite.
You can redirect the stdout to $seqres.full, but better *not* to change
the stderr, for we can find errors if they fail (except that expected ENOSPC).
Ok, I'll remove the unnecessary "2>&1"!
+
+umount "${SCRATCH_MNT}"
The $SCRATCH_MNT will be unmounted at the end of the testing, don't need to do
it manually, except this's a necessary step of the reproducer.
Okay, I will remove it.
+echo "Silence is golden"
+
+# success, all done
+status=0
+exit
diff --git a/tests/ext4/062.out b/tests/ext4/062.out
new file mode 100644
index 00000000..a1578f48
--- /dev/null
+++ b/tests/ext4/062.out
@@ -0,0 +1,2 @@
+QA output created by 062
+Silence is golden
--
2.31.1
Thanks!
--
With Best Regards,
Baokun Li
.
