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

[Nirjhar Roy <nirjhar@linux.ibm.com>]

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