[xfstests PATCH] generic/567: add partial pages zeroing out case

[xfstests PATCH] generic/567: add partial pages zeroing out case

[Zhang Yi]

From: Zhang Yi <yi.zhang@huawei.com>

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@huaweicloud.com/ [1]
Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
---
 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
 _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"      \
-$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
+}
+
+_straddling_test "fpunch"
+_straddling_test "fzero"
 
 status=0
 exit
diff --git a/tests/generic/567.out b/tests/generic/567.out
index 0e826ed3..df89b8f3 100644
--- a/tests/generic/567.out
+++ b/tests/generic/567.out
@@ -17,3 +17,21 @@ XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 002800 58 58 58 58 58 58 58 58 58 58 58 58 58 58 58 58  >XXXXXXXXXXXXXXXX<
 *
 003000
+wrote 12288/12288 bytes at offset 0
+XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+==== Pre-Remount ===
+000000 58 58 58 58 58 58 58 58 58 58 58 58 58 58 58 58  >XXXXXXXXXXXXXXXX<
+*
+000800 59 59 59 59 59 59 59 59 59 59 59 59 59 59 59 59  >YYYYYYYYYYYYYYYY<
+*
+002800 58 58 58 58 58 58 58 58 58 58 58 58 58 58 58 58  >XXXXXXXXXXXXXXXX<
+*
+003000
+==== Post-Remount ==
+000000 58 58 58 58 58 58 58 58 58 58 58 58 58 58 58 58  >XXXXXXXXXXXXXXXX<
+*
+000800 59 59 59 59 59 59 59 59 59 59 59 59 59 59 59 59  >YYYYYYYYYYYYYYYY<
+*
+002800 58 58 58 58 58 58 58 58 58 58 58 58 58 58 58 58  >XXXXXXXXXXXXXXXX<
+*
+003000
-- 
2.46.1

Re: [xfstests PATCH] generic/567: add partial pages zeroing out case

[Ojaswin Mujoo]

On Mon, Dec 23, 2024 at 10:39:30AM +0800, Zhang Yi wrote:
> From: Zhang Yi <yi.zhang@huawei.com>
> 
> 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@huaweicloud.com/ [1]
> Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
> ---
>  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
>  _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"      \
> -$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

Hey Zhang,

While we are at it, can we generalize the test to work for
non-4k page sizes as well.

Regards,
ojaswin
> +
> +	echo "==== Pre-Remount ==="
> +	_hexdump $testfile
> +	_scratch_cycle_mount
> +	echo "==== Post-Remount =="
> +	_hexdump $testfile
> +}
> +
> +_straddling_test "fpunch"
> +_straddling_test "fzero"
>  
>  status=0
>  exit
> diff --git a/tests/generic/567.out b/tests/generic/567.out
> index 0e826ed3..df89b8f3 100644
> --- a/tests/generic/567.out
> +++ b/tests/generic/567.out
> @@ -17,3 +17,21 @@ XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>  002800 58 58 58 58 58 58 58 58 58 58 58 58 58 58 58 58  >XXXXXXXXXXXXXXXX<
>  *
>  003000
> +wrote 12288/12288 bytes at offset 0
> +XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +==== Pre-Remount ===
> +000000 58 58 58 58 58 58 58 58 58 58 58 58 58 58 58 58  >XXXXXXXXXXXXXXXX<
> +*
> +000800 59 59 59 59 59 59 59 59 59 59 59 59 59 59 59 59  >YYYYYYYYYYYYYYYY<
> +*
> +002800 58 58 58 58 58 58 58 58 58 58 58 58 58 58 58 58  >XXXXXXXXXXXXXXXX<
> +*
> +003000
> +==== Post-Remount ==
> +000000 58 58 58 58 58 58 58 58 58 58 58 58 58 58 58 58  >XXXXXXXXXXXXXXXX<
> +*
> +000800 59 59 59 59 59 59 59 59 59 59 59 59 59 59 59 59  >YYYYYYYYYYYYYYYY<
> +*
> +002800 58 58 58 58 58 58 58 58 58 58 58 58 58 58 58 58  >XXXXXXXXXXXXXXXX<
> +*
> +003000
> -- 
> 2.46.1
> 

Re: [xfstests PATCH] generic/567: add partial pages zeroing out case

[Darrick J. Wong]

On Mon, Dec 23, 2024 at 10:39:30AM +0800, Zhang Yi wrote:
> From: Zhang Yi <yi.zhang@huawei.com>
> 
> 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@huaweicloud.com/ [1]
> Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
> ---
>  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"

