Re: [PATCH v3 25/25] simplebench: add bench-backup.py

[Max Reitz <mreitz@redhat.com>]

On 26.10.20 18:18, Vladimir Sementsov-Ogievskiy wrote:
> Add script to benchmark new backup architecture.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   scripts/simplebench/bench-backup.py | 165 ++++++++++++++++++++++++++++
>   1 file changed, 165 insertions(+)
>   create mode 100755 scripts/simplebench/bench-backup.py
Loose review, because I’m not too involved, and this is “just” a 
performance test, too.

Result: Looks good to me, two minor things below.

> diff --git a/scripts/simplebench/bench-backup.py b/scripts/simplebench/bench-backup.py
> new file mode 100755
> index 0000000000..5c62b1a7ed
> --- /dev/null
> +++ b/scripts/simplebench/bench-backup.py

[...]

> +def bench(args):
> +    test_cases = []
> +
> +    sources = {}
> +    targets = {}
> +    for d in args.dir:
> +        label, path = d.split(':')

Should this be d.split(':', 1) to accommodate path names with colons in 
them?

> +        sources[label] = drv_file(path + '/test-source')
> +        targets[label] = drv_file(path + '/test-target')
> +
> +    if args.nbd:
> +        nbd = args.nbd.split(':')
> +        host = nbd[0]
> +        port = '10809' if len(nbd) == 1 else nbd[1]
> +        drv = drv_nbd(host, port)
> +        sources['nbd'] = drv
> +        targets['nbd'] = drv
> +
> +    for t in args.test:
> +        src, dst = t.split(':')
> +
> +        test_cases.append({
> +            'id': t,
> +            'source': sources[src],
> +            'target': targets[dst]
> +        })
> +
> +    binaries = []  # list of (<label>, <path>, [<options>])
> +    for i, q in enumerate(args.env):
> +        name_path = q.split(':')

(Same here)

> +        if len(name_path) == 1:
> +            label = f'q{i}'
> +            path_opts = name_path[0].split(',')
> +        else:
> +            label = name_path[0]
> +            path_opts = name_path[1].split(',')
> +
> +        binaries.append((label, path_opts[0], path_opts[1:]))
> +
> +    test_envs = []
> +
> +    bin_paths = {}
> +    for i, q in enumerate(args.env):
> +        opts = q.split(',')
> +        label_path = opts[0]
> +        opts = opts[1:]
> +
> +        if ':' in label_path:
> +            label, path = label_path.split(':')

(And here)

> +            bin_paths[label] = path
> +        elif label_path in bin_paths:
> +            label = label_path
> +            path = bin_paths[label]
> +        else:
> +            path = label_path
> +            label = f'q{i}'
> +            bin_paths[label] = path

[...]

> +if __name__ == '__main__':
> +    p = argparse.ArgumentParser('Backup benchmark', epilog='''
> +ENV format
> +
> +    (LABEL:PATH|LABEL|PATH)[,max-workers=N][,use-copy-range=(on|off)][,mirror]
> +
> +    LABEL                short name for the binary
> +    PATH                 path to the binary
> +    max-workers          set x-perf.max-workers of backup job
> +    use-copy-range       set x-perf.disable-copy-range of backup job

s/disable/use/?

Max