Re: [Qemu-devel] [patch 2/3] Add support for live block copy

[Anthony Liguori <anthony@codemonkey.ws>]

On 02/25/2011 06:02 PM, Marcelo Tosatti wrote:
> On Wed, Feb 23, 2011 at 01:06:46PM -0600, Anthony Liguori wrote:
>    
>> On 02/22/2011 11:00 AM, Marcelo Tosatti wrote:
>>      
>>> Index: qemu/qerror.h
>>> ===================================================================
>>> --- qemu.orig/qerror.h
>>> +++ qemu/qerror.h
>>> @@ -171,4 +171,13 @@ QError *qobject_to_qerror(const QObject
>>>   #define QERR_VNC_SERVER_FAILED \
>>>       "{ 'class': 'VNCServerFailed', 'data': { 'target': %s } }"
>>>
>>> +#define QERR_BLOCKCOPY_IN_PROGRESS \
>>> +    "{ 'class': 'BlockCopyInProgress', 'data': { 'device': %s } }"
>>>        
>> The caller already knows the device name by virtue of issuing the
>> command so this is redundant.
>>
>> I think a better approach would be a QERR_IN_PROGRESS 'data': {
>> 'operation': %s }
>>
>> For block copy, we'd say QERR_IN_PROGRESS("block copy").
>>
>>      
>>> +
>>> +#define QERR_BLOCKCOPY_IMAGE_SIZE_DIFFERS \
>>> +    "{ 'class': 'BlockCopyImageSizeDiffers', 'data': {} }"
>>> +
>>> +#define QERR_MIGRATION_IN_PROGRESS \
>>> +    "{ 'class': 'MigrationInProgress', 'data': {} }"
>>>        
>> Then QERR_IN_PROGRESS("live migration")
>>      
> Can the error format change like that? What about applications that make
> use of it? If it can change, sure. (libvirt.git does not seem to be
> aware of MigrationInProgress).
>    

You're adding MigrationInProgress in this patch so I don't see what 
could be using it.

>    
>>>   #endif /* QERROR_H */
>>> Index: qemu/qmp-commands.hx
>>> ===================================================================
>>> --- qemu.orig/qmp-commands.hx
>>> +++ qemu/qmp-commands.hx
>>> @@ -581,6 +581,75 @@ Example:
>>>   EQMP
>>>
>>>       {
>>> +        .name       = "block_copy",
>>> +        .args_type  = "device:s,filename:s,commit_filename:s?,inc:-i",
>>> +        .params     = "device filename [commit_filename] [-i]",
>>> +        .help       = "live block copy device to image"
>>> +                      "\n\t\t\t optional commit filename "
>>> +                      "\n\t\t\t -i for incremental copy "
>>> +                      "(base image shared between src and destination)",
>>> +        .user_print = monitor_user_noop,
>>> +        .mhandler.cmd_new = do_bdrv_copy,
>>> +    },
>>> +
>>> +SQMP
>>> +block-copy
>>> +-------
>>> +
>>> +Live block copy.
>>>        
>> I'm not sure copy really describes what we're doing here.  Maybe
>> migrate-block?
>>      
> Problem its easy to confuse "migrate-block" with "block migration". I
> could not come up with a better, non-confusing name than "live block
> copy".
>    

Yeah, I don't think I like my suggested names any better to be honest.

>>> +Arguments:
>>> +
>>> +- "device": device name (json-string)
>>> +- "filename": target image filename (json-string)
>>>        
>> Is this a created image?  Is this an image to create?
>>      
> A previously created image.
>
>    
>> To future proof for blockdev, we should make this argument optional
>> and if it's not specified throw an error about missing argument.
>> This let's us introduce an optional blockdev argument such that we
>> can use a blockdev name.
>>      
> What you mean "blockdev"?
>    

-drive file=image.qcow2,if=none,id=foo

'foo' is a named blockdev.  We don't have a way to add these through the 
monitor (yet) but we will for 0.15.

>>> +- "commit_filename": target commit filename (json-string, optional)
>>>        
>> I think we should drop this.
>>      
> Why? Sorry but this can't wait for non-config persistent storage. This
> mistake was made in the past with irqchip for example, lets not repeat
> it.
>
> Its OK to deprecate "commit_filename" in favour of its location in
> non-config persistent storage.
>
> Its not the end of the world for a mgmt app to handle change (not saying
> its not a good principle) such as this.
>    

Even as a one off, it's not a very good solution to the problem.  We'd 
be way better of just having nothing here than using the commit file.  
What are the semantics of a half written file?  How does a management 
tool detect a half written file?

>> What happens if:
>>   - No block copy is active anymore (it's completed)
>>      
> cancel succeeds.
>    

I think it would be better to fail.

>>   - device refers to a device with no media present
>>      
> Right, this should be dealt with.
>
>    
>> If this command succeeds, what the state of target?
>>      
> It will be used as the image backing the guest block device. But
> probably i don't understand your question.
>    

If I execute block-copy, then I do block-copy-cancel, if it succeeds, am 
I guaranteed that we're still using the old block device?  (The answer 
is no because we don't error out if the migration has completed already).

>> If I resume the
>> operation with the incremental flag set, will it Just Work?
>>      
> As in:
>
> - block_copy ide0-hd1 /mnt/aa.img
> - block_copy ide0-hd1 /mnt/aa.img -i
>
> ?
>
> The second command will fail with QERR_BLOCKCOPY_IN_PROGRESS.
>    

No, as in:

block_copy ide0-hd1 /mnt/aa.img
block_copy_cancel ide0-h1
block_copy ide0-h1 /mnt/aa.img -i

Does it pick up where it left off or does it start all over again?

Regards,

Anthony Liguori