[PATCH fstests v2 0/3] fstests: add appropriate checks for fs features for some tests

[PATCH fstests v2 0/3] fstests: add appropriate checks for fs features for some tests

[Jeff Layton]

A number of fstests fail on NFS, primarily because it lacks certain
features that are widely supported on local filesystems. This set fixes
up the test for POSIX ACLs and adds new checks for FIEMAP and file
capability support on tests that require these features.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
Jeff Layton (3):
      common/attr: fix the _require_acl test
      generic/513: limit to filesystems that support capabilities
      generic/578: only run on filesystems that support FIEMAP

 common/attr       |  9 ++++-----
 common/rc         | 26 ++++++++++++++++++++++++++
 tests/generic/513 |  1 +
 tests/generic/578 |  1 +
 4 files changed, 32 insertions(+), 5 deletions(-)
---
base-commit: 8de535c53887bb49adae74a1b2e83e77d7e8457d
change-id: 20230824-fixes-914cc3f9ef72

Best regards,
-- 
Jeff Layton <jlayton@kernel.org>

[PATCH fstests v2 1/3] common/attr: fix the _require_acl test

[Jeff Layton]

_require_acl tests whether you're able to fetch the ACL from a file
using chacl, and then tests for an -EOPNOTSUPP error return.
Unfortunately, filesystems that don't support them (like NFSv4) just
return -ENODATA when someone calls getxattr for the POSIX ACL, so the
test doesn't work.

Fix the test to have chacl set an ACL on the file instead, which should
reliably fail on filesystems that don't support them.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 common/attr | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/common/attr b/common/attr
index cce4d1b201b2..3ebba682c894 100644
--- a/common/attr
+++ b/common/attr
@@ -163,13 +163,12 @@ _require_acls()
     [ -n "$CHACL_PROG" ] || _notrun "chacl command not found"
 
     #
-    # Test if chacl is able to list ACLs on the target filesystems.  On really
-    # old kernels the system calls might not be implemented at all, but the
-    # more common case is that the tested filesystem simply doesn't support
-    # ACLs.
+    # Test if chacl is able to set an ACL on a file.  On really old kernels
+    # the system calls might not be implemented at all, but the more common
+    # case is that the tested filesystem simply doesn't support ACLs.
     #
     touch $TEST_DIR/syscalltest
-    chacl -l $TEST_DIR/syscalltest > $TEST_DIR/syscalltest.out 2>&1
+    chacl 'u::rw-,g::---,o::---' $TEST_DIR/syscalltest > $TEST_DIR/syscalltest.out 2>&1
     cat $TEST_DIR/syscalltest.out >> $seqres.full
 
     if grep -q 'Function not implemented' $TEST_DIR/syscalltest.out; then

-- 
2.41.0

[PATCH fstests v2 2/3] generic/513: limit to filesystems that support capabilities

[Jeff Layton]

This test requires being able to set file capabilities which some
filesystems (namely NFS) do not support. Add a _require_setcap test
and only run it on filesystems that pass it.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 common/rc         | 13 +++++++++++++
 tests/generic/513 |  1 +
 2 files changed, 14 insertions(+)

diff --git a/common/rc b/common/rc
index 5c4429ed0425..33e74d20c28b 100644
--- a/common/rc
+++ b/common/rc
@@ -5048,6 +5048,19 @@ _require_mknod()
 	rm -f $TEST_DIR/$seq.null
 }
 
