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

[Kevin Wolf]

Am 31.07.2012 13:25, schrieb Paolo Bonzini:
> Il 31/07/2012 13:13, Kevin Wolf ha scritto:
>> Am 31.07.2012 12:51, schrieb Paolo Bonzini:
>>> Il 31/07/2012 12:25, Kevin Wolf ha scritto:
>>>> 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.
>>
>> bdrv_co_write won't return until the request has been throttled, so it
>> should be mostly transparent.
> 
> At the end of this series I use bdrv_aio_readv/writev.
> 
>> The effect that it could have is that
>> pausing the mirror could take a little bit longer to complete (though
>> not much, as there is only one mirror request at the same time).
> 
> Not anymore. :)

Hm, I see. Makes it a bit more involved, but then the logic should be
almost the same as you already need to complete the mirror.

>> But iirc, pausing a block job was async anyway.
> 
> Yes, it is, and job->busy nicely abstracts the hairy parts.
> 
>> Any other aspect I'm missing?
> 
> No, that should be ok.  Though I'm not sure if it's so useful to apply
> throttling on the target.  It's more useful to throttle the source
> (making writes slower than reads will help the job's convergence) and
> copy at full steam to the target.

But doesn't the rate limiting of the mirror already throttle the target?
Which isn't too bad, because I think at least in the initial phase
you'll want to have both source and target throttled (later the target
is automatically throttled to the level of the source, except for bitmap
granularity artefacts).

>>>>> 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.
>>
>> blockdev-snapshot isn't taken yet. However, having the two side by side
>> would imply that blockdev-snapshot is async, which I believe is
>> currently not the most urgent of our concerns...
>>
>> Or actually, it might not even matter any more, because the thing that
>> really takes some time is creating and opening the image. Once you have
>> the blockdev, there's no point in making things async any more.
> 
> Right, blockdev-snapshot would really be just a bdrv_append operation.
> /me smiles. :)  So let's keep drive-mirror as is, and later add
> blockdev-mirror.

Ok, that's fair enough.

Kevin