diff mbox

[v3,3/4] block: add a 'blockdev-snapshot' QMP command

Message ID 40f3f51fb44da70929f7055e95582351d1aa3f34.1441890725.git.berto@igalia.com
State New
Headers show

Commit Message

Alberto Garcia Sept. 10, 2015, 1:39 p.m. UTC
One of the limitations of the 'blockdev-snapshot-sync' command is that
it does not allow passing BlockdevOptions to the newly created
snapshots, so they are always opened using the default values.

Extending the command to allow passing options is not a practical
solution because there is overlap between those options and some of
the existing parameters of the command.

This patch introduces a new 'blockdev-snapshot' command with a simpler
interface: it just takes two references to existing block devices that
will be used as the source and target for the snapshot.

Since the main difference between the two commands is that one of them
creates and opens the target image, while the other uses an already
opened one, the bulk of the implementation is shared.

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 blockdev.c           | 163 ++++++++++++++++++++++++++++++++-------------------
 qapi-schema.json     |   2 +
 qapi/block-core.json |  26 ++++++++
 qmp-commands.hx      |  29 +++++++++
 4 files changed, 160 insertions(+), 60 deletions(-)

Comments

Eric Blake Sept. 11, 2015, 5:58 p.m. UTC | #1
On 09/10/2015 07:39 AM, Alberto Garcia wrote:
> One of the limitations of the 'blockdev-snapshot-sync' command is that
> it does not allow passing BlockdevOptions to the newly created
> snapshots, so they are always opened using the default values.
> 
> Extending the command to allow passing options is not a practical
> solution because there is overlap between those options and some of
> the existing parameters of the command.
> 
> This patch introduces a new 'blockdev-snapshot' command with a simpler
> interface: it just takes two references to existing block devices that
> will be used as the source and target for the snapshot.
> 
> Since the main difference between the two commands is that one of them
> creates and opens the target image, while the other uses an already
> opened one, the bulk of the implementation is shared.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  blockdev.c           | 163 ++++++++++++++++++++++++++++++++-------------------
>  qapi-schema.json     |   2 +
>  qapi/block-core.json |  26 ++++++++
>  qmp-commands.hx      |  29 +++++++++
>  4 files changed, 160 insertions(+), 60 deletions(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index 6b787c1..78cfb79 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1183,6 +1183,18 @@ void qmp_blockdev_snapshot_sync(bool has_device, const char *device,
>                         &snapshot, errp);
>  }
>  
> +void qmp_blockdev_snapshot(const char *device, const char *snapshot,
> +                           Error **errp)
> +{
> +    BlockdevSnapshot snapshot_data = {
> +        .device = (char *) device,
> +        .snapshot = (char *) snapshot
> +    };

Hmm. Sounds like you'd love to use my pending 'box':true qapi glue to
make this function have the saner signature of

void qmp_blockdev_snapshot(BlockdevSnapshot *arg, Error **errp);

rather than having to rebuild the struct yourself (with an annoying cast
to lose const) :)
https://lists.gnu.org/archive/html/qemu-devel/2015-09/msg02599.html

But no need to hold this series up waiting for the qapi review queue to
flush. We can simplify later.

>  
> -    options = qdict_new();
> -    if (has_snapshot_node_name) {
> -        qdict_put(options, "node-name",
> -                  qstring_from_str(snapshot_node_name));
> +        if (snapshot_node_name && bdrv_find_node(snapshot_node_name)) {
> +            error_setg(errp, "New snapshot node name already existing");

Pre-existing, but s/existing/exists/ while you are reindenting this.


> +++ b/qapi/block-core.json
> @@ -705,6 +705,19 @@
>              '*format': 'str', '*mode': 'NewImageMode' } }
>  
>  ##
> +# @BlockdevSnapshot
> +#
> +# @device: device or node name to generate the snapshot from.
> +#
> +# @snapshot: reference to the existing block device that will be used
> +#            for the snapshot.

Maybe mention that it must NOT have a current backing file, and point to
the "backing":"" trick to get it that way.

> +Create a snapshot, by installing 'device' as the backing image of
> +'snapshot'. Additionally, if 'device' is associated with a block
> +device, the block device changes to using 'snapshot' as its new active
> +image.

Still didn't answer the question from the earlier review of whether the
blockdev-snapshot-sync behavior of specifying the node name of an active
layer in order to not pivot the block device to the snapshot still makes
sense to support in the blockdev-snapshot case.  But we could always add
an optional boolean flag later if someone comes up for a use case where
they'd need to create a snapshot of the active layer without the block
device pivoting, so I don't think it should hold up this patch.

> +
> +Arguments:
> +
> +- "device": snapshot source (json-string)
> +- "snapshot": snapshot target (json-string)
> +
> +Example:
> +
> +-> { "execute": "blockdev-snapshot", "arguments": { "device": "ide-hd0",
> +                                                    "snapshot": "node1534" } }
> +<- { "return": {} }

