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

[Peter Lieven]

On 20.10.2014 10:59, Max Reitz wrote:
> On 2014-10-20 at 08:14, Peter Lieven wrote:
> > the block layer silently merges write requests since
>
> 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                   |    4 ++++
> >   block/qapi.c              |    1 +
> >   blockdev.c                |    7 +++++++
> >   hmp.c                     |    4 ++++
> >   include/block/block_int.h |    1 +
> >   qapi/block-core.json      |   10 +++++++++-
> >   qemu-options.hx           |    1 +
> >   qmp-commands.hx           |    2 ++
> >   8 files changed, 29 insertions(+), 1 deletion(-)
> >
> > diff --git a/block.c b/block.c
> > index 27533f3..1658a72 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -4531,6 +4531,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 9733ebd..02251dd 100644
> > --- a/block/qapi.c
> > +++ b/block/qapi.c
> > @@ -58,6 +58,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/blockdev.c b/blockdev.c
> > index e595910..13e47b8 100644
> > --- a/blockdev.c
> > +++ b/blockdev.c
> > @@ -378,6 +378,7 @@ static DriveInfo *blockdev_init(const char *file, QDict 
> > *bs_opts,
> >       const char *id;
> >       bool has_driver_specific_opts;
> >       BlockdevDetectZeroesOptions detect_zeroes;
> > +    bool write_merging;
> >       BlockDriver *drv = NULL;
> >         /* Check common options by copying from bs_opts to opts, all other 
> > options
> > @@ -405,6 +406,7 @@ static DriveInfo *blockdev_init(const char *file, QDict 
> > *bs_opts,
> >       snapshot = qemu_opt_get_bool(opts, "snapshot", 0);
> >       ro = qemu_opt_get_bool(opts, "read-only", 0);
> >       copy_on_read = qemu_opt_get_bool(opts, "copy-on-read", false);
> > +    write_merging = qemu_opt_get_bool(opts, "write-merging", true);
>
> Using this option in blockdev_init() means that you can only enable or disable merging for the top layer (the root BDS). Furthermore, since you don't set bs->write_merging in bdrv_new() (or at least bdrv_open()), it actually defaults to false and only 
> for the top layer it defaults to true.
>
> Therefore, if after this patch a format block driver issues a multiwrite to its 
> file, the write will not be merged and the user can do nothing about it. I 
> don't suppose this is intentional...?

I am not sure if a block driver actually can do this at all? The only way to 
enter multiwrite is from virtio_blk_handle_request in virtio-blk.c.

> I propose evaluating the option in bdrv_open() and setting bs->write_merging 
> there.

I wasn't aware actually. I remember that someone asked me to implement 
discard_zeroes in blockdev_init. I think it was something related to QMP. So we 
still might
need to check parameters at 2 positions? It is quite confusing which paramter 
has to be parsed where.

Peter

> >       if ((buf = qemu_opt_get(opts, "discard")) != NULL) {
> >           if (bdrv_parse_discard_flags(buf, &bdrv_flags) != 0) {
> > @@ -530,6 +532,7 @@ static DriveInfo *blockdev_init(const char *file, QDict 
> > *bs_opts,
> >       bs->open_flags = snapshot ? BDRV_O_SNAPSHOT : 0;
> >       bs->read_only = ro;
> >       bs->detect_zeroes = detect_zeroes;
> > +    bs->write_merging = write_merging;
> >         bdrv_set_on_error(bs, on_read_error, on_write_error);
> >   @@ -2746,6 +2749,10 @@ QemuOptsList qemu_common_drive_opts = {
> >               .name = "detect-zeroes",
> >               .type = QEMU_OPT_STRING,
> >               .help = "try to optimize zero writes (off, on, unmap)",
> > +        },{
> > +            .name = "write-merging",
> > +            .type = QEMU_OPT_BOOL,
> > +            .help = "enable write merging (default: true)",
> >           },
> >           { /* end of list */ }
> >       },
> > diff --git a/hmp.c b/hmp.c
> > index 63d7686..8d6ad0b 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");
> > +        }
> > +
> >           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 8d86a6c..39bbde2 100644
> > --- a/include/block/block_int.h
> > +++ b/include/block/block_int.h
> > @@ -407,6 +407,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 8f7089e..4931bd9 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)
> > +#
> >   # @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',
> > @@ -1180,6 +1183,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)
> >   #
> >   # Since: 1.7
> >   ##
> > @@ -1193,7 +1200,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..529a563 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 multiwrite_merge feature (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",
>
> Hm, thanks for reminding me of that file. There are some things which I forgot 
> to update (ImageInfoSpecific at least)...
>
> Max


--

Mit freundlichen Grüßen

Peter Lieven

...........................................................

  KAMP Netzwerkdienste GmbH
  Vestische Str. 89-91 | 46117 Oberhausen
  Tel: +49 (0) 208.89 402-50 | Fax: +49 (0) 208.89 402-40
  address@hidden | http://www.kamp.de

  Geschäftsführer: Heiner Lante | Michael Lante
  Amtsgericht Duisburg | HRB Nr. 12154
  USt-Id-Nr.: DE 120607556

...........................................................