From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:35306) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Sf851-0006tv-IA for qemu-devel@nongnu.org; Thu, 14 Jun 2012 07:19:16 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Sf84r-0001p0-AS for qemu-devel@nongnu.org; Thu, 14 Jun 2012 07:19:11 -0400 Received: from mail-we0-f173.google.com ([74.125.82.173]:52337) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Sf84q-0001oa-Uq for qemu-devel@nongnu.org; Thu, 14 Jun 2012 07:19:01 -0400 Received: by werf3 with SMTP id f3so1335332wer.4 for ; Thu, 14 Jun 2012 04:18:58 -0700 (PDT) Sender: Paolo Bonzini Message-ID: <4FD9C8A0.6080504@redhat.com> Date: Thu, 14 Jun 2012 13:18:56 +0200 From: Paolo Bonzini MIME-Version: 1.0 References: <1339598189-17933-1-git-send-email-wdongxu@linux.vnet.ibm.com> <1339598189-17933-5-git-send-email-wdongxu@linux.vnet.ibm.com> <4FD9C40C.1@redhat.com> In-Reply-To: <4FD9C40C.1@redhat.com> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 5/6] add-cow: support snapshot_blkdev List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: Dong Xu Wang , qemu-devel@nongnu.org Il 14/06/2012 12:59, Kevin Wolf ha scritto: > Am 13.06.2012 16:36, schrieb Dong Xu Wang: >> add-cow will let raw file support snapshot_blkdev indirectly. >> >> Signed-off-by: Dong Xu Wang > > Paolo, what do you think about this magic? Besides the obvious layering violation (it would be better to add a new method bdrv_ext_snapshot_create perhaps) I don't see very much a problem in it. Passing image creation options sounds like a good idea that we can add in the future as an extension. But honestly, I don't really see the point of add-cow in general... The raw image is anyway not usable without a pass through qemu-io convert, and mirroring will also allow collapsing an image to raw (with the persistent dirty bitmap playing the role of the add-cow metadata). > I think I can see its use especially for HMP because it's quite > convenient, but on the other hand it assumes a fixed image path for the > new raw image. This isn't going to work for block devices, for example. True, but then probably you would use mode='existing', because you need to pre-create the logical volume. > If we don't do it this way, we need to allow passing image creation > options to the snapshotting command so that you can pass a precreated > image file. This sounds like a useful extension anyway, except that passing an unstructured string for image creation options is ugly... Perhaps we can base a better implementation of options on Laszlo's QemuOpts visitor. Paolo > > Kevin > >> --- >> blockdev.c | 31 +++++++++++++++++++++++++++---- >> docs/live-block-ops.txt | 10 +++++++++- >> 2 files changed, 36 insertions(+), 5 deletions(-) >> >> diff --git a/blockdev.c b/blockdev.c >> index 622ecba..2d89e5e 100644 >> --- a/blockdev.c >> +++ b/blockdev.c >> @@ -783,15 +783,38 @@ void qmp_transaction(BlockdevActionList *dev_list, Error **errp) >> >> /* create new image w/backing file */ >> if (mode != NEW_IMAGE_MODE_EXISTING) { >> - ret = bdrv_img_create(new_image_file, format, >> + if (strcmp(format, "add-cow")) { >> + ret = bdrv_img_create(new_image_file, format, >> states->old_bs->filename, >> states->old_bs->drv->format_name, >> NULL, -1, flags); >> - if (ret) { >> - error_set(errp, QERR_OPEN_FILE_FAILED, new_image_file); >> - goto delete_and_fail; >> + } else { >> + char image_file[1024]; >> + char option[1024]; >> + uint64_t size; >> + >> + bdrv_get_geometry(states->old_bs, &size); >> + size *= BDRV_SECTOR_SIZE; >> + >> + sprintf(image_file, "%s.raw", new_image_file); >> + >> + ret = bdrv_img_create(image_file, "raw", NULL, >> + NULL, NULL, size, flags); >> + if (ret) { >> + error_set(errp, QERR_UNDEFINED_ERROR); >> + return; >> + } >> + sprintf(option, "image_file=%s.raw", new_image_file); >> + ret = bdrv_img_create(new_image_file, format, >> + states->old_bs->filename, >> + states->old_bs->drv->format_name, >> + option, -1, flags); >> } >> } >> + if (ret) { >> + error_set(errp, QERR_OPEN_FILE_FAILED, new_image_file); >> + goto delete_and_fail; >> + } >> >> /* We will manually add the backing_hd field to the bs later */ >> states->new_bs = bdrv_new(""); >> diff --git a/docs/live-block-ops.txt b/docs/live-block-ops.txt >> index a257087..c97344b 100644 >> --- a/docs/live-block-ops.txt >> +++ b/docs/live-block-ops.txt >> @@ -2,7 +2,8 @@ LIVE BLOCK OPERATIONS >> ===================== >> >> High level description of live block operations. Note these are not >> -supported for use with the raw format at the moment. >> +supported for use with the raw format at the moment, but we can use >> +add-cow as metadata to suport raw format. >> >> Snapshot live merge >> =================== >> @@ -56,3 +57,10 @@ into that image. Example: >> (qemu) block_stream ide0-hd0 >> >> >> + >> +Raw is not supported, but we can use add-cow in the 1st step: >> + >> +(qemu) snapshot_blkdev ide0-hd0 /new-path/disk.img add-cow >> + >> +It will create a raw file named disk.img.raw, with the same virtual size of >> +ide0-hd0 first, and then create disk.img. > > >