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

[Marcelo Tosatti <mtosatti@redhat.com>]

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

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

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

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

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

No, it does not attempt to identify identical blocks, yet.

You are right, i'll write down a document to describe these details.

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

Fair.

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

Again, conflicts with "block migration" from live migration.

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

cancel succeeds.

>  - A block copy was never started

qerror_report(QERR_DEVICE_NOT_FOUND, device);

>  - device refers to a device that no longer exists

qerror_report(QERR_DEVICE_NOT_FOUND, device);

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

> >+-------------
> >+
> >+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'] ]

That makes sense, other than the name conflict already mentioned.

Thanks