2022-02-09 10:04:34

by Anna Schumaker

[permalink] [raw]
Subject: [PATCH 2/4] generic/531: Move test from 'quick' group to 'stress'

From: Anna Schumaker <[email protected]>

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 <[email protected]>
---
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



2022-02-23 22:20:52

by Darrick J. Wong

[permalink] [raw]
Subject: Re: [PATCH 2/4] generic/531: Move test from 'quick' group to 'stress'

On Tue, Feb 08, 2022 at 04:52:30PM -0500, Anna Schumaker wrote:
> From: Anna Schumaker <[email protected]>
>
> 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 <[email protected]>
> ---
> 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 <[email protected]>

--D


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

2022-02-24 01:26:18

by Anna Schumaker

[permalink] [raw]
Subject: Re: [PATCH 2/4] generic/531: Move test from 'quick' group to 'stress'

On Wed, Feb 23, 2022 at 11:54 AM Darrick J. Wong <[email protected]> wrote:
>
> On Tue, Feb 08, 2022 at 04:52:30PM -0500, Anna Schumaker wrote:
> > From: Anna Schumaker <[email protected]>
> >
> > 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 <[email protected]>
> > ---
> > 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 <[email protected]>
>
> --D
>
>
> > testfile=$TEST_DIR/$seq.txt
> >
> > # Import common functions.
> > --
> > 2.35.1
> >

2022-02-24 05:26:33

by Darrick J. Wong

[permalink] [raw]
Subject: Re: [PATCH 2/4] generic/531: Move test from 'quick' group to 'stress'

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 <[email protected]> wrote:
> >
> > On Tue, Feb 08, 2022 at 04:52:30PM -0500, Anna Schumaker wrote:
> > > From: Anna Schumaker <[email protected]>
> > >
> > > 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 <[email protected]>
> > > ---
> > > 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 <[email protected]>
> >
> > --D
> >
> >
> > > testfile=$TEST_DIR/$seq.txt
> > >
> > > # Import common functions.
> > > --
> > > 2.35.1
> > >