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

[Max Reitz <mreitz@redhat.com>]

On 20.10.2014 at 15:59, Peter Lieven wrote:
> On 20.10.2014 15:55, Kevin Wolf wrote:
>> Am 20.10.2014 um 15:47 hat Peter Lieven geschrieben:
>>> On 20.10.2014 15:31, Kevin Wolf wrote:
>>>> Am 20.10.2014 um 15:22 hat Max Reitz geschrieben:
>>>>> On 20.10.2014 at 15:19, Peter Lieven wrote:
>>>>>> On 20.10.2014 15:15, Max Reitz wrote:
>>>>>>> On 20.10.2014 at 14:48, Peter Lieven wrote:
>>>>>>>> On 20.10.2014 14:19, Max Reitz wrote:
>>>>>>>>> On 2014-10-20 at 14:16, Peter Lieven wrote:
>>>>>>>>>> On 20.10.2014 13:51, Max Reitz wrote:
>>>>>>>>>>> On 2014-10-20 at 12:03, Peter Lieven wrote:
>>>>>>>>>>> [...]
>>>>>>>>>> Can you further help here. I think my problem was that I
>>>>>>>>>> don't have access to the commandline options in
>>>>>>>>>> bdrv_open?!
>>>>>>>>> You do. It's the "options" QDict. :-)
>>>>>>>> Maybe I just don't get it.
>>>>>>>>
>>>>>>>> If I specify
>>>>>>>>
>>>>>>>> qemu -drive if=virtio,file=image.qcow2,write-merging=off
>>>>>>>>
>>>>>>>> and check with
>>>>>>>>
>>>>>>>> qdict_get_try_bool(options, "write-merging", true);
>>>>>>>>
>>>>>>>> in bdrv_open() directly before bdrv_swap I always get true.
>>>>>>> Hm, judging from fprintf(stderr, "%s\n",
>>>>>>> qstring_get_str(qobject_to_json_pretty(QOBJECT(options))));,
>>>>>>> it's there for me (directly after qdict_del(options,
>>>>>>> "node-name"). The output is:
>>>>>>>
>>>>>>> Qemu wrote:
>>>>>>>> {
>>>>>>>>     "filename": "image.qcow2"
>>>>>>>> }
>>>>>>>> {
>>>>>>>>     "write-merging": "off"
>>>>>>>> }
>>>>>>>> qemu-system-x86_64: -drive
>>>>>>>> if=virtio,file=image.qcow2,write-merging=off: could not open
>>>>>>>> disk image image.qcow2: Block format 'qcow2' used by device
>>>>>>>> 'virtio0' doesn't support the option 'write-merging'
>>>>>>> But as you can see, it's a string and not a bool. So the problem
>>>>>>> is that there are (at least) two parameter "types" in qemu: One
>>>>>>> is just giving a QDict, and the other are QemuOpts. QDicts are
>>>>>>> just the raw user input and the user can only input strings, so
>>>>>>> everything is just a string. As far as I know, typing everything
>>>>>>> correctly is done by converting the QDict to a QemuOpts object
>>>>>>> (as you can see in generally every block driver which supports
>>>>>>> some options (e.g. qcow2) and also in blockdev_init(), it's
>>>>>>> qemu_opts_absorb_qdict()).
>>>>>>>
>>>>>>> Sooo, right, I forgot that. Currently, there are no non-string
>>>>>>> non-block-driver-specific options for mid-tree BDS (in contrast
>>>>>>> to the root BDS, which are parsed in blockdev_init()), so you
>>>>>>> now have the honorable task of introducing such a QemuOptsList
>>>>>>> along with qemu_opts_absorb_qdict() and everything to
>>>>>>> bdrv_open_common(). *cough*
>>>>>> I would appreciate if someone with better knowledge of this whole
>>>>>> stuff would start this. Or we postpone this know until all the
>>>>>> ongoing conversions are done.
>>>>> I can try and create some barebone which your patches can then be
>>>>> based on. I probably don't have the knowledge either, but I'm daring
>>>>> enough to do it anyway. ;-)
>>>> Actually I have some patches somewhere [1] that introduce a 
>>>> QemuOpts for
>>>> bdrv_open_common(). I intended to use that for cache modes, but as I
>>>> explained in our KVM Forum presentation, it's not quite as easy as I
>>>> thought it would be and so the patch series isn't ready yet.
>>>>
>>>> Anyway, having the QemuOpts there for driver-independent options is
>>>> probably the way to go. Feel free to remove the caching from my
>>>> patch and keep only the node-name part. Then it can be a preparatory
>>>> patch for your series where you simply add a new option to the list.
>>>>
>>>> Kevin
>>>>
>>>> [1] 
>>>> http://repo.or.cz/w/qemu/kevin.git/commitdiff/9c22aee04cf0bdf6a3858340bc6ff27d6805254f
>>> Thank you.
>>>
>>> Would it be legit to recycle qemu_common_drive_opts from blockdev.c 
>>> for this?
>> No, I don't think so. That one should in theory be only for BlockBackend
>> options. For the short term, it still mixes BB and BDS options, but BDS
>> options should be moved out step by step. In any case, it is only used
>> for the top level.
>>
>> Any option that is parsed with qemu_opts_absorb_qdict() in
>> bdrv_open_common() must also be handled there. If you don't ensure that
>> and extract all the options that blockdev_init() knows without actually
>> handling them, it can happen that invalid options are silently ignored
>> (e.g. backing.werror should error out, but would be accepted).
>>
>> And please coordinate with Max, if both of you write a patch, that's
>> wasted time.
>
> Max, if you don't have started I would use Kevins patch as basis?

No, I haven't. Feel free to.

Max