Patchwork [4/6,v11] add-cow: support snapshot_blkde

login
register
mail settings
Submitter Robert Wang
Date July 31, 2012, 4:51 p.m.
Message ID <1343753510-24661-4-git-send-email-wdongxu@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/174287/
State New
Headers show

Comments

Robert Wang - July 31, 2012, 4:51 p.m.
add-cow will let raw file support snapshot_blkdev indirectly.

Signed-off-by: Dong Xu Wang <wdongxu@linux.vnet.ibm.com>
---
 blockdev.c              |   45 +++++++++++++++++++++++++++++++++++++--------
 docs/live-block-ops.txt |   11 ++++++++++-
 2 files changed, 47 insertions(+), 9 deletions(-)
Stefan Hajnoczi - Aug. 1, 2012, 3:37 p.m.
On Tue, Jul 31, 2012 at 5:51 PM, Dong Xu Wang
<wdongxu@linux.vnet.ibm.com> wrote:
> add-cow will let raw file support snapshot_blkdev indirectly.
>
> Signed-off-by: Dong Xu Wang <wdongxu@linux.vnet.ibm.com>
> ---
>  blockdev.c              |   45 +++++++++++++++++++++++++++++++++++++--------
>  docs/live-block-ops.txt |   11 ++++++++++-
>  2 files changed, 47 insertions(+), 9 deletions(-)

This patch would need to update qapi-schema.json and qmp-commands.hx
to make use of the new arguments.

I don't think QEMU compiles cleanly after this patch.  It's important
to keep the build clean after every patch so that git-bisect(1) can be
used (if you hit a broken build during a bisect it makes things
harder).

However, I think the QMP/HMP changes should not be included in this
series unless you are sure the interface is stable.  When you convert
QEMUOptionsParameter will transaction or snapshot-blkdev-sync change?
If so, please only send the add-cow image format and leave the QMP/HMP
changes until you have the QemuOpts solution.

Stefan
Robert Wang - Aug. 2, 2012, 7:28 a.m.
On Wed, Aug 1, 2012 at 11:37 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Tue, Jul 31, 2012 at 5:51 PM, Dong Xu Wang
> <wdongxu@linux.vnet.ibm.com> wrote:
>> add-cow will let raw file support snapshot_blkdev indirectly.
>>
>> Signed-off-by: Dong Xu Wang <wdongxu@linux.vnet.ibm.com>
>> ---
>>  blockdev.c              |   45 +++++++++++++++++++++++++++++++++++++--------
>>  docs/live-block-ops.txt |   11 ++++++++++-
>>  2 files changed, 47 insertions(+), 9 deletions(-)
>
> This patch would need to update qapi-schema.json and qmp-commands.hx
> to make use of the new arguments.
>
> I don't think QEMU compiles cleanly after this patch.  It's important
> to keep the build clean after every patch so that git-bisect(1) can be
> used (if you hit a broken build during a bisect it makes things
> harder).
>
> However, I think the QMP/HMP changes should not be included in this
> series unless you are sure the interface is stable.  When you convert
> QEMUOptionsParameter will transaction or snapshot-blkdev-sync change?
> If so, please only send the add-cow image format and leave the QMP/HMP
> changes until you have the QemuOpts solution.

Ah, yes, sorry, the patch must be compiled with 5/6, HMP  now works like:
[-n] device [new-image-file] [format] [image-file] [image-format]

snapshot_blockdev /dev/ide-id0 1.add-cow add-cow t.raw raw

Also qmp's args_type:
.args_type  = "device:B,snapshot-file:s,format:s?,mode:s?,image-file:s?,image-format:s?",

Before  converting QEMUOptionsParameter, can the code be like this?

Thanks.


