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

[Zorro Lang <zlang@redhat.com>]

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 ?

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.

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.

Thanks,
Zorro

> 
> > Up to you if you want to change the test to do that; otherwise,
> > Reviewed-by: Darrick J. Wong <djwong@kernel.org>
> >
> 
> Thanks for the review.
> 
> -- 
> chandan
>