fstests.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/4] Improvements for NFS
@ 2022-02-08 21:52 Anna Schumaker
  2022-02-08 21:52 ` [PATCH 1/4] check: Export CHECK_OPTIONS and PLATFORM for Xunit Reporting Anna Schumaker
                   ` (3 more replies)
  0 siblings, 4 replies; 12+ messages in thread
From: Anna Schumaker @ 2022-02-08 21:52 UTC (permalink / raw)
  To: fstests; +Cc: linux-nfs

From: Anna Schumaker <Anna.Schumaker@Netapp.com>

This is a collection of fixes that I've had in my private xfstests
instance on my NFS client VM for testing.

 I've been using the "-R xunit" option lately, so the first patch fixes up 
the PLATFORM and CHECK_OPTIONS output in the resulting xunit file. I
wouldn't be surprised if there is a better way to do this, so please let
me know!

The second patch moves a very long running test (generic/531) out of the
quick group because I'm tired of killing it after a half hour. Maybe
it's quick on non networked filesystems, though? I had some other ideas
on how to fix my issue that I included in the patch description.

The last two patches add some extra checks for functionality that NFS
doesn't support, otherwise the tests are getting marked as "failed" with
"whatever isn't supposted" in the test output log. After these patches
the tests are simply skipped.

Thoughts?

Anna


Anna Schumaker (4):
  check: Export CHECK_OPTIONS and PLATFORM for Xunit Reporting
  generic/531: Move test from 'quick' group to 'stress'
  generic/578: Test that filefrag is supported before running
  generic/633: Check if idmapped mounts are supported before running

 check             |  2 ++
 common/rc         | 14 ++++++++++++++
 tests/generic/531 |  2 +-
 tests/generic/578 |  2 +-
 tests/generic/633 |  1 +
 5 files changed, 19 insertions(+), 2 deletions(-)

-- 
2.35.1


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 1/4] check: Export CHECK_OPTIONS and PLATFORM for Xunit Reporting
  2022-02-08 21:52 [PATCH 0/4] Improvements for NFS Anna Schumaker
@ 2022-02-08 21:52 ` Anna Schumaker
  2022-02-08 21:52 ` [PATCH 2/4] generic/531: Move test from 'quick' group to 'stress' Anna Schumaker
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 12+ messages in thread
From: Anna Schumaker @ 2022-02-08 21:52 UTC (permalink / raw)
  To: fstests; +Cc: linux-nfs

From: Anna Schumaker <Anna.Schumaker@Netapp.com>

I found that running ./check by hand with Xunit reporting enabled caused
the PLATFORM variable to be unset, and the CHECK_OPTIONS was the default
"-g auto" instead of the "-g quick" that I also passed in. This patch
sets both variables so Xunit reporting is accurate.

Signed-off-by: Anna Schumaker <Anna.Schumaker@Netapp.com>
---
Maybe there is a better way to do this? If so please let me know!
---
 check | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/check b/check
index a08631213cbb..c8a38345123a 100755
--- a/check
+++ b/check
@@ -273,6 +273,7 @@ _prepare_test_list()
 }
 
 # Process command arguments first.
+export CHECK_OPTIONS="$*"
 while [ $# -gt 0 ]; do
 	case "$1" in
 	-\? | -h | --help) usage ;;
@@ -360,6 +361,7 @@ if ! . ./common/rc; then
 	echo "check: failed to source common/rc"
 	exit 1
 fi
+export PLATFORM=$(_full_platform_details)
 
 if [ -n "$subdir_xfile" ]; then
 	for d in $SRC_GROUPS $FSTYP; do
