On Wed, Jun 21, 2023 at 03:09:35PM +0300, Amir Goldstein wrote:
> 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.
> >
Hi Amir and David,
Sorry for the late reply, I'm on a holiday of Dragon Boat Festival recently,
will be back tomorrow :)
>
> I didn't want to drop it because I didn't want to change legacy behavior.
Sure, I didn't want to remove this line in this patch, due to it's not
match the target of this patch. Just speak out to check with you and
others :)
As we agree this line is useless, we can deal with it in later patch
which improves ./check.
> 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?
I think the "tests/" prefix is not necessary to support :)
>
> 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##*/}
OK, this makes more sense to me. I'll give this a test, and merge this
change if no regression. Or you'd like to send a V2 to change more.
>
> 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.
Thanks for this feedback. I understand now.
Thanks,
Zorro
> >
>
> 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.
>
