Group Live Snapshots

+/*
+ * Add new bs contents at the top of an image chain while the chain is live,
+ * while keeping required fields on the top layer.

+ * It is assumed that bs_new already points to an existing image,
+ * with the correct backing filename of top->backing_file

block.c +9 -0

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

+        /* 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);
+        states->is_open = true;

On Sun, Feb 26, 2012 at 7:31 PM, Jeff Cody <[email protected]> wrote:

Do you have automated tests for this feature?

Quoted 3 lines

+/*
+ * Add new bs contents at the top of an image chain while the chain is live,
+ * while keeping required fields on the top layer.

Please also document the swap behavior. It's pretty important for the caller to realize that once this function returns, their BlockDriverState arguments with have swapped.

Quoted 3 lines

+ * It is assumed that bs_new already points to an existing image,
+ * with the correct backing filename of top->backing_file

Not sure what this means. Isn't bs_new going to use bs_top as its backing file? Why "top->backing_file"?

block.c void bdrv_make_anon(BlockDriverState *bs) · line 898

Quoted 10 lines

block.c +9 -0

+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;
+    tmp.backing_hd = bs_new;

This is tricky, would be nice to comment that we will swap bs_new and bs_top later, therefore we need a pointer to bs_new here even though it doesn't make sense yet.

Quoted 15 lines

block.c +14 -0

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

block.c void bdrv_make_anon(BlockDriverState *bs) · line 917

Quoted 5 lines

block.c +5 -0

+    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;

Please add a comment into BlockDriverState to warn that changes to fields may require updates to this function too!

blockdev.c void qmp_blockdev_snapshot_sync(const char *device, const char *snapshot_file, · line 802

Quoted 6 lines

blockdev.c +5 -0

+        /* 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);
+        states->is_open = true;

What is the point of is_open? If we failed then new_bs is actually not open here.

I think you can remove this field and just do the following in close_and_fail:

if (states->new_bs) {
    bdrv_delete(states->new_bs);
}

(BTW I think close_and_fail is currently leaking new_bs because it only closes it and does not delete it!)

Quoted 4 lines · Stefan Hajnoczi

On Sun, Feb 26, 2012 at 7:31 PM, Jeff Cody <[email protected]> wrote:

Do you have automated tests for this feature?

No, not yet.  The testing has been manual.

Quoted 8 lines

Quoted 4 lines

+/*
+ * Add new bs contents at the top of an image chain while the chain is live,
+ * while keeping required fields on the top layer.

Please also document the swap behavior.  It's pretty important for the caller to realize that once this function returns, their BlockDriverState arguments with have swapped.

Good point.  How about this:

/*
 * 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.

Quoted 5 lines

Quoted 2 lines

+ * It is assumed that bs_new already points to an existing image,
+ * with the correct backing filename of top->backing_file

Not sure what this means.  Isn't bs_new going to use bs_top as its backing file?  Why "top->backing_file"?

Sorry, that should have been 'bs_top->backing_file'. The image file is not created by this function.  I added some more explanation, and corrected that typo, in the above comment block.  Let me know if you think it still needs more clarification.

Quoted 6 lines · Jeff Cody

Quoted 4 lines · Stefan Hajnoczi

On Sun, Feb 26, 2012 at 7:31 PM, Jeff Cody <[email protected]> wrote:

Do you have automated tests for this feature?

Quoted 1 line

No, not yet. The testing has been manual.

For image streaming I used the Python unittest framework along with QMP/qmp.py to create tests. I am going to submit it as a qemu-iotest.
We really need something along the lines of a harness with QMP
support so that these block layer features can be tested. I will CC you on the email.

Quoted 10 lines

Quoted 9 lines

Quoted 5 lines

+/*
+ * Add new bs contents at the top of an image chain while the chain is live,
+ * while keeping required fields on the top layer.

Please also document the swap behavior. It's pretty important for the caller to realize that once this function returns, their BlockDriverState arguments with have swapped.

Quoted 8 lines

Good point. How about this:

/*
* 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.

Looks good.

Quoted 10 lines

Quoted 5 lines

Quoted 2 lines

+ * It is assumed that bs_new already points to an existing image,
+ * with the correct backing filename of top->backing_file

Not sure what this means. Isn't bs_new going to use bs_top as its backing file? Why "top->backing_file"?

Sorry, that should have been 'bs_top->backing_file'. The image file is not created by this function. I added some more explanation, and corrected that typo, in the above comment block. Let me know if you think it still needs more clarification.

I still don't follow. Old bs_top's image file itself becomes the backing file, not bs_top->backing_file. Perhaps I'm misinterpreting because of how swap changes bs_top and bs_new, but I'm reading it from the perspective of the caller when they pass in bs_top.

The rest looks good.

Stefan

Quoted 14 lines · Stefan Hajnoczi

Quoted 5 lines · Jeff Cody

Quoted 4 lines · Stefan Hajnoczi

Quoted 2 lines · Jeff Cody

+ * It is assumed that bs_new already points to an existing image,
+ * with the correct backing filename of top->backing_file

Quoted 7 lines

Quoted 2 lines

Not sure what this means.  Isn't bs_new going to use bs_top as its backing file?  Why "top->backing_file"?

Sorry, that should have been 'bs_top->backing_file'. The image file is not created by this function.  I added some more explanation, and corrected that typo, in the above comment block.  Let me know if you think it still needs more clarification.

Quoted 5 lines

I still don't follow.  Old bs_top's image file itself becomes the backing file, not bs_top->backing_file.  Perhaps I'm misinterpreting because of how swap changes bs_top and bs_new, but I'm reading it from the perspective of the caller when they pass in bs_top.

Maybe it would be better to just replace that part of the comment with something that says "This function does not create the image file".

The function bdrv_append() will neither create the image file, or set (in the case of qcow2) the extended headers of the file to have the backing filename.  It is only concerned with the bs contents.

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

---
 block.c          |   47 ++++++++++++++++++++
 block.h          |    1 +
 blockdev.c       |  128 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 qapi-schema.json |   38 ++++++++++++++++
 4 files changed, 214 insertions(+), 0 deletions(-)

block.c +25 -0

index 3621d11..0045ab1 100644
@@ -880,6 +880,53 @@ 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.
+ *
+ * It is assumed that bs_new already points to an existing image,
+ * with the correct backing filename of top->backing_file
+ */
+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;
+    tmp.backing_hd = 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;