-- 
2.35.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 2/4] generic/531: Move test from 'quick' group to 'stress'
  2022-02-08 21:52 [PATCH 0/4] Improvements for NFS Anna Schumaker
  2022-02-08 21:52 ` [PATCH 1/4] check: Export CHECK_OPTIONS and PLATFORM for Xunit Reporting Anna Schumaker
@ 2022-02-08 21:52 ` Anna Schumaker
  2022-02-23 16:54   ` Darrick J. Wong
  2022-02-08 21:52 ` [PATCH 3/4] generic/578: Test that filefrag is supported before running Anna Schumaker
  2022-02-08 21:52 ` [PATCH 4/4] generic/633: Check if idmapped mounts are " Anna Schumaker
  3 siblings, 1 reply; 12+ messages in thread
From: Anna Schumaker @ 2022-02-08 21:52 UTC (permalink / raw)
  To: fstests; +Cc: linux-nfs

From: Anna Schumaker <Anna.Schumaker@Netapp.com>

The comment up top says this is a stress test, so at the very least it
should be added to this group. As for removing it from the quick group,
making this test variable on the number of CPUs means this test could
take a very long time to finish (I'm unsure exactly how long on NFS v4.1
because I usually kill it after a half hour or so)

Signed-off-by: Anna Schumaker <Anna.Schumaker@Netapp.com>
---
I have thought of two alternatives to this patch that would work for me:
  1) Could we add an _unsupported_fs function which is the opposite of
     _supported_fs to prevent tests from running on specific filesystems?
  2) Would it be okay to check if $FSTYP == "nfs" when setting nr_cpus,
     and set it to 1 instead? Perhaps through a function in common/rc
     that other tests can use if they scale work based on cpu-count?
---
 tests/generic/531 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/generic/531 b/tests/generic/531
index 5e84ca977b44..62e3cac92423 100755
--- a/tests/generic/531
+++ b/tests/generic/531
@@ -12,7 +12,7 @@
 # Use every CPU possible to stress the filesystem.
 #
 . ./common/preamble
-_begin_fstest auto quick unlink
+_begin_fstest auto stress unlink
 testfile=$TEST_DIR/$seq.txt
 
 # Import common functions.
-- 
2.35.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 3/4] generic/578: Test that filefrag is supported before running
  2022-02-08 21:52 [PATCH 0/4] Improvements for NFS Anna Schumaker
  2022-02-08 21:52 ` [PATCH 1/4] check: Export CHECK_OPTIONS and PLATFORM for Xunit Reporting Anna Schumaker
  2022-02-08 21:52 ` [PATCH 2/4] generic/531: Move test from 'quick' group to 'stress' Anna Schumaker
@ 2022-02-08 21:52 ` Anna Schumaker
  2022-02-23 16:57   ` Darrick J. Wong
  2022-02-08 21:52 ` [PATCH 4/4] generic/633: Check if idmapped mounts are " Anna Schumaker
  3 siblings, 1 reply; 12+ messages in thread
From: Anna Schumaker @ 2022-02-08 21:52 UTC (permalink / raw)
  To: fstests; +Cc: linux-nfs

From: Anna Schumaker <Anna.Schumaker@Netapp.com>

NFS does not support FIBMAP/FIEMAP, so the check for non-shared extents
on NFS v4.2 always fails with the message: "FIBMAP/FIEMAP unsupported".
I added the _require_filefrag check for NFS and other filesystems that
don't have FIEMAP or FIBMAP support.

Signed-off-by: Anna Schumaker <Anna.Schumaker@Netapp.com>
---
 common/rc         | 14 ++++++++++++++
 tests/generic/578 |  2 +-
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/common/rc b/common/rc
index b3289de985d8..73d17da9430e 100644
--- a/common/rc
+++ b/common/rc
@@ -4673,6 +4673,20 @@ _require_inode_limits()
 	fi
 }
 
