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 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.

> > > >  
> > > >  compare() {
> > > >  	for i in $(seq 1 8); do
> > > > 
> > > > -- 
> > > > 2.41.0
> > > > 
> > 
> > -- 
> > Jeff Layton <jlayton@kernel.org>
> > 
> 

-- 
Jeff Layton <jlayton@kernel.org>