From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=49729 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1PtKTA-0007rJ-E5 for qemu-devel@nongnu.org; Sat, 26 Feb 2011 08:46:05 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1PtKT5-0001M4-3E for qemu-devel@nongnu.org; Sat, 26 Feb 2011 08:46:00 -0500 Received: from mail-gw0-f46.google.com ([74.125.83.46]:33267) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1PtKT4-0001Lx-Tj for qemu-devel@nongnu.org; Sat, 26 Feb 2011 08:45:55 -0500 Received: by gwj20 with SMTP id 20so1530531gwj.33 for ; Sat, 26 Feb 2011 05:45:54 -0800 (PST) Message-ID: <4D690408.1020404@codemonkey.ws> Date: Sat, 26 Feb 2011 07:45:44 -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> <4D655AC6.3090503@codemonkey.ws> <20110226000214.GA30160@amt.cnet> In-Reply-To: <20110226000214.GA30160@amt.cnet> 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/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