[Qemu-devel] [PATCH v5 0/2] Group Live Snapshots

[Qemu-devel] [PATCH v5 0/2] Group Live Snapshots

[Jeff Cody]

This patchset adds the ability to take a snapshot of a group of devices,
rather than each device individually.  Upon failure of any snapshot, all
snapshots taken by the command will be abandoned, and the appropriate failure
code returned.

All the changes from v4 to v5 are in patch 1/2. This differs from v4 in that:
    * blockdev.c
		- all failures in the group snapshot command should go to
		  delete_and_fail

Jeff Cody (2):
  qapi: Introduce blockdev-group-snapshot-sync command
  QMP: Add qmp command for blockdev-group-snapshot-sync

 block.c          |   81 +++++++++++++++++++++++++++++++++
 block.h          |    1 +
 block_int.h      |    6 +++
 blockdev.c       |  131 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 qapi-schema.json |   38 ++++++++++++++++
 qmp-commands.hx  |   39 ++++++++++++++++
 6 files changed, 296 insertions(+), 0 deletions(-)

-- 
1.7.9.rc2.1.g69204

[Qemu-devel] [PATCH v5 1/2] qapi: Introduce blockdev-group-snapshot-sync command

[Jeff Cody]

This is a QAPI/QMP only command to take a snapshot of a group of
devices. This is similar to the blockdev-snapshot-sync command, except
blockdev-group-snapshot-sync accepts a list devices, filenames, and
formats.

It is attempted to keep the snapshot of the group atomic; if the
creation or open of any of the new snapshots fails, then all of
the new snapshots are abandoned, and the name of the snapshot image
that failed is returned.  The failure case should not interrupt
any operations.

Rather than use bdrv_close() along with a subsequent bdrv_open() to
perform the pivot, the original image is never closed and the new
image is placed 'in front' of the original image via manipulation
of the BlockDriverState fields.  Thus, once the new snapshot image
has been successfully created, there are no more failure points
before pivoting to the new snapshot.

This allows the group of disks to remain consistent with each other,
even across snapshot failures.

Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block.c          |   81 +++++++++++++++++++++++++++++++++
 block.h          |    1 +
 block_int.h      |    6 +++
 blockdev.c       |  131 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 qapi-schema.json |   38 ++++++++++++++++
 5 files changed, 257 insertions(+), 0 deletions(-)

diff --git a/block.c b/block.c
index 3621d11..90232d9 100644
--- a/block.c
+++ b/block.c
@@ -880,6 +880,87 @@ void bdrv_make_anon(BlockDriverState *bs)
     bs->device_name[0] = '\0';
 }
 
