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

[Peter Lieven <pl@kamp.de>]

On 28.10.2014 12:11, Stefan Hajnoczi wrote:
> On Sat, Oct 25, 2014 at 06:55:50PM +0200, Peter Lieven wrote:
>> diff --git a/block.c b/block.c
>> index f05ea0c..f3da5dd 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)",
> QEMU_OPT_BOOL takes "on"/"off" instead of "true"/"false":
> s/true/on/
>
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index 2095f9e..74d1960 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',
>> @@ -1185,6 +1188,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
>>   ##
>> @@ -1198,7 +1205,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",
> The big question is whether these user-visible interfaces make sense if
> write merging will be moved from block.c into virtio-blk.c in the
> future.  Once an interface has been added it cannot be removed.
>
> I think write merging in block.c is fine since other emulated storage
> controllers like virtio-scsi might also choose to use it.
>
> But I want to raise the point for discussion in case others have strong
> feelings.

It would be interesting to hear from Kevin what was the point why having
the merging outside block.c was an issue for his coroutine optimizations.

Maybe if you have the time the other alpha patch I sent you see that moving it
into virtio-blk.c makes some sense.

The reason merging was added in the past was that virtio-blk chops requests.
Maybe virtio-scsi doesn't do that. I personally would feel better if qemu would
not merge requests at all. But for virtio-blk it seems to be a big benefit in
some cases also for reading.

Peter