+_require_filefrag()
+{
+	_require_command "$FILEFRAG_PROG" filefrag
+
+	local file="$TEST_DIR/filefrag_testfile"
+
+	echo "XX" > $file
+	${FILEFRAG_PROG} $file 2>&1 | grep -q "FIBMAP/FIEMAP[[:space:]]*unsupported"
+	if [ $? -eq 0 ]; then
+		_notrun "FIBMAP/FIEMAP not supported by this filesystem"
+	fi
+	rm -f $file
+}
+
 _require_filefrag_options()
 {
 	_require_command "$FILEFRAG_PROG" filefrag
diff --git a/tests/generic/578 b/tests/generic/578
index 01929a280f8c..64c813032cf8 100755
--- a/tests/generic/578
+++ b/tests/generic/578
@@ -23,7 +23,7 @@ _cleanup()
 # real QA test starts here
 _supported_fs generic
 _require_test_program "mmap-write-concurrent"
-_require_command "$FILEFRAG_PROG" filefrag
+_require_filefrag
 _require_test_reflink
 _require_cp_reflink
 
-- 
2.35.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 4/4] generic/633: Check if idmapped mounts are supported before running
  2022-02-08 21:52 [PATCH 0/4] Improvements for NFS Anna Schumaker
                   ` (2 preceding siblings ...)
  2022-02-08 21:52 ` [PATCH 3/4] generic/578: Test that filefrag is supported before running Anna Schumaker
@ 2022-02-08 21:52 ` Anna Schumaker
  2022-02-19  8:40   ` Su Yue
  3 siblings, 1 reply; 12+ messages in thread
From: Anna Schumaker @ 2022-02-08 21:52 UTC (permalink / raw)
  To: fstests; +Cc: linux-nfs

From: Anna Schumaker <Anna.Schumaker@Netapp.com>

This appears to have been missed when the test was added.

Signed-off-by: Anna Schumaker <Anna.Schumaker@Netapp.com>
---
 tests/generic/633 | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/generic/633 b/tests/generic/633
index 382806471223..6750117735f7 100755
--- a/tests/generic/633
+++ b/tests/generic/633
@@ -15,6 +15,7 @@ _begin_fstest auto quick atime attr cap idmapped io_uring mount perms rw unlink
 # real QA test starts here
 
 _supported_fs generic
+_require_idmapped_mounts
 _require_test
 
 echo "Silence is golden"
-- 
2.35.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH 4/4] generic/633: Check if idmapped mounts are supported before running
  2022-02-08 21:52 ` [PATCH 4/4] generic/633: Check if idmapped mounts are " Anna Schumaker
@ 2022-02-19  8:40   ` Su Yue
  0 siblings, 0 replies; 12+ messages in thread
From: Su Yue @ 2022-02-19  8:40 UTC (permalink / raw)
  To: Anna Schumaker; +Cc: fstests, linux-nfs


On Tue 08 Feb 2022 at 16:52, Anna Schumaker <anna@kernel.org> 
wrote:

> From: Anna Schumaker <Anna.Schumaker@Netapp.com>
>
> This appears to have been missed when the test was added.
>
It's removed on purpose to also verify idmapped mounts unrelated 
tests.

Generic/633 fails on NFS because of setgid inheritance behavior of 
NFS.
Already reported it to the community[1].

[1]: 
https://lore.kernel.org/linux-nfs/OS3PR01MB770539462BE3E7959DAF8B5789389@OS3PR01MB7705.jpnprd01.prod.outlook.com/T/#u

