On Wed, Jun 21, 2023 at 12:24 PM David Disseldorp <ddiss@xxxxxxx> wrote:
>
> Hi Zorro and Amir,
>
> On Wed, 21 Jun 2023 10:23:55 +0800, Zorro Lang wrote:
>
> > On Mon, Jun 19, 2023 at 01:50:58PM +0300, Amir Goldstein wrote:
> > > kvm-xfstests invokes check -n twice to pre-process and generate the
> > > tests-to-run list, which is then being passed as a long list of tests
> > > for invkoing check in the command line.
> > >
> > > check invokes dirname, basename and sed several times per test just
> > > for doing basic string prefix/suffix trimming.
> > > Use bash string pattern matching instead which is much faster.
> > >
> > > Note that the following pattern matching expression change:
> > > < test_dir=${test_dir#$SRC_DIR/*}
> > > > t=${t#$SRC_DIR/}
> > > does not change the meaning of the expression, because the
> > > shortest match of "$SRC_DIR/*" that is being trimmed is "$SRC_DIR/"
> > > and removing the tests/ prefix is what this code intended to do.
> > >
> > > With check -n, there is no need to cleanup the results dir,
> > > but check -n is doing that for every single listed test.
> > > Move the cleanup of results dir to before actually running the test.
> > >
> > > These improvements to check pre-test code cut down several minutes
> > > from the time until tests actually start to run with kvm-xfstests.
> > >
> > > Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx>
> > > ---
> > >
> > > Zorro,
> > >
> > > Just to clarify, this change is not expected to change behavior -
> > > only to improve runtime of check -n and the things that happen
> > > before the first test is invoked.
> > >
> > > Thanks,
> > > Amir.
> > >
> > > check | 22 +++++++++-------------
> > > 1 file changed, 9 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/check b/check
> > > index e36978c1..1a16eccb 100755
> > > --- a/check
> > > +++ b/check
> > > @@ -399,9 +399,9 @@ if $have_test_arg; then
> > > *) # Expand test pattern (e.g. xfs/???, *fs/001)
> > > list=$(cd $SRC_DIR; echo $1)
> > > for t in $list; do
> > > - test_dir=`dirname $t`
> > > - test_dir=${test_dir#$SRC_DIR/*}
> > > - test_name=`basename $t`
> > > + t=${t#$SRC_DIR/}
> >
> > Actually I'm not sure what's this line for. We get $list by "cd $SRC_DIR; echo $1",
> > is there any chance to have "$SRC_DIR" in $t (or $test_dir) ?
>
> Indeed, this line can be dropped. One minor change in behaviour here is
> that a test pattern with a wacky path prefix e.g. ../tests/xfs/00? will
> no longer work. I think breaking such patterns is okay, but it could
> also be resolved by more carefully extracting the last two path
> components.
>
I didn't want to drop it because I didn't want to change legacy behavior.
When user's cwd is fstests root directory, it may be natural for some
users to use bash auto completion to run the command
./check tests/generic/001
so there may well be users out these that pass test names including
the tests/ prefix and check has always trimmed this prefix.
There is something a bit confusing about expanding of bash
patterns:
When the user uses this syntax:
./check tests/generic/00?
the shell expands that pattern to a list of 9 arguments passed to
./check tests/generic/001 tests/generic/002 ...
But when the user uses the documented syntax:
./check generic/00?
check gets a single argument and $(cd $SRC_DIR; echo $1)
expands this single argument to a list
All the above is existing behavior which my patch should
not have changed.
> > > + test_dir=${t%/*}
> > > + test_name=${t#*/}
> >
You may change the above to:
+ test_dir=${t%%/*}
+ test_name=${t##*/}
I tested and this deals with "xfs//001" and "xfs//00?"
correctly (it trimms the longest match to */ prefix or /* suffix
instead of the shortest match.
The line below group= could be changed to %%/* as well
but it does not matter because $group is used as part of
the results path which can live with //.
Unfortunately, trimming tests// from tests//xfs/001 without
also trimming xfs/ is less trivial and while I gave a reason
why tests/ prefix may be used in the wild, but I can think
of no good reason why tests// prefix would be used in the wild.
> > This change looks isn't related with the "check -n". Any reason to change
> > that? Does the 'dirname' and 'basename' have poor performance, or some systems
> > miss them?
> >
> > I prefer the 'dirname' and 'basename' ways, if there's not a specific reason
> > to change that. Due to they can deal with more complicated path name, e.g.
> > "xfs//001". But the "$(cd $SRC_DIR; echo $1)" looks won't generate a path name
> > likes that, so if you have a proper reason to make this change, that's fine.
>
> The per-iteration `dirname` and `basename` subshells add multiple
> seconds of latency to `./check -n *fs/*` in my test env.
>
Exactly.
Whoever looks at strace of check will observe that it is very noisy with
execve of dirname and basename.
The next step would be to use bash dictionary instead of multiple
invocations of grep to filter tests by match to -g <group> or -g xfs/<group>,
but that is left for another day...
Thanks,
Amir.