block.c +1 -0

+
 void bdrv_delete(BlockDriverState *bs)
 {
     assert(!bs->dev);

block.h +1 -0

index cae289b..190a780 100644
@@ -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);

blockdev.c +6 -0

index 05e7c5e..560f7e8 100644
@@ -714,6 +714,134 @@ 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;
+    bool is_open;

blockdev.c +66 -0

+{
+    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);
+
+    /* 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));
+        states->is_open = false;
+
+        states->old_bs = bdrv_find(dev_info->device);
+
+        if (!states->old_bs) {
+            error_set(errp, QERR_DEVICE_NOT_FOUND, dev_info->device);
+            goto fail;
+        }
+
+        if (bdrv_in_use(states->old_bs)) {
+            error_set(errp, QERR_DEVICE_IN_USE, dev_info->device);
+            goto 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 fail;
+        }
+
+        proto_drv = bdrv_find_protocol(snapshot_file);
+        if (!proto_drv) {
+            error_set(errp, QERR_INVALID_BLOCK_FORMAT, format);
+            goto 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 fail;
+        }
+
+        /* We will manually add the backing_hd field to the bs later */

blockdev.c +5 -0

+        /* This removes our old bs from the bdrv_states, and adds the new bs */
+        bdrv_append(states->new_bs, states->old_bs);
+    }
+
+    /* success */