--
Su
> Signed-off-by: Anna Schumaker <Anna.Schumaker@Netapp.com>
> ---
>  tests/generic/633 | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/tests/generic/633 b/tests/generic/633
> index 382806471223..6750117735f7 100755
> --- a/tests/generic/633
> +++ b/tests/generic/633
> @@ -15,6 +15,7 @@ _begin_fstest auto quick atime attr cap 
> idmapped io_uring mount perms rw unlink
>  # real QA test starts here
>
>  _supported_fs generic
> +_require_idmapped_mounts
>  _require_test
>
>  echo "Silence is golden"

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/4] generic/531: Move test from 'quick' group to 'stress'
  2022-02-08 21:52 ` [PATCH 2/4] generic/531: Move test from 'quick' group to 'stress' Anna Schumaker
@ 2022-02-23 16:54   ` Darrick J. Wong
  2022-02-23 19:38     ` Anna Schumaker
  0 siblings, 1 reply; 12+ messages in thread
From: Darrick J. Wong @ 2022-02-23 16:54 UTC (permalink / raw)
  To: Anna Schumaker; +Cc: fstests, linux-nfs

On Tue, Feb 08, 2022 at 04:52:30PM -0500, Anna Schumaker wrote:
> From: Anna Schumaker <Anna.Schumaker@Netapp.com>
> 
> The comment up top says this is a stress test, so at the very least it
> should be added to this group. As for removing it from the quick group,
> making this test variable on the number of CPUs means this test could
> take a very long time to finish (I'm unsure exactly how long on NFS v4.1
> because I usually kill it after a half hour or so)
> 
> Signed-off-by: Anna Schumaker <Anna.Schumaker@Netapp.com>
> ---
> I have thought of two alternatives to this patch that would work for me:
>   1) Could we add an _unsupported_fs function which is the opposite of
>      _supported_fs to prevent tests from running on specific filesystems?
>   2) Would it be okay to check if $FSTYP == "nfs" when setting nr_cpus,
>      and set it to 1 instead? Perhaps through a function in common/rc
>      that other tests can use if they scale work based on cpu-count?

How about we create a function to estimate fs threading scalability?
There are probably (simple) filesystems out there with a Big Filesystem
Lock that won't benefit from more CPUs pounding on it...

# Estimate how many writer threads we should start to stress test this
# type of filesystem.
_estimate_threading_factor() {
	case "$FSTYP" in
	"nfs")
		echo 1;;
	*)
		echo $((2 * $(getconf _NPROCESSORS_ONLN) ));;
	esac
}

and later:

nr_cpus=$(_estimate_threading_factor)

Once something like this is landed, we can customize for each FSTYP.  I
suspect that XFS on spinning rust might actually want "2" here, not
nr_cpus*2, given the sporadic complaints about this test taking much
longer for a few people.

> ---
>  tests/generic/531 | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tests/generic/531 b/tests/generic/531
> index 5e84ca977b44..62e3cac92423 100755
> --- a/tests/generic/531
> +++ b/tests/generic/531
> @@ -12,7 +12,7 @@
>  # Use every CPU possible to stress the filesystem.
>  #
>  . ./common/preamble
> -_begin_fstest auto quick unlink
> +_begin_fstest auto stress unlink

As for this change itself,
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D


>  testfile=$TEST_DIR/$seq.txt
>  
>  # Import common functions.
> -- 
> 2.35.1
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 3/4] generic/578: Test that filefrag is supported before running
  2022-02-08 21:52 ` [PATCH 3/4] generic/578: Test that filefrag is supported before running Anna Schumaker
@ 2022-02-23 16:57   ` Darrick J. Wong
  2022-02-23 18:24     ` Anna Schumaker
  0 siblings, 1 reply; 12+ messages in thread
From: Darrick J. Wong @ 2022-02-23 16:57 UTC (permalink / raw)
  To: Anna Schumaker; +Cc: fstests, linux-nfs

On Tue, Feb 08, 2022 at 04:52:31PM -0500, Anna Schumaker wrote:
> From: Anna Schumaker <Anna.Schumaker@Netapp.com>
> 
> NFS does not support FIBMAP/FIEMAP, so the check for non-shared extents
> on NFS v4.2 always fails with the message: "FIBMAP/FIEMAP unsupported".
> I added the _require_filefrag check for NFS and other filesystems that
> don't have FIEMAP or FIBMAP support.
> 
> Signed-off-by: Anna Schumaker <Anna.Schumaker@Netapp.com>
> ---
>  common/rc         | 14 ++++++++++++++
>  tests/generic/578 |  2 +-
>  2 files changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/common/rc b/common/rc
> index b3289de985d8..73d17da9430e 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -4673,6 +4673,20 @@ _require_inode_limits()
>  	fi
>  }
>  
> +_require_filefrag()
> +{
> +	_require_command "$FILEFRAG_PROG" filefrag
> +
> +	local file="$TEST_DIR/filefrag_testfile"
> +
> +	echo "XX" > $file

Nit: You might want to rm -f $file before echoing into it, just in case
some future knave ;) sets up that pathname to point to a named pipe or
something that will hang fstests...

