Re: [PATCH v2] scripts/simplebench: compare write request performance

[Vladimir Sementsov-Ogievskiy]

26.06.2020 17:31, Andrey Shinkevich wrote:
> The script 'bench_write_req.py' allows comparing performances of write
> request for two qemu-img binary files.
> An example with (qemu-img binary 1) and without (qemu-img binary 2) the
> applied patch "qcow2: skip writing zero buffers to empty COW areas"
> (git commit ID: c8bb23cbdbe32f5)
> The <unaligned> case does not involve the COW optimization.
>
> SSD:
> -----------------  -------------------  -------------------
>                     <qemu-img binary 1>  <qemu-img binary 2>
> <simple case>      2.72 +- 0.00         11.67 +- 1.04
> <general case>     0.34 +- 0.00         8.64 +- 1.55
> <cluster middle>   0.33 +- 0.01         8.13 +- 2.05
> <cluster overlap>  8.46 +- 0.06         12.97 +- 1.07
> <unaligned>        9.27 +- 2.04         8.83 +- 0.84
> -----------------  -------------------  -------------------
> HDD:
> -----------------  -------------------  -------------------
>                     <qemu-img binary 1>  <qemu-img binary 2>
> <simple case>      617.86 +- 6.78       608.84 +- 10.72
> <general case>     57.53 +- 3.56        52.99 +- 7.48
> <cluster middle>   60.50 +- 1.92        56.11 +- 5.20
> <cluster overlap>  12.10 +- 1.10        15.16 +- 2.56
> <unaligned>        6.23 +- 0.05         6.40 +- 0.07
> -----------------  -------------------  -------------------

Good, this proves that c8bb23cbdbe32f5 makes sense.

> Suggested-by: Denis V. Lunev <den@openvz.org>
> Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> ---
> v2:
>    01: Three more test cases added to the script:
>        <simple case>
>        <general case>
>        <unaligned>
>
>   scripts/simplebench/bench_write_req.py | 201 +++++++++++++++++++++++++++++++++
>   1 file changed, 201 insertions(+)
>   create mode 100755 scripts/simplebench/bench_write_req.py
>
> diff --git a/scripts/simplebench/bench_write_req.py 
> b/scripts/simplebench/bench_write_req.py
> new file mode 100755
> index 0000000..fe92d01
> --- /dev/null
> +++ b/scripts/simplebench/bench_write_req.py
> @@ -0,0 +1,201 @@
> +#!/usr/bin/env python3
> +#
> +# Test to compare performance of write requests for two qemu-img binary files.
> +#
> +# Copyright (c) 2020 Virtuozzo International GmbH.
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +#
> +
> +
> +import sys
> +import os
> +import subprocess
> +import simplebench
> +
> +
> +def bench_func(env, case):
> +    """ Handle one "cell" of benchmarking table. """
> +    return bench_write_req(env['qemu_img'], env['image_name'],
> +                           case['block_size'], case['block_offset'],
> +                           case['requests'], case['empty_image'])
> +
> +
> +def qemu_img_pipe(*args):
> +    '''Run qemu-img and return its output'''
> +    subp = subprocess.Popen(list(args),
> +                            stdout=subprocess.PIPE,
> +                            stderr=subprocess.STDOUT,
> +                            universal_newlines=True)
> +    exitcode = subp.wait()
> +    if exitcode < 0:
> +        sys.stderr.write('qemu-img received signal %i: %s\n'
> +                         % (-exitcode, ' '.join(list(args))))
> +    return subp.communicate()[0]

I understand that it's duplicated from iotests.py.. Hmm, we probably should 
move most of its functionality into python/ and reuse here, but I don't ask you 
do it, this patch my keep the copy I think.

> +
> +
> +def bench_write_req(qemu_img, image_name, block_size, block_offset, requests,
> +                    empty_image):
> +    """Benchmark write requests
> +
> +    qemu_img     -- path to qemu_img executable file
> +    image_name   -- QCOW2 image name to create
> +    block_size   -- size of a block to write to clusters
> +    block_offset -- offset of the block in clusters
> +    requests     -- number of write requests per cluster, customize if zero
> +    empty_image  -- if True, fill image with random data
> +

Some comment on the behavior of the function won't hurt.

> +    Returns {'seconds': int} on success and {'error': str} on failure.
> +    Return value is compatible with simplebench lib.
> +    """
> +
> +    if not os.path.isfile(qemu_img):
> +        print('File not found: {}'.format(qemu_img))
> +        sys.exit(1)
> +
> +    image_dir = os.path.dirname(os.path.abspath(image_name))
> +    if not os.path.isdir(image_dir):
> +        print('Path not found: {}'.format(image_name))
> +        sys.exit(1)
> +
> +    cluster_size = 1024 * 1024
> +    image_size = 1024 * cluster_size
> +    seek = 4
> +    dd_count = int(image_size / cluster_size) - seek
> +
> +    args_create = [qemu_img, 'create', '-f', 'qcow2', '-o',
> +                   'cluster_size={}'.format(cluster_size),
> +                   image_name, str(image_size)]
> +
> +    if requests:
> +        count = requests * int(image_size / cluster_size)
> +        step = str(cluster_size)
> +    else:
> +        # Create unaligned write requests
> +        assert block_size
> +        shift = int(block_size * 1.01)
> +        count = int((image_size - block_offset) / shift)
> +        step = str(shift)
> +        depth = ['-d', '2']
> +
> +    offset = str(block_offset)
> +    cnt = str(count)
> +    size = []
> +    if block_size:
> +        size = ['-s', '{}'.format(block_size)]
> +
> +    args_bench = [qemu_img, 'bench', '-w', '-n', '-t', 'none', '-c', cnt,
> +                  '-S', step, '-o', offset, '-f', 'qcow2', image_name]
> +    if block_size:
> +        args_bench.extend(size)
> +    if not requests:
> +        args_bench.extend(depth)
> +
> +    try:
> +        qemu_img_pipe(*args_create)
> +
> +        if not empty_image:
> +            dd = ['dd', 'if=/dev/urandom', 'of={}'.format(image_name),
> +                  'bs={}'.format(cluster_size), 'seek={}'.format(seek),
> +                  'count={}'.format(dd_count), '&&', 'sync']