blockdev.c void qmp_blockdev_snapshot_sync(const char *device, const char *snapshot_file, · line 815

Quoted 5 lines

blockdev.c +5 -0

+    /*
+     * Now we will drain, flush, & pivot everything - we are committed at this
+     * point.
+     */
+    bdrv_drain_all();

I would feel more comfortable if we could do the bdrv_drain_all() at the very beginning of the function. Not that I know of a specific scenario that would go wrong, but you have a nested main loop here that could do more or less anything.

Am 26.02.2012 20:31, schrieb Jeff Cody:

Quoted 68 lines

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

---
 block.c          |   47 ++++++++++++++++++++
 block.h          |    1 +
 blockdev.c       |  128 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 qapi-schema.json |   38 ++++++++++++++++
 4 files changed, 214 insertions(+), 0 deletions(-)

block.c +31 -0

index 3621d11..0045ab1 100644
@@ -880,6 +880,53 @@ 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.
+ *
+ * It is assumed that bs_new already points to an existing image,
+ * with the correct backing filename of top->backing_file
+ */
+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;
+    tmp.backing_hd = 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;

block.c void bdrv_make_anon(BlockDriverState *bs) · line 917

Quoted 4 lines

block.c +4 -0

+    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;

What about iostatus_enabled/iostatus? Possibly on_read/write_error and geometry, too. (Maybe the correct answer is that you're doing the right thing, but I wanted to bring it up)

block.c void bdrv_make_anon(BlockDriverState *bs) · line 928

Quoted 12 lines

block.c +11 -0

+
+    /* keep the same entry in bdrv_states */
+    pstrcpy(tmp.device_name, sizeof(tmp.device_name), bs_top->device_name);
+    tmp.list = bs_top->list;
+
+    /* swap contents of the fixed new bs and the current top */
+    *bs_new = *bs_top;
+    *bs_top = tmp;
+
+    bdrv_detach_dev(bs_new, bs_new->dev);
+}

The last line would actually deserve a comment /* clear the copied fields in the new backing file */, which makes clear that there are probably some more fields missing in this section.

Quoted 35 lines

block.c +1 -0

+
 void bdrv_delete(BlockDriverState *bs)
 {
     assert(!bs->dev);

block.h +1 -0

index cae289b..190a780 100644
@@ -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);

blockdev.c +10 -0

index 05e7c5e..560f7e8 100644
@@ -714,6 +714,134 @@ 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;
+    bool is_open;
+    QSIMPLEQ_ENTRY(BlkGroupSnapshotStates) entry;
+} BlkGroupSnapshotStates;
+
+/*

blockdev.c void qmp_blockdev_snapshot_sync(const char *device, const char *snapshot_file, · line 732

Quoted 6 lines

blockdev.c +6 -0

+ * '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)

Too much indentation?

Quoted 71 lines

blockdev.c +70 -0

+{
+    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);
+
+    /* 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));
+        states->is_open = false;
+
+        states->old_bs = bdrv_find(dev_info->device);
+
+        if (!states->old_bs) {
+            error_set(errp, QERR_DEVICE_NOT_FOUND, dev_info->device);
+            goto fail;
+        }
+
+        if (bdrv_in_use(states->old_bs)) {
+            error_set(errp, QERR_DEVICE_IN_USE, dev_info->device);
+            goto 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 fail;
+        }
+
+        proto_drv = bdrv_find_protocol(snapshot_file);
+        if (!proto_drv) {
+            error_set(errp, QERR_INVALID_BLOCK_FORMAT, format);
+            goto 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 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);
+        states->is_open = true;

blockdev.c void qmp_blockdev_snapshot_sync(const char *device, const char *snapshot_file, · line 808

Quoted 6 lines

blockdev.c +6 -0

+
+        if (ret != 0) {
+            error_set(errp, QERR_OPEN_FILE_FAILED, snapshot_file);
+            goto close_and_fail;
+        }
+        QSIMPLEQ_INSERT_TAIL(&snap_bdrv_states, states, entry);

Inserting it only here means that all the error cases above leak states.

blockdev.c void qmp_blockdev_snapshot_sync(const char *device, const char *snapshot_file, · line 815

Quoted 8 lines

blockdev.c +7 -0

+    }
+
+    /*
+     * Now we will drain, flush, & pivot everything - we are committed at this
+     * point.
+     */
+    bdrv_drain_all();

I would feel more comfortable if we could do the bdrv_drain_all() at the very beginning of the function. Not that I know of a specific scenario that would go wrong, but you have a nested main loop here that could do more or less anything.

blockdev.c void qmp_blockdev_snapshot_sync(const char *device, const char *snapshot_file, · line 817

Quoted 3 lines

blockdev.c +2 -0

+    QSIMPLEQ_FOREACH(states, &snap_bdrv_states, entry) {
+        bdrv_flush(states->old_bs);

This can return an error which must be checked. And of course, we must do it before committing to the snapshot (but after bdrv_drain_all).

Quoted 6 lines

blockdev.c +5 -0

+        /* This removes our old bs from the bdrv_states, and adds the new bs */
+        bdrv_append(states->new_bs, states->old_bs);
+    }
+
+    /* success */

blockdev.c void qmp_blockdev_snapshot_sync(const char *device, const char *snapshot_file, · line 833

Quoted 10 lines

blockdev.c +10 -0

+    goto exit;
+
+close_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->is_open) {
+             bdrv_close(states->new_bs);

bdrv_delete, as Stefan already mentioned.

blockdev.c void qmp_blockdev_snapshot_sync(const char *device, const char *snapshot_file, · line 842

Quoted 9 lines

blockdev.c +9 -0

+        }
+    }
+fail:
+exit:
+    QSIMPLEQ_FOREACH(states, &snap_bdrv_states, entry) {
+        g_free(states);
+    }
+    return;
+}

Kevin

Quoted 7 lines

Quoted 7 lines

block.c +7 -0

+
+    /* keep the same entry in bdrv_states */
+    pstrcpy(tmp.device_name, sizeof(tmp.device_name), bs_top->device_name);
+    tmp.list = bs_top->list;
+
+    /* swap contents of the fixed new bs and the current top */
+    *bs_new = *bs_top;

Quoted 8 lines

block.c void bdrv_make_anon(BlockDriverState *bs) · line 928

Quoted 4 lines

block.c +4 -0

+    *bs_top = tmp;
+
+    bdrv_detach_dev(bs_new, bs_new->dev);
+}

The last line would actually deserve a comment /* clear the copied fields in the new backing file */, which makes clear that there are probably some more fields missing in this section.

OK, added.

Quoted 8 lines

Quoted 7 lines

blockdev.c +7 -0

+    }
+
+    /*
+     * Now we will drain, flush, & pivot everything - we are committed at this
+     * point.
+     */
+    bdrv_drain_all();

Quoted 4 lines

I would feel more comfortable if we could do the bdrv_drain_all() at the very beginning of the function. Not that I know of a specific scenario that would go wrong, but you have a nested main loop here that could do more or less anything.

I can move it up to the beginning if desired... My thought was that it was best to drain closer to the point of commit.

Quoted 6 lines

blockdev.c void qmp_blockdev_snapshot_sync(const char *device, const char *snapshot_file, · line 817

Quoted 3 lines

blockdev.c +2 -0

+    QSIMPLEQ_FOREACH(states, &snap_bdrv_states, entry) {
+        bdrv_flush(states->old_bs);

This can return an error which must be checked. And of course, we must do it before committing to the snapshot (but after bdrv_drain_all).

Hmm. If the flush returns error, do we abandon at this point? Perhaps it would be best to loop through and flush each device first, and if no flushes fail, _then_ loop through and perform the bdrv_append(). Once we are calling bdrv_append(), we want no possible failure points.

Am 27.02.2012 18:02, schrieb Jeff Cody:

Quoted 16 lines

Quoted 15 lines

block.c void bdrv_make_anon(BlockDriverState *bs) · line 928

Quoted 11 lines

block.c +11 -0

+
+    /* keep the same entry in bdrv_states */
+    pstrcpy(tmp.device_name, sizeof(tmp.device_name), bs_top->device_name);
+    tmp.list = bs_top->list;
+
+    /* swap contents of the fixed new bs and the current top */
+    *bs_new = *bs_top;
+    *bs_top = tmp;
+
+    bdrv_detach_dev(bs_new, bs_new->dev);
+}

The last line would actually deserve a comment /* clear the copied fields in the new backing file */, which makes clear that there are probably some more fields missing in this section.

Quoted 1 line

OK, added.

Only the comment or also clearing other fields? For some of them it's very obvious that they need to be cleared (copy on read, I/O throttling).

Quoted 9 lines

Quoted 9 lines

Quoted 8 lines

blockdev.c +7 -0

+    }
+
+    /*
+     * Now we will drain, flush, & pivot everything - we are committed at this
+     * point.
+     */
+    bdrv_drain_all();

Quoted 7 lines

Quoted 4 lines

I would feel more comfortable if we could do the bdrv_drain_all() at the very beginning of the function. Not that I know of a specific scenario that would go wrong, but you have a nested main loop here that could do more or less anything.

I can move it up to the beginning if desired... My thought was that it was best to drain closer to the point of commit.

As long as we don't create new AIO requests here, drained is drained.

But anyway, I'm not requesting a change here, it was just a feeling.

Quoted 11 lines

Quoted 6 lines

blockdev.c void qmp_blockdev_snapshot_sync(const char *device, const char *snapshot_file, · line 817

Quoted 3 lines

blockdev.c +2 -0

+    QSIMPLEQ_FOREACH(states, &snap_bdrv_states, entry) {
+        bdrv_flush(states->old_bs);

This can return an error which must be checked. And of course, we must do it before committing to the snapshot (but after bdrv_drain_all).

Hmm. If the flush returns error, do we abandon at this point? Perhaps it would be best to loop through and flush each device first, and if no flushes fail, _then_ loop through and perform the bdrv_append(). Once we are calling bdrv_append(), we want no possible failure points.

Yes, this is what I meant. Sorry for the somewhat vague wording.

Kevin