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

[Eric Blake <eblake@redhat.com>]

[-- 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 --]