Maybe enhance the example to show the preliminary blockdev-add that
created node1534?

I've pointed out some potential wording improvements, but think they are
minor enough that you can have:

Reviewed-by: Eric Blake <eblake@redhat.com>
Max Reitz Sept. 11, 2015, 6:11 p.m. UTC | #2
On 10.09.2015 15:39, Alberto Garcia wrote:
> One of the limitations of the 'blockdev-snapshot-sync' command is that
> it does not allow passing BlockdevOptions to the newly created
> snapshots, so they are always opened using the default values.
> 
> Extending the command to allow passing options is not a practical
> solution because there is overlap between those options and some of
> the existing parameters of the command.
> 
> This patch introduces a new 'blockdev-snapshot' command with a simpler
> interface: it just takes two references to existing block devices that
> will be used as the source and target for the snapshot.
> 
> Since the main difference between the two commands is that one of them
> creates and opens the target image, while the other uses an already
> opened one, the bulk of the implementation is shared.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  blockdev.c           | 163 ++++++++++++++++++++++++++++++++-------------------
>  qapi-schema.json     |   2 +
>  qapi/block-core.json |  26 ++++++++
>  qmp-commands.hx      |  29 +++++++++
>  4 files changed, 160 insertions(+), 60 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>
Alberto Garcia Sept. 14, 2015, 2:08 p.m. UTC | #3
On Fri 11 Sep 2015 07:58:11 PM CEST, Eric Blake wrote:

>> +void qmp_blockdev_snapshot(const char *device, const char *snapshot,
>> +                           Error **errp)
>> +{
>> +    BlockdevSnapshot snapshot_data = {
>> +        .device = (char *) device,
>> +        .snapshot = (char *) snapshot
>> +    };
>
> Hmm. Sounds like you'd love to use my pending 'box':true qapi glue to
> make this function have the saner signature of
>
> void qmp_blockdev_snapshot(BlockdevSnapshot *arg, Error **errp);
>
> rather than having to rebuild the struct yourself (with an annoying
> cast to lose const) :)
> https://lists.gnu.org/archive/html/qemu-devel/2015-09/msg02599.html

That's cool! We can simplify this later as you suggest.

>> +Create a snapshot, by installing 'device' as the backing image of
>> +'snapshot'. Additionally, if 'device' is associated with a block
>> +device, the block device changes to using 'snapshot' as its new
>> +active image.
>
> Still didn't answer the question from the earlier review of whether
> the blockdev-snapshot-sync behavior of specifying the node name of an
> active layer in order to not pivot the block device to the snapshot
> still makes sense to support in the blockdev-snapshot case.

Sorry if I overlooked the explanation, but I still didn't get the use
case for this.

And particularly for this command, that doesn't create any image but
simply uses a previously-opened one.

What would 'pivot': false do ? As far as I can see, specifying the node
name of an active layer in blockdev-snapshot-sync seems to pivot the
block device to the snapshot.

{ "execute": "blockdev-snapshot-sync",
             "arguments": { "node-name": "hd0",
                            "snapshot-file": "hd1.qcow2",
                            "snapshot-node-name": "hd1" } }

(qemu) info block
virtio0 (hd1): hd1.qcow2 (qcow2)
    Cache mode:       writeback
    Backing file:     hd0.qcow2 (chain depth: 1)

Berto
diff mbox

Patch

diff --git a/blockdev.c b/blockdev.c
index 6b787c1..78cfb79 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1183,6 +1183,18 @@  void qmp_blockdev_snapshot_sync(bool has_device, const char *device,
                        &snapshot, errp);
 }
 
+void qmp_blockdev_snapshot(const char *device, const char *snapshot,
+                           Error **errp)
+{
+    BlockdevSnapshot snapshot_data = {
+        .device = (char *) device,
+        .snapshot = (char *) snapshot
+    };
+
+    blockdev_do_action(TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT,
+                       &snapshot_data, errp);
+}
+
 void qmp_blockdev_snapshot_internal_sync(const char *device,
                                          const char *name,
                                          Error **errp)
@@ -1521,57 +1533,48 @@  typedef struct ExternalSnapshotState {
 static void external_snapshot_prepare(BlkTransactionState *common,
                                       Error **errp)
 {
-    int flags, ret;
-    QDict *options;
+    int flags = 0, ret;
+    QDict *options = NULL;
     Error *local_err = NULL;
-    bool has_device = false;
+    /* Device and node name of the image to generate the snapshot from */
     const char *device;
-    bool has_node_name = false;
     const char *node_name;
-    bool has_snapshot_node_name = false;
-    const char *snapshot_node_name;
+    /* Reference to the new image (for 'blockdev-snapshot') */
+    const char *snapshot_ref;
+    /* File name of the new image (for 'blockdev-snapshot-sync') */
     const char *new_image_file;
-    const char *format = "qcow2";
-    enum NewImageMode mode = NEW_IMAGE_MODE_ABSOLUTE_PATHS;
     ExternalSnapshotState *state =
                              DO_UPCAST(ExternalSnapshotState, common, common);
     TransactionAction *action = common->action;
 
-    /* get parameters */
-    g_assert(action->kind == TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_SYNC);
-
-    has_device = action->blockdev_snapshot_sync->has_device;
-    device = action->blockdev_snapshot_sync->device;
-    has_node_name = action->blockdev_snapshot_sync->has_node_name;
-    node_name = action->blockdev_snapshot_sync->node_name;
-    has_snapshot_node_name =
-        action->blockdev_snapshot_sync->has_snapshot_node_name;
-    snapshot_node_name = action->blockdev_snapshot_sync->snapshot_node_name;
-
-    new_image_file = action->blockdev_snapshot_sync->snapshot_file;
-    if (action->blockdev_snapshot_sync->has_format) {
-        format = action->blockdev_snapshot_sync->format;
-    }
-    if (action->blockdev_snapshot_sync->has_mode) {
-        mode = action->blockdev_snapshot_sync->mode;
+    /* 'blockdev-snapshot' and 'blockdev-snapshot-sync' have similar
+     * purpose but a different set of parameters */
+    switch (action->kind) {
+    case TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT:
+        {
+            BlockdevSnapshot *s = action->blockdev_snapshot;
+            device = s->device;
+            node_name = s->device;
+            new_image_file = NULL;
+            snapshot_ref = s->snapshot;
+        }
+        break;
+    case TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_SYNC:
+        {
+            BlockdevSnapshotSync *s = action->blockdev_snapshot_sync;
+            device = s->has_device ? s->device : NULL;
+            node_name = s->has_node_name ? s->node_name : NULL;
+            new_image_file = s->snapshot_file;
+            snapshot_ref = NULL;
+        }
+        break;
+    default:
+        g_assert_not_reached();
     }
 
     /* start processing */
-    state->old_bs = bdrv_lookup_bs(has_device ? device : NULL,
-                                   has_node_name ? node_name : NULL,
-                                   &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
-        return;
-    }
-
-    if (has_node_name && !has_snapshot_node_name) {
-        error_setg(errp, "New snapshot node name missing");
-        return;
-    }
-
-    if (has_snapshot_node_name && bdrv_find_node(snapshot_node_name)) {
-        error_setg(errp, "New snapshot node name already existing");
+    state->old_bs = bdrv_lookup_bs(device, node_name, errp);
+    if (!state->old_bs) {
         return;
     }
 
@@ -1601,35 +1604,69 @@  static void external_snapshot_prepare(BlkTransactionState *common,
         return;
     }
 
-    flags = state->old_bs->open_flags;
+    if (action->kind == TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_SYNC) {
+        BlockdevSnapshotSync *s = action->blockdev_snapshot_sync;
+        const char *format = s->has_format ? s->format : "qcow2";
+        enum NewImageMode mode;
+        const char *snapshot_node_name =
+            s->has_snapshot_node_name ? s->snapshot_node_name : NULL;
 
-    /* create new image w/backing file */
-    if (mode != NEW_IMAGE_MODE_EXISTING) {
-        bdrv_img_create(new_image_file, format,
-                        state->old_bs->filename,
-                        state->old_bs->drv->format_name,
-                        NULL, -1, flags, &local_err, false);
-        if (local_err) {
-            error_propagate(errp, local_err);
+        if (node_name && !snapshot_node_name) {
+            error_setg(errp, "New snapshot node name missing");
             return;
         }
-    }
 
-    options = qdict_new();
-    if (has_snapshot_node_name) {
-        qdict_put(options, "node-name",
-                  qstring_from_str(snapshot_node_name));
+        if (snapshot_node_name && bdrv_find_node(snapshot_node_name)) {
+            error_setg(errp, "New snapshot node name already existing");
+            return;
+        }
+
+        flags = state->old_bs->open_flags;
+
+        /* create new image w/backing file */
+        mode = s->has_mode ? s->mode : NEW_IMAGE_MODE_ABSOLUTE_PATHS;
+        if (mode != NEW_IMAGE_MODE_EXISTING) {
+            bdrv_img_create(new_image_file, format,
+                            state->old_bs->filename,
+                            state->old_bs->drv->format_name,
+                            NULL, -1, flags, &local_err, false);
+            if (local_err) {
+                error_propagate(errp, local_err);
+                return;
+            }
+        }
+
+        options = qdict_new();
+        if (s->has_snapshot_node_name) {
+            qdict_put(options, "node-name",
+                      qstring_from_str(snapshot_node_name));
+        }
+        qdict_put(options, "driver", qstring_from_str(format));
+
+        flags |= BDRV_O_NO_BACKING;
     }
-    qdict_put(options, "driver", qstring_from_str(format));
 
-    /* TODO Inherit bs->options or only take explicit options with an
-     * extended QMP command? */
     assert(state->new_bs == NULL);
-    ret = bdrv_open(&state->new_bs, new_image_file, NULL, options,
-                    flags | BDRV_O_NO_BACKING, &local_err);
+    ret = bdrv_open(&state->new_bs, new_image_file, snapshot_ref, options,
+                    flags, errp);
     /* We will manually add the backing_hd field to the bs later */
     if (ret != 0) {
-        error_propagate(errp, local_err);
+        return;
+    }
+
+    if (state->new_bs->blk != NULL) {
+        error_setg(errp, "The snapshot is already in use by %s",
+                   blk_name(state->new_bs->blk));
+        return;
+    }
+
+    if (bdrv_op_is_blocked(state->new_bs, BLOCK_OP_TYPE_EXTERNAL_SNAPSHOT,
+                           errp)) {
+        return;
+    }
+
+    if (state->new_bs->backing_hd != NULL) {
+        error_setg(errp, "The snapshot already has a backing image");
     }
 }
 
@@ -1818,6 +1855,12 @@  static void abort_commit(BlkTransactionState *common)
 }
 
 static const BdrvActionOps actions[] = {
+    [TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT] = {
+        .instance_size = sizeof(ExternalSnapshotState),
+        .prepare  = external_snapshot_prepare,
+        .commit   = external_snapshot_commit,
+        .abort = external_snapshot_abort,
+    },
     [TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_SYNC] = {
         .instance_size = sizeof(ExternalSnapshotState),
         .prepare  = external_snapshot_prepare,
diff --git a/qapi-schema.json b/qapi-schema.json
index c32dc20..71dacea 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1493,9 +1493,11 @@ 
 # abort since 1.6
 # blockdev-snapshot-internal-sync since 1.7
 # blockdev-backup since 2.3
+# blockdev-snapshot since 2.5
 ##
 { 'union': 'TransactionAction',
   'data': {
+       'blockdev-snapshot': 'BlockdevSnapshot',
        'blockdev-snapshot-sync': 'BlockdevSnapshotSync',
        'drive-backup': 'DriveBackup',
        'blockdev-backup': 'BlockdevBackup',
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 0f797d7..7d05166 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -705,6 +705,19 @@ 
             '*format': 'str', '*mode': 'NewImageMode' } }
 
 ##
+# @BlockdevSnapshot
+#
+# @device: device or node name to generate the snapshot from.
+#
+# @snapshot: reference to the existing block device that will be used
+#            for the snapshot.
+#
+# Since 2.5
+##
+{ 'struct': 'BlockdevSnapshot',
+  'data': { 'device': 'str', 'snapshot': 'str' } }
+
+##
 # @DriveBackup
 #
 # @device: the name of the device which should be copied.
@@ -800,6 +813,19 @@ 
 { 'command': 'blockdev-snapshot-sync',
   'data': 'BlockdevSnapshotSync' }
 
+
+##
+# @blockdev-snapshot
+#
+# Generates a snapshot of a block device.
+#
+# For the arguments, see the documentation of BlockdevSnapshot.
+#
+# Since 2.5
+##
+{ 'command': 'blockdev-snapshot',
+  'data': 'BlockdevSnapshot' }
+
 ##
 # @change-backing-file
 #
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 495670b..05a5675 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1454,6 +1454,35 @@  Example:
 EQMP
 
     {
+        .name       = "blockdev-snapshot",
+        .args_type  = "device:s,snapshot:s",
+        .mhandler.cmd_new = qmp_marshal_input_blockdev_snapshot,
+    },
+
+SQMP
+blockdev-snapshot
+-----------------
+Since 2.5
+
+Create a snapshot, by installing 'device' as the backing image of
+'snapshot'. Additionally, if 'device' is associated with a block
+device, the block device changes to using 'snapshot' as its new active
+image.
+
+Arguments:
+
+- "device": snapshot source (json-string)
+- "snapshot": snapshot target (json-string)
+
+Example:
+
+-> { "execute": "blockdev-snapshot", "arguments": { "device": "ide-hd0",
+                                                    "snapshot": "node1534" } }
+<- { "return": {} }
+
+EQMP
+
+    {
         .name       = "blockdev-snapshot-internal-sync",
         .args_type  = "device:B,name:s",
         .mhandler.cmd_new = qmp_marshal_input_blockdev_snapshot_internal_sync,