> +	${FILEFRAG_PROG} $file 2>&1 | grep -q "FIBMAP/FIEMAP[[:space:]]*unsupported"
> +	if [ $? -eq 0 ]; then

...and rm it again here to avoid leaving test files around.

Otherwise this looks ok to me.

--D

> +		_notrun "FIBMAP/FIEMAP not supported by this filesystem"
> +	fi
> +	rm -f $file
> +}
> +
>  _require_filefrag_options()
>  {
>  	_require_command "$FILEFRAG_PROG" filefrag
> diff --git a/tests/generic/578 b/tests/generic/578
> index 01929a280f8c..64c813032cf8 100755
> --- a/tests/generic/578
> +++ b/tests/generic/578
> @@ -23,7 +23,7 @@ _cleanup()
>  # real QA test starts here
>  _supported_fs generic
>  _require_test_program "mmap-write-concurrent"
> -_require_command "$FILEFRAG_PROG" filefrag
> +_require_filefrag
>  _require_test_reflink
>  _require_cp_reflink
>  
> -- 
> 2.35.1
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 3/4] generic/578: Test that filefrag is supported before running
  2022-02-23 16:57   ` Darrick J. Wong
@ 2022-02-23 18:24     ` Anna Schumaker
  2022-02-24  4:15       ` Darrick J. Wong
  0 siblings, 1 reply; 12+ messages in thread
From: Anna Schumaker @ 2022-02-23 18:24 UTC (permalink / raw)
  To: Darrick J. Wong; +Cc: fstests, Linux NFS Mailing List

On Wed, Feb 23, 2022 at 11:57 AM Darrick J. Wong <djwong@kernel.org> wrote:
>
> On Tue, Feb 08, 2022 at 04:52:31PM -0500, Anna Schumaker wrote:
> > From: Anna Schumaker <Anna.Schumaker@Netapp.com>
> >
> > NFS does not support FIBMAP/FIEMAP, so the check for non-shared extents
> > on NFS v4.2 always fails with the message: "FIBMAP/FIEMAP unsupported".
> > I added the _require_filefrag check for NFS and other filesystems that
> > don't have FIEMAP or FIBMAP support.
> >
> > Signed-off-by: Anna Schumaker <Anna.Schumaker@Netapp.com>
> > ---
> >  common/rc         | 14 ++++++++++++++
> >  tests/generic/578 |  2 +-
> >  2 files changed, 15 insertions(+), 1 deletion(-)
> >
> > diff --git a/common/rc b/common/rc
> > index b3289de985d8..73d17da9430e 100644
> > --- a/common/rc
> > +++ b/common/rc
> > @@ -4673,6 +4673,20 @@ _require_inode_limits()
> >       fi
> >  }
> >
> > +_require_filefrag()
> > +{
> > +     _require_command "$FILEFRAG_PROG" filefrag
> > +
> > +     local file="$TEST_DIR/filefrag_testfile"
> > +
> > +     echo "XX" > $file
>
> Nit: You might want to rm -f $file before echoing into it, just in case
> some future knave ;) sets up that pathname to point to a named pipe or
> something that will hang fstests...
>
> > +     ${FILEFRAG_PROG} $file 2>&1 | grep -q "FIBMAP/FIEMAP[[:space:]]*unsupported"
> > +     if [ $? -eq 0 ]; then
>
> ...and rm it again here to avoid leaving test files around.

Sure, I can make that change. Should I update
_require_filefrag_options() and _require_fibmap() to follow the same
pattern while I'm at it? The new function was based off of those.

Anna

>
> Otherwise this looks ok to me.
>
> --D
>
> > +             _notrun "FIBMAP/FIEMAP not supported by this filesystem"
> > +     fi
> > +     rm -f $file
> > +}
> > +
> >  _require_filefrag_options()
> >  {
> >       _require_command "$FILEFRAG_PROG" filefrag
> > diff --git a/tests/generic/578 b/tests/generic/578
> > index 01929a280f8c..64c813032cf8 100755
> > --- a/tests/generic/578
> > +++ b/tests/generic/578
> > @@ -23,7 +23,7 @@ _cleanup()
> >  # real QA test starts here
> >  _supported_fs generic
> >  _require_test_program "mmap-write-concurrent"
> > -_require_command "$FILEFRAG_PROG" filefrag
> > +_require_filefrag
> >  _require_test_reflink
> >  _require_cp_reflink
> >
> > --
> > 2.35.1
> >

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/4] generic/531: Move test from 'quick' group to 'stress'
  2022-02-23 16:54   ` Darrick J. Wong
