On 12/27/24 13:59, Zhang Yi wrote:
On 2024/12/27 13:28, Nirjhar Roy wrote:
On Mon, 2024-12-23 at 10:39 +0800, Zhang Yi wrote:
From: Zhang Yi <yi.zhang@xxxxxxxxxx>
This addresses a data corruption issue encountered during partial
page
zeroing in ext4 which the block size is smaller than the page size
[1].
Expand this test to include a zeroing range test that spans two
partial
pages to cover this case.
Link:
https://lore.kernel.org/linux-ext4/20241220011637.1157197-2-yi.zhang@xxxxxxxxxxxxxxx/
[1]
Signed-off-by: Zhang Yi <yi.zhang@xxxxxxxxxx>
---
tests/generic/567 | 50 +++++++++++++++++++++++++--------------
----
tests/generic/567.out | 18 ++++++++++++++++
2 files changed, 47 insertions(+), 21 deletions(-)
diff --git a/tests/generic/567 b/tests/generic/567
index fc109d0d..756280e8 100755
--- a/tests/generic/567
+++ b/tests/generic/567
@@ -4,43 +4,51 @@
#
# FS QA Test No. generic/567
#
-# Test mapped writes against punch-hole to ensure we get the data
-# correctly written. This can expose data corruption bugs on
filesystems
-# where the block size is smaller than the page size.
+# Test mapped writes against punch-hole and zero-range to ensure we
get
+# the data correctly written. This can expose data corruption bugs
on
+# filesystems where the block size is smaller than the page size.
#
# (generic/029 is a similar test but for truncate.)
#
. ./common/preamble
-_begin_fstest auto quick rw punch
+_begin_fstest auto quick rw punch zero
# Import common functions.
. ./common/filter
_require_scratch
_require_xfs_io_command "fpunch"
+_require_xfs_io_command "fzero"
testfile=$SCRATCH_MNT/testfile
_scratch_mkfs > /dev/null 2>&1
Since this test requires block size < page size, do you think it is a
good idea to hard code the _scratch_mkfs parameters to explicitly pass
the block size to < less than zero? This will require less manipulation
with the local.config file. Or maybe have a _notrun to _notrun the test
if the block size is not less than the page size?
Hi, Nirjhar. Thank you for the review!
Although the issue we encountered is on the configuration that block
size is less than page size, I believe it is also harmless to run this
test in an environment where the block size is equal to the page size.
This is a quick and basic test.
Okay makes sense. So with block size equal to page size, the actual
functionality that we want to test won't be tested(but the test will
pass), is that what you mean?
_scratch_mount
-# Punch a hole straddling two pages to check that the mapped write
after the
-# hole-punching is correctly handled.
-
-$XFS_IO_PROG -t -f \
--c "pwrite -S 0x58 0 12288" \
--c "mmap -rw 0 12288" \
--c "mwrite -S 0x5a 2048 8192" \
--c "fpunch 2048 8192" \
--c "mwrite -S 0x59 2048 8192" \
--c "close"
Minor: isn't the close command redundant? xfs_io will in any case close
the file right?
Yes, but this explicit close is from the original text and appears
harmless, so I'd suggest keeping it.
Okay.
\
-$testfile | _filter_xfs_io
-
-echo "==== Pre-Remount ==="
-_hexdump $testfile
-_scratch_cycle_mount
-echo "==== Post-Remount =="
-_hexdump $testfile
+# Punch a hole and zero out straddling two pages to check that the
mapped
+# write after the hole-punching and range-zeroing are correctly
handled.
+_straddling_test()
+{
+ local test_cmd=$1
+
+ $XFS_IO_PROG -t -f \
+ -c "pwrite -S 0x58 0 12288" \
+ -c "mmap -rw 0 12288" \
+ -c "mwrite -S 0x5a 2048 8192" \
+ -c "$test_cmd 2048 8192" \
+ -c "mwrite -S 0x59 2048 8192" \
+ -c "close" \
+ $testfile | _filter_xfs_io
+
+ echo "==== Pre-Remount ==="
+ _hexdump $testfile
+ _scratch_cycle_mount
+ echo "==== Post-Remount =="
+ _hexdump $testfile
Just guessing here: Do you think it is makes sense to test with both
delayed and non-delayed allocation? I mean with and without "msync"?
Sorry, I don't understand why we need msync. The umount should flush
the dirty pages to the disk even without msync, I mean this this test
does not focus on the functionality of msync now.
Okay makes sense.
+}
+
+_straddling_test "fpunch"
+_straddling_test "fzero"
Minor: Since we are running 2 independant sub-tests, isn't it better to
use 2 different files?
Yes, Darrick had the same suggestion, and I have separated this into
generic/758 in my v2.
Okay.
https://lore.kernel.org/fstests/20241225125120.1952219-1-yi.zhang@xxxxxxxxxxxxxxx/
Thanks,
Yi.
--
Nirjhar Roy
Linux Kernel Developer
IBM, Bangalore
