From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=57366 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1PsK3C-0001wx-1I for qemu-devel@nongnu.org; Wed, 23 Feb 2011 14:07:03 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1PsK3A-0001x5-PK for qemu-devel@nongnu.org; Wed, 23 Feb 2011 14:07:02 -0500 Received: from mail-vx0-f173.google.com ([209.85.220.173]:39282) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1PsK3A-0001wv-LE for qemu-devel@nongnu.org; Wed, 23 Feb 2011 14:07:00 -0500 Received: by vxb41 with SMTP id 41so3025855vxb.4 for ; Wed, 23 Feb 2011 11:06:59 -0800 (PST) Message-ID: <4D655AC6.3090503@codemonkey.ws> Date: Wed, 23 Feb 2011 13:06:46 -0600 From: Anthony Liguori MIME-Version: 1.0 Subject: Re: [Qemu-devel] [patch 2/3] Add support for live block copy References: <20110222170004.808373778@redhat.com> <20110222170115.710717278@redhat.com> In-Reply-To: <20110222170115.710717278@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Marcelo Tosatti Cc: Jes.Sorensen@redhat.com, qemu-devel@nongnu.org, avi@redhat.com 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); > > >