@ 2022-02-23 19:38     ` Anna Schumaker
  2022-02-24  4:14       ` Darrick J. Wong
  0 siblings, 1 reply; 12+ messages in thread
From: Anna Schumaker @ 2022-02-23 19:38 UTC (permalink / raw)
  To: Darrick J. Wong; +Cc: fstests, Linux NFS Mailing List

On Wed, Feb 23, 2022 at 11:54 AM Darrick J. Wong <djwong@kernel.org> wrote:
>
> On Tue, Feb 08, 2022 at 04:52:30PM -0500, Anna Schumaker wrote:
> > From: Anna Schumaker <Anna.Schumaker@Netapp.com>
> >
> > The comment up top says this is a stress test, so at the very least it
> > should be added to this group. As for removing it from the quick group,
> > making this test variable on the number of CPUs means this test could
> > take a very long time to finish (I'm unsure exactly how long on NFS v4.1
> > because I usually kill it after a half hour or so)
> >
> > Signed-off-by: Anna Schumaker <Anna.Schumaker@Netapp.com>
> > ---
> > I have thought of two alternatives to this patch that would work for me:
> >   1) Could we add an _unsupported_fs function which is the opposite of
> >      _supported_fs to prevent tests from running on specific filesystems?
> >   2) Would it be okay to check if $FSTYP == "nfs" when setting nr_cpus,
> >      and set it to 1 instead? Perhaps through a function in common/rc
> >      that other tests can use if they scale work based on cpu-count?
>
> How about we create a function to estimate fs threading scalability?
> There are probably (simple) filesystems out there with a Big Filesystem
> Lock that won't benefit from more CPUs pounding on it...
>
> # Estimate how many writer threads we should start to stress test this
> # type of filesystem.
> _estimate_threading_factor() {
>         case "$FSTYP" in
>         "nfs")
>                 echo 1;;
>         *)
>                 echo $((2 * $(getconf _NPROCESSORS_ONLN) ));;
>         esac
> }
>
> and later:
>
> nr_cpus=$(_estimate_threading_factor)
>
> Once something like this is landed, we can customize for each FSTYP.  I
> suspect that XFS on spinning rust might actually want "2" here, not
> nr_cpus*2, given the sporadic complaints about this test taking much
> longer for a few people.

Sure. Should I do a `git grep` for "nr_cpus" on the other tests and
update them all at the same time, or just leave it with this one file
to start?

Anna
>
> > ---
> >  tests/generic/531 | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/tests/generic/531 b/tests/generic/531
> > index 5e84ca977b44..62e3cac92423 100755
> > --- a/tests/generic/531
> > +++ b/tests/generic/531
> > @@ -12,7 +12,7 @@
> >  # Use every CPU possible to stress the filesystem.
> >  #
> >  . ./common/preamble
> > -_begin_fstest auto quick unlink
> > +_begin_fstest auto stress unlink
>
> As for this change itself,
> Reviewed-by: Darrick J. Wong <djwong@kernel.org>
>
> --D
>
>
> >  testfile=$TEST_DIR/$seq.txt
> >
> >  # Import common functions.
> > --
> > 2.35.1
> >

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/4] generic/531: Move test from 'quick' group to 'stress'
  2022-02-23 19:38     ` Anna Schumaker
@ 2022-02-24  4:14       ` Darrick J. Wong
  0 siblings, 0 replies; 12+ messages in thread
