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@xxxxxxxxxx>
> > > > > > ---
> > > > > > 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@xxxxxxxxxx>