>
> Stefan
>
Stefan Hajnoczi - Aug. 2, 2012, 10:37 a.m.
On Thu, Aug 2, 2012 at 8:28 AM, Dong Xu Wang <wdongxu@linux.vnet.ibm.com> wrote:
> On Wed, Aug 1, 2012 at 11:37 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
>> On Tue, Jul 31, 2012 at 5:51 PM, Dong Xu Wang
>> <wdongxu@linux.vnet.ibm.com> wrote:
>>> add-cow will let raw file support snapshot_blkdev indirectly.
>>>
>>> Signed-off-by: Dong Xu Wang <wdongxu@linux.vnet.ibm.com>
>>> ---
>>>  blockdev.c              |   45 +++++++++++++++++++++++++++++++++++++--------
>>>  docs/live-block-ops.txt |   11 ++++++++++-
>>>  2 files changed, 47 insertions(+), 9 deletions(-)
>>
>> This patch would need to update qapi-schema.json and qmp-commands.hx
>> to make use of the new arguments.
>>
>> I don't think QEMU compiles cleanly after this patch.  It's important
>> to keep the build clean after every patch so that git-bisect(1) can be
>> used (if you hit a broken build during a bisect it makes things
>> harder).
>>
>> However, I think the QMP/HMP changes should not be included in this
>> series unless you are sure the interface is stable.  When you convert
>> QEMUOptionsParameter will transaction or snapshot-blkdev-sync change?
>> If so, please only send the add-cow image format and leave the QMP/HMP
>> changes until you have the QemuOpts solution.
>
> Ah, yes, sorry, the patch must be compiled with 5/6, HMP  now works like:
> [-n] device [new-image-file] [format] [image-file] [image-format]
>
> snapshot_blockdev /dev/ide-id0 1.add-cow add-cow t.raw raw
>
> Also qmp's args_type:
> .args_type  = "device:B,snapshot-file:s,format:s?,mode:s?,image-file:s?,image-format:s?",
>
> Before  converting QEMUOptionsParameter, can the code be like this?

I think it's cleanest to submit just the image format without
snapshot_blockdev interface changes.  It's definitely not a good idea
to temporarily change the QMP/HMP interface if the
QEMUOptionsParameter conversion will change it again.

Stefan

Patch

diff --git a/blockdev.c b/blockdev.c
index 3d75015..a1e9268 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -19,7 +19,7 @@ 
 #include "qmp-commands.h"
 #include "trace.h"
 #include "arch_init.h"
-
+#include "block/add-cow.h"
 static QTAILQ_HEAD(drivelist, DriveInfo) drives = QTAILQ_HEAD_INITIALIZER(drives);
 
 static const char *const if_name[IF_COUNT] = {
@@ -665,6 +665,8 @@  static void blockdev_do_action(int kind, void *data, Error **errp)
 void qmp_blockdev_snapshot_sync(const char *device, const char *snapshot_file,
                                 bool has_format, const char *format,
                                 bool has_mode, enum NewImageMode mode,
+                                bool has_image_file, const char *image_filename,
+                                bool has_image_format, const char *image_format,
                                 Error **errp)
 {
     BlockdevSnapshot snapshot = {
@@ -674,6 +676,10 @@  void qmp_blockdev_snapshot_sync(const char *device, const char *snapshot_file,
         .format = (char *) format,
         .has_mode = has_mode,
         .mode = mode,
+        .has_image_file = has_image_file,
+        .image_file = (char *) image_filename,
+        .has_image_format = has_image_format,
+        .image_format = (char *) image_format,
     };
     blockdev_do_action(BLOCKDEV_ACTION_KIND_BLOCKDEV_SNAPSHOT_SYNC, &snapshot,
                        errp);
@@ -776,15 +782,30 @@  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,
-                                  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;
+            char option[1024];
+            uint64_t size;
+
+            bdrv_get_geometry(states->old_bs, &size);
+            size *= BDRV_SECTOR_SIZE;
+            if (dev_info->blockdev_snapshot_sync->image_file) {
+                sprintf(option, "image_file=%s,image_format=%s",
+                    dev_info->blockdev_snapshot_sync->image_file,
+                    dev_info->blockdev_snapshot_sync->has_image_format ? dev_info->blockdev_snapshot_sync->image_format : "raw");
+                ret = bdrv_img_create(new_image_file, format,
+                                      states->old_bs->filename,
+                                      states->old_bs->drv->format_name,
+                                      option, -1, flags);
+            } else {
+                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;
+        }
 
         /* We will manually add the backing_hd field to the bs later */
         states->new_bs = bdrv_new("");
@@ -1083,6 +1104,14 @@  static void block_stream_cb(void *opaque, int ret)
     }
     qobject_decref(obj);
 
+    if (strcmp(bs->drv->format_name, "add-cow") == 0) {
+        BDRVAddCowState *s = bs->opaque;
+        char *format = s->image_file_format;
+        BlockDriver *drv = bdrv_find_format(format);
+        assert(drv);
+        bdrv_drain_all();
+        bdrv_swap(s->image_hd, bs);
+    }
     drive_put_ref_bh_schedule(drive_get_by_blockdev(bs));
 }
 
diff --git a/docs/live-block-ops.txt b/docs/live-block-ops.txt
index a257087..a53bab3 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,11 @@  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 source.img raw
+
+source.img is a raw file, and should be pre-created. disk.img will be created
+using soure.img as image file. After block_stream process finished, it will
+reassociate the drive with source.img automatically.