| Message ID | 20230824-fixes-v2-3-d60c2faf1057@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fstests: add appropriate checks for fs features for some tests | expand |
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" ? --D > + > _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 > > compare() { > for i in $(seq 1 8); do > > -- > 2.41.0 >
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. > > + > > _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 > > > > compare() { > > for i in $(seq 1 8); do > > > > -- > > 2.41.0 > >
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" > > > > > > compare() { > > > for i in $(seq 1 8); do > > > > > > -- > > > 2.41.0 > > > > > -- > 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> > > >
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" ;; --D > > > > > > > > > > compare() { > > > > > for i in $(seq 1 8); do > > > > > > > > > > -- > > > > > 2.41.0 > > > > > > > > > > > -- > > > Jeff Layton <jlayton@kernel.org> > > > > > > > -- > 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!
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 +} + _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 compare() { for i in $(seq 1 8); do
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(+)