Re: [PATCH fstests v2 3/3] generic/578: only run on filesystems that support FIEMAP

[Jeff Layton <jlayton@kernel.org>]

On Fri, 2023-08-25 at 08:18 -0700, Darrick J. Wong wrote:
> On Fri, Aug 25, 2023 at 10:57:20AM -0400, Jeff Layton wrote:
> > On Fri, 2023-08-25 at 22:16 +0800, Zorro Lang wrote:
> > > On Thu, Aug 24, 2023 at 01:28:26PM -0400, Jeff Layton wrote:
> > > > On Thu, 2023-08-24 at 10:09 -0700, Darrick J. Wong wrote:
> > > > > On Thu, Aug 24, 2023 at 12:44:19PM -0400, Jeff Layton wrote:
> > > > > > Some filesystems (e.g. NFS) don't support FIEMAP. Limit generic/578 to
> > > > > > filesystems that do.
> > > > > > 
> > > > > > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > > > > > ---
> > > > > >  common/rc         | 13 +++++++++++++
> > > > > >  tests/generic/578 |  1 +
> > > > > >  2 files changed, 14 insertions(+)
> > > > > > 
> > > > > > diff --git a/common/rc b/common/rc
> > > > > > index 33e74d20c28b..98d27890f6f7 100644
> > > > > > --- a/common/rc
> > > > > > +++ b/common/rc
> > > > > > @@ -3885,6 +3885,19 @@ _require_metadata_journaling()
> > > > > >  	fi
> > > > > >  }
> > > > > >  
> > > > > > +_require_fiemap()
> > > > > > +{
> > > > > > +	local testfile=$TEST_DIR/fiemaptest.$$
> > > > > > +
> > > > > > +	touch $testfile
> > > > > > +	$XFS_IO_PROG -r -c "fiemap" $testfile 1>$testfile.out 2>&1
> > > > > > +	if grep -q 'Operation not supported' $testfile.out; then
> > > > > > +	  _notrun "FIEMAP is not supported by this filesystem"
> > > > > > +	fi
> > > > > > +
> > > > > > +	rm -f $testfile $testfile.out
> > > > > > +}
> > > > > 
> > > > > _require_xfs_io_command "fiemap" ?
> > > > > 
> > > > > 
> > > > 
> > > > Ok, I figured we'd probably do this test after testing for that
> > > > separately, but you're correct that we do require it here.
> > > > 
> > > > If we add that, should we also do this, at least in all of the general
> > > > tests?
> > > > 
> > > >     s/_require_xfs_io_command "fiemap"/_require_fiemap/
> > > > 
> > > > I think we end up excluding some of those tests on NFS for other
> > > > reasons, but other filesystems that don't support fiemap might still try
> > > > to run these tests.
> > > 
> > > We have lots of cases contains _require_xfs_io_command "fiemap", so I think
> > > we can keep this "tradition", don't bring a new _require_fiemap for now,
> > > so ...
> > > 
> > > >  
> > > > > > +
> > > > > >  _count_extents()
> > > > > >  {
> > > > > >  	$XFS_IO_PROG -r -c "fiemap" $1 | tail -n +2 | grep -v hole | wc -l
> > > > > > diff --git a/tests/generic/578 b/tests/generic/578
> > > > > > index b024f6ff90b4..903055b2ca58 100755
> > > > > > --- a/tests/generic/578
> > > > > > +++ b/tests/generic/578
> > > > > > @@ -26,6 +26,7 @@ _require_test_program "mmap-write-concurrent"
> > > > > >  _require_command "$FILEFRAG_PROG" filefrag
> > > > > >  _require_test_reflink
> > > > > >  _require_cp_reflink
> > > > > > +_require_fiemap
> > > 
> > > _require_xfs_io_command "fiemap"
> > > 
> > 
> > That's not sufficient -- there is already a call to that in this test.
> > 
> > _require_xfs_io_command just validates that the xfs_io binary has
> > plumbing for that command (which just issues an ioctl to the file).
> > Even if the binary has support, the underlying filesystem has to support
> > the ioctl.
> > 
> > Many don't, so we need to test for that specifically.
> 
> It /does/ test the kernel support for fiemap...
> 
> _require_xfs_io_command()
> {
> ...
> 	"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="$param"
> 		;;
> 

I stand corrected! We just need to add this to generic/578, like Zorro
suggested:

    _require_xfs_io_command "fiemap"

I'll respin the patch to just do that instead.

Thanks!
-- 
Jeff Layton <jlayton@kernel.org>