From: Darrick J. Wong @ 2022-02-24  4:14 UTC (permalink / raw)
  To: Anna Schumaker; +Cc: fstests, Linux NFS Mailing List

On Wed, Feb 23, 2022 at 02:38:30PM -0500, Anna Schumaker wrote:
> On Wed, Feb 23, 2022 at 11:54 AM Darrick J. Wong <djwong@kernel.org> wrote:
> >
> > On Tue, Feb 08, 2022 at 04:52:30PM -0500, Anna Schumaker wrote:
> > > From: Anna Schumaker <Anna.Schumaker@Netapp.com>
> > >
> > > The comment up top says this is a stress test, so at the very least it
> > > should be added to this group. As for removing it from the quick group,
> > > making this test variable on the number of CPUs means this test could
> > > take a very long time to finish (I'm unsure exactly how long on NFS v4.1
> > > because I usually kill it after a half hour or so)
> > >
> > > Signed-off-by: Anna Schumaker <Anna.Schumaker@Netapp.com>
> > > ---
> > > I have thought of two alternatives to this patch that would work for me:
> > >   1) Could we add an _unsupported_fs function which is the opposite of
> > >      _supported_fs to prevent tests from running on specific filesystems?
> > >   2) Would it be okay to check if $FSTYP == "nfs" when setting nr_cpus,
> > >      and set it to 1 instead? Perhaps through a function in common/rc
> > >      that other tests can use if they scale work based on cpu-count?
> >
> > How about we create a function to estimate fs threading scalability?
> > There are probably (simple) filesystems out there with a Big Filesystem
> > Lock that won't benefit from more CPUs pounding on it...
> >
> > # Estimate how many writer threads we should start to stress test this
> > # type of filesystem.
> > _estimate_threading_factor() {
> >         case "$FSTYP" in
> >         "nfs")
> >                 echo 1;;
> >         *)
> >                 echo $((2 * $(getconf _NPROCESSORS_ONLN) ));;
> >         esac
> > }
> >
> > and later:
> >
> > nr_cpus=$(_estimate_threading_factor)
> >
> > Once something like this is landed, we can customize for each FSTYP.  I
> > suspect that XFS on spinning rust might actually want "2" here, not
> > nr_cpus*2, given the sporadic complaints about this test taking much
> > longer for a few people.
> 
> Sure. Should I do a `git grep` for "nr_cpus" on the other tests and
> update them all at the same time, or just leave it with this one file
> to start?

Ugh, what a mess...

Unbeknownst to me, "$here/src/feature -o" also returns the CPU count.
If you want to get started on fixing other tests, I'd say ... add the
new helper here for this test, and attach any other conversions as new
patches at the end of the series.

--D

> Anna
> >
> > > ---
> > >  tests/generic/531 | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/tests/generic/531 b/tests/generic/531
> > > index 5e84ca977b44..62e3cac92423 100755
> > > --- a/tests/generic/531
> > > +++ b/tests/generic/531
> > > @@ -12,7 +12,7 @@
> > >  # Use every CPU possible to stress the filesystem.
> > >  #
> > >  . ./common/preamble
> > > -_begin_fstest auto quick unlink
> > > +_begin_fstest auto stress unlink
> >
> > As for this change itself,
> > Reviewed-by: Darrick J. Wong <djwong@kernel.org>
> >
> > --D
> >
> >
> > >  testfile=$TEST_DIR/$seq.txt
> > >
> > >  # Import common functions.
> > > --
> > > 2.35.1
> > >

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 3/4] generic/578: Test that filefrag is supported before running
  2022-02-23 18:24     ` Anna Schumaker
@ 2022-02-24  4:15       ` Darrick J. Wong
  0 siblings, 0 replies; 12+ messages in thread
From: Darrick J. Wong @ 2022-02-24  4:15 UTC (permalink / raw)
  To: Anna Schumaker; +Cc: fstests, Linux NFS Mailing List

