[Qemu-devel] [PATCH] qmp: add 'drive' to enum NewImageMode

[Qemu-devel] [PATCH] qmp: add 'drive' to enum NewImageMode

[Fam Zheng]

Introduce a "drive" option to new image mode. With this mode, QMP command
should (this patch only modified drive-backup to support it, and report invalid
parameter error for drive-mirror) skip creating the image file or trying to
open it, it should just reuse the existing BDS by looking for the named drive
with bdrv_find(). It will be useful to utilize "none" sync mode of drive-backup
for point-in-time snapshot.

The example with drive-backup is:

    -> { "execute": "drive-backup", "arguments": { "device": "ide0-hd0",
                                                   "mode": "drive",
                                                   "target": "drive_id_here" } }
    <- { "return": {} }


Target bs is not released when block job completes in this case since it's
still used as a device drive or exported by nbd server.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/backup.c            |  9 ++++++--
 blockdev.c                | 57 ++++++++++++++++++++++++++++++++---------------
 include/block/block_int.h |  3 ++-
 qapi-schema.json          |  4 +++-
 4 files changed, 51 insertions(+), 22 deletions(-)

diff --git a/block/backup.c b/block/backup.c
index 6515e04..5ef4724 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -45,6 +45,7 @@ typedef struct CowRequest {
 typedef struct BackupBlockJob {
     BlockJob common;
     BlockDriverState *target;
+    bool release_target;
     RateLimit limit;
     CoRwlock flush_rwlock;
     uint64_t sectors_read;
@@ -255,14 +256,17 @@ static void coroutine_fn backup_run(void *opaque)
 
     hbitmap_free(job->bitmap);
 
-    bdrv_delete(job->target);
+    if (job->release_target) {
+        bdrv_close(job->target);
+        bdrv_delete(job->target);
+    }
 
     DPRINTF("backup_run complete %d\n", ret);
     block_job_completed(&job->common, ret);
 }
 
 void backup_start(BlockDriverState *bs, BlockDriverState *target,
-                  int64_t speed,
+                  int64_t speed, bool release_target,
                   BlockDriverCompletionFunc *cb, void *opaque,
                   Error **errp)
 {
@@ -279,6 +283,7 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target,
     }
 
     job->target = target;
+    job->release_target = release_target;
     job->common.len = bdrv_getlength(bs);
     job->common.co = qemu_coroutine_create(backup_run);
     qemu_coroutine_enter(job->common.co, job);
diff --git a/blockdev.c b/blockdev.c
index 692803f..1d0c4dc 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -908,6 +908,11 @@ static void external_snapshot_prepare(BlkTransactionState *common,
 
     flags = state->old_bs->open_flags;
 
+    if (mode == NEW_IMAGE_MODE_DRIVE) {
+        error_set(errp, QERR_INVALID_PARAMETER, "mode");
+        return;
+    }
+
     /* create new image w/backing file */
     if (mode != NEW_IMAGE_MODE_EXISTING) {
         bdrv_img_create(new_image_file, format,
@@ -1505,28 +1510,39 @@ void qmp_drive_backup(const char *device, const char *target,
         return;
     }
 
-    if (mode != NEW_IMAGE_MODE_EXISTING) {
-        assert(format && drv);
-        bdrv_img_create(target, format,
-                        NULL, NULL, NULL, size, flags, &local_err, false);
-    }
-
-    if (error_is_set(&local_err)) {
-        error_propagate(errp, local_err);
-        return;
-    }
+    if (mode == NEW_IMAGE_MODE_DRIVE) {
+        target_bs = bdrv_find(target);
+        if (!target_bs) {
+            error_set(errp, QERR_DEVICE_NOT_FOUND, target);
+            return;
+        }
+    } else {
+        if (mode != NEW_IMAGE_MODE_EXISTING) {
+            assert(format && drv);
+            bdrv_img_create(target, format,
+                    NULL, NULL, NULL, size, flags, &local_err, false);
+            if (error_is_set(&local_err)) {
+                error_propagate(errp, local_err);
+                return;
+            }
+        }
 
-    target_bs = bdrv_new("");
-    ret = bdrv_open(target_bs, target, NULL, flags, drv);
-    if (ret < 0) {
-        bdrv_delete(target_bs);
-        error_set(errp, QERR_OPEN_FILE_FAILED, target);
-        return;
+        target_bs = bdrv_new("");
+        ret = bdrv_open(target_bs, target, NULL, flags, drv);
+        if (ret < 0) {
+            bdrv_delete(target_bs);
+            error_set(errp, QERR_OPEN_FILE_FAILED, target);
+            return;
+        }
     }
 
-    backup_start(bs, target_bs, speed, block_job_cb, bs, &local_err);
+    backup_start(bs, target_bs, speed,
+                 mode != NEW_IMAGE_MODE_DRIVE,
+                 block_job_cb, bs, &local_err);
     if (local_err != NULL) {
-        bdrv_delete(target_bs);
+        if (mode != NEW_IMAGE_MODE_DRIVE) {
+            bdrv_delete(target_bs);
+        }
         error_propagate(errp, local_err);
         return;
     }
@@ -1577,6 +1593,11 @@ void qmp_drive_mirror(const char *device, const char *target,
         buf_size = DEFAULT_MIRROR_BUF_SIZE;
     }
 
+    if (mode == NEW_IMAGE_MODE_DRIVE) {
+        error_set(errp, QERR_INVALID_PARAMETER, "mode");
+        return;
+    }
+
     if (granularity != 0 && (granularity < 512 || granularity > 1048576 * 64)) {
         error_set(errp, QERR_INVALID_PARAMETER, device);
         return;
diff --git a/include/block/block_int.h b/include/block/block_int.h
index f7a7892..e1fde9e 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -403,6 +403,7 @@ void mirror_start(BlockDriverState *bs, BlockDriverState *target,
  * @bs: Block device to operate on.
  * @target: Block device to write to.
  * @speed: The maximum speed, in bytes per second, or 0 for unlimited.
+ * @release_target: If block job should release target bs before completion
  * @cb: Completion function for the job.
  * @opaque: Opaque pointer value passed to @cb.
  *
@@ -410,7 +411,7 @@ void mirror_start(BlockDriverState *bs, BlockDriverState *target,
  * until the job is cancelled or manually completed.
  */
 void backup_start(BlockDriverState *bs, BlockDriverState *target,
-                  int64_t speed,
+                  int64_t speed, bool release_target,
                   BlockDriverCompletionFunc *cb, void *opaque,
                   Error **errp);
 
diff --git a/qapi-schema.json b/qapi-schema.json
index 134a92d..9391e39 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1593,10 +1593,12 @@
 # @absolute-paths: QEMU should create a new image with absolute paths
 # for the backing file.
 #
+# @drive: QEMU should look for an existing drive
+#
 # Since: 1.1
 ##
 { 'enum': 'NewImageMode'
-  'data': [ 'existing', 'absolute-paths' ] }
+  'data': [ 'existing', 'absolute-paths', 'drive' ] }
 
 ##
 # @BlockdevSnapshot
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH] qmp: add 'drive' to enum NewImageMode

[Stefan Hajnoczi]

On Mon, Jun 17, 2013 at 02:44:46PM +0800, Fam Zheng wrote:
> Introduce a "drive" option to new image mode. With this mode, QMP command
> should (this patch only modified drive-backup to support it, and report invalid
> parameter error for drive-mirror) skip creating the image file or trying to
> open it, it should just reuse the existing BDS by looking for the named drive
> with bdrv_find(). It will be useful to utilize "none" sync mode of drive-backup
> for point-in-time snapshot.
> 
> The example with drive-backup is:
> 
>     -> { "execute": "drive-backup", "arguments": { "device": "ide0-hd0",
>                                                    "mode": "drive",
>                                                    "target": "drive_id_here" } }
>     <- { "return": {} }
> 
> 
> Target bs is not released when block job completes in this case since it's
> still used as a device drive or exported by nbd server.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block/backup.c            |  9 ++++++--
>  blockdev.c                | 57 ++++++++++++++++++++++++++++++++---------------
>  include/block/block_int.h |  3 ++-
>  qapi-schema.json          |  4 +++-
>  4 files changed, 51 insertions(+), 22 deletions(-)

Fam and I chatted about this yesterday.  The QMP API change here is that
traditionally 'target' and 'mode' always referred to existing filenames.
Now you can set 'mode': 'drive' and then 'target' becomes a QEMU block
device name.

I think this API extension is reasonable but I think we haven't nailed
down the lifecycle of the snapshot block drive yet...

Stefan