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

[Paolo Bonzini]

Il 31/07/2012 12:25, Kevin Wolf ha scritto:
>> 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?

bdrv_swap takes care of it just fine.

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

Actually it is interesting for the mirror.  Passive mirroring can only
benefit from lower latency.

But yes, bdrv_swap would not copy this one.  Right now we always use the
same aio method as the source (at worst it is ignored by the protocol),
so it is not a problem.

> 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?

I thought about it, but I'm worried of what happens when I/O throttling
kicks in, and how it interacts with pause/resume/cancel.

>>>>> 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.

Playing devil's advocate because you've almost convinced me, we have the
same problem for blockdev-snapshot-sync.  Now drive-mirror is a bit
different because you can use it to "reshape" an image to something
else, but the same could be done with snapshot + streaming in many cases.

>> 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...

I think having a few limited knobs for image creation make some sense
(not all QMP clients need to be as sophisticated as libvirt), but that's
actually an interesting idea (as it is in general to piggyback on
drive_add).

Still, it leaves something to be desired.  It's not that it feels
HMP-ish, it's that it's overloading target a bit too much.  I would
prefer to keep drive-mirror for simple clients, and have a separate
blockdev-mirror that must have a blockdev target.  But doing the same
with blockdev-snapshot-sync will always look like duct-tape, because the
blockdev name is already taken. :(  Man, sometimes it feels like we're
not getting one thing right.

Paolo