Re: [PATCH 3/4] xfs/547: Verify that the correct inode extent counters are updated with/without nrext64

[Chandan Babu R <chandan.babu@oracle.com>]

On Wed, Jun 08, 2022 at 11:59:33 AM +0800, Zorro Lang wrote:
> On Tue, Jun 07, 2022 at 03:06:58PM +0530, Chandan Babu R wrote:
>> On Mon, Jun 06, 2022 at 08:40:47 AM -0700, Darrick J. Wong wrote:
>> > On Mon, Jun 06, 2022 at 06:11:00PM +0530, Chandan Babu R wrote:
>> >> This commit adds a new test to verify if the correct inode extent counter
>> >> fields are updated with/without nrext64 mkfs option.
>> >> 
>> >> Signed-off-by: Chandan Babu R <chandan.babu@oracle.com>
>> >> ---
>> >>  tests/xfs/547     | 91 +++++++++++++++++++++++++++++++++++++++++++++++
>> >>  tests/xfs/547.out | 13 +++++++
>> >>  2 files changed, 104 insertions(+)
>> >>  create mode 100755 tests/xfs/547
>> >>  create mode 100644 tests/xfs/547.out
>> >> 
>> >> diff --git a/tests/xfs/547 b/tests/xfs/547
>> >> new file mode 100755
>> >> index 00000000..d5137ca7
>> >> --- /dev/null
>> >> +++ b/tests/xfs/547
>> >> @@ -0,0 +1,91 @@
>> >> +#! /bin/bash
>> >> +# SPDX-License-Identifier: GPL-2.0
>> >> +# Copyright (c) 2022 Oracle.  All Rights Reserved.
>> >> +#
>> >> +# FS QA Test 547
>> >> +#
>> >> +# Verify that correct inode extent count fields are populated with and without
>> >> +# nrext64 feature.
>> >> +#
>> >> +. ./common/preamble
>> >> +_begin_fstest auto quick metadata
>> >> +
>> >> +# Import common functions.
>> >> +. ./common/filter
>> >> +. ./common/attr
>> >> +. ./common/inject
>> >> +. ./common/populate
>> >> +
>> >> +# real QA test starts here
>> >> +_supported_fs xfs
>> >> +_require_scratch
>> >> +_require_scratch_xfs_nrext64
>> >> +_require_attrs
>> >> +_require_xfs_debug
>> >> +_require_test_program "punch-alternating"
>> >> +_require_xfs_io_error_injection "bmap_alloc_minlen_extent"
>> >> +
>> >> +for nrext64 in 0 1; do
>> >> +	echo "* Verify extent counter fields with nrext64=${nrext64} option"
>> >> +
>> >> +	_scratch_mkfs -i nrext64=${nrext64} -d size=$((512 * 1024 * 1024)) \
>> >> +		      >> $seqres.full
>> >> +	_scratch_mount >> $seqres.full
>> >> +
>> >> +	bsize=$(_get_file_block_size $SCRATCH_MNT)
>> >> +
>> >> +	testfile=$SCRATCH_MNT/testfile
>> >> +
>> >> +	nr_blks=20
>> >> +
>> >> +	echo "Add blocks to test file's data fork"
>> >> +	$XFS_IO_PROG -f -c "pwrite 0 $((nr_blks * bsize))" $testfile \
>> >> +		     >> $seqres.full
>> >> +	$here/src/punch-alternating $testfile
>> >> +
>> >> +	echo "Consume free space"
>> >> +	fillerdir=$SCRATCH_MNT/fillerdir
>> >> +	nr_free_blks=$(stat -f -c '%f' $SCRATCH_MNT)
>> >> +	nr_free_blks=$((nr_free_blks * 90 / 100))
>> >> +
>> >> +	_fill_fs $((bsize * nr_free_blks)) $fillerdir $bsize 0 \
>> >> +		 >> $seqres.full 2>&1
>> >> +
>> >> +	echo "Create fragmented filesystem"
>> >> +	for dentry in $(ls -1 $fillerdir/); do
>> >> +		$here/src/punch-alternating $fillerdir/$dentry >> $seqres.full
>> >> +	done
>> >> +
>> >> +	echo "Inject bmap_alloc_minlen_extent error tag"
>> >> +	_scratch_inject_error bmap_alloc_minlen_extent 1
>> >> +
>> >> +	echo "Add blocks to test file's attr fork"
>> >> +	attr_len=255
>> >> +	nr_attrs=$((nr_blks * bsize / attr_len))
>> >> +	for i in $(seq 1 $nr_attrs); do
>> >> +		attr="$(printf "trusted.%0247d" $i)"
>> >> +		$SETFATTR_PROG -n "$attr" $testfile >> $seqres.full 2>&1
>> >> +		[[ $? != 0 ]] && break
>> >> +	done
>> >> +
>> >> +	testino=$(stat -c '%i' $testfile)
>> >> +
>> >> +	_scratch_unmount >> $seqres.full
>> >> +
>> >> +	dcnt=$(_scratch_xfs_get_metadata_field core.nextents "inode $testino")
>> >> +	acnt=$(_scratch_xfs_get_metadata_field core.naextents "inode $testino")
>> >
>> > Note: For any test requiring functionality added after 5.10, you can use
>> > the xfs_db path command to avoid this sort of inode number saving:
>> >
>> > dcnt=$(_scratch_xfs_get_metadata_field core.nextents "path /testfile")
>> >
>> 
>> Ok. I will post a v2 of the patchset to include the above suggestion.
>
> _require_xfs_db_command path ?
>

I think I should include the above line in the script to make explicit to the
reader the requirements to run the test.

> Looks like the 'path' command is a new command will be in linux and xfsprogs
> 5.10.
>
> It's not always recommended to use latest features/tools, that depends what does
> this case test for. If this case is only for a bug/feature in 5.10, then the
> 'path' command is fine. If it's a common test case for old and new kernels, then
> this new command isn't recommended, that will cause this test can't be run on
> old system.

Large extent counters will be introduced in Linux v5.19. Hence I think it is
safe to assume that xfs_db's 'path' command will be available in xfsprogs
which supports Large extent counters.

>
> BTW, you'd better to not use a fixed case number in the patch subject, if the
> patch is a new case. Due to the number might be changed when we merge it. And
> a fixed case number in subject, might cause others feel this's a known case
> update, not a new case.

Sorry about that. I will add only the summary of the test in the subject line.

Thanks for the review comments!

-- 
chandan