Re: [PATCH v5] xfs: initial fiemap range query test

[Nikolay Borisov <nborisov@suse.com>]

On 22.11.2017 08:47, Eryu Guan wrote:
> On Tue, Nov 21, 2017 at 05:16:56PM +0200, Nikolay Borisov wrote:
>> Fiemap gained support for passing in optional offset len
>> which denote the range requested, so this patch adds
>> testcases for this functionality. Aditionally, a special "ranged"
>> argument is added to the require_xfs_io_command which checks
>> for the presence of fiemap range support.
>>
>> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
>> ---
>> V5: 
>>  * Drop changes to existing generic punch hole tests since 
>>  the new fiemap implementation don't require them 
>>  * Merge the common/rc change with this patch
>>  * Added Data + Hole + Data and Hole + Data tests as per Eryu's request
>>  * Adjusted output of some tests which fall in a hole, since holes are
>>  truncated to passed range
>>  * Simplified the logic to check for fiemap range support (Eryu)
> 
> Thanks for the 5th version, looks good overall, though I have more
> comments, sorry for not bringing them up in previous reviews...
> 
>>
>> V4: 
>>  * Added test description
>>  * Added new test for past-eof behavior
>>  * Removed supported_generic_fs line
>>  * Switched to using the "ranged" param require
>>  * Revert v3 changes
>>
>> V3:
>>  * Adjusted tests for '-r' fiemap param
>>  * Tests for invalid -r combination
>>
>> V2: No change
>> V1: No change
>>  common/rc         |   7 +++
>>  tests/xfs/900     | 101 +++++++++++++++++++++++++++++++++++++++++++
>>  tests/xfs/900.out | 125 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 233 insertions(+)
>>  create mode 100755 tests/xfs/900
>>  create mode 100644 tests/xfs/900.out
>>
>> diff --git a/common/rc b/common/rc
>> index e2a8229..d0dd32d 100644
>> --- a/common/rc
>> +++ b/common/rc
>> @@ -2053,6 +2053,13 @@ _require_xfs_io_command()
>>  			-c "$command 4k 8k" $testfile 2>&1`
>>  		;;
>>  	"fiemap")
>> +		# If 'ranged' is passed as argument then we check to see if fiemap supports
>> +		# ranged query params
>> +		if echo "$param" | grep -q "ranged"; then
>> +			param=$(echo "$param" | sed "s/ranged//")
>> +			$XFS_IO_PROG -c "help fiemap" | grep -q "\[offset \[len\]\]"
>> +			[ $? -eq 0 ] || _notrun "xfs_io $command ranged support is missing"
>> +		fi
>>  		testio=`$XFS_IO_PROG -F -f -c "pwrite 0 20k" -c "fsync" \
>>  			-c "fiemap -v $param" $testfile 2>&1`
>>  		param_checked=1
>> diff --git a/tests/xfs/900 b/tests/xfs/900
>> new file mode 100755
>> index 0000000..07f4f40
>> --- /dev/null
>> +++ b/tests/xfs/900
>> @@ -0,0 +1,101 @@
>> +#! /bin/bash
>> +# FS QA Test No. 900
>> +
>> +# Test for the new ranged query functionality in xfs_io's fiemap command.
>> +# This tests various combinations of hole + data layout being printed.
>> +# Also the test used 16k holes to be compatible with 16k block filesystems
>> +
>> +#-----------------------------------------------------------------------
>> +# Copyright (c) 2017 SUSE Linux Products GmbH. All Rights Reserved.
>> +# Author: Nikolay Borisov <nborisov@suse.com>
>> +#
>> +# This program is free software; you can redistribute it and/or
>> +# modify it under the terms of the GNU General Public License as
>> +# published by the Free Software Foundation.
>> +#
>> +# This program is distributed in the hope that it would be useful,
>> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> +# GNU General Public License for more details.
>> +#
>> +# You should have received a copy of the GNU General Public License
>> +# along with this program; if not, write the Free Software Foundation,
>> +# Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
>> +#-----------------------------------------------------------------------
>> +#
>> +
>> +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/punch
>> +
>> +# remove previous $seqres.full before test
>> +rm -f $seqres.full
>> +
>> +# real QA test starts here
>> +
>> +# Modify as appropriate.
>> +_supported_os Linux
>> +_require_scratch
> 
> This test is in xfs dir and that means it's an xfs-specific test, better
> to have "_supported_fs xfs" for consistency.
> 
> But I'm wondering why not make it a generic test? There seems nothing
> xfs-specific. And I hacked the test and tested with ext4/btrfs, tests
> all passed.
> 
> Interesting thing is, test failed on XFS as:
> 
>  Hole + Data
>  0: [0..31]: hole
> -1: [32..63]: unwritten
> +1: [32..33]: unwritten
>  Hole + Data + Hole
>  0: [64..95]: hole
>  1: [96..127]: unwritten
> @@ -14,7 +14,7 @@
>  Data + Hole + Data
>  0: [32..63]: unwritten
>  1: [64..95]: hole
> -2: [96..127]: unwritten
> +2: [96..97]: unwritten

This is due to xfs always trimming extents. I've sent a patch:
 xfs: Don't trim extents for fiemap

That harmonizes xfs' behavior in that regard to the other major
filesystems by not trimming extents when called on behalf of iomap. For
once it wasn't a problem in xfs_io's fiemap code :D

> 
> The data extents were truncated. I was using Eric's v6 patch and v4.14+
> kernel, which contains the xfs updates for 4.15-rc1.
> 
>> +_require_xfs_io_command "falloc"
> 
> fpunch is used as well, maybe need "_require_xfs_io_command fpunch".
> 
> But I'd like to avoid falloc and fpunch, so test runs on filesystems
> that don't support falloc/fpunch but support fiemap, e.g. ext3, to get
> better test coverage.
> 
>> +# ranged is a special argument which checks if fiemap supports
>> +# [offset [len]] args
>> +_require_xfs_io_command "fiemap" "ranged"
>> +
>> +_scratch_mkfs > $seqres.full 2>&1
>> +_scratch_mount || _fail "mount failure"
>> +
>> +file=$SCRATCH_MNT/testfile
>> +$XFS_IO_PROG -f -c "falloc 0 1m" $file
>> +for i in {0..31}; do $XFS_IO_PROG -c "fpunch $(($i*32))k 16k" $file; done
> 
> So I'd like to create $file with the following command:
> 
> # Create a file with 16k hole followed by 16k data, and this pattern
> # repeats till it reaches 1M file size, so each extent has 16k data.
> # But truncate file to its final size first, otherwise XFS would merge
> # some extents due to speculative preallocation.
> $XFS_IO_PROG -fc "truncate 1m" $file
> for i in {0..31}; do
> 	$XFS_IO_PROG -c "pwrite $(($i*32+16))k 16k" $file >/dev/null
> done
> 
> Then update .out file, replace all "unwritten" with "data".
> 
> 
> And test is missing an entry in group file.
> 
> Thanks,
> Eryu
>