consider using new python f-strings, like f'bs={cluster_size}'

> +            devnull = open('/dev/null', 'w')
> +            subprocess.call(dd, stderr=devnull, stdout=devnull)

subprocess.call is outdated API, use .run()

And, I really doubt that "'&&", 'sync'" would work without shell=True. Probably 
better to call run() twice: for dd and for sync.

Next, I don't understand, are you trying to fill qcow2 image by dd directly? 
This is strange. Even if you don't break metadata, you don't change it, so all 
cluster will remain empty.

> +
> +    except OSError as e:
> +        return {'error': 'qemu_img create failed: ' + str(e)}
so, here, you don't care to remove image_name


> +
> +    try:
> +        ret = qemu_img_pipe(*args_bench)
> +    except OSError as e:
> +        return {'error': 'qemu_img bench failed: ' + str(e)}
> +    finally:
> +        os.remove(image_name)
> +        if not ret:
> +            return {'error': 'qemu_img bench failed'}
> +        if 'seconds' in ret:
> +            ret = ret.split()

Note, that such thing (reassigning another type to variable)

> +            index = ret.index('seconds.')
> +            return {'seconds': float(ret[index-1])}
> +        else:
> +            return {'error': 'qemu_img bench failed: ' + ret}

return both in except and in finally doesn't seem good thing to do: note that 
"finally is executed even if you try to return from except, so your return in 
except doesn't make sence at all

> +
> +
> +if __name__ == '__main__':
> +
> +    if len(sys.argv) < 4:
> +        print('USAGE: {} <path to qemu-img binary file> '
> +              '<path to another qemu-img to compare performance with> '
> +              '<full or relative name for QCOW2 image to create>'
> +              ''.format(os.path.basename(sys.argv[0])))
> +        exit(1)
> +
> +    # Test-cases are "rows" in benchmark resulting table, 'id' is a caption
> +    # for the row, other fields are handled by bench_func.
> +    test_cases = [
> +        {
> +            'id': '<simple case>',
> +            'block_size': 0,
> +            'block_offset': 0,
> +            'requests': 10,
> +            'empty_image': False
> +        },
> +        {
> +            'id': '<general case>',
> +            'block_size': 4096,
> +            'block_offset': 0,
> +            'requests': 10,
> +            'empty_image': False
> +        },
> +        {
> +            'id': '<cluster middle>',
> +            'block_size': 4096,
> +            'block_offset': 524288,
> +            'requests': 10,
> +            'empty_image': False
> +        },
> +        {
> +            'id': '<cluster overlap>',
> +            'block_size': 524288,
> +            'block_offset': 4096,
> +            'requests': 2,
> +            'empty_image': False
> +        },
> +        {
> +            'id': '<unaligned>',
> +            'block_size': 104857600,
> +            'block_offset': 524288,
> +            'requests': 0,
> +            'empty_image': False
> +        },
> +    ]
> +
> +    # Test-envs are "columns" in benchmark resulting table, 'id is a caption
> +    # for the column, other fields are handled by bench_func.
> +    # Set the paths below to desired values
> +    test_envs = [
> +        {
> +            'id': '<qemu-img binary 1>',
> +            'qemu_img': '{}'.format(sys.argv[1]),
> +            'image_name': '{}'.format(sys.argv[3])
> +        },
> +        {
> +            'id': '<qemu-img binary 2>',
> +            'qemu_img': '{}'.format(sys.argv[2]),
> +            'image_name': '{}'.format(sys.argv[3])
> +        },
> +    ]
> +
> +    result = simplebench.bench(bench_func, test_envs, test_cases, count=3,
> +                               initial_run=False)
> +    print(simplebench.ascii(result))


--
Best regards,
Vladimir