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

[Anthony Liguori <anthony@codemonkey.ws>]

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")

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

> +Arguments:
> +
> +- "device": device name (json-string)
> +- "filename": target image filename (json-string)
>    

Is this a created image?  Is this an image to create?

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.

> +- "commit_filename": target commit filename (json-string, optional)
>    

I think we should drop this.

> +- "inc": incremental disk copy (json-bool, optional)
>    

Let's use the full name (incremental) and we need to describe in detail 
what the semantics of this are.  Will it scan the target block device to 
identify identical blocks?

> +Example:
> +
> +->  { "execute": "block_copy",
> +                            "arguments": { "device": "ide0-hd1",
> +                               "filename": "/mnt/new-disk.img",
> +                               "commit_filename: "/mnt/commit-new-disk.img"
> +                             } }
> +
> +<- { "return": {} }
> +
> +Notes:
> +
> +(1) The 'query-block-copy' command should be used to check block copy progress
> +    and final result (this information is provided by the 'status' member)
> +(2) Boolean argument "inc" defaults to false
>    

We should also document error semantics.  What errors are expected and why?

> +EQMP
> +
> +    {
> +        .name       = "block_copy_cancel",
> +        .args_type  = "device:s",
> +        .params     = "device",
> +        .help       = "cancel live block copy",
> +        .user_print = monitor_user_noop,
> +        .mhandler.cmd_new = do_bdrv_copy_cancel,
> +    },
> +
> +SQMP
> +block_copy_cancel
> +--------------
> +
> +Cancel live block copy.
> +
> +Arguments:
> +
> +- device: device name (json-string)
> +
> +Example:
> +
> +->  { "execute": "block_copy_cancel", "arguments": { "device": "ide0-hd1" } }
> +<- { "return": {} }
>    

cancel-block-migration?

What happens if:
  - No block copy is active anymore (it's completed)
  - A block copy was never started
  - device refers to a device that no longer exists
  - device refers to a device with no media present

If this command succeeds, what the state of target?  If I resume the 
operation with the incremental flag set, will it Just Work?

> +
> +EQMP
> +
> +    {
>           .name       = "netdev_add",
>           .args_type  = "netdev:O",
>           .params     = "[user|tap|socket],id=str[,prop=value][,...]",
> @@ -1740,6 +1809,44 @@ Examples:
>   EQMP
>
>   SQMP
> +query-block-copy
> +-------------
> +
> +Live block copy status.
> +
> +Each block copy instance information is stored in a json-object and the returned
> +value is a json-array of all instances.
> +
> +Each json-object contains the following:
> +
> +- "device": device name (json-string)
> +- "status": block copy status (json-string)
> +    - Possible values: "active", "failed", "completed"
> +- "info": A json-object with the statistics information, if status is "active":
> +    - "percentage": percentage completed (json-int)
>    

So let's fold this into query-block

And let's also report the statistics as MigrationStats which is the format:

{ 'MigrationStats': {'transferred': 'int', 'remaining': 'int', 'total': 
'int' } }

So I'd propose changing:

{ 'BlockDeviceStats': {'rd_bytes': 'int', 'wr_bytes': 'int',
                        'rd_operations': 'int', 'wr_operations': 'int',
                        'wr_highest_offset': 'int' } }
{ 'BlockStats': {'*device': 'str', 'stats': 'BlockDeviceStats',
                  '*parent': 'BlockStats'} }

[ 'query-blockstats', {}, {}, ['BlockStats'] ]

To:

{ 'BlockMigrationStatus': [ 'active', 'inactive', 'cancelled' ] }

{ 'BlockMigrationInfo': {'status': 'BlockMigrationStatus', 'stats': 
'BlockMigrationStats', '*target_filename': 'str' } }

{ 'BlockDeviceStats': {'rd_bytes': 'int', 'wr_bytes': 'int',
                        'rd_operations': 'int', 'wr_operations': 'int',
                        'wr_highest_offset': 'int', '*migration': 
'BlockMigrationInfo' } }
{ 'BlockStats': {'*device': 'str', 'stats': 'BlockDeviceStats',
                  '*parent': 'BlockStats'} }

[ 'query-blockstats', {}, {}, ['BlockStats'] ]

Regards,

Anthony Liguori

> +
> +Example:
> +
> +Block copy for "ide1-hd0" active and block copy for "ide1-hd1" failed:
> +
> +->  { "execute": "query-block-copy" }
> +<- {
> +      "return":[
> +        {"device":"ide1-hd0",
> +            "status":"active",
> +            "info":{
> +               "percentage":23,
> +            }
> +        },
> +        {"device":"ide1-hd1",
> +         "status":"failed"
> +        }
> +      ]
> +   }
> +
> +EQMP
> +
> +SQMP
>   query-balloon
>   -------------
>
> Index: qemu/Makefile.objs
> ===================================================================
> --- qemu.orig/Makefile.objs
> +++ qemu/Makefile.objs
> @@ -98,7 +98,7 @@ common-obj-y += buffered_file.o migratio
>   common-obj-y += qemu-char.o savevm.o #aio.o
>   common-obj-y += msmouse.o ps2.o
>   common-obj-y += qdev.o qdev-properties.o
> -common-obj-y += block-migration.o
> +common-obj-y += block-migration.o block-copy.o
>   common-obj-y += pflib.o
>
>   common-obj-$(CONFIG_BRLAPI) += baum.o
> Index: qemu/sysemu.h
> ===================================================================
> --- qemu.orig/sysemu.h
> +++ qemu/sysemu.h
> @@ -46,6 +46,7 @@ void qemu_del_vm_change_state_handler(VM
>   #define VMSTOP_SAVEVM    6
>   #define VMSTOP_LOADVM    7
>   #define VMSTOP_MIGRATE   8
> +#define VMSTOP_BLOCKCOPY 9
>
>   void vm_start(void);
>   void vm_stop(int reason);
>
>
>