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

[Kevin Wolf]

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.

Kevin