+_require_setcap()
+{
+	local testfile=$TEST_DIR/setcaptest.$$
+
+	touch $testfile
+	$SETCAP_PROG "cap_sys_module=p" $testfile > $testfile.out 2>&1
+	if grep -q 'Operation not supported' $testfile.out; then
+	  _notrun "File capabilities are not supported on this filesystem"
+	fi
+
+	rm -f $testfile $testfile.out
+}
+
 _getcap()
 {
 	$GETCAP_PROG "$@" | _filter_getcap
diff --git a/tests/generic/513 b/tests/generic/513
index dc082787ae4e..52f9eb916b4a 100755
--- a/tests/generic/513
+++ b/tests/generic/513
@@ -18,6 +18,7 @@ _supported_fs generic
 _require_scratch_reflink
 _require_command "$GETCAP_PROG" getcap
 _require_command "$SETCAP_PROG" setcap
+_require_setcap
 
 _scratch_mkfs >>$seqres.full 2>&1
 _scratch_mount

-- 
2.41.0

Re: [PATCH fstests v2 2/3] generic/513: limit to filesystems that support capabilities

[Zorro Lang]

On Thu, Aug 24, 2023 at 12:44:18PM -0400, Jeff Layton wrote:
> This test requires being able to set file capabilities which some
> filesystems (namely NFS) do not support. Add a _require_setcap test
> and only run it on filesystems that pass it.
> 
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>  common/rc         | 13 +++++++++++++
>  tests/generic/513 |  1 +
>  2 files changed, 14 insertions(+)
> 
> diff --git a/common/rc b/common/rc
> index 5c4429ed0425..33e74d20c28b 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -5048,6 +5048,19 @@ _require_mknod()
>  	rm -f $TEST_DIR/$seq.null
>  }
>  
> +_require_setcap()
> +{
> +	local testfile=$TEST_DIR/setcaptest.$$
> +
> +	touch $testfile
> +	$SETCAP_PROG "cap_sys_module=p" $testfile > $testfile.out 2>&1

Actually we talked about the capabilities checking helper last year, as below:

https://lore.kernel.org/fstests/20220323023845.saj5en74km7aibdx@zlang-mailbox/

As you bring this discussion back, how about the _require_capabilities() in
above link?

Thanks,
Zorro

> +	if grep -q 'Operation not supported' $testfile.out; then
> +	  _notrun "File capabilities are not supported on this filesystem"
> +	fi
> +
> +	rm -f $testfile $testfile.out
> +}
> +
>  _getcap()
>  {
>  	$GETCAP_PROG "$@" | _filter_getcap
> diff --git a/tests/generic/513 b/tests/generic/513
> index dc082787ae4e..52f9eb916b4a 100755
> --- a/tests/generic/513
> +++ b/tests/generic/513
> @@ -18,6 +18,7 @@ _supported_fs generic
>  _require_scratch_reflink
>  _require_command "$GETCAP_PROG" getcap
>  _require_command "$SETCAP_PROG" setcap
> +_require_setcap
>  
>  _scratch_mkfs >>$seqres.full 2>&1
>  _scratch_mount
> 
> -- 
> 2.41.0
> 

Re: [PATCH fstests v2 2/3] generic/513: limit to filesystems that support capabilities

[Jeff Layton]

On Fri, 2023-08-25 at 22:11 +0800, Zorro Lang wrote:
> On Thu, Aug 24, 2023 at 12:44:18PM -0400, Jeff Layton wrote:
> > This test requires being able to set file capabilities which some
> > filesystems (namely NFS) do not support. Add a _require_setcap test
> > and only run it on filesystems that pass it.
> > 
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> >  common/rc         | 13 +++++++++++++
> >  tests/generic/513 |  1 +
> >  2 files changed, 14 insertions(+)
> > 
> > diff --git a/common/rc b/common/rc
> > index 5c4429ed0425..33e74d20c28b 100644
> > --- a/common/rc
> > +++ b/common/rc
> > @@ -5048,6 +5048,19 @@ _require_mknod()
> >  	rm -f $TEST_DIR/$seq.null
> >  }
> >  
> > +_require_setcap()
> > +{
> > +	local testfile=$TEST_DIR/setcaptest.$$
> > +
> > +	touch $testfile
> > +	$SETCAP_PROG "cap_sys_module=p" $testfile > $testfile.out 2>&1
> 
> Actually we talked about the capabilities checking helper last year, as below:
> 
> https://lore.kernel.org/fstests/20220323023845.saj5en74km7aibdx@zlang-mailbox/
> 
> As you bring this discussion back, how about the _require_capabilities() in
> above link?
> 

I was testing a similar patch, but your version looks better. Should I
drop mine and you re-post yours?

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

Re: [PATCH fstests v2 2/3] generic/513: limit to filesystems that support capabilities

[Zorro Lang]

On Fri, Aug 25, 2023 at 11:02:40AM -0400, Jeff Layton wrote:
> On Fri, 2023-08-25 at 22:11 +0800, Zorro Lang wrote:
> > On Thu, Aug 24, 2023 at 12:44:18PM -0400, Jeff Layton wrote:
> > > This test requires being able to set file capabilities which some
> > > filesystems (namely NFS) do not support. Add a _require_setcap test
> > > and only run it on filesystems that pass it.
> > > 
> > > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > > ---
> > >  common/rc         | 13 +++++++++++++
> > >  tests/generic/513 |  1 +
> > >  2 files changed, 14 insertions(+)
> > > 
> > > diff --git a/common/rc b/common/rc
> > > index 5c4429ed0425..33e74d20c28b 100644
> > > --- a/common/rc
> > > +++ b/common/rc
> > > @@ -5048,6 +5048,19 @@ _require_mknod()
> > >  	rm -f $TEST_DIR/$seq.null
> > >  }
> > >  
> > > +_require_setcap()
> > > +{
> > > +	local testfile=$TEST_DIR/setcaptest.$$
> > > +
> > > +	touch $testfile
> > > +	$SETCAP_PROG "cap_sys_module=p" $testfile > $testfile.out 2>&1
> > 
> > Actually we talked about the capabilities checking helper last year, as below:
> > 
> > https://lore.kernel.org/fstests/20220323023845.saj5en74km7aibdx@zlang-mailbox/
> > 
> > As you bring this discussion back, how about the _require_capabilities() in
> > above link?
> > 
> 
> I was testing a similar patch, but your version looks better. Should I
> drop mine and you re-post yours?

Actually we decided to use `_require_attrs security`, rather than a new
_require_capabilities() helper. We need a chance/requirement to add
that helper (when a test case really need it).

So I hope know is `_require_attrs security` enough for you? Or you really
need a specific _require_capabilities helper?

Thanks,
Zorro

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

Re: [PATCH fstests v2 2/3] generic/513: limit to filesystems that support capabilities

[Jeff Layton]

On Sun, 2023-08-27 at 20:45 +0800, Zorro Lang wrote:
> On Fri, Aug 25, 2023 at 11:02:40AM -0400, Jeff Layton wrote:
> > On Fri, 2023-08-25 at 22:11 +0800, Zorro Lang wrote:
> > > On Thu, Aug 24, 2023 at 12:44:18PM -0400, Jeff Layton wrote:
> > > > This test requires being able to set file capabilities which some
> > > > filesystems (namely NFS) do not support. Add a _require_setcap test
> > > > and only run it on filesystems that pass it.
> > > > 
> > > > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > > > ---
> > > >  common/rc         | 13 +++++++++++++
> > > >  tests/generic/513 |  1 +
> > > >  2 files changed, 14 insertions(+)
> > > > 
> > > > diff --git a/common/rc b/common/rc
> > > > index 5c4429ed0425..33e74d20c28b 100644
> > > > --- a/common/rc
> > > > +++ b/common/rc
> > > > @@ -5048,6 +5048,19 @@ _require_mknod()
> > > >  	rm -f $TEST_DIR/$seq.null
> > > >  }
> > > >  
> > > > +_require_setcap()
> > > > +{
> > > > +	local testfile=$TEST_DIR/setcaptest.$$
> > > > +
> > > > +	touch $testfile
> > > > +	$SETCAP_PROG "cap_sys_module=p" $testfile > $testfile.out 2>&1
> > > 
> > > Actually we talked about the capabilities checking helper last year, as below:
> > > 
> > > https://lore.kernel.org/fstests/20220323023845.saj5en74km7aibdx@zlang-mailbox/
> > > 
> > > As you bring this discussion back, how about the _require_capabilities() in
> > > above link?
> > > 
> > 
> > I was testing a similar patch, but your version looks better. Should I
> > drop mine and you re-post yours?
> 
> Actually we decided to use `_require_attrs security`, rather than a new
> _require_capabilities() helper. We need a chance/requirement to add
> that helper (when a test case really need it).
> 
> So I hope know is `_require_attrs security` enough for you? Or you really
> need a specific _require_capabilities helper?
> 
> Thanks,
> Zorro
> 
> 


Yeah, it looks like that should work:

    generic/513       [not run] attr namespace security not supported by this filesystem type: nfs

I'll plan to respin this patch to add that instead.

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

Re: [PATCH fstests v2 2/3] generic/513: limit to filesystems that support capabilities

[Zorro Lang]

On Sun, Aug 27, 2023 at 09:43:41AM -0400, Jeff Layton wrote:
> On Sun, 2023-08-27 at 20:45 +0800, Zorro Lang wrote:
> > On Fri, Aug 25, 2023 at 11:02:40AM -0400, Jeff Layton wrote:
> > > On Fri, 2023-08-25 at 22:11 +0800, Zorro Lang wrote:
> > > > On Thu, Aug 24, 2023 at 12:44:18PM -0400, Jeff Layton wrote:
> > > > > This test requires being able to set file capabilities which some
> > > > > filesystems (namely NFS) do not support. Add a _require_setcap test
> > > > > and only run it on filesystems that pass it.
> > > > > 
> > > > > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > > > > ---
> > > > >  common/rc         | 13 +++++++++++++
> > > > >  tests/generic/513 |  1 +
> > > > >  2 files changed, 14 insertions(+)
> > > > > 
> > > > > diff --git a/common/rc b/common/rc
> > > > > index 5c4429ed0425..33e74d20c28b 100644
> > > > > --- a/common/rc
> > > > > +++ b/common/rc
> > > > > @@ -5048,6 +5048,19 @@ _require_mknod()
> > > > >  	rm -f $TEST_DIR/$seq.null
> > > > >  }
> > > > >  
> > > > > +_require_setcap()
> > > > > +{
> > > > > +	local testfile=$TEST_DIR/setcaptest.$$
> > > > > +
> > > > > +	touch $testfile
> > > > > +	$SETCAP_PROG "cap_sys_module=p" $testfile > $testfile.out 2>&1
> > > > 
> > > > Actually we talked about the capabilities checking helper last year, as below:
> > > > 
> > > > https://lore.kernel.org/fstests/20220323023845.saj5en74km7aibdx@zlang-mailbox/
> > > > 
> > > > As you bring this discussion back, how about the _require_capabilities() in
> > > > above link?
> > > > 
> > > 
> > > I was testing a similar patch, but your version looks better. Should I
> > > drop mine and you re-post yours?
> > 
> > Actually we decided to use `_require_attrs security`, rather than a new
> > _require_capabilities() helper. We need a chance/requirement to add
> > that helper (when a test case really need it).
> > 
> > So I hope know is `_require_attrs security` enough for you? Or you really
> > need a specific _require_capabilities helper?
> > 
> > Thanks,
> > Zorro
> > 
> > 
> 
> 
> Yeah, it looks like that should work:
> 
>     generic/513       [not run] attr namespace security not supported by this filesystem type: nfs
> 
> I'll plan to respin this patch to add that instead.

Thanks:) I saw you've sent a V3:
[PATCH fstests v3 0/2] fstests: add appropriate checks for fs features for some tests

so will you sent a V4 contains 3 patches?

Thanks,
Zorro

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

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

[Jeff Layton]

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
+}
+
 _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

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

[Darrick J. Wong]

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
> 

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

[Jeff Layton]

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

-- 
Jeff Layton <jlayton@kernel.org>

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

[Zorro Lang]

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

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

[Jeff Layton]

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>

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

[Darrick J. Wong]

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>

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

[Jeff Layton]

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>