Re: [Qemu-devel] [PATCH 3/4] block: add a knob to disable multiwrite_merge

[Max Reitz]

On 20.10.2014 at 16:35, Peter Lieven wrote:
> the block layer silently merges write requests since

It's still s/^t/T/ ;-)

> commit 40b4f539. This patch adds a knob to disable
> this feature as there has been some discussion lately
> if multiwrite is a good idea at all and as it falsifies
> benchmarks.
>
> Signed-off-by: Peter Lieven <address@hidden>
> ---
>   block.c                   |    9 +++++++++
>   block/qapi.c              |    1 +
>   hmp.c                     |    4 ++++
>   include/block/block_int.h |    1 +
>   qapi/block-core.json      |   10 +++++++++-
>   qemu-options.hx           |    1 +
>   qmp-commands.hx           |    2 ++
>   7 files changed, 27 insertions(+), 1 deletion(-)
>
> diff --git a/block.c b/block.c
> index 3b4a59a..ffb2b47 100644
> --- a/block.c
> +++ b/block.c
> @@ -884,6 +884,10 @@ static QemuOptsList bdrv_runtime_opts = {
>               .name = "node-name",
>               .type = QEMU_OPT_STRING,
>               .help = "Node name of the block device node",
> +        },{
> +            .name = "write-merging",
> +            .type = QEMU_OPT_BOOL,
> +            .help = "enable write merging (default: true)",
>           },
>           { /* end of list */ }
>       },
> @@ -985,6 +989,7 @@ static int bdrv_open_common(BlockDriverState *bs, 
> BlockDriverState *file,
>       bs->drv = drv;
>       bs->opaque = g_malloc0(drv->instance_size);
>       bs->enable_write_cache = !!(flags & BDRV_O_CACHE_WB);
> +    bs->write_merging = qemu_opt_get_bool(opts, "write-merging", true);
>   
>       /* Open the image, either directly or using a protocol */
>       if (drv->bdrv_file_open) {
> @@ -4439,6 +4444,10 @@ static int multiwrite_merge(BlockDriverState *bs, 
> BlockRequest *reqs,
>   {
>       int i, outidx;
>   
> +    if (!bs->write_merging) {
> +        return num_reqs;
> +    }
> +
>       // Sort requests by start sector
>       qsort(reqs, num_reqs, sizeof(*reqs), &multiwrite_req_compare);
>   
> diff --git a/block/qapi.c b/block/qapi.c
> index a0f26e9..5f09967 100644
> --- a/block/qapi.c
> +++ b/block/qapi.c
> @@ -59,6 +59,7 @@ BlockDeviceInfo *bdrv_block_device_info(BlockDriverState *bs)
>   
>       info->backing_file_depth = bdrv_get_backing_file_depth(bs);
>       info->detect_zeroes = bs->detect_zeroes;
> +    info->write_merging = bs->write_merging;
>   
>       if (bs->io_limits_enabled) {
>           ThrottleConfig cfg;
> diff --git a/hmp.c b/hmp.c
> index baaa9e5..5762444 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -348,6 +348,10 @@ void hmp_info_block(Monitor *mon, const QDict *qdict)
>                              
> BlockdevDetectZeroesOptions_lookup[info->value->inserted->detect_zeroes]);
>           }
>   
> +        if (!info->value->inserted->write_merging) {
> +            monitor_printf(mon, "    Write Merging:    off\n");
> +        }
> +

Currently, write_merging == true is the default; but as you yourself 
describe in block-core.json, this may change. I think always printing 
ths field's value would be fine, too.

>           if (info->value->inserted->bps
>               || info->value->inserted->bps_rd
>               || info->value->inserted->bps_wr
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 8898c6c..e3d382f 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -402,6 +402,7 @@ struct BlockDriverState {
>   
>       QDict *options;
>       BlockdevDetectZeroesOptions detect_zeroes;
> +    bool write_merging;
>   
>       /* The error object in use for blocking operations on backing_hd */
>       Error *backing_blocker;
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 952d50e..9ac9085 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -214,6 +214,8 @@
>   #
>   # @detect_zeroes: detect and optimize zero writes (Since 2.1)
>   #
> +# @write_merging: true if write merging is enabled (Since 2.2)
> +#

If this is 2.2, patch 1 must say 2.2 as well.

>   # @bps: total throughput limit in bytes per second is specified
>   #
>   # @bps_rd: read throughput limit in bytes per second is specified
> @@ -250,6 +252,7 @@
>               '*backing_file': 'str', 'backing_file_depth': 'int',
>               'encrypted': 'bool', 'encryption_key_missing': 'bool',
>               'detect_zeroes': 'BlockdevDetectZeroesOptions',
> +            'write_merging': 'bool',
>               'bps': 'int', 'bps_rd': 'int', 'bps_wr': 'int',
>               'iops': 'int', 'iops_rd': 'int', 'iops_wr': 'int',
>               'image': 'ImageInfo',
> @@ -1187,6 +1190,10 @@
>   #                 (default: false)
>   # @detect-zeroes: #optional detect and optimize zero writes (Since 2.1)
>   #                 (default: off)
> +# @write-merging: #optional enable the merging of write requests
> +#                 also known as multiwrite_merge (Since 2.2)
> +#                 (default: true, but this might change in the future
> +#                  depending on format/protocol/features used)

I'd ignore the parenthesis for alignment (which means remove one space 
in front of "depending"), but I don't object either.

>   #
>   # Since: 1.7
>   ##
> @@ -1200,7 +1207,8 @@
>               '*rerror': 'BlockdevOnError',
>               '*werror': 'BlockdevOnError',
>               '*read-only': 'bool',
> -            '*detect-zeroes': 'BlockdevDetectZeroesOptions' } }
> +            '*detect-zeroes': 'BlockdevDetectZeroesOptions',
> +            '*write-merging': 'bool' } }
>   
>   ##
>   # @BlockdevOptionsFile
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 22cf3b9..d2f756f 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -432,6 +432,7 @@ DEF("drive", HAS_ARG, QEMU_OPTION_drive,
>       "       
> [,werror=ignore|stop|report|enospc][,id=name][,aio=threads|native]\n"
>       "       [,readonly=on|off][,copy-on-read=on|off]\n"
>       "       [,discard=ignore|unmap][,detect-zeroes=on|off|unmap]\n"
> +    "       [,write-merging=on|off]\n"
>       "       [[,bps=b]|[[,bps_rd=r][,bps_wr=w]]]\n"
>       "       [[,iops=i]|[[,iops_rd=r][,iops_wr=w]]]\n"
>       "       [[,bps_max=bm]|[[,bps_rd_max=rm][,bps_wr_max=wm]]]\n"
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 1abd619..2c20207 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -2104,6 +2104,7 @@ Each json-object contain the following:
>            - "iops_size": I/O size when limiting by iops (json-int)
>            - "detect_zeroes": detect and optimize zero writing (json-string)
>                - Possible values: "off", "on", "unmap"
> +         - "write_merging": enable merging of write requests (json-bool)
>            - "image": the detail of the image, it is a json-object containing
>               the following:
>                - "filename": image file name (json-string)
> @@ -2181,6 +2182,7 @@ Example:
>                  "iops_wr_max": 0,
>                  "iops_size": 0,
>                  "detect_zeroes": "on",
> +               "write_merging": "true",
>                  "image":{
>                     "filename":"disks/test.qcow2",
>                     "format":"qcow2",

Reviewed-by: Max Reitz <address@hidden>