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.
> > + test_dir=${t%/*}
> > + test_name=${t#*/}
>
> 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.
Cheers, David