Please don't overload an existing test.  Filesystems such as gfs2,
hugetblfs, nfs, ntfs3, and ocfs2 support fpunch but don't support fzero.
This change opens a testing gap for those filesystems.

--D

>  testfile=$SCRATCH_MNT/testfile
>  
>  _scratch_mkfs > /dev/null 2>&1
>  _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"      \
> -$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
> +}
> +
> +_straddling_test "fpunch"
> +_straddling_test "fzero"
>  
>  status=0
>  exit
> diff --git a/tests/generic/567.out b/tests/generic/567.out
> index 0e826ed3..df89b8f3 100644
> --- a/tests/generic/567.out
> +++ b/tests/generic/567.out
> @@ -17,3 +17,21 @@ XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>  002800 58 58 58 58 58 58 58 58 58 58 58 58 58 58 58 58  >XXXXXXXXXXXXXXXX<
>  *
>  003000
> +wrote 12288/12288 bytes at offset 0
> +XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +==== Pre-Remount ===
> +000000 58 58 58 58 58 58 58 58 58 58 58 58 58 58 58 58  >XXXXXXXXXXXXXXXX<
> +*
> +000800 59 59 59 59 59 59 59 59 59 59 59 59 59 59 59 59  >YYYYYYYYYYYYYYYY<
> +*
> +002800 58 58 58 58 58 58 58 58 58 58 58 58 58 58 58 58  >XXXXXXXXXXXXXXXX<
> +*
> +003000
> +==== Post-Remount ==
> +000000 58 58 58 58 58 58 58 58 58 58 58 58 58 58 58 58  >XXXXXXXXXXXXXXXX<
> +*
> +000800 59 59 59 59 59 59 59 59 59 59 59 59 59 59 59 59  >YYYYYYYYYYYYYYYY<
> +*
> +002800 58 58 58 58 58 58 58 58 58 58 58 58 58 58 58 58  >XXXXXXXXXXXXXXXX<
> +*
> +003000
> -- 
> 2.46.1
> 
> 

Re: [xfstests PATCH] generic/567: add partial pages zeroing out case

[Zhang Yi]

On 2024/12/24 14:45, Ojaswin Mujoo wrote:
> On Mon, Dec 23, 2024 at 10:39:30AM +0800, Zhang Yi wrote:
>> From: Zhang Yi <yi.zhang@huawei.com>
>>
>> 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@huaweicloud.com/ [1]
>> Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
>> ---
>>  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
>>  _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"      \
>> -$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
> 
> Hey Zhang,
> 
> While we are at it, can we generalize the test to work for
> non-4k page sizes as well.
> 

Hi, Ojaswin.

Yeah, I suppose we can do it.

Thanks,
Yi.

Re: [xfstests PATCH] generic/567: add partial pages zeroing out case

[Zhang Yi]

On 2024/12/25 3:40, Darrick J. Wong wrote:
> On Mon, Dec 23, 2024 at 10:39:30AM +0800, Zhang Yi wrote:
>> From: Zhang Yi <yi.zhang@huawei.com>
>>
>> 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@huaweicloud.com/ [1]
>> Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
>> ---
>>  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"
> 
> Please don't overload an existing test.  Filesystems such as gfs2,
> hugetblfs, nfs, ntfs3, and ocfs2 support fpunch but don't support fzero.
> This change opens a testing gap for those filesystems.
> 

OK, thanks for point this out, I will create a new test for this case.

Thanks,
Yi.

