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

[Anthony Liguori]

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