On Wed, Feb 23, 2022 at 01:24:08PM -0500, Anna Schumaker wrote:
> On Wed, Feb 23, 2022 at 11:57 AM Darrick J. Wong <djwong@kernel.org> wrote:
> >
> > On Tue, Feb 08, 2022 at 04:52:31PM -0500, Anna Schumaker wrote:
> > > From: Anna Schumaker <Anna.Schumaker@Netapp.com>
> > >
> > > NFS does not support FIBMAP/FIEMAP, so the check for non-shared extents
> > > on NFS v4.2 always fails with the message: "FIBMAP/FIEMAP unsupported".
> > > I added the _require_filefrag check for NFS and other filesystems that
> > > don't have FIEMAP or FIBMAP support.
> > >
> > > Signed-off-by: Anna Schumaker <Anna.Schumaker@Netapp.com>
> > > ---
> > >  common/rc         | 14 ++++++++++++++
> > >  tests/generic/578 |  2 +-
> > >  2 files changed, 15 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/common/rc b/common/rc
> > > index b3289de985d8..73d17da9430e 100644
> > > --- a/common/rc
> > > +++ b/common/rc
> > > @@ -4673,6 +4673,20 @@ _require_inode_limits()
> > >       fi
> > >  }
> > >
> > > +_require_filefrag()
> > > +{
> > > +     _require_command "$FILEFRAG_PROG" filefrag
> > > +
> > > +     local file="$TEST_DIR/filefrag_testfile"
> > > +
> > > +     echo "XX" > $file
> >
> > Nit: You might want to rm -f $file before echoing into it, just in case
> > some future knave ;) sets up that pathname to point to a named pipe or
> > something that will hang fstests...
> >
> > > +     ${FILEFRAG_PROG} $file 2>&1 | grep -q "FIBMAP/FIEMAP[[:space:]]*unsupported"
> > > +     if [ $? -eq 0 ]; then
> >
> > ...and rm it again here to avoid leaving test files around.
> 
> Sure, I can make that change. Should I update
> _require_filefrag_options() and _require_fibmap() to follow the same
> pattern while I'm at it? The new function was based off of those.

Yes please, and thank you! :)

--D

> Anna
> 
> >
> > Otherwise this looks ok to me.
> >
> > --D
> >
> > > +             _notrun "FIBMAP/FIEMAP not supported by this filesystem"
> > > +     fi
> > > +     rm -f $file
> > > +}
> > > +
> > >  _require_filefrag_options()
> > >  {
> > >       _require_command "$FILEFRAG_PROG" filefrag
> > > diff --git a/tests/generic/578 b/tests/generic/578
> > > index 01929a280f8c..64c813032cf8 100755
> > > --- a/tests/generic/578
> > > +++ b/tests/generic/578
> > > @@ -23,7 +23,7 @@ _cleanup()
> > >  # real QA test starts here
> > >  _supported_fs generic
> > >  _require_test_program "mmap-write-concurrent"
> > > -_require_command "$FILEFRAG_PROG" filefrag
> > > +_require_filefrag
> > >  _require_test_reflink
> > >  _require_cp_reflink
> > >
> > > --
> > > 2.35.1
> > >

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2022-02-24  4:15 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-02-08 21:52 [PATCH 0/4] Improvements for NFS Anna Schumaker
2022-02-08 21:52 ` [PATCH 1/4] check: Export CHECK_OPTIONS and PLATFORM for Xunit Reporting Anna Schumaker
2022-02-08 21:52 ` [PATCH 2/4] generic/531: Move test from 'quick' group to 'stress' Anna Schumaker
2022-02-23 16:54   ` Darrick J. Wong
2022-02-23 19:38     ` Anna Schumaker
2022-02-24  4:14       ` Darrick J. Wong
2022-02-08 21:52 ` [PATCH 3/4] generic/578: Test that filefrag is supported before running Anna Schumaker
2022-02-23 16:57   ` Darrick J. Wong
2022-02-23 18:24     ` Anna Schumaker
2022-02-24  4:15       ` Darrick J. Wong
2022-02-08 21:52 ` [PATCH 4/4] generic/633: Check if idmapped mounts are " Anna Schumaker
2022-02-19  8:40   ` Su Yue

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).