> 
>>  testfile=$SCRATCH_MNT/testfile
>>  
>>  _scratch_mkfs > /dev/null 2>&1
>>  _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"      \
>> -$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
>> +}
>> +
>> +_straddling_test "fpunch"
>> +_straddling_test "fzero"
>>  
>>  status=0
>>  exit
>> diff --git a/tests/generic/567.out b/tests/generic/567.out
>> index 0e826ed3..df89b8f3 100644
>> --- a/tests/generic/567.out
>> +++ b/tests/generic/567.out
>> @@ -17,3 +17,21 @@ XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>>  002800 58 58 58 58 58 58 58 58 58 58 58 58 58 58 58 58  >XXXXXXXXXXXXXXXX<
>>  *
>>  003000
>> +wrote 12288/12288 bytes at offset 0
>> +XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>> +==== Pre-Remount ===
>> +000000 58 58 58 58 58 58 58 58 58 58 58 58 58 58 58 58  >XXXXXXXXXXXXXXXX<
>> +*
>> +000800 59 59 59 59 59 59 59 59 59 59 59 59 59 59 59 59  >YYYYYYYYYYYYYYYY<
>> +*
>> +002800 58 58 58 58 58 58 58 58 58 58 58 58 58 58 58 58  >XXXXXXXXXXXXXXXX<
>> +*
>> +003000
>> +==== Post-Remount ==
>> +000000 58 58 58 58 58 58 58 58 58 58 58 58 58 58 58 58  >XXXXXXXXXXXXXXXX<
>> +*
>> +000800 59 59 59 59 59 59 59 59 59 59 59 59 59 59 59 59  >YYYYYYYYYYYYYYYY<
>> +*
>> +002800 58 58 58 58 58 58 58 58 58 58 58 58 58 58 58 58  >XXXXXXXXXXXXXXXX<
>> +*
>> +003000
>> -- 
>> 2.46.1
>>
>>

Re: [xfstests PATCH] generic/567: add partial pages zeroing out case

[Nirjhar Roy]

On Mon, 2024-12-23 at 10:39 +0800, Zhang Yi wrote:
> From: Zhang Yi <yi.zhang@huawei.com>
> 
> 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@huaweicloud.com/
>  [1]
> Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
> ---
>  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?
>  _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?
>      \
> -$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"?
> +}
> +
> +_straddling_test "fpunch"
> +_straddling_test "fzero"
Minor: Since we are running 2 independant sub-tests, isn't it better to
use 2 different files?

--NR
>  
>  status=0
>  exit
> diff --git a/tests/generic/567.out b/tests/generic/567.out
> index 0e826ed3..df89b8f3 100644
> --- a/tests/generic/567.out
> +++ b/tests/generic/567.out
> @@ -17,3 +17,21 @@ XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX
> ops/sec)
>  002800 58 58 58 58 58 58 58 58 58 58 58 58 58 58 58
> 58  >XXXXXXXXXXXXXXXX<
>  *
>  003000
> +wrote 12288/12288 bytes at offset 0
> +XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +==== Pre-Remount ===
> +000000 58 58 58 58 58 58 58 58 58 58 58 58 58 58 58
> 58  >XXXXXXXXXXXXXXXX<
> +*
> +000800 59 59 59 59 59 59 59 59 59 59 59 59 59 59 59
> 59  >YYYYYYYYYYYYYYYY<
> +*
> +002800 58 58 58 58 58 58 58 58 58 58 58 58 58 58 58
> 58  >XXXXXXXXXXXXXXXX<
> +*
> +003000
> +==== Post-Remount ==
> +000000 58 58 58 58 58 58 58 58 58 58 58 58 58 58 58
> 58  >XXXXXXXXXXXXXXXX<
> +*
> +000800 59 59 59 59 59 59 59 59 59 59 59 59 59 59 59
> 59  >YYYYYYYYYYYYYYYY<
> +*
> +002800 58 58 58 58 58 58 58 58 58 58 58 58 58 58 58
> 58  >XXXXXXXXXXXXXXXX<
> +*
> +003000

Re: [xfstests PATCH] generic/567: add partial pages zeroing out case

[Zhang Yi]

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@huawei.com>
>>
>> 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@huaweicloud.com/
>>  [1]
>> Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
>> ---
>>  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.

>>  _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.

>>      \
>> -$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.

>> +}
>> +
>> +_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.

  https://lore.kernel.org/fstests/20241225125120.1952219-1-yi.zhang@huaweicloud.com/

Thanks,
Yi.

Re: [xfstests PATCH] generic/567: add partial pages zeroing out case

[Nirjhar Roy]

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@huawei.com>
>>>
>>> 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@huaweicloud.com/
>>>   [1]
>>> Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
>>> ---
>>>   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@huaweicloud.com/
>
> Thanks,
> Yi.
>
>
-- 
Nirjhar Roy
Linux Kernel Developer
IBM, Bangalore

Re: [xfstests PATCH] generic/567: add partial pages zeroing out case

[Zhang Yi]

On 2024/12/30 12:16, Nirjhar Roy wrote:
> 
> 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@huawei.com>
>>>>
>>>> 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@huaweicloud.com/
>>>>   [1]
>>>> Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
>>>> ---
>>>>   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?

Yes, this test is very simple and should pass in an environment where
the block size is equal to the page size.

Thanks,
Yi.