Re: [Qemu-devel] [PATCH 5/6] add-cow: support snapshot_blkdev

[Paolo Bonzini <pbonzini@redhat.com>]

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 <wdongxu@linux.vnet.ibm.com>
> 
> 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.
> 
> 
>