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

["Darrick J. Wong" <djwong@kernel.org>]

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