+/*
+ * Add new bs contents at the top of an image chain while the chain is
+ * live, while keeping required fields on the top layer.
+ *
+ * This will modify the BlockDriverState fields, and swap contents
+ * between bs_new and bs_top. Both bs_new and bs_top are modified.
+ *
+ * This function does not create any image files.
+ */
+void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top)
+{
+    BlockDriverState tmp;
+
+    /* the new bs must not be in bdrv_states */
+    bdrv_make_anon(bs_new);
+
+    tmp = *bs_new;
+
+    /* there are some fields that need to stay on the top layer: */
+
+    /* dev info */
+    tmp.dev_ops           = bs_top->dev_ops;
+    tmp.dev_opaque        = bs_top->dev_opaque;
+    tmp.dev               = bs_top->dev;
+    tmp.buffer_alignment  = bs_top->buffer_alignment;
+    tmp.copy_on_read      = bs_top->copy_on_read;
+
+    /* i/o timing parameters */
+    tmp.slice_time        = bs_top->slice_time;
+    tmp.slice_start       = bs_top->slice_start;
+    tmp.slice_end         = bs_top->slice_end;
+    tmp.io_limits         = bs_top->io_limits;
+    tmp.io_base           = bs_top->io_base;
+    tmp.throttled_reqs    = bs_top->throttled_reqs;
+    tmp.block_timer       = bs_top->block_timer;
+    tmp.io_limits_enabled = bs_top->io_limits_enabled;
+
+    /* geometry */
+    tmp.cyls              = bs_top->cyls;
+    tmp.heads             = bs_top->heads;
+    tmp.secs              = bs_top->secs;
+    tmp.translation       = bs_top->translation;
+
+    /* r/w error */
+    tmp.on_read_error     = bs_top->on_read_error;
+    tmp.on_write_error    = bs_top->on_write_error;
+
+    /* i/o status */
+    tmp.iostatus_enabled  = bs_top->iostatus_enabled;
+    tmp.iostatus          = bs_top->iostatus;
+
+    /* keep the same entry in bdrv_states */
+    pstrcpy(tmp.device_name, sizeof(tmp.device_name), bs_top->device_name);
+    tmp.list = bs_top->list;
+
+    /* The contents of 'tmp' will become bs_top, as we are
+     * swapping bs_new and bs_top contents. */
+    tmp.backing_hd = bs_new;
+    pstrcpy(tmp.backing_file, sizeof(tmp.backing_file), bs_top->filename);
+
+    /* swap contents of the fixed new bs and the current top */
+    *bs_new = *bs_top;
+    *bs_top = tmp;
+
+    /* clear the copied fields in the new backing file */
+    bdrv_detach_dev(bs_new, bs_new->dev);
+
+    qemu_co_queue_init(&bs_new->throttled_reqs);
+    memset(&bs_new->io_base,   0, sizeof(bs_new->io_base));
+    memset(&bs_new->io_limits, 0, sizeof(bs_new->io_limits));
+    bdrv_iostatus_disable(bs_new);
+
+    /* we don't use bdrv_io_limits_disable() for this, because we don't want
+     * to affect or delete the block_timer, as it has been moved to bs_top */
+    bs_new->io_limits_enabled = false;
+    bs_new->block_timer       = NULL;
+    bs_new->slice_time        = 0;
+    bs_new->slice_start       = 0;
+    bs_new->slice_end         = 0;
+}
+
 void bdrv_delete(BlockDriverState *bs)
 {
     assert(!bs->dev);
diff --git a/block.h b/block.h
index cae289b..190a780 100644
--- a/block.h
+++ b/block.h
@@ -114,6 +114,7 @@ int bdrv_create(BlockDriver *drv, const char* filename,
 int bdrv_create_file(const char* filename, QEMUOptionParameter *options);
 BlockDriverState *bdrv_new(const char *device_name);
 void bdrv_make_anon(BlockDriverState *bs);
+void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top);
 void bdrv_delete(BlockDriverState *bs);
 int bdrv_parse_cache_flags(const char *mode, int *flags);
 int bdrv_file_open(BlockDriverState **pbs, const char *filename, int flags);
diff --git a/block_int.h b/block_int.h
index 7be2988..5edc8c1 100644
--- a/block_int.h
+++ b/block_int.h
@@ -219,6 +219,12 @@ struct BlockDriver {
     QLIST_ENTRY(BlockDriver) list;
 };
 
+/*
+ * Note: the function bdrv_append() copies and swaps contents of
+ * BlockDriverStates, so if you add new fields to this struct, please
+ * inspect bdrv_append() to determine if the new fields need to be
+ * copied as well.
+ */
 struct BlockDriverState {
     int64_t total_sectors; /* if we are reading a disk image, give its
                               size in sectors */
diff --git a/blockdev.c b/blockdev.c
index ab9fd21..4be9947 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -719,6 +719,137 @@ void qmp_blockdev_snapshot_sync(const char *device, const char *snapshot_file,
     }
 }
 
+
+/* New and old BlockDriverState structs for group snapshots */
+typedef struct BlkGroupSnapshotStates {
+    BlockDriverState *old_bs;
+    BlockDriverState *new_bs;
+    QSIMPLEQ_ENTRY(BlkGroupSnapshotStates) entry;
+} BlkGroupSnapshotStates;
+
+/*
+ * 'Atomic' group snapshots.  The snapshots are taken as a set, and if any fail
+ *  then we do not pivot any of the devices in the group, and abandon the
+ *  snapshots
+ */
+void qmp_blockdev_group_snapshot_sync(SnapshotDevList *dev_list,
+                                      Error **errp)
+{
+    int ret = 0;
+    SnapshotDevList *dev_entry = dev_list;
+    SnapshotDev *dev_info = NULL;
+    BlkGroupSnapshotStates *states;
+    BlockDriver *proto_drv;
+    BlockDriver *drv;
+    int flags;
+    const char *format;
+    const char *snapshot_file;
+
+    QSIMPLEQ_HEAD(snap_bdrv_states, BlkGroupSnapshotStates) snap_bdrv_states;
+    QSIMPLEQ_INIT(&snap_bdrv_states);
+
+    /* drain all i/o before any snapshots */
+    bdrv_drain_all();
+
+    /* We don't do anything in this loop that commits us to the snapshot */
+    while (NULL != dev_entry) {
+        dev_info = dev_entry->value;
+        dev_entry = dev_entry->next;
+
+        states = g_malloc0(sizeof(BlkGroupSnapshotStates));
+        QSIMPLEQ_INSERT_TAIL(&snap_bdrv_states, states, entry);
+
+        states->old_bs = bdrv_find(dev_info->device);
+
+        if (!states->old_bs) {
+            error_set(errp, QERR_DEVICE_NOT_FOUND, dev_info->device);
+            goto delete_and_fail;
+        }
+
+        if (bdrv_in_use(states->old_bs)) {
+            error_set(errp, QERR_DEVICE_IN_USE, dev_info->device);
+            goto delete_and_fail;
+        }
+
+        if (!bdrv_is_read_only(states->old_bs) &&
+             bdrv_is_inserted(states->old_bs)) {
+
+            if (bdrv_flush(states->old_bs)) {
+                error_set(errp, QERR_IO_ERROR);
+                goto delete_and_fail;
+            }
+        }
+
+        snapshot_file = dev_info->snapshot_file;
+
+        flags = states->old_bs->open_flags;
+
+        if (!dev_info->has_format) {
+            format = "qcow2";
+        } else {
+            format = dev_info->format;
+        }
+
+        drv = bdrv_find_format(format);
+        if (!drv) {
+            error_set(errp, QERR_INVALID_BLOCK_FORMAT, format);
+            goto delete_and_fail;
+        }
+
+        proto_drv = bdrv_find_protocol(snapshot_file);
+        if (!proto_drv) {
+            error_set(errp, QERR_INVALID_BLOCK_FORMAT, format);
+            goto delete_and_fail;
+        }
+
+        /* create new image w/backing file */
+        ret = bdrv_img_create(snapshot_file, format,
+                              states->old_bs->filename,
+                              drv->format_name, NULL, -1, flags);
+        if (ret) {
+            error_set(errp, QERR_OPEN_FILE_FAILED, snapshot_file);
+            goto delete_and_fail;
+        }
+
+        /* We will manually add the backing_hd field to the bs later */
+        states->new_bs = bdrv_new("");
+        ret = bdrv_open(states->new_bs, snapshot_file,
+                        flags | BDRV_O_NO_BACKING, drv);
+        if (ret != 0) {
+            error_set(errp, QERR_OPEN_FILE_FAILED, snapshot_file);
+            goto delete_and_fail;
+        }
+    }
+
+
+    /* Now we are going to do the actual pivot.  Everything up to this point
+     * is reversible, but we are committed at this point */
+    QSIMPLEQ_FOREACH(states, &snap_bdrv_states, entry) {
+        /* This removes our old bs from the bdrv_states, and adds the new bs */
+        bdrv_append(states->new_bs, states->old_bs);
+    }
+
+    /* success */
+    goto exit;
+
+delete_and_fail:
+    /*
+    * failure, and it is all-or-none; abandon each new bs, and keep using
+    * the original bs for all images
+    */
+    QSIMPLEQ_FOREACH(states, &snap_bdrv_states, entry) {
+        if (states->new_bs) {
+             bdrv_delete(states->new_bs);
+        }
+    }
+exit:
+    QSIMPLEQ_FOREACH(states, &snap_bdrv_states, entry) {
+        g_free(states);
+    }
+    return;
+}
+
+
 static void eject_device(BlockDriverState *bs, int force, Error **errp)
 {
     if (bdrv_in_use(bs)) {
diff --git a/qapi-schema.json b/qapi-schema.json
index d02ee86..165e22e 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1107,6 +1107,44 @@
 { 'command': 'block_resize', 'data': { 'device': 'str', 'size': 'int' }}
 
 ##
+# @SnapshotDev
+#
+# @device:  the name of the device to generate the snapshot from.
+#
+# @snapshot-file: the target of the new image. A new file will be created.
+#
+# @format: #optional the format of the snapshot image, default is 'qcow2'.
+##
+{ 'type': 'SnapshotDev',
+  'data': {'device': 'str', 'snapshot-file': 'str', '*format': 'str' } }
+
+##
+# @blockdev-group-snapshot-sync
+#
+# Generates a synchronous snapshot of a group of one or more block devices,
+# as atomically as possible.  If the snapshot of any device in the group
+# fails, then the entire group snapshot will be abandoned and the
+# appropriate error returned.
+#
+#  List of:
+#  @SnapshotDev: information needed for the device snapshot
+#
+# Returns: nothing on success
+#          If @device is not a valid block device, DeviceNotFound
+#          If @device is busy, DeviceInUse will be returned
+#          If @snapshot-file can't be created, OpenFileFailed
+#          If @snapshot-file can't be opened, OpenFileFailed
+#          If @format is invalid, InvalidBlockFormat
+#
+# Note: The group snapshot attempt returns failure on the first snapshot
+# device failure.  Therefore, there will be only one device or snapshot file
+# returned in an error condition, and subsequent devices will not have been
+# attempted.
+##
+{ 'command': 'blockdev-group-snapshot-sync',
+  'data': { 'devlist': [ 'SnapshotDev' ] } }
+
+##
 # @blockdev-snapshot-sync
 #
 # Generates a synchronous snapshot of a block device.
-- 
1.7.9.rc2.1.g69204

Re: [Qemu-devel] [PATCH v5 1/2] qapi: Introduce blockdev-group-snapshot-sync command

[Luiz Capitulino]

On Tue, 28 Feb 2012 15:54:06 -0500
Jeff Cody <jcody@redhat.com> wrote:

> This is a QAPI/QMP only command to take a snapshot of a group of
> devices. This is similar to the blockdev-snapshot-sync command, except
> blockdev-group-snapshot-sync accepts a list devices, filenames, and
> formats.
> 
> It is attempted to keep the snapshot of the group atomic; if the
> creation or open of any of the new snapshots fails, then all of
> the new snapshots are abandoned, and the name of the snapshot image
> that failed is returned.  The failure case should not interrupt
> any operations.
> 
> Rather than use bdrv_close() along with a subsequent bdrv_open() to
> perform the pivot, the original image is never closed and the new
> image is placed 'in front' of the original image via manipulation
> of the BlockDriverState fields.  Thus, once the new snapshot image
> has been successfully created, there are no more failure points
> before pivoting to the new snapshot.
> 
> This allows the group of disks to remain consistent with each other,
> even across snapshot failures.
> 
> Signed-off-by: Jeff Cody <jcody@redhat.com>

This looks fine to me, I have some small remarks that can be addressed by a
follow up patch or ignored :)

> ---
>  block.c          |   81 +++++++++++++++++++++++++++++++++
>  block.h          |    1 +
>  block_int.h      |    6 +++
>  blockdev.c       |  131 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  qapi-schema.json |   38 ++++++++++++++++
>  5 files changed, 257 insertions(+), 0 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 3621d11..90232d9 100644
> --- a/block.c
> +++ b/block.c
> @@ -880,6 +880,87 @@ void bdrv_make_anon(BlockDriverState *bs)
>      bs->device_name[0] = '\0';
>  }
>  
> +/*
> + * Add new bs contents at the top of an image chain while the chain is
> + * live, while keeping required fields on the top layer.
> + *
> + * This will modify the BlockDriverState fields, and swap contents
> + * between bs_new and bs_top. Both bs_new and bs_top are modified.
> + *
> + * This function does not create any image files.
> + */
> +void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top)
> +{
> +    BlockDriverState tmp;
> +
> +    /* the new bs must not be in bdrv_states */
> +    bdrv_make_anon(bs_new);

That should be the case already no? If so, then an assert() is better.

> +
> +    tmp = *bs_new;
> +
> +    /* there are some fields that need to stay on the top layer: */
> +
> +    /* dev info */
> +    tmp.dev_ops           = bs_top->dev_ops;
> +    tmp.dev_opaque        = bs_top->dev_opaque;
> +    tmp.dev               = bs_top->dev;
> +    tmp.buffer_alignment  = bs_top->buffer_alignment;
> +    tmp.copy_on_read      = bs_top->copy_on_read;
> +
> +    /* i/o timing parameters */
> +    tmp.slice_time        = bs_top->slice_time;
> +    tmp.slice_start       = bs_top->slice_start;
> +    tmp.slice_end         = bs_top->slice_end;
> +    tmp.io_limits         = bs_top->io_limits;
> +    tmp.io_base           = bs_top->io_base;
> +    tmp.throttled_reqs    = bs_top->throttled_reqs;
> +    tmp.block_timer       = bs_top->block_timer;
> +    tmp.io_limits_enabled = bs_top->io_limits_enabled;
> +
> +    /* geometry */
> +    tmp.cyls              = bs_top->cyls;
> +    tmp.heads             = bs_top->heads;
> +    tmp.secs              = bs_top->secs;
> +    tmp.translation       = bs_top->translation;
> +
> +    /* r/w error */
> +    tmp.on_read_error     = bs_top->on_read_error;
> +    tmp.on_write_error    = bs_top->on_write_error;
> +
> +    /* i/o status */
> +    tmp.iostatus_enabled  = bs_top->iostatus_enabled;
> +    tmp.iostatus          = bs_top->iostatus;
> +
> +    /* keep the same entry in bdrv_states */
> +    pstrcpy(tmp.device_name, sizeof(tmp.device_name), bs_top->device_name);
> +    tmp.list = bs_top->list;
> +
> +    /* The contents of 'tmp' will become bs_top, as we are
> +     * swapping bs_new and bs_top contents. */
> +    tmp.backing_hd = bs_new;
> +    pstrcpy(tmp.backing_file, sizeof(tmp.backing_file), bs_top->filename);
> +
> +    /* swap contents of the fixed new bs and the current top */
> +    *bs_new = *bs_top;
> +    *bs_top = tmp;
> +
> +    /* clear the copied fields in the new backing file */
> +    bdrv_detach_dev(bs_new, bs_new->dev);
> +
> +    qemu_co_queue_init(&bs_new->throttled_reqs);
> +    memset(&bs_new->io_base,   0, sizeof(bs_new->io_base));
> +    memset(&bs_new->io_limits, 0, sizeof(bs_new->io_limits));
> +    bdrv_iostatus_disable(bs_new);
> +
> +    /* we don't use bdrv_io_limits_disable() for this, because we don't want
> +     * to affect or delete the block_timer, as it has been moved to bs_top */
> +    bs_new->io_limits_enabled = false;
> +    bs_new->block_timer       = NULL;
> +    bs_new->slice_time        = 0;
> +    bs_new->slice_start       = 0;
> +    bs_new->slice_end         = 0;
> +}
> +
>  void bdrv_delete(BlockDriverState *bs)
>  {
>      assert(!bs->dev);
> diff --git a/block.h b/block.h
> index cae289b..190a780 100644
> --- a/block.h
> +++ b/block.h
> @@ -114,6 +114,7 @@ int bdrv_create(BlockDriver *drv, const char* filename,
>  int bdrv_create_file(const char* filename, QEMUOptionParameter *options);
>  BlockDriverState *bdrv_new(const char *device_name);
>  void bdrv_make_anon(BlockDriverState *bs);
> +void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top);
>  void bdrv_delete(BlockDriverState *bs);
>  int bdrv_parse_cache_flags(const char *mode, int *flags);
>  int bdrv_file_open(BlockDriverState **pbs, const char *filename, int flags);
> diff --git a/block_int.h b/block_int.h
> index 7be2988..5edc8c1 100644
> --- a/block_int.h
> +++ b/block_int.h
> @@ -219,6 +219,12 @@ struct BlockDriver {
>      QLIST_ENTRY(BlockDriver) list;
>  };
>  
> +/*
> + * Note: the function bdrv_append() copies and swaps contents of
> + * BlockDriverStates, so if you add new fields to this struct, please
> + * inspect bdrv_append() to determine if the new fields need to be
> + * copied as well.
> + */
>  struct BlockDriverState {
>      int64_t total_sectors; /* if we are reading a disk image, give its
>                                size in sectors */
> diff --git a/blockdev.c b/blockdev.c
> index ab9fd21..4be9947 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -719,6 +719,137 @@ void qmp_blockdev_snapshot_sync(const char *device, const char *snapshot_file,
>      }
>  }
>  
> +
> +/* New and old BlockDriverState structs for group snapshots */
> +typedef struct BlkGroupSnapshotStates {
> +    BlockDriverState *old_bs;
> +    BlockDriverState *new_bs;
> +    QSIMPLEQ_ENTRY(BlkGroupSnapshotStates) entry;
> +} BlkGroupSnapshotStates;
> +
> +/*
> + * 'Atomic' group snapshots.  The snapshots are taken as a set, and if any fail
> + *  then we do not pivot any of the devices in the group, and abandon the
> + *  snapshots
> + */
> +void qmp_blockdev_group_snapshot_sync(SnapshotDevList *dev_list,
> +                                      Error **errp)
> +{
> +    int ret = 0;
> +    SnapshotDevList *dev_entry = dev_list;
> +    SnapshotDev *dev_info = NULL;
> +    BlkGroupSnapshotStates *states;
> +    BlockDriver *proto_drv;
> +    BlockDriver *drv;
> +    int flags;
> +    const char *format;
> +    const char *snapshot_file;
> +
> +    QSIMPLEQ_HEAD(snap_bdrv_states, BlkGroupSnapshotStates) snap_bdrv_states;
> +    QSIMPLEQ_INIT(&snap_bdrv_states);
> +
> +    /* drain all i/o before any snapshots */
> +    bdrv_drain_all();
> +
> +    /* We don't do anything in this loop that commits us to the snapshot */
> +    while (NULL != dev_entry) {
> +        dev_info = dev_entry->value;
> +        dev_entry = dev_entry->next;
> +
> +        states = g_malloc0(sizeof(BlkGroupSnapshotStates));
> +        QSIMPLEQ_INSERT_TAIL(&snap_bdrv_states, states, entry);
> +
> +        states->old_bs = bdrv_find(dev_info->device);
> +
> +        if (!states->old_bs) {
> +            error_set(errp, QERR_DEVICE_NOT_FOUND, dev_info->device);
> +            goto delete_and_fail;
> +        }
> +
> +        if (bdrv_in_use(states->old_bs)) {
> +            error_set(errp, QERR_DEVICE_IN_USE, dev_info->device);
> +            goto delete_and_fail;
> +        }
> +
> +        if (!bdrv_is_read_only(states->old_bs) &&
> +             bdrv_is_inserted(states->old_bs)) {
> +
> +            if (bdrv_flush(states->old_bs)) {
> +                error_set(errp, QERR_IO_ERROR);
> +                goto delete_and_fail;
> +            }
> +        }
> +
> +        snapshot_file = dev_info->snapshot_file;
> +
> +        flags = states->old_bs->open_flags;
> +
> +        if (!dev_info->has_format) {
> +            format = "qcow2";
> +        } else {
> +            format = dev_info->format;
> +        }

I think it would have been better to default to the format of the imagine
being "snapshotted", but maybe it's even better to be compatible with the
original snapshot-sync command...

> +
> +        drv = bdrv_find_format(format);
> +        if (!drv) {
> +            error_set(errp, QERR_INVALID_BLOCK_FORMAT, format);
> +            goto delete_and_fail;
> +        }
> +
> +        proto_drv = bdrv_find_protocol(snapshot_file);
> +        if (!proto_drv) {
> +            error_set(errp, QERR_INVALID_BLOCK_FORMAT, format);
> +            goto delete_and_fail;
> +        }
> +
> +        /* create new image w/backing file */
> +        ret = bdrv_img_create(snapshot_file, format,
> +                              states->old_bs->filename,
> +                              drv->format_name, NULL, -1, flags);
> +        if (ret) {
> +            error_set(errp, QERR_OPEN_FILE_FAILED, snapshot_file);
> +            goto delete_and_fail;
> +        }
> +
> +        /* We will manually add the backing_hd field to the bs later */
> +        states->new_bs = bdrv_new("");
> +        ret = bdrv_open(states->new_bs, snapshot_file,
> +                        flags | BDRV_O_NO_BACKING, drv);
> +        if (ret != 0) {
> +            error_set(errp, QERR_OPEN_FILE_FAILED, snapshot_file);
> +            goto delete_and_fail;
> +        }
> +    }
> +
> +
> +    /* Now we are going to do the actual pivot.  Everything up to this point
> +     * is reversible, but we are committed at this point */
> +    QSIMPLEQ_FOREACH(states, &snap_bdrv_states, entry) {
> +        /* This removes our old bs from the bdrv_states, and adds the new bs */
> +        bdrv_append(states->new_bs, states->old_bs);
> +    }
> +
> +    /* success */
> +    goto exit;
> +
> +delete_and_fail:
> +    /*
> +    * failure, and it is all-or-none; abandon each new bs, and keep using
> +    * the original bs for all images
> +    */
> +    QSIMPLEQ_FOREACH(states, &snap_bdrv_states, entry) {
> +        if (states->new_bs) {
> +             bdrv_delete(states->new_bs);

Isn't it a good idea to remove any created image files?

> +        }
> +    }
> +exit:
> +    QSIMPLEQ_FOREACH(states, &snap_bdrv_states, entry) {
> +        g_free(states);
> +    }
> +    return;
> +}
> +
> +
>  static void eject_device(BlockDriverState *bs, int force, Error **errp)
>  {
>      if (bdrv_in_use(bs)) {
> diff --git a/qapi-schema.json b/qapi-schema.json
> index d02ee86..165e22e 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -1107,6 +1107,44 @@
>  { 'command': 'block_resize', 'data': { 'device': 'str', 'size': 'int' }}
>  
>  ##
> +# @SnapshotDev
> +#
> +# @device:  the name of the device to generate the snapshot from.
> +#
> +# @snapshot-file: the target of the new image. A new file will be created.
> +#
> +# @format: #optional the format of the snapshot image, default is 'qcow2'.
> +##
> +{ 'type': 'SnapshotDev',
> +  'data': {'device': 'str', 'snapshot-file': 'str', '*format': 'str' } }
> +
> +##
> +# @blockdev-group-snapshot-sync
> +#
> +# Generates a synchronous snapshot of a group of one or more block devices,
> +# as atomically as possible.  If the snapshot of any device in the group
> +# fails, then the entire group snapshot will be abandoned and the
> +# appropriate error returned.
> +#
> +#  List of:
> +#  @SnapshotDev: information needed for the device snapshot
> +#
> +# Returns: nothing on success
> +#          If @device is not a valid block device, DeviceNotFound
> +#          If @device is busy, DeviceInUse will be returned
> +#          If @snapshot-file can't be created, OpenFileFailed
> +#          If @snapshot-file can't be opened, OpenFileFailed
> +#          If @format is invalid, InvalidBlockFormat
> +#
> +# Note: The group snapshot attempt returns failure on the first snapshot
> +# device failure.  Therefore, there will be only one device or snapshot file
> +# returned in an error condition, and subsequent devices will not have been
> +# attempted.
> +##
> +{ 'command': 'blockdev-group-snapshot-sync',
> +  'data': { 'devlist': [ 'SnapshotDev' ] } }
> +
> +##
>  # @blockdev-snapshot-sync
>  #
>  # Generates a synchronous snapshot of a block device.

[Qemu-devel] [PATCH v5 2/2] QMP: Add qmp command for blockdev-group-snapshot-sync

[Jeff Cody]

This adds the QMP command for blockdev-group-snapshot-sync. It
takes an array in as the input, for the argument devlist.  The
array consists of the following elements:

    + device:        device to snapshot. e.g. "ide-hd0", "virtio0"
    + snapshot-file: path & file for the snapshot image. e.g. "/tmp/file.img"
    + format:        snapshot format. e.g., "qcow2". Optional

There is no HMP equivalent for the command.

Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 qmp-commands.hx |   39 +++++++++++++++++++++++++++++++++++++++
 1 files changed, 39 insertions(+), 0 deletions(-)

diff --git a/qmp-commands.hx b/qmp-commands.hx
index b5e2ab8..15d35c1 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -665,6 +665,45 @@ EQMP
         .args_type  = "device:B",
         .mhandler.cmd_new = qmp_marshal_input_block_job_cancel,
     },
+    {
+        .name       = "blockdev-group-snapshot-sync",
+        .args_type  = "devlist:O",
+        .params  = "device:B,snapshot-file:s,format:s?",
+        .mhandler.cmd_new = qmp_marshal_input_blockdev_group_snapshot_sync,
+    },
+
+SQMP
+blockdev-group-snapshot-sync
+----------------------
+
+Synchronous snapshot of one or more block devices.  A list array input
+is accepted, that contains the device and snapshot file information for
+each device in group. The default format, if not specified, is qcow2.
+
+If there is any failure creating or opening a new snapshot, all snapshots
+for the group are abandoned, and the original disks pre-snapshot attempt
+are used.
+
+
+Arguments:
+
+devlist array:
+    - "device": device name to snapshot (json-string)
+    - "snapshot-file": name of new image file (json-string)
+    - "format": format of new image (json-string, optional)
+
+Example:
+
+-> { "execute": "blockdev-group-snapshot-sync", "arguments":
+                      { "devlist": [{ "device": "ide-hd0",
+                                      "snapshot-file": "/some/place/my-image",
+                                      "format": "qcow2" },
+                                    { "device": "ide-hd1",
+                                      "snapshot-file": "/some/place/my-image2",
+                                      "format": "qcow2" }] } }
+<- { "return": {} }
+
+EQMP
 
     {
         .name       = "blockdev-snapshot-sync",
-- 
1.7.9.rc2.1.g69204

Re: [Qemu-devel] [PATCH v5 2/2] QMP: Add qmp command for blockdev-group-snapshot-sync

[Luiz Capitulino]

On Tue, 28 Feb 2012 15:54:07 -0500
Jeff Cody <jcody@redhat.com> wrote:

> This adds the QMP command for blockdev-group-snapshot-sync. It
> takes an array in as the input, for the argument devlist.  The
> array consists of the following elements:
> 
>     + device:        device to snapshot. e.g. "ide-hd0", "virtio0"
>     + snapshot-file: path & file for the snapshot image. e.g. "/tmp/file.img"
>     + format:        snapshot format. e.g., "qcow2". Optional
> 
> There is no HMP equivalent for the command.
> 
> Signed-off-by: Jeff Cody <jcody@redhat.com>

Could have been squashed with the previous patch, but of course that you
don't have to respin because of that.

> ---
>  qmp-commands.hx |   39 +++++++++++++++++++++++++++++++++++++++
>  1 files changed, 39 insertions(+), 0 deletions(-)
> 
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index b5e2ab8..15d35c1 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -665,6 +665,45 @@ EQMP
>          .args_type  = "device:B",
>          .mhandler.cmd_new = qmp_marshal_input_block_job_cancel,
>      },
> +    {
> +        .name       = "blockdev-group-snapshot-sync",
> +        .args_type  = "devlist:O",
> +        .params  = "device:B,snapshot-file:s,format:s?",
> +        .mhandler.cmd_new = qmp_marshal_input_blockdev_group_snapshot_sync,
> +    },
> +
> +SQMP
> +blockdev-group-snapshot-sync
> +----------------------
> +
> +Synchronous snapshot of one or more block devices.  A list array input
> +is accepted, that contains the device and snapshot file information for
> +each device in group. The default format, if not specified, is qcow2.
> +
> +If there is any failure creating or opening a new snapshot, all snapshots
> +for the group are abandoned, and the original disks pre-snapshot attempt
> +are used.
> +
> +
> +Arguments:
> +
> +devlist array:
> +    - "device": device name to snapshot (json-string)
> +    - "snapshot-file": name of new image file (json-string)
> +    - "format": format of new image (json-string, optional)
> +
> +Example:
> +
> +-> { "execute": "blockdev-group-snapshot-sync", "arguments":
> +                      { "devlist": [{ "device": "ide-hd0",
> +                                      "snapshot-file": "/some/place/my-image",
> +                                      "format": "qcow2" },
> +                                    { "device": "ide-hd1",
> +                                      "snapshot-file": "/some/place/my-image2",
> +                                      "format": "qcow2" }] } }
> +<- { "return": {} }
> +
> +EQMP
>  
>      {
>          .name       = "blockdev-snapshot-sync",

Re: [Qemu-devel] [PATCH v5 0/2] Group Live Snapshots

[Kevin Wolf]

Am 28.02.2012 21:54, schrieb Jeff Cody:
> This patchset adds the ability to take a snapshot of a group of devices,
> rather than each device individually.  Upon failure of any snapshot, all
> snapshots taken by the command will be abandoned, and the appropriate failure
> code returned.
> 
> All the changes from v4 to v5 are in patch 1/2. This differs from v4 in that:
>     * blockdev.c
> 		- all failures in the group snapshot command should go to
> 		  delete_and_fail
> 
> Jeff Cody (2):
>   qapi: Introduce blockdev-group-snapshot-sync command
>   QMP: Add qmp command for blockdev-group-snapshot-sync
> 
>  block.c          |   81 +++++++++++++++++++++++++++++++++
>  block.h          |    1 +
>  block_int.h      |    6 +++
>  blockdev.c       |  131 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  qapi-schema.json |   38 ++++++++++++++++
>  qmp-commands.hx  |   39 ++++++++++++++++
>  6 files changed, 296 insertions(+), 0 deletions(-)
> 

Thanks, applied to the block branch.

Kevin

Re: [Qemu-devel] [PATCH v5 0/2] Group Live Snapshots

[Luiz Capitulino]

On Wed, 29 Feb 2012 12:54:50 +0100
Kevin Wolf <kwolf@redhat.com> wrote:

> Am 28.02.2012 21:54, schrieb Jeff Cody:
> > This patchset adds the ability to take a snapshot of a group of devices,
> > rather than each device individually.  Upon failure of any snapshot, all
> > snapshots taken by the command will be abandoned, and the appropriate failure
> > code returned.
> > 
> > All the changes from v4 to v5 are in patch 1/2. This differs from v4 in that:
> >     * blockdev.c
> > 		- all failures in the group snapshot command should go to
> > 		  delete_and_fail
> > 
> > Jeff Cody (2):
> >   qapi: Introduce blockdev-group-snapshot-sync command
> >   QMP: Add qmp command for blockdev-group-snapshot-sync
> > 
> >  block.c          |   81 +++++++++++++++++++++++++++++++++
> >  block.h          |    1 +
> >  block_int.h      |    6 +++
> >  blockdev.c       |  131 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  qapi-schema.json |   38 ++++++++++++++++
> >  qmp-commands.hx  |   39 ++++++++++++++++
> >  6 files changed, 296 insertions(+), 0 deletions(-)
> > 
> 
> Thanks, applied to the block branch.

FWIW:

Acked-by: Luiz Capitulino <lcapitulino@redhat.com>