[Qemu-devel] [PATCH v2 0/2] Add 'blockdev-snapshot' command

[Qemu-devel] [PATCH v2 0/2] Add 'blockdev-snapshot' command

[Alberto Garcia]

Previous attempt to extend blockdev-snapshot-sync:
   https://lists.gnu.org/archive/html/qemu-block/2015-08/msg00236.html

As discussed in the previous thread, this adds a new command called
'blockdev-snapshot', that simply takes references to two existing
block devices. I decided to share most of the implementation with the
previous 'blockdev-snapshot-sync' since the only differences are in
the 'prepare' step.

The tests are missing, but I wanted to know first if you are otherwise
OK with this approach. If the patches look good I'll write the tests
next week.

These patches apply on top of two series from Max:

 - This one allows creating BDS that are not attached to a BlockBackend
   https://lists.gnu.org/archive/html/qemu-block/2015-07/msg00382.html

 - This one drops the drv parameter from bdrv_open(), which was
   preventing us from creating snapshots of images opened with custom
   runtime options:
   https://lists.gnu.org/archive/html/qemu-block/2015-08/msg00189.html

You cannot apply these two series cleanly on top of each other, so if
you want to try my patches without having to merge everything
manually, you can get the whole thing here:

   https://github.com/bertogg/qemu/commits/blockdev-snapshot

Comments are welcome!

Berto

Alberto Garcia (2):
  block: rename BlockdevSnapshot to BlockdevSnapshotSync
  block: add a 'blockdev-snapshot' QMP command

 blockdev.c           | 156 ++++++++++++++++++++++++++++++++-------------------
 qapi-schema.json     |   3 +-
 qapi/block-core.json |  32 ++++++++++-
 qmp-commands.hx      |  28 +++++++++
 4 files changed, 158 insertions(+), 61 deletions(-)

-- 
2.5.1

[Qemu-devel] [PATCH v2 1/2] block: rename BlockdevSnapshot to BlockdevSnapshotSync

[Alberto Garcia]

We will introduce the 'blockdev-snapshot' command that will require
its own struct for the parameters, so we need to rename this one in
order to avoid name clashes.

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 blockdev.c           | 2 +-
 qapi-schema.json     | 2 +-
 qapi/block-core.json | 8 ++++----
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 3f42863..6b787c1 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1166,7 +1166,7 @@ void qmp_blockdev_snapshot_sync(bool has_device, const char *device,
                                 bool has_format, const char *format,
                                 bool has_mode, NewImageMode mode, Error **errp)
 {
-    BlockdevSnapshot snapshot = {
+    BlockdevSnapshotSync snapshot = {
         .has_device = has_device,
         .device = (char *) device,
         .has_node_name = has_node_name,
diff --git a/qapi-schema.json b/qapi-schema.json
index 1521eb7..c32dc20 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1496,7 +1496,7 @@
 ##
 { 'union': 'TransactionAction',
   'data': {
-       'blockdev-snapshot-sync': 'BlockdevSnapshot',
+       'blockdev-snapshot-sync': 'BlockdevSnapshotSync',
        'drive-backup': 'DriveBackup',
        'blockdev-backup': 'BlockdevBackup',
        'abort': 'Abort',
diff --git a/qapi/block-core.json b/qapi/block-core.json
index cb99cad..ec50f06 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -682,7 +682,7 @@
   'data': [ 'existing', 'absolute-paths' ] }
 
 ##
-# @BlockdevSnapshot
+# @BlockdevSnapshotSync
 #
 # Either @device or @node-name must be set but not both.
 #
@@ -699,7 +699,7 @@
 # @mode: #optional whether and how QEMU should create a new image, default is
 #        'absolute-paths'.
 ##
-{ 'struct': 'BlockdevSnapshot',
+{ 'struct': 'BlockdevSnapshotSync',
   'data': { '*device': 'str', '*node-name': 'str',
             'snapshot-file': 'str', '*snapshot-node-name': 'str',
             '*format': 'str', '*mode': 'NewImageMode' } }
@@ -790,7 +790,7 @@
 #
 # Generates a synchronous snapshot of a block device.
 #
-# For the arguments, see the documentation of BlockdevSnapshot.
+# For the arguments, see the documentation of BlockdevSnapshotSync.
 #
 # Returns: nothing on success
 #          If @device is not a valid block device, DeviceNotFound
@@ -798,7 +798,7 @@
 # Since 0.14.0
 ##
 { 'command': 'blockdev-snapshot-sync',
-  'data': 'BlockdevSnapshot' }
+  'data': 'BlockdevSnapshotSync' }
 
 ##
 # @change-backing-file
-- 
2.5.1

Re: [Qemu-devel] [PATCH v2 1/2] block: rename BlockdevSnapshot to BlockdevSnapshotSync

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 597 bytes --]

On 09/04/2015 06:37 AM, Alberto Garcia wrote:
> We will introduce the 'blockdev-snapshot' command that will require
> its own struct for the parameters, so we need to rename this one in
> order to avoid name clashes.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  blockdev.c           | 2 +-
>  qapi-schema.json     | 2 +-
>  qapi/block-core.json | 8 ++++----
>  3 files changed, 6 insertions(+), 6 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

[Qemu-devel] [PATCH v2 2/2] block: add a 'blockdev-snapshot' QMP command

[Alberto Garcia]

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           | 154 ++++++++++++++++++++++++++++++++-------------------
 qapi-schema.json     |   1 +
 qapi/block-core.json |  26 +++++++++
 qmp-commands.hx      |  28 ++++++++++
 4 files changed, 153 insertions(+), 56 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 6b787c1..db6e3bf 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,60 +1533,52 @@ 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);
+    state->old_bs = bdrv_lookup_bs(device, node_name, &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");
-        return;
-    }
-
     /* Acquire AioContext now so any threads operating on old_bs stop */
     state->aio_context = bdrv_get_aio_context(state->old_bs);
     aio_context_acquire(state->aio_context);
@@ -1601,35 +1605,67 @@ static void external_snapshot_prepare(BlkTransactionState *common,
         return;
     }
 
-    flags = state->old_bs->open_flags;
-
-    /* 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) {
+    if (snapshot_ref) {
+        if (!bdrv_lookup_bs(snapshot_ref, snapshot_ref, &local_err)) {
             error_propagate(errp, local_err);
             return;
         }
     }
 
-    options = qdict_new();
-    if (has_snapshot_node_name) {
-        qdict_put(options, "node-name",
-                  qstring_from_str(snapshot_node_name));
+    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;
+
+        if (node_name && !snapshot_node_name) {
+            error_setg(errp, "New snapshot node name missing");
+            return;
+        }
+
+        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, &local_err);
     /* 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));
     }
 }
 
@@ -1818,6 +1854,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..7dfaea9 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1496,6 +1496,7 @@
 ##
 { '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 ec50f06..adfae23 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..5bc2991 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1454,6 +1454,34 @@ 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 of a block device. 'device' and 'snapshot' both
+refer to existing block devices. The former is the one to generate the
+snapshot from, and the latter is the target of the snapshot.
+
+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,
-- 
2.5.1

Re: [Qemu-devel] [PATCH v2 2/2] block: add a 'blockdev-snapshot' QMP command

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 7645 bytes --]

On 09/04/2015 06:37 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.

I like it. Obviously, we have to get the pre-requisites in first, to
allow opening a BDS without a BB, but it is nicer overall.

> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  blockdev.c           | 154 ++++++++++++++++++++++++++++++++-------------------
>  qapi-schema.json     |   1 +
>  qapi/block-core.json |  26 +++++++++
>  qmp-commands.hx      |  28 ++++++++++
>  4 files changed, 153 insertions(+), 56 deletions(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index 6b787c1..db6e3bf 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,

Is 'node' a better name than 'device' here?

>                                           Error **errp)
> @@ -1521,60 +1533,52 @@ typedef struct ExternalSnapshotState {
>  static void external_snapshot_prepare(BlkTransactionState *common,
>                                        Error **errp)
>  {

> +    /* '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;

Again, I think this should be setting s->node_name, not 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);
> +    state->old_bs = bdrv_lookup_bs(device, node_name, &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");
> -        return;
> -    }
> -
>      /* Acquire AioContext now so any threads operating on old_bs stop */
>      state->aio_context = bdrv_get_aio_context(state->old_bs);
>      aio_context_acquire(state->aio_context);
> @@ -1601,35 +1605,67 @@ static void external_snapshot_prepare(BlkTransactionState *common,
>          return;
>      }
>  
> -    flags = state->old_bs->open_flags;
> -
> -    /* 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) {
> +    if (snapshot_ref) {
> +        if (!bdrv_lookup_bs(snapshot_ref, snapshot_ref, &local_err)) {
>              error_propagate(errp, local_err);
>              return;
>          }
>      }

Shouldn't you also check that snapshot_ref does not currently have a
backing BDS (as it is the act of creating the snapshot that sets the
current device as the backing of the snapshot_ref BDS before altering
the BB to point to snapshot_ref as its new BDS)?

> +++ b/qapi-schema.json
> @@ -1496,6 +1496,7 @@
>  ##
>  { 'union': 'TransactionAction',
>    'data': {
> +       'blockdev-snapshot': 'BlockdevSnapshot',

Missing 'since 2.5' documentation.

>         'blockdev-snapshot-sync': 'BlockdevSnapshotSync',
>         'drive-backup': 'DriveBackup',
>         'blockdev-backup': 'BlockdevBackup',
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index ec50f06..adfae23 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.

Would naming this 'node' make more sense?

> +#
> +# @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' }
> +

Overall, I like the syntax.

> +SQMP
> +blockdev-snapshot
> +-----------------
> +Since 2.5
> +
> +Create a snapshot of a block device. 'device' and 'snapshot' both
> +refer to existing block devices. The former is the one to generate the
> +snapshot from, and the latter is the target of the snapshot.

Is there any better terminology?  Maybe:

The act of creating a snapshot installs '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.

Hmm - I wonder if that means we should have an optional boolean
parameter that allows us to avoid the automatic pivot.  After all, with
'blockdev-snapshot-sync', you can specify 'device' and omit 'node-name'
to update the device's active layer, or you can omit 'device' and
specify 'node-name' to create another qcow2 file but NOT install it as
the active layer, regardless of which 'node-name' that serves as the
starting point. So when 'node-name' is the BDS node that 'device' is
currently visiting, you have control over whether 'device' auto-updates
to the new BDS.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v2 2/2] block: add a 'blockdev-snapshot' QMP command

[Alberto Garcia]

On Fri 04 Sep 2015 04:42:17 PM CEST, Eric Blake <eblake@redhat.com> wrote:
>> @@ -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,
>
> Is 'node' a better name than 'device' here?

I don't have a strong preference (I was just following Kevin's
suggestion), but 'device' seems to be the most common name for this
parameter. This one can take a node name as well, but it will only
accept an active layer anyway... I can change the name if you prefer.

>> +    if (snapshot_ref) {
>> +        if (!bdrv_lookup_bs(snapshot_ref, snapshot_ref, &local_err)) {
>>              error_propagate(errp, local_err);
>>              return;
>>          }
>>      }
>
> Shouldn't you also check that snapshot_ref does not currently have a
> backing BDS (as it is the act of creating the snapshot that sets the
> current device as the backing of the snapshot_ref BDS before altering
> the BB to point to snapshot_ref as its new BDS)?

I think you're right, thanks!

>> +SQMP
>> +blockdev-snapshot
>> +-----------------
>> +Since 2.5
>> +
>> +Create a snapshot of a block device. 'device' and 'snapshot' both
>> +refer to existing block devices. The former is the one to generate
>> +the snapshot from, and the latter is the target of the snapshot.
>
> Is there any better terminology?  Maybe:
>
> The act of creating a snapshot installs '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.

Sounds good.

> Hmm - I wonder if that means we should have an optional boolean
> parameter that allows us to avoid the automatic pivot.  After all,
> with 'blockdev-snapshot-sync', you can specify 'device' and omit
> 'node-name' to update the device's active layer, or you can omit
> 'device' and specify 'node-name' to create another qcow2 file but NOT
> install it as the active layer, regardless of which 'node-name' that
> serves as the starting point. So when 'node-name' is the BDS node that
> 'device' is currently visiting, you have control over whether 'device'
> auto-updates to the new BDS.

What's the use case for that?

Berto

Re: [Qemu-devel] [PATCH v2 2/2] block: add a 'blockdev-snapshot' QMP command

[Alberto Garcia]

On Fri 04 Sep 2015 04:42:17 PM CEST, Eric Blake <eblake@redhat.com> wrote:
>> +    if (snapshot_ref) {
>> +        if (!bdrv_lookup_bs(snapshot_ref, snapshot_ref, &local_err)) {
>>              error_propagate(errp, local_err);
>>              return;
>>          }
>>      }
>
> Shouldn't you also check that snapshot_ref does not currently have a
> backing BDS (as it is the act of creating the snapshot that sets the
> current device as the backing of the snapshot_ref BDS before altering
> the BB to point to snapshot_ref as its new BDS)?

Wait, I actually think that it should have a backing BDS. This new
command is roughly equivalent to the 'existing' mode of the
'blockdev-snapshot-sync' command, so the new image must be created
externally in both cases having the current image as its backing file.

The difference is that 'blockdev-snapshot-sync' will open the new image
and not its backing chain (using the BDRV_O_NO_BACKING flag), but in
'blockdev-snapshot' the image must be opened previously with
'blockdev-add', which will open the whole chain.

So I think that we should expect a backing image and we have to unref it
during the 'commit' part of the transaction (before bdrv_append()).

Berto

Re: [Qemu-devel] [PATCH v2 2/2] block: add a 'blockdev-snapshot' QMP command

[Kevin Wolf]

Am 07.09.2015 um 13:31 hat Alberto Garcia geschrieben:
> On Fri 04 Sep 2015 04:42:17 PM CEST, Eric Blake <eblake@redhat.com> wrote:
> >> +    if (snapshot_ref) {
> >> +        if (!bdrv_lookup_bs(snapshot_ref, snapshot_ref, &local_err)) {
> >>              error_propagate(errp, local_err);
> >>              return;
> >>          }
> >>      }
> >
> > Shouldn't you also check that snapshot_ref does not currently have a
> > backing BDS (as it is the act of creating the snapshot that sets the
> > current device as the backing of the snapshot_ref BDS before altering
> > the BB to point to snapshot_ref as its new BDS)?
> 
> Wait, I actually think that it should have a backing BDS. This new
> command is roughly equivalent to the 'existing' mode of the
> 'blockdev-snapshot-sync' command, so the new image must be created
> externally in both cases having the current image as its backing file.
> 
> The difference is that 'blockdev-snapshot-sync' will open the new image
> and not its backing chain (using the BDRV_O_NO_BACKING flag), but in
> 'blockdev-snapshot' the image must be opened previously with
> 'blockdev-add', which will open the whole chain.
> 
> So I think that we should expect a backing image and we have to unref it
> during the 'commit' part of the transaction (before bdrv_append()).

This means that the images in the backing chain are opened twice before
doing the bdrv_append(). This definitely includes the active layer as
one example of an illegal setup, and with block jobs running on the
backing files it might involve even more writable image files.

I think Eric's approach is better, where we check the required
conditions instead of adding magic to somehow transfer a questionable
state into what we want.

Of course, this means that we need to provide a way to actually fulfill
the right conditions, and you might be right that it doesn't exist yet.
Could the same effect as BDRV_O_NO_BACKING possibly be achieved with
blockdev-add with something like '"backing": {}'?

We might actually have the same problem in more places: In optional
fields, we need to distinguish "use the default" from "I don't want
any". Especially if we want to support reopen in QMP at some point, we
need to figure this out (default is leaving options unchanged, but
unsetting must be possible).

Kevin

Re: [Qemu-devel] [PATCH v2 2/2] block: add a 'blockdev-snapshot' QMP command

[Alberto Garcia]

On Mon 07 Sep 2015 04:55:47 PM CEST, Kevin Wolf wrote:

> Of course, this means that we need to provide a way to actually
> fulfill the right conditions, and you might be right that it doesn't
> exist yet.  Could the same effect as BDRV_O_NO_BACKING possibly be
> achieved with blockdev-add with something like '"backing": {}'?

I'm not sure it that would be the best API, but the overall idea looks
good to me.

Berto