Re: [Qemu-devel] [PATCH 28/47] qmp: add drive-mirror command

[Kevin Wolf]

Am 31.07.2012 12:02, schrieb Paolo Bonzini:
> Il 31/07/2012 11:46, Kevin Wolf ha scritto:
>>>> I'm not even sure about the QMP mirror command itself.
>>>>
>>>> I don't really like it, it does too many things at once: It can create
>>>> the target image file, it opens the target and it actually starts the
>>>> mirroring. It's rather bad at the first two steps, because it doesn't
>>>> take any options. This means that it can't create qcow2v3 images, for
>>>> example. Or you can't mirror into a backup with cache=unsafe while
>>>> running your real VM on cache=writethrough.
>>>
>>> Yes, though this can be worked around with mode: 'existing'.
>>
>> True. Only the problem with image creation, though, not the one with
>> bdrv_open() flags, right?
> 
> Yeah, but do you really care about for example io=threads vs. io=native?
>  The only interesting one is cache=unsafe; the mirror should enable
> writeback caching on the target (bdrv_swap will disable it if needed;
> I'll change this in the next submission), so cache=writethrough vs.
> writeback doesn't matter.

Can we really make it writeback unconditionally? For a passive mirror it
probably doesn't make a difference, but what happens when the user stops
the mirroring and switches to the target? Will it stay writeback?

The same goes for aio=native/threads. Probably not interesting for the
mirror, but well afterwards.

Another interesting thing is I/O throttling. The mirror currently
implements rate limiting itself, but is there really a reason why we
can't reuse regular I/O throttling on the target?

>>>> Having an all-in-one mirror command is a nice feature for HMP, but for
>>>> QMP it's more like a design problem.
>>>>
>>>> Now I see you have called it drive-mirror
>>>
>>> I thought this was your idea. :)
>>
>> Hm, then probably we discussed similar things before. :-)
>>
>>>> , so that kind of implies that
>>>> it's not the final blockdev-mirror but just a QMP version of a command
>>>> primarily designed for HMP. As such this restricted functionality may be
>>>> acceptable, but it's not like everything is already perfect and there's
>>>> no room for discussion.
>>>
>>> We keep going back to the same point that we do not have -blockdev, but
>>> it's becoming a bit frustrating to always rehash this same point...
>>
>> The question is whether we need it at all. We do have a drive_add
>> if=none, and for creating a mirror target that should actually be enough.
> 
> But not for creating images.  That would require qemu-img invocation.

Yeah, either qemu-img or another monitor command. I believe that in
practice libvirt will do this anyway if this is the only way to specify
image creation options.

> If you're okay with always using an existing image in the QMP case (and
> moving image creation to the HMP implementation), we can do it.  But I'm
> not sure I like it, I think it's excessive in the other direction.

If you think it's helpful, we could make it optional and have a mode
'blockdev' where you don't specify a file name but a blockdev name. But
this is an approach that feels a bit HMPish...

Kevin