Re: [PATCH 1/1] ext4/046: Add test to verify unwritten extent conversion in buff-io

[Ritesh Harjani <riteshh@linux.ibm.com>]

Thanks Eryu for detailed review.

On 10/11/20 11:41 AM, Eryu Guan wrote:
> On Fri, Oct 09, 2020 at 05:12:24PM +0530, Ritesh Harjani wrote:
>> There was an issue where with with filesize > 4G, map.m_lblk
>> was getting overflow in buff-IO path while converting unwritten to
>> written extent with dioread_nolock mount option with bs < ps.
>> Adding a testcase for the same.
>> This test doesn't force any explicit mount option within the test,
>> as it will be set anyway along with test config, although it does
>> force for blocksize < pagesize config (as this test is specific to
> 
> I don't think we need to force blksize < pagesize either, as testing
> different blocksize configs is a common test setup and is part of
> tester's resposibility.

Right, but generally bs < ps configuration is not tested for every mount 
option. The issue mentioned here happens with bs < ps for dioread_nolock 
mount opt.
So I need to force at least bs < ps or the mount option of dioread_nolock.
I think since it is mainly ext4 specific, maybe I will change the patch 
to enforce this for dioread_nolock mount opt. Then since bs < ps by 
default everyone tests, so this test will get exercised automatically.


> 
>> that). Patch at [1] fixes the issue.
>>
>> [1]: https://patchwork.ozlabs.org/patch/1378632
>>
>> Signed-off-by: Ritesh Harjani <riteshh@linux.ibm.com>
>> ---
>>   tests/ext4/046     | 101 +++++++++++++++++++++++++++++++++++++++++++++
>>   tests/ext4/046.out |   2 +
>>   tests/ext4/group   |   1 +
>>   3 files changed, 104 insertions(+)
>>   create mode 100755 tests/ext4/046
>>   create mode 100644 tests/ext4/046.out
>>
>> diff --git a/tests/ext4/046 b/tests/ext4/046
>> new file mode 100755
>> index 000000000000..49de6bbb2c89
>> --- /dev/null
>> +++ b/tests/ext4/046
>> @@ -0,0 +1,101 @@
>> +#! /bin/bash
>> +# SPDX-License-Identifier: GPL-2.0
>> +# Copyright (c) 2020 IBM Corporation. All Rights Reserved.
>> +#
>> +# FS QA Test No. generic/046
>> +#
>> +# Test writes to falloc file with filesize > 4GB and make sure to verify
>> +# the file checksum both before and after mount.
>> +# This test is to check whether unwritten extents gets properly converted
>> +# to written extent on a filesystem with bs < ps.
> 
> Mention dioread_nolock mount option as well here?

yup, will mention that.

> 
>> +#
>> +seq=`basename $0`
>> +seqres=$RESULT_DIR/$seq
>> +echo "QA output created by $seq"
>> +
>> +here=`pwd`
>> +tmp=/tmp/$$
>> +status=1	# failure is the default!
>> +trap "_cleanup; exit \$status" 0 1 2 3 15
>> +
>> +_cleanup()
>> +{
>> +	cd /
>> +	rm -f $tmp.*
>> +}
>> +
>> +# get standard environment, filters and checks
>> +. ./common/rc
>> +. ./common/filter
>> +
>> +# remove previous $seqres.full before test
>> +rm -f $seqres.full
>> +
>> +# Modify as appropriate.
>> +_supported_fs ext4
> 
> I don't see any ext4 specific setups in the test, so it could be a
> generic test.

as mentioned in above comments. Let me enforce the dioread_nolock
ext4 mount opt for this test.

> 
>> +_supported_os Linux
> 
> "_suppored_os" is removed, just drop this line.

ok.

> 
>> +_require_scratch
>> +_require_xfs_io_command "falloc"
>> +
>> +# keep 4k as a blksz for 64k pagesz
>> +pagesz=$(getconf PAGE_SIZE)
>> +if [ $pagesz -eq 65536 ]; then
>> +	blksz=4096
>> +else
>> +	blksz=$(($pagesz/4))
>> +fi
>> +
>> +devsize=`blockdev --getsize64 $SCRATCH_DEV`
>> +if [ $devsize -lt 6442450944 ]; then
>> +	_notrun "Too small scratch device, need at least 6G"
>> +fi
> 
> _require_scratch_size is used to do this check.

ok.

> 
>> +
>> +# Test for bs < ps
>> +export MKFS_OPTIONS="-F -b $blksz"
>> +_scratch_mkfs >> $seqres.full 2>&1
>> +_scratch_mount >> $seqres.full 2>&1
>> +
>> +# check blksz
>> +real_blksz=$(_get_file_block_size $SCRATCH_MNT)
>> +test $real_blksz != $blksz && _notrun "Failed to format with small blocksize."
>> +
>> +testfile=$SCRATCH_MNT/testfile-$seq
>> +
>> +# Fallocate testfile with size > 4G
>> +fsize=$((5 * 1024 * 1024 * 1024))
>> +$XFS_IO_PROG -f -c "falloc 0 $fsize" $testfile >> $seqres.full 2>&1
>> +
>> +# First write at offset < 4G (at few alternative blks)
>> +off=$((3 * 1024 * 1024 * 1024))
>> +for i in 1 2 3 4; do
>> +	$XFS_IO_PROG -f \
>> +		-c "pwrite $off $blksz" \
>> +		$testfile >> $seqres.full 2>&1
>> +	off=$(($off + (2*$blksz)))
>> +done
>> +
>> +# Then write at offset > 4G (at few alternative blks) to check
>> +# any 32bit overflow case in map.m_lblk
>> +off=$((4 * 1024 * 1024 * 1024))
>> +for i in 1 2 3 4; do
>> +	$XFS_IO_PROG -f \
>> +		-c "pwrite $off $blksz" \
>> +		$testfile >> $seqres.full 2>&1
>> +	off=$(($off + (2*$blksz)))
>> +done
>> +
>> +# ==== Pre-Remount ===
>> +md5_pre=`md5sum $testfile | cut -d' ' -f1`
>> +echo "Pre-Remount md5sum of $testfile = $md5_pre" >> $seqres.full
>> +
>> +_scratch_cycle_mount
>> +
>> +# ==== Post-Remount ===
>> +md5_post=`md5sum $testfile | cut -d' ' -f1`
>> +echo "Post-Remount md5sum of $testfile = $md5_post" >> $seqres.full
>> +test $md5_pre != $md5_post && echo "md5sum mismatch"
>> +
>> +# success, all done
>> +echo "Silence is golden"
>> +status=0
>> +exit
>> diff --git a/tests/ext4/046.out b/tests/ext4/046.out
>> new file mode 100644
>> index 000000000000..52c445eb70bb
>> --- /dev/null
>> +++ b/tests/ext4/046.out
>> @@ -0,0 +1,2 @@
>> +QA output created by 046
>> +Silence is golden
>> diff --git a/tests/ext4/group b/tests/ext4/group
>> index 40351fd9ca0c..02a499a9d220 100644
>> --- a/tests/ext4/group
>> +++ b/tests/ext4/group
>> @@ -48,6 +48,7 @@
>>   043 auto quick
>>   044 auto quick
>>   045 auto dir
>> +046 auto quick
> 
> Also add 'prealloc' group

ok.

> 
> Thanks,
> Eryu
> 
>>   271 auto rw quick
>>   301 aio auto ioctl rw stress defrag
>>   302 aio auto ioctl rw stress defrag
>> -- 
>> 2.26.2