[Qemu-devel] [PATCH 0/3] block: make qmp_transaction extendable

[Qemu-devel] [PATCH 0/3] block: make qmp_transaction extendable

[Wenchao Xia]

  This serial will package backing chain snapshot code as one case to make it
possible adding more operations later. It is splitted out from previous snapshot
patches since it have slightly different purpose, adjusting qmp_transaction.
Previous patch and discuss could be found at:
http://lists.nongnu.org/archive/html/qemu-devel/2013-01/msg00645.html

  I can't find a way to split patch 2/3 to smaller ones unless breaking the build,
so kepted it as a relative larger patch.

Wenchao Xia (3):
  1 block: add function deappend()
  2 block: adjust qmp_transaction to be extendable
  3 block: change rollback sequence in qmp_transaction

 block.c               |   66 +++++++++++++
 blockdev.c            |  252 ++++++++++++++++++++++++++++++-------------------
 include/block/block.h |    1 +
 3 files changed, 221 insertions(+), 98 deletions(-)

[Qemu-devel] [PATCH 1/3] block: add function deappend()

[Wenchao Xia]

  This function should be used to remove contents of active *bs on the
top of backing chain, when top *bs was committed to bs->backing_hd or
*bs was just appended.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 block.c               |   66 +++++++++++++++++++++++++++++++++++++++++++++++++
 include/block/block.h |    1 +
 2 files changed, 67 insertions(+), 0 deletions(-)

diff --git a/block.c b/block.c
index 0ae2e93..0cacc2f 100644
--- a/block.c
+++ b/block.c
@@ -1536,6 +1536,72 @@ void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top)
             bs_new->drv ? bs_new->drv->format_name : "");
 }
 
+/*
+ * Remove top bs contents in the image backing chain while the chain is live.
+ * Required fields on the top layer will be kept to let qemu continue use
+ * @bs_top.
+ *
+ * This will modify the BlockDriverState fields, and swap contents between
+ * bs_top and bs_top->backing_hd. Both bs_top and bs_top->backing_hd are
+ * modified, and bs_top->backing_hd will be deleted.
+ *
+ * This function does not delete any image files. If @bs_top have no backing
+ * image, the function will do nothing.
+ *
+ * Warning: Caller must guarantee @bs_top have same contents with its backing
+ * image, that is, when @bs_top was just appended or committed to
+ * bs_top->backing_hd.
+ */
+void bdrv_deappend(BlockDriverState *bs_top)
+{
+    BlockDriverState tmp;
+    BlockDriverState *bs_backing = bs_top->backing_hd;
+
+    if (!bs_backing) {
+        return;
+    }
+
+    BlockDriverState *bs_bottom = bs_backing->backing_hd;
+    /* Now only support deappend when bs_top is in following conditions. */
+    g_assert(bs_top->device_name[0] != '\0');
+    g_assert(bs_top->dev != NULL);
+
+    g_assert(bs_top->dirty_bitmap == NULL);
+    g_assert(bs_top->job == NULL);
+    g_assert(bs_top->in_use == 0);
+    g_assert(bs_top->io_limits_enabled == false);
+    g_assert(bs_top->block_timer == NULL);
+
+
+    tmp = *bs_top;
+    *bs_top = *bs_backing;
+    *bs_backing = tmp;
+
+    /* there are some fields which should not be swapped, move them back */
+    bdrv_move_feature_fields(&tmp, bs_top);
+    bdrv_move_feature_fields(bs_top, bs_backing);
+    bdrv_move_feature_fields(bs_backing, &tmp);
+
+    /* bs_top should keep device attached  */
+    g_assert(bs_top->device_name[0] != '\0');
+    g_assert(bs_top->dev != NULL);
+
+    /* check a few fields that should remain */
+    g_assert(bs_top->job == NULL);
+    g_assert(bs_top->in_use == 0);
+    g_assert(bs_top->io_limits_enabled == false);
+    g_assert(bs_top->block_timer == NULL);
+    g_assert(bs_top->backing_hd == bs_bottom);
+
+    /* delete bs_backing */
+    bs_backing->backing_hd = NULL;
+    bs_backing->backing_file[0] = 0;
+    bs_backing->backing_format[0] = 0;
+    bdrv_delete(bs_backing);
+
+    bdrv_rebind(bs_top);
+}
+
 void bdrv_delete(BlockDriverState *bs)
 {
     assert(!bs->dev);
diff --git a/include/block/block.h b/include/block/block.h
index 9dc6aad..4450121 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -132,6 +132,7 @@ BlockDriverState *bdrv_new(const char *device_name);
 void bdrv_make_anon(BlockDriverState *bs);
 void bdrv_swap(BlockDriverState *bs_new, BlockDriverState *bs_old);
 void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top);
+void bdrv_deappend(BlockDriverState *bs_top);
 void bdrv_delete(BlockDriverState *bs);
 int bdrv_parse_cache_flags(const char *mode, int *flags);
 int bdrv_parse_discard_flags(const char *mode, int *flags);
-- 
1.7.1

[Qemu-devel] [PATCH 2/3] block: adjust qmp_transaction to be extendable

[Wenchao Xia]

  Now code for external snapshot are packaged as one case
in qmp_transaction, so later other operation could be added.
  The logic in qmp_transaction is changed a bit: Original code
tries to create all images first and then update all *bdrv
once together, new code create and update one *bdrv one time,
and use bdrv_deappend() to rollback on fail. This allows mixing
different kind of requests in qmp_transaction() later.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 blockdev.c |  250 +++++++++++++++++++++++++++++++++++++-----------------------
 1 files changed, 153 insertions(+), 97 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 8cdc9ce..75416fb 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -779,9 +779,155 @@ void qmp_blockdev_snapshot_sync(const char *device, const char *snapshot_file,
 
 
 /* New and old BlockDriverState structs for group snapshots */
-typedef struct BlkTransactionStates {
+typedef struct BdrvActionOps {
+    int (*commit)(BlockdevAction *action, void **p_opaque, Error **errp);
+    void (*rollback)(BlockdevAction *action, void *opaque);
+    void (*clean)(BlockdevAction *action, void *opaque);
+} BdrvActionOps;
+
+typedef struct ExternalSnapshotState {
     BlockDriverState *old_bs;
     BlockDriverState *new_bs;
+} ExternalSnapshotState;
+
+static int external_snapshot_commit(BlockdevAction *action,
+                                    void **p_opaque,
+                                    Error **errp)
+{
+    const char *device;
+    const char *new_image_file;
+    const char *format = "qcow2";
+    enum NewImageMode mode = NEW_IMAGE_MODE_ABSOLUTE_PATHS;
+    BlockDriver *drv, *proto_drv;
+    ExternalSnapshotState *states = NULL;
+    int flags, ret;
+    Error *local_err = NULL;
+
+    g_assert(action->kind == BLOCKDEV_ACTION_KIND_BLOCKDEV_SNAPSHOT_SYNC);
+
+    /* get parameters */
+    device = action->blockdev_snapshot_sync->device;
+    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;
+    }
+
+    /* start processing */
+    states = g_malloc0(sizeof(*states));
+    drv = bdrv_find_format(format);
+    if (!drv) {
+        error_set(errp, QERR_INVALID_BLOCK_FORMAT, format);
+        goto fail;
+    }
+
+    states->old_bs = bdrv_find(device);
+    if (!states->old_bs) {
+        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
+        goto fail;
+    }
+
+    if (!bdrv_is_inserted(states->old_bs)) {
+        error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, device);
+        goto fail;
+    }
+
+    if (bdrv_in_use(states->old_bs)) {
+        error_set(errp, QERR_DEVICE_IN_USE, device);
+        goto fail;
+    }
+
+    if (!bdrv_is_read_only(states->old_bs)) {
+        if (bdrv_flush(states->old_bs)) {
+            error_set(errp, QERR_IO_ERROR);
+            goto fail;
+        }
+    }
+
+    proto_drv = bdrv_find_protocol(new_image_file);
+    if (!proto_drv) {
+        error_set(errp, QERR_INVALID_BLOCK_FORMAT, format);
+        goto fail;
+    }
+
+    flags = states->old_bs->open_flags;
+
+    /* create new image w/backing file */
+    if (mode != NEW_IMAGE_MODE_EXISTING) {
+        bdrv_img_create(new_image_file, format,
+                        states->old_bs->filename,
+                        states->old_bs->drv->format_name,
+                        NULL, -1, flags, &local_err, false);
+        if (error_is_set(&local_err)) {
+            error_propagate(errp, local_err);
+            goto fail;
+        }
+    }
+
+    states->new_bs = bdrv_new("");
+    /* TODO Inherit bs->options or only take explicit options with an
+     * extended QMP command? */
+    ret = bdrv_open(states->new_bs, new_image_file, NULL,
+                    flags | BDRV_O_NO_BACKING, drv);
+    if (ret != 0) {
+        error_set(errp, QERR_OPEN_FILE_FAILED, new_image_file);
+        goto fail;
+    }
+
+    /* This removes our old bs from the bdrv_states, and adds the new bs */
+    bdrv_append(states->new_bs, states->old_bs);
+    /* We don't need (or want) to use the transactional
+     * bdrv_reopen_multiple() across all the entries at once, because we
+     * don't want to abort all of them if one of them fails the reopen */
+    bdrv_reopen(states->new_bs, states->new_bs->open_flags & ~BDRV_O_RDWR,
+                    NULL);
+
+    *p_opaque = states;
+    return 0;
+
+fail:
+    if (states) {
+        if (states->new_bs) {
+            bdrv_delete(states->new_bs);
+        }
+        g_free(states);
+    }
+    return -1;
+}
+
+static void external_snapshot_rollback(BlockdevAction *action,
+                                       void *opaque)
+{
+    ExternalSnapshotState *states = opaque;
+    int open_flags;
+
+    if (states) {
+        open_flags = states->old_bs->open_flags;
+        bdrv_deappend(states->old_bs);
+        bdrv_reopen(states->old_bs, open_flags, NULL);
+    }
+}
+
+static void external_snapshot_clean(BlockdevAction *action,
+                                    void *opaque)
+{
+    ExternalSnapshotState *states = opaque;
+
+    g_free(states);
+}
+
+const BdrvActionOps external_snapshot_ops = {
+    .commit = external_snapshot_commit,
+    .rollback = external_snapshot_rollback,
+    .clean = external_snapshot_clean,
+};
+
+typedef struct BlkTransactionStates {
+    BlockdevAction *action;
+    void *opaque;
+    const BdrvActionOps *ops;
     QSIMPLEQ_ENTRY(BlkTransactionStates) entry;
 } BlkTransactionStates;
 
@@ -792,10 +938,8 @@ typedef struct BlkTransactionStates {
  */
 void qmp_transaction(BlockdevActionList *dev_list, Error **errp)
 {
-    int ret = 0;
     BlockdevActionList *dev_entry = dev_list;
     BlkTransactionStates *states, *next;
-    Error *local_err = NULL;
 
     QSIMPLEQ_HEAD(snap_bdrv_states, BlkTransactionStates) snap_bdrv_states;
     QSIMPLEQ_INIT(&snap_bdrv_states);
@@ -806,125 +950,37 @@ void qmp_transaction(BlockdevActionList *dev_list, Error **errp)
     /* We don't do anything in this loop that commits us to the snapshot */
     while (NULL != dev_entry) {
         BlockdevAction *dev_info = NULL;
-        BlockDriver *proto_drv;
-        BlockDriver *drv;
-        int flags;
-        enum NewImageMode mode;
-        const char *new_image_file;
-        const char *device;
-        const char *format = "qcow2";
-
         dev_info = dev_entry->value;
         dev_entry = dev_entry->next;
 
         states = g_malloc0(sizeof(BlkTransactionStates));
         QSIMPLEQ_INSERT_TAIL(&snap_bdrv_states, states, entry);
 
+        states->action = dev_info;
         switch (dev_info->kind) {
         case BLOCKDEV_ACTION_KIND_BLOCKDEV_SNAPSHOT_SYNC:
-            device = dev_info->blockdev_snapshot_sync->device;
-            if (!dev_info->blockdev_snapshot_sync->has_mode) {
-                dev_info->blockdev_snapshot_sync->mode = NEW_IMAGE_MODE_ABSOLUTE_PATHS;
-            }
-            new_image_file = dev_info->blockdev_snapshot_sync->snapshot_file;
-            if (dev_info->blockdev_snapshot_sync->has_format) {
-                format = dev_info->blockdev_snapshot_sync->format;
-            }
-            mode = dev_info->blockdev_snapshot_sync->mode;
+            states->ops = &external_snapshot_ops;
             break;
         default:
             abort();
         }
 
-        drv = bdrv_find_format(format);
-        if (!drv) {
-            error_set(errp, QERR_INVALID_BLOCK_FORMAT, format);
-            goto delete_and_fail;
-        }
-
-        states->old_bs = bdrv_find(device);
-        if (!states->old_bs) {
-            error_set(errp, QERR_DEVICE_NOT_FOUND, device);
-            goto delete_and_fail;
-        }
-
-        if (!bdrv_is_inserted(states->old_bs)) {
-            error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, device);
-            goto delete_and_fail;
-        }
-
-        if (bdrv_in_use(states->old_bs)) {
-            error_set(errp, QERR_DEVICE_IN_USE, device);
-            goto delete_and_fail;
-        }
-
-        if (!bdrv_is_read_only(states->old_bs)) {
-            if (bdrv_flush(states->old_bs)) {
-                error_set(errp, QERR_IO_ERROR);
-                goto delete_and_fail;
-            }
-        }
-
-        flags = states->old_bs->open_flags;
-
-        proto_drv = bdrv_find_protocol(new_image_file);
-        if (!proto_drv) {
-            error_set(errp, QERR_INVALID_BLOCK_FORMAT, format);
-            goto delete_and_fail;
-        }
-
-        /* create new image w/backing file */
-        if (mode != NEW_IMAGE_MODE_EXISTING) {
-            bdrv_img_create(new_image_file, format,
-                            states->old_bs->filename,
-                            states->old_bs->drv->format_name,
-                            NULL, -1, flags, &local_err, false);
-            if (error_is_set(&local_err)) {
-                error_propagate(errp, local_err);
-                goto delete_and_fail;
-            }
-        }
-
-        /* We will manually add the backing_hd field to the bs later */
-        states->new_bs = bdrv_new("");
-        /* TODO Inherit bs->options or only take explicit options with an
-         * extended QMP command? */
-        ret = bdrv_open(states->new_bs, new_image_file, NULL,
-                        flags | BDRV_O_NO_BACKING, drv);
-        if (ret != 0) {
-            error_set(errp, QERR_OPEN_FILE_FAILED, new_image_file);
+        if (states->ops->commit(states->action, &states->opaque, errp)) {
             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);
-        /* We don't need (or want) to use the transactional
-         * bdrv_reopen_multiple() across all the entries at once, because we
-         * don't want to abort all of them if one of them fails the reopen */
-        bdrv_reopen(states->new_bs, states->new_bs->open_flags & ~BDRV_O_RDWR,
-                    NULL);
-    }
-
     /* 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);
-        }
+        states->ops->rollback(states->action, states->opaque);
     }
+
 exit:
     QSIMPLEQ_FOREACH_SAFE(states, &snap_bdrv_states, entry, next) {
+        states->ops->clean(states->action, states->opaque);
         g_free(states);
     }
 }
-- 
1.7.1

Re: [Qemu-devel] [PATCH 2/3] block: adjust qmp_transaction to be extendable

[Kevin Wolf]

Am 01.04.2013 um 12:01 hat Wenchao Xia geschrieben:
>   Now code for external snapshot are packaged as one case
> in qmp_transaction, so later other operation could be added.
>   The logic in qmp_transaction is changed a bit: Original code
> tries to create all images first and then update all *bdrv
> once together, new code create and update one *bdrv one time,
> and use bdrv_deappend() to rollback on fail. This allows mixing
> different kind of requests in qmp_transaction() later.
> 
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
>  blockdev.c |  250 +++++++++++++++++++++++++++++++++++++-----------------------
>  1 files changed, 153 insertions(+), 97 deletions(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index 8cdc9ce..75416fb 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -779,9 +779,155 @@ void qmp_blockdev_snapshot_sync(const char *device, const char *snapshot_file,
>  
>  
>  /* New and old BlockDriverState structs for group snapshots */
> -typedef struct BlkTransactionStates {
> +typedef struct BdrvActionOps {
> +    int (*commit)(BlockdevAction *action, void **p_opaque, Error **errp);
> +    void (*rollback)(BlockdevAction *action, void *opaque);
> +    void (*clean)(BlockdevAction *action, void *opaque);
> +} BdrvActionOps;

You don't really implement the transactional pattern that was used by
the original code (and is used elsewhere). It should work like this:

1. Prepare: This stage performs all operations that can fail. They are
   not made active yet. For example with snapshotting, open a new
   BlockDriver state, but don't change the backing file chain yet.

2. Commit: Activate the change. This operation can never fail. For this
   reason, you never have to undo anything done here.

3. Rollback: Basically just free everything that prepare did up to the
   error.

If you do it your way, you get into serious trouble if rollback involves
operations that can fail.

In the original code, here start the prepare:

> @@ -806,125 +950,37 @@ void qmp_transaction(BlockdevActionList *dev_list, Error **errp)
>      /* We don't do anything in this loop that commits us to the snapshot */
>      while (NULL != dev_entry) {
>          BlockdevAction *dev_info = NULL;
> -        BlockDriver *proto_drv;
> -        BlockDriver *drv;
> -        int flags;
> -        enum NewImageMode mode;
> -        const char *new_image_file;
> -        const char *device;
> -        const char *format = "qcow2";
> -
>          dev_info = dev_entry->value;
>          dev_entry = dev_entry->next;
>  
>          states = g_malloc0(sizeof(BlkTransactionStates));
>          QSIMPLEQ_INSERT_TAIL(&snap_bdrv_states, states, entry);
>  
> +        states->action = dev_info;
>          switch (dev_info->kind) {
>          case BLOCKDEV_ACTION_KIND_BLOCKDEV_SNAPSHOT_SYNC:
> -            device = dev_info->blockdev_snapshot_sync->device;
> -            if (!dev_info->blockdev_snapshot_sync->has_mode) {
> -                dev_info->blockdev_snapshot_sync->mode = NEW_IMAGE_MODE_ABSOLUTE_PATHS;
> -            }
> -            new_image_file = dev_info->blockdev_snapshot_sync->snapshot_file;
> -            if (dev_info->blockdev_snapshot_sync->has_format) {
> -                format = dev_info->blockdev_snapshot_sync->format;
> -            }
> -            mode = dev_info->blockdev_snapshot_sync->mode;
> +            states->ops = &external_snapshot_ops;
>              break;
>          default:
>              abort();
>          }
>  
> -        drv = bdrv_find_format(format);
> -        if (!drv) {
> -            error_set(errp, QERR_INVALID_BLOCK_FORMAT, format);
> -            goto delete_and_fail;
> -        }
> -
> -        states->old_bs = bdrv_find(device);
> -        if (!states->old_bs) {
> -            error_set(errp, QERR_DEVICE_NOT_FOUND, device);
> -            goto delete_and_fail;
> -        }
> -
> -        if (!bdrv_is_inserted(states->old_bs)) {
> -            error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, device);
> -            goto delete_and_fail;
> -        }
> -
> -        if (bdrv_in_use(states->old_bs)) {
> -            error_set(errp, QERR_DEVICE_IN_USE, device);
> -            goto delete_and_fail;
> -        }
> -
> -        if (!bdrv_is_read_only(states->old_bs)) {
> -            if (bdrv_flush(states->old_bs)) {
> -                error_set(errp, QERR_IO_ERROR);
> -                goto delete_and_fail;
> -            }
> -        }
> -
> -        flags = states->old_bs->open_flags;
> -
> -        proto_drv = bdrv_find_protocol(new_image_file);
> -        if (!proto_drv) {
> -            error_set(errp, QERR_INVALID_BLOCK_FORMAT, format);
> -            goto delete_and_fail;
> -        }
> -
> -        /* create new image w/backing file */
> -        if (mode != NEW_IMAGE_MODE_EXISTING) {
> -            bdrv_img_create(new_image_file, format,
> -                            states->old_bs->filename,
> -                            states->old_bs->drv->format_name,
> -                            NULL, -1, flags, &local_err, false);
> -            if (error_is_set(&local_err)) {
> -                error_propagate(errp, local_err);
> -                goto delete_and_fail;
> -            }
> -        }
> -
> -        /* We will manually add the backing_hd field to the bs later */
> -        states->new_bs = bdrv_new("");
> -        /* TODO Inherit bs->options or only take explicit options with an
> -         * extended QMP command? */
> -        ret = bdrv_open(states->new_bs, new_image_file, NULL,
> -                        flags | BDRV_O_NO_BACKING, drv);
> -        if (ret != 0) {
> -            error_set(errp, QERR_OPEN_FILE_FAILED, new_image_file);
> +        if (states->ops->commit(states->action, &states->opaque, errp)) {
>              goto delete_and_fail;
>          }
>      }

The following block is the commit:

> -
> -    /* 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);
> -        /* We don't need (or want) to use the transactional
> -         * bdrv_reopen_multiple() across all the entries at once, because we
> -         * don't want to abort all of them if one of them fails the reopen */
> -        bdrv_reopen(states->new_bs, states->new_bs->open_flags & ~BDRV_O_RDWR,
> -                    NULL);
> -    }
> -
>      /* success */
>      goto exit;

And this is rollback:

>  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);
> -        }
> +        states->ops->rollback(states->action, states->opaque);
>      }

Kevin

Re: [Qemu-devel] [PATCH 2/3] block: adjust qmp_transaction to be extendable

[Wenchao Xia]

于 2013-4-2 21:55, Kevin Wolf 写道:
> Am 01.04.2013 um 12:01 hat Wenchao Xia geschrieben:
>>    Now code for external snapshot are packaged as one case
>> in qmp_transaction, so later other operation could be added.
>>    The logic in qmp_transaction is changed a bit: Original code
>> tries to create all images first and then update all *bdrv
>> once together, new code create and update one *bdrv one time,
>> and use bdrv_deappend() to rollback on fail. This allows mixing
>> different kind of requests in qmp_transaction() later.
>>
>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>> ---
>>   blockdev.c |  250 +++++++++++++++++++++++++++++++++++++-----------------------
>>   1 files changed, 153 insertions(+), 97 deletions(-)
>>
>> diff --git a/blockdev.c b/blockdev.c
>> index 8cdc9ce..75416fb 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -779,9 +779,155 @@ void qmp_blockdev_snapshot_sync(const char *device, const char *snapshot_file,
>>
>>
>>   /* New and old BlockDriverState structs for group snapshots */
>> -typedef struct BlkTransactionStates {
>> +typedef struct BdrvActionOps {
>> +    int (*commit)(BlockdevAction *action, void **p_opaque, Error **errp);
>> +    void (*rollback)(BlockdevAction *action, void *opaque);
>> +    void (*clean)(BlockdevAction *action, void *opaque);
>> +} BdrvActionOps;
>
> You don't really implement the transactional pattern that was used by
> the original code (and is used elsewhere). It should work like this:
>
> 1. Prepare: This stage performs all operations that can fail. They are
>     not made active yet. For example with snapshotting, open a new
>     BlockDriver state, but don't change the backing file chain yet.
>
> 2. Commit: Activate the change. This operation can never fail. For this
>     reason, you never have to undo anything done here.
>
> 3. Rollback: Basically just free everything that prepare did up to the
>     error.
>
> If you do it your way, you get into serious trouble if rollback involves
> operations that can fail.
>
> In the original code, here start the prepare:
>
   That is a clear comment, thanks. I changed the model since expecting
commit operation may need rollback, if not I am confident to keep
original model.
   Since bdrv_snapshot_delete() can fail, I guess assertion of its
success should be used in the rollback later.

>> @@ -806,125 +950,37 @@ void qmp_transaction(BlockdevActionList *dev_list, Error **errp)
>>       /* We don't do anything in this loop that commits us to the snapshot */
>>       while (NULL != dev_entry) {
>>           BlockdevAction *dev_info = NULL;
>> -        BlockDriver *proto_drv;
>> -        BlockDriver *drv;
>> -        int flags;
>> -        enum NewImageMode mode;
>> -        const char *new_image_file;
>> -        const char *device;
>> -        const char *format = "qcow2";
>> -
>>           dev_info = dev_entry->value;
>>           dev_entry = dev_entry->next;
>>
>>           states = g_malloc0(sizeof(BlkTransactionStates));
>>           QSIMPLEQ_INSERT_TAIL(&snap_bdrv_states, states, entry);
>>
>> +        states->action = dev_info;
>>           switch (dev_info->kind) {
>>           case BLOCKDEV_ACTION_KIND_BLOCKDEV_SNAPSHOT_SYNC:
>> -            device = dev_info->blockdev_snapshot_sync->device;
>> -            if (!dev_info->blockdev_snapshot_sync->has_mode) {
>> -                dev_info->blockdev_snapshot_sync->mode = NEW_IMAGE_MODE_ABSOLUTE_PATHS;
>> -            }
>> -            new_image_file = dev_info->blockdev_snapshot_sync->snapshot_file;
>> -            if (dev_info->blockdev_snapshot_sync->has_format) {
>> -                format = dev_info->blockdev_snapshot_sync->format;
>> -            }
>> -            mode = dev_info->blockdev_snapshot_sync->mode;
>> +            states->ops = &external_snapshot_ops;
>>               break;
>>           default:
>>               abort();
>>           }
>>
>> -        drv = bdrv_find_format(format);
>> -        if (!drv) {
>> -            error_set(errp, QERR_INVALID_BLOCK_FORMAT, format);
>> -            goto delete_and_fail;
>> -        }
>> -
>> -        states->old_bs = bdrv_find(device);
>> -        if (!states->old_bs) {
>> -            error_set(errp, QERR_DEVICE_NOT_FOUND, device);
>> -            goto delete_and_fail;
>> -        }
>> -
>> -        if (!bdrv_is_inserted(states->old_bs)) {
>> -            error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, device);
>> -            goto delete_and_fail;
>> -        }
>> -
>> -        if (bdrv_in_use(states->old_bs)) {
>> -            error_set(errp, QERR_DEVICE_IN_USE, device);
>> -            goto delete_and_fail;
>> -        }
>> -
>> -        if (!bdrv_is_read_only(states->old_bs)) {
>> -            if (bdrv_flush(states->old_bs)) {
>> -                error_set(errp, QERR_IO_ERROR);
>> -                goto delete_and_fail;
>> -            }
>> -        }
>> -
>> -        flags = states->old_bs->open_flags;
>> -
>> -        proto_drv = bdrv_find_protocol(new_image_file);
>> -        if (!proto_drv) {
>> -            error_set(errp, QERR_INVALID_BLOCK_FORMAT, format);
>> -            goto delete_and_fail;
>> -        }
>> -
>> -        /* create new image w/backing file */
>> -        if (mode != NEW_IMAGE_MODE_EXISTING) {
>> -            bdrv_img_create(new_image_file, format,
>> -                            states->old_bs->filename,
>> -                            states->old_bs->drv->format_name,
>> -                            NULL, -1, flags, &local_err, false);
>> -            if (error_is_set(&local_err)) {
>> -                error_propagate(errp, local_err);
>> -                goto delete_and_fail;
>> -            }
>> -        }
>> -
>> -        /* We will manually add the backing_hd field to the bs later */
>> -        states->new_bs = bdrv_new("");
>> -        /* TODO Inherit bs->options or only take explicit options with an
>> -         * extended QMP command? */
>> -        ret = bdrv_open(states->new_bs, new_image_file, NULL,
>> -                        flags | BDRV_O_NO_BACKING, drv);
>> -        if (ret != 0) {
>> -            error_set(errp, QERR_OPEN_FILE_FAILED, new_image_file);
>> +        if (states->ops->commit(states->action, &states->opaque, errp)) {
>>               goto delete_and_fail;
>>           }
>>       }
>
> The following block is the commit:
>
>> -
>> -    /* 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);
>> -        /* We don't need (or want) to use the transactional
>> -         * bdrv_reopen_multiple() across all the entries at once, because we
>> -         * don't want to abort all of them if one of them fails the reopen */
>> -        bdrv_reopen(states->new_bs, states->new_bs->open_flags & ~BDRV_O_RDWR,
>> -                    NULL);
>> -    }
>> -
>>       /* success */
>>       goto exit;
>
> And this is rollback:
>
>>   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);
>> -        }
>> +        states->ops->rollback(states->action, states->opaque);
>>       }
>
> Kevin
>


-- 
Best Regards

Wenchao Xia

Re: [Qemu-devel] [PATCH 2/3] block: adjust qmp_transaction to be extendable

[Paolo Bonzini]

----- Messaggio originale -----
> Da: "Wenchao Xia" <xiawenc@linux.vnet.ibm.com>
> A: "Kevin Wolf" <kwolf@redhat.com>
> Cc: pbonzini@redhat.com, qemu-devel@nongnu.org, dietmar@proxmox.com, stefanha@gmail.com
> Inviato: Mercoledì, 3 aprile 2013 7:51:43
> Oggetto: Re: [Qemu-devel] [PATCH 2/3] block: adjust qmp_transaction to be extendable
> 
> 于 2013-4-2 21:55, Kevin Wolf 写道:
> > Am 01.04.2013 um 12:01 hat Wenchao Xia geschrieben:
> >>    Now code for external snapshot are packaged as one case
> >> in qmp_transaction, so later other operation could be added.
> >>    The logic in qmp_transaction is changed a bit: Original code
> >> tries to create all images first and then update all *bdrv
> >> once together, new code create and update one *bdrv one time,
> >> and use bdrv_deappend() to rollback on fail. This allows mixing
> >> different kind of requests in qmp_transaction() later.
> >>
> >> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> >> ---
> >>   blockdev.c |  250
> >>   +++++++++++++++++++++++++++++++++++++-----------------------
> >>   1 files changed, 153 insertions(+), 97 deletions(-)
> >>
> >> diff --git a/blockdev.c b/blockdev.c
> >> index 8cdc9ce..75416fb 100644
> >> --- a/blockdev.c
> >> +++ b/blockdev.c
> >> @@ -779,9 +779,155 @@ void qmp_blockdev_snapshot_sync(const char *device,
> >> const char *snapshot_file,
> >>
> >>
> >>   /* New and old BlockDriverState structs for group snapshots */
> >> -typedef struct BlkTransactionStates {
> >> +typedef struct BdrvActionOps {
> >> +    int (*commit)(BlockdevAction *action, void **p_opaque, Error **errp);
> >> +    void (*rollback)(BlockdevAction *action, void *opaque);
> >> +    void (*clean)(BlockdevAction *action, void *opaque);
> >> +} BdrvActionOps;
> >
> > You don't really implement the transactional pattern that was used by
> > the original code (and is used elsewhere). It should work like this:
> >
> > 1. Prepare: This stage performs all operations that can fail. They are
> >     not made active yet. For example with snapshotting, open a new
> >     BlockDriver state, but don't change the backing file chain yet.
> >
> > 2. Commit: Activate the change. This operation can never fail. For this
> >     reason, you never have to undo anything done here.
> >
> > 3. Rollback: Basically just free everything that prepare did up to the
> >     error.
> >
> > If you do it your way, you get into serious trouble if rollback involves
> > operations that can fail.
> >
> > In the original code, here start the prepare:
>
>    That is a clear comment, thanks. I changed the model since expecting
> commit operation may need rollback, if not I am confident to keep
> original model.
>    Since bdrv_snapshot_delete() can fail, I guess assertion of its
> success should be used in the rollback later.

No, if bdrv_snapshot_delete() can fail, you need to split it in two
parts: one that can fail, and one that cannot.  If you cannot, then
there are two possibilities:

- if the failures are minor and could be repaired with "qemu-img check -r"
(e.g. lost clusters), then this is not optimal but can still be done;

- otherwise, the operation simply cannot be made transactionable.

In the case of qcow2_snapshot_delete, everything except

    ret = bdrv_pwrite_sync(bs->file, offsetof(QCowHeader, nb_snapshots),
                           &header_data, sizeof(header_data));
    if (ret < 0) {
        goto fail;
    }

must be in the prepare phase.  Everything after "fail" (which right now
is nothing, but it should at least undo the qcow2_alloc_clusters operation)
must be in the rollback phase.  Everything in the middle is the commit
phase.

Paolo
 
> 
> >> @@ -806,125 +950,37 @@ void qmp_transaction(BlockdevActionList *dev_list,
> >> Error **errp)
> >>       /* We don't do anything in this loop that commits us to the snapshot
> >>       */
> >>       while (NULL != dev_entry) {
> >>           BlockdevAction *dev_info = NULL;
> >> -        BlockDriver *proto_drv;
> >> -        BlockDriver *drv;
> >> -        int flags;
> >> -        enum NewImageMode mode;
> >> -        const char *new_image_file;
> >> -        const char *device;
> >> -        const char *format = "qcow2";
> >> -
> >>           dev_info = dev_entry->value;
> >>           dev_entry = dev_entry->next;
> >>
> >>           states = g_malloc0(sizeof(BlkTransactionStates));
> >>           QSIMPLEQ_INSERT_TAIL(&snap_bdrv_states, states, entry);
> >>
> >> +        states->action = dev_info;
> >>           switch (dev_info->kind) {
> >>           case BLOCKDEV_ACTION_KIND_BLOCKDEV_SNAPSHOT_SYNC:
> >> -            device = dev_info->blockdev_snapshot_sync->device;
> >> -            if (!dev_info->blockdev_snapshot_sync->has_mode) {
> >> -                dev_info->blockdev_snapshot_sync->mode =
> >> NEW_IMAGE_MODE_ABSOLUTE_PATHS;
> >> -            }
> >> -            new_image_file =
> >> dev_info->blockdev_snapshot_sync->snapshot_file;
> >> -            if (dev_info->blockdev_snapshot_sync->has_format) {
> >> -                format = dev_info->blockdev_snapshot_sync->format;
> >> -            }
> >> -            mode = dev_info->blockdev_snapshot_sync->mode;
> >> +            states->ops = &external_snapshot_ops;
> >>               break;
> >>           default:
> >>               abort();
> >>           }
> >>
> >> -        drv = bdrv_find_format(format);
> >> -        if (!drv) {
> >> -            error_set(errp, QERR_INVALID_BLOCK_FORMAT, format);
> >> -            goto delete_and_fail;
> >> -        }
> >> -
> >> -        states->old_bs = bdrv_find(device);
> >> -        if (!states->old_bs) {
> >> -            error_set(errp, QERR_DEVICE_NOT_FOUND, device);
> >> -            goto delete_and_fail;
> >> -        }
> >> -
> >> -        if (!bdrv_is_inserted(states->old_bs)) {
> >> -            error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, device);
> >> -            goto delete_and_fail;
> >> -        }
> >> -
> >> -        if (bdrv_in_use(states->old_bs)) {
> >> -            error_set(errp, QERR_DEVICE_IN_USE, device);
> >> -            goto delete_and_fail;
> >> -        }
> >> -
> >> -        if (!bdrv_is_read_only(states->old_bs)) {
> >> -            if (bdrv_flush(states->old_bs)) {
> >> -                error_set(errp, QERR_IO_ERROR);
> >> -                goto delete_and_fail;
> >> -            }
> >> -        }
> >> -
> >> -        flags = states->old_bs->open_flags;
> >> -
> >> -        proto_drv = bdrv_find_protocol(new_image_file);
> >> -        if (!proto_drv) {
> >> -            error_set(errp, QERR_INVALID_BLOCK_FORMAT, format);
> >> -            goto delete_and_fail;
> >> -        }
> >> -
> >> -        /* create new image w/backing file */
> >> -        if (mode != NEW_IMAGE_MODE_EXISTING) {
> >> -            bdrv_img_create(new_image_file, format,
> >> -                            states->old_bs->filename,
> >> -                            states->old_bs->drv->format_name,
> >> -                            NULL, -1, flags, &local_err, false);
> >> -            if (error_is_set(&local_err)) {
> >> -                error_propagate(errp, local_err);
> >> -                goto delete_and_fail;
> >> -            }
> >> -        }
> >> -
> >> -        /* We will manually add the backing_hd field to the bs later */
> >> -        states->new_bs = bdrv_new("");
> >> -        /* TODO Inherit bs->options or only take explicit options with an
> >> -         * extended QMP command? */
> >> -        ret = bdrv_open(states->new_bs, new_image_file, NULL,
> >> -                        flags | BDRV_O_NO_BACKING, drv);
> >> -        if (ret != 0) {
> >> -            error_set(errp, QERR_OPEN_FILE_FAILED, new_image_file);
> >> +        if (states->ops->commit(states->action, &states->opaque, errp)) {
> >>               goto delete_and_fail;
> >>           }
> >>       }
> >
> > The following block is the commit:
> >
> >> -
> >> -    /* 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);
> >> -        /* We don't need (or want) to use the transactional
> >> -         * bdrv_reopen_multiple() across all the entries at once, because
> >> we
> >> -         * don't want to abort all of them if one of them fails the
> >> reopen */
> >> -        bdrv_reopen(states->new_bs, states->new_bs->open_flags &
> >> ~BDRV_O_RDWR,
> >> -                    NULL);
> >> -    }
> >> -
> >>       /* success */
> >>       goto exit;
> >
> > And this is rollback:
> >
> >>   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);
> >> -        }
> >> +        states->ops->rollback(states->action, states->opaque);
> >>       }
> >
> > Kevin
> >
> 
> 
> --
> Best Regards
> 
> Wenchao Xia
> 
> 

Re: [Qemu-devel] [PATCH 2/3] block: adjust qmp_transaction to be extendable

[Wenchao Xia]

>
> No, if bdrv_snapshot_delete() can fail, you need to split it in two
> parts: one that can fail, and one that cannot.  If you cannot, then
> there are two possibilities:
>
> - if the failures are minor and could be repaired with "qemu-img check -r"
> (e.g. lost clusters), then this is not optimal but can still be done;
>
> - otherwise, the operation simply cannot be made transactionable.
>
> In the case of qcow2_snapshot_delete, everything except
>
>      ret = bdrv_pwrite_sync(bs->file, offsetof(QCowHeader, nb_snapshots),
>                             &header_data, sizeof(header_data));
>      if (ret < 0) {
>          goto fail;
>      }
>
> must be in the prepare phase.  Everything after "fail" (which right now
> is nothing, but it should at least undo the qcow2_alloc_clusters operation)
> must be in the rollback phase.  Everything in the middle is the commit
> phase.
>
> Paolo
>
   Sorry I haven't state it clearly. What about bdrv_snapshot_create()
operation? If it need to be rolled back, I think bdrv_snapshot_delete()
will get called and it may fail. But in most case if
bdrv_snapshot_create() succeed before, the bdrv_snapshot_delete should
succeed also, so if fail there may be unexpected error below, could
assert be used for this?

>>
>>>> @@ -806,125 +950,37 @@ void qmp_transaction(BlockdevActionList *dev_list,
>>>> Error **errp)
>>>>        /* We don't do anything in this loop that commits us to the snapshot
>>>>        */
>>>>        while (NULL != dev_entry) {
>>>>            BlockdevAction *dev_info = NULL;
>>>> -        BlockDriver *proto_drv;
>>>> -        BlockDriver *drv;
>>>> -        int flags;
>>>> -        enum NewImageMode mode;
>>>> -        const char *new_image_file;
>>>> -        const char *device;
>>>> -        const char *format = "qcow2";
>>>> -
>>>>            dev_info = dev_entry->value;
>>>>            dev_entry = dev_entry->next;
>>>>
>>>>            states = g_malloc0(sizeof(BlkTransactionStates));
>>>>            QSIMPLEQ_INSERT_TAIL(&snap_bdrv_states, states, entry);
>>>>
>>>> +        states->action = dev_info;
>>>>            switch (dev_info->kind) {
>>>>            case BLOCKDEV_ACTION_KIND_BLOCKDEV_SNAPSHOT_SYNC:
>>>> -            device = dev_info->blockdev_snapshot_sync->device;
>>>> -            if (!dev_info->blockdev_snapshot_sync->has_mode) {
>>>> -                dev_info->blockdev_snapshot_sync->mode =
>>>> NEW_IMAGE_MODE_ABSOLUTE_PATHS;
>>>> -            }
>>>> -            new_image_file =
>>>> dev_info->blockdev_snapshot_sync->snapshot_file;
>>>> -            if (dev_info->blockdev_snapshot_sync->has_format) {
>>>> -                format = dev_info->blockdev_snapshot_sync->format;
>>>> -            }
>>>> -            mode = dev_info->blockdev_snapshot_sync->mode;
>>>> +            states->ops = &external_snapshot_ops;
>>>>                break;
>>>>            default:
>>>>                abort();
>>>>            }
>>>>
>>>> -        drv = bdrv_find_format(format);
>>>> -        if (!drv) {
>>>> -            error_set(errp, QERR_INVALID_BLOCK_FORMAT, format);
>>>> -            goto delete_and_fail;
>>>> -        }
>>>> -
>>>> -        states->old_bs = bdrv_find(device);
>>>> -        if (!states->old_bs) {
>>>> -            error_set(errp, QERR_DEVICE_NOT_FOUND, device);
>>>> -            goto delete_and_fail;
>>>> -        }
>>>> -
>>>> -        if (!bdrv_is_inserted(states->old_bs)) {
>>>> -            error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, device);
>>>> -            goto delete_and_fail;
>>>> -        }
>>>> -
>>>> -        if (bdrv_in_use(states->old_bs)) {
>>>> -            error_set(errp, QERR_DEVICE_IN_USE, device);
>>>> -            goto delete_and_fail;
>>>> -        }
>>>> -
>>>> -        if (!bdrv_is_read_only(states->old_bs)) {
>>>> -            if (bdrv_flush(states->old_bs)) {
>>>> -                error_set(errp, QERR_IO_ERROR);
>>>> -                goto delete_and_fail;
>>>> -            }
>>>> -        }
>>>> -
>>>> -        flags = states->old_bs->open_flags;
>>>> -
>>>> -        proto_drv = bdrv_find_protocol(new_image_file);
>>>> -        if (!proto_drv) {
>>>> -            error_set(errp, QERR_INVALID_BLOCK_FORMAT, format);
>>>> -            goto delete_and_fail;
>>>> -        }
>>>> -
>>>> -        /* create new image w/backing file */
>>>> -        if (mode != NEW_IMAGE_MODE_EXISTING) {
>>>> -            bdrv_img_create(new_image_file, format,
>>>> -                            states->old_bs->filename,
>>>> -                            states->old_bs->drv->format_name,
>>>> -                            NULL, -1, flags, &local_err, false);
>>>> -            if (error_is_set(&local_err)) {
>>>> -                error_propagate(errp, local_err);
>>>> -                goto delete_and_fail;
>>>> -            }
>>>> -        }
>>>> -
>>>> -        /* We will manually add the backing_hd field to the bs later */
>>>> -        states->new_bs = bdrv_new("");
>>>> -        /* TODO Inherit bs->options or only take explicit options with an
>>>> -         * extended QMP command? */
>>>> -        ret = bdrv_open(states->new_bs, new_image_file, NULL,
>>>> -                        flags | BDRV_O_NO_BACKING, drv);
>>>> -        if (ret != 0) {
>>>> -            error_set(errp, QERR_OPEN_FILE_FAILED, new_image_file);
>>>> +        if (states->ops->commit(states->action, &states->opaque, errp)) {
>>>>                goto delete_and_fail;
>>>>            }
>>>>        }
>>>
>>> The following block is the commit:
>>>
>>>> -
>>>> -    /* 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);
>>>> -        /* We don't need (or want) to use the transactional
>>>> -         * bdrv_reopen_multiple() across all the entries at once, because
>>>> we
>>>> -         * don't want to abort all of them if one of them fails the
>>>> reopen */
>>>> -        bdrv_reopen(states->new_bs, states->new_bs->open_flags &
>>>> ~BDRV_O_RDWR,
>>>> -                    NULL);
>>>> -    }
>>>> -
>>>>        /* success */
>>>>        goto exit;
>>>
>>> And this is rollback:
>>>
>>>>    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);
>>>> -        }
>>>> +        states->ops->rollback(states->action, states->opaque);
>>>>        }
>>>
>>> Kevin
>>>
>>
>>
>> --
>> Best Regards
>>
>> Wenchao Xia
>>
>>
>


-- 
Best Regards

Wenchao Xia

Re: [Qemu-devel] [PATCH 2/3] block: adjust qmp_transaction to be extendable

[Paolo Bonzini]

Il 03/04/2013 11:02, Wenchao Xia ha scritto:
>>
>   Sorry I haven't state it clearly. What about bdrv_snapshot_create()
> operation? If it need to be rolled back, I think bdrv_snapshot_delete()
> will get called and it may fail. But in most case if
> bdrv_snapshot_create() succeed before, the bdrv_snapshot_delete should
> succeed also, so if fail there may be unexpected error below, could
> assert be used for this?

No.  Transactionable means that commit and rollback cannot fail.

For bdrv_snapshot_create() it is the same as for bdrv_snapshot_delete()
(the overview I wrote was for qcow2_write_snapshots, which is the common
part of bdrv_snapshot_create() and bdrv_snapshot_delete()).

Please do one thing at a time.  Split the series in multiple pieces if
possible.  Otherwise you will just fail, and you will have wasted a lot
of your and other people's time.

Paolo

Re: [Qemu-devel] [PATCH 2/3] block: adjust qmp_transaction to be extendable

[Kevin Wolf]

Am 03.04.2013 um 11:02 hat Wenchao Xia geschrieben:
> >
> >No, if bdrv_snapshot_delete() can fail, you need to split it in two
> >parts: one that can fail, and one that cannot.  If you cannot, then
> >there are two possibilities:
> >
> >- if the failures are minor and could be repaired with "qemu-img check -r"
> >(e.g. lost clusters), then this is not optimal but can still be done;
> >
> >- otherwise, the operation simply cannot be made transactionable.
> >
> >In the case of qcow2_snapshot_delete, everything except
> >
> >     ret = bdrv_pwrite_sync(bs->file, offsetof(QCowHeader, nb_snapshots),
> >                            &header_data, sizeof(header_data));
> >     if (ret < 0) {
> >         goto fail;
> >     }
> >
> >must be in the prepare phase.  Everything after "fail" (which right now
> >is nothing, but it should at least undo the qcow2_alloc_clusters operation)
> >must be in the rollback phase.  Everything in the middle is the commit
> >phase.
> >
> >Paolo
> >
>   Sorry I haven't state it clearly. What about bdrv_snapshot_create()
> operation? If it need to be rolled back, I think bdrv_snapshot_delete()
> will get called and it may fail.

This means that you're doing it wrong. Instead of deleting the snapshot
on rollback, you shouldn't complete the creation until commit.

If the one bdrv_pwrite_sync() in the commit stage fails, we cannot
maintain transactional semantics. I guess its unlikely enough.

> But in most case if
> bdrv_snapshot_create() succeed before, the bdrv_snapshot_delete should
> succeed also, so if fail there may be unexpected error below, could
> assert be used for this?

assert() is only for programming errors, not for things like hardware
failure.

Kevin

Re: [Qemu-devel] [PATCH 2/3] block: adjust qmp_transaction to be extendable

[Wenchao Xia]

于 2013-4-3 17:02, Wenchao Xia 写道:
>>
>> No, if bdrv_snapshot_delete() can fail, you need to split it in two
>> parts: one that can fail, and one that cannot.  If you cannot, then
>> there are two possibilities:
>>
>> - if the failures are minor and could be repaired with "qemu-img check
>> -r"
>> (e.g. lost clusters), then this is not optimal but can still be done;
>>
>> - otherwise, the operation simply cannot be made transactionable.
>>
>> In the case of qcow2_snapshot_delete, everything except
>>
>>      ret = bdrv_pwrite_sync(bs->file, offsetof(QCowHeader, nb_snapshots),
>>                             &header_data, sizeof(header_data));
>>      if (ret < 0) {
>>          goto fail;
>>      }
>>
>> must be in the prepare phase.  Everything after "fail" (which right now
>> is nothing, but it should at least undo the qcow2_alloc_clusters
>> operation)
>> must be in the rollback phase.  Everything in the middle is the commit
>> phase.
>>
>> Paolo
>>
>    Sorry I haven't state it clearly. What about bdrv_snapshot_create()
> operation? If it need to be rolled back, I think bdrv_snapshot_delete()
> will get called and it may fail. But in most case if
> bdrv_snapshot_create() succeed before, the bdrv_snapshot_delete should
> succeed also, so if fail there may be unexpected error below, could
> assert be used for this?
>
   After consideration again, I think bdrv_snapshot_create() should be
split apart like above, thank u for the tip.

-- 
Best Regards

Wenchao Xia

Re: [Qemu-devel] [PATCH 2/3] block: adjust qmp_transaction to be extendable

[Stefan Hajnoczi]

On Mon, Apr 01, 2013 at 06:01:30PM +0800, Wenchao Xia wrote:
>  /* New and old BlockDriverState structs for group snapshots */
> -typedef struct BlkTransactionStates {
> +typedef struct BdrvActionOps {
> +    int (*commit)(BlockdevAction *action, void **p_opaque, Error **errp);
> +    void (*rollback)(BlockdevAction *action, void *opaque);
> +    void (*clean)(BlockdevAction *action, void *opaque);

Please document these functions.

> +const BdrvActionOps external_snapshot_ops = {
> +    .commit = external_snapshot_commit,
> +    .rollback = external_snapshot_rollback,
> +    .clean = external_snapshot_clean,
> +};
> +
> +typedef struct BlkTransactionStates {
> +    BlockdevAction *action;
> +    void *opaque;
> +    const BdrvActionOps *ops;
>      QSIMPLEQ_ENTRY(BlkTransactionStates) entry;
>  } BlkTransactionStates;

The relationship between BlkTransactionStates and ExternalSnapshotState
can be simplified:

typedef struct {
    int (*commit)(BlkTransactionState *state, Error **errp);
    void (*rollback)(BlkTransactionState *state);
    void (*clean)(BlkTransactionState *state);
    size_t instance_size;
} BdrvActionOps;

typedef struct BlkTransactionState {
    BlockDevAction *action;
    const BdrvActionOps *ops;
    QSIMPLEQ_ENTRY(BlkTransactionState) entry;
} BlkTransactionState;

typedef struct {
    BlkTransactionState common;
    BlockDriverState *old_bs;
    BlockDriverState *new_bs;
} ExternalSnapshotState;

const BdrvActionOps external_snapshot_ops = {
    .commit = external_snapshot_commit,
    .rollback = external_snapshot_rollback,
    .clean = external_snapshot_clean,
    .instance_size = sizeof(ExternalSnapshotState);
};

This eliminates the opaque pointer and g_free(state) can be handled by
qmp_transaction().  This way action types no longer need to free opaque.

Re: [Qemu-devel] [PATCH 2/3] block: adjust qmp_transaction to be extendable

[Wenchao Xia]

于 2013-4-17 22:42, Stefan Hajnoczi 写道:
> On Mon, Apr 01, 2013 at 06:01:30PM +0800, Wenchao Xia wrote:
>>   /* New and old BlockDriverState structs for group snapshots */
>> -typedef struct BlkTransactionStates {
>> +typedef struct BdrvActionOps {
>> +    int (*commit)(BlockdevAction *action, void **p_opaque, Error **errp);
>> +    void (*rollback)(BlockdevAction *action, void *opaque);
>> +    void (*clean)(BlockdevAction *action, void *opaque);
>
> Please document these functions.
>
   OK.

>> +const BdrvActionOps external_snapshot_ops = {
>> +    .commit = external_snapshot_commit,
>> +    .rollback = external_snapshot_rollback,
>> +    .clean = external_snapshot_clean,
>> +};
>> +
>> +typedef struct BlkTransactionStates {
>> +    BlockdevAction *action;
>> +    void *opaque;
>> +    const BdrvActionOps *ops;
>>       QSIMPLEQ_ENTRY(BlkTransactionStates) entry;
>>   } BlkTransactionStates;
>
> The relationship between BlkTransactionStates and ExternalSnapshotState
> can be simplified:
>
> typedef struct {
>      int (*commit)(BlkTransactionState *state, Error **errp);
>      void (*rollback)(BlkTransactionState *state);
>      void (*clean)(BlkTransactionState *state);
>      size_t instance_size;
> } BdrvActionOps;
>
> typedef struct BlkTransactionState {
>      BlockDevAction *action;
>      const BdrvActionOps *ops;
>      QSIMPLEQ_ENTRY(BlkTransactionState) entry;
> } BlkTransactionState;
>
> typedef struct {
>      BlkTransactionState common;
>      BlockDriverState *old_bs;
>      BlockDriverState *new_bs;
> } ExternalSnapshotState;
>
> const BdrvActionOps external_snapshot_ops = {
>      .commit = external_snapshot_commit,
>      .rollback = external_snapshot_rollback,
>      .clean = external_snapshot_clean,
>      .instance_size = sizeof(ExternalSnapshotState);
> };
>
> This eliminates the opaque pointer and g_free(state) can be handled by
> qmp_transaction().  This way action types no longer need to free opaque.
>
   Where should be ExternalSnapshotState* placed, insert it into
BlkTransactionState, or BdrvActionOps?

-- 
Best Regards

Wenchao Xia

Re: [Qemu-devel] [PATCH 2/3] block: adjust qmp_transaction to be extendable

[Stefan Hajnoczi]

On Thu, Apr 18, 2013 at 5:00 AM, Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
> 于 2013-4-17 22:42, Stefan Hajnoczi 写道:
>
>> On Mon, Apr 01, 2013 at 06:01:30PM +0800, Wenchao Xia wrote:
>>>
>>>   /* New and old BlockDriverState structs for group snapshots */
>>> -typedef struct BlkTransactionStates {
>>> +typedef struct BdrvActionOps {
>>> +    int (*commit)(BlockdevAction *action, void **p_opaque, Error
>>> **errp);
>>> +    void (*rollback)(BlockdevAction *action, void *opaque);
>>> +    void (*clean)(BlockdevAction *action, void *opaque);
>>
>>
>> Please document these functions.
>>
>   OK.
>
>
>>> +const BdrvActionOps external_snapshot_ops = {
>>> +    .commit = external_snapshot_commit,
>>> +    .rollback = external_snapshot_rollback,
>>> +    .clean = external_snapshot_clean,
>>> +};
>>> +
>>> +typedef struct BlkTransactionStates {
>>> +    BlockdevAction *action;
>>> +    void *opaque;
>>> +    const BdrvActionOps *ops;
>>>       QSIMPLEQ_ENTRY(BlkTransactionStates) entry;
>>>   } BlkTransactionStates;
>>
>>
>> The relationship between BlkTransactionStates and ExternalSnapshotState
>> can be simplified:
>>
>> typedef struct {
>>      int (*commit)(BlkTransactionState *state, Error **errp);
>>      void (*rollback)(BlkTransactionState *state);
>>      void (*clean)(BlkTransactionState *state);
>>      size_t instance_size;
>> } BdrvActionOps;
>>
>> typedef struct BlkTransactionState {
>>      BlockDevAction *action;
>>      const BdrvActionOps *ops;
>>      QSIMPLEQ_ENTRY(BlkTransactionState) entry;
>> } BlkTransactionState;
>>
>> typedef struct {
>>      BlkTransactionState common;
>>      BlockDriverState *old_bs;
>>      BlockDriverState *new_bs;
>> } ExternalSnapshotState;
>>
>> const BdrvActionOps external_snapshot_ops = {
>>      .commit = external_snapshot_commit,
>>      .rollback = external_snapshot_rollback,
>>      .clean = external_snapshot_clean,
>>      .instance_size = sizeof(ExternalSnapshotState);
>> };
>>
>> This eliminates the opaque pointer and g_free(state) can be handled by
>> qmp_transaction().  This way action types no longer need to free opaque.
>>
>   Where should be ExternalSnapshotState* placed, insert it into
> BlkTransactionState, or BdrvActionOps?

No explicit pointer is needed since ExternalSnapshotState embeds
BlkTransactionState.  See container_of() or DO_UPCAST().

Stefan

[Qemu-devel] [PATCH 3/3] block: change rollback sequence in qmp_transaction

[Wenchao Xia]

  Last operaton should be cancelled first.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 blockdev.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 75416fb..a24d10e 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -954,7 +954,7 @@ void qmp_transaction(BlockdevActionList *dev_list, Error **errp)
         dev_entry = dev_entry->next;
 
         states = g_malloc0(sizeof(BlkTransactionStates));
-        QSIMPLEQ_INSERT_TAIL(&snap_bdrv_states, states, entry);
+        QSIMPLEQ_INSERT_HEAD(&snap_bdrv_states, states, entry);
 
         states->action = dev_info;
         switch (dev_info->kind) {
-- 
1.7.1

Re: [Qemu-devel] [PATCH 3/3] block: change rollback sequence in qmp_transaction

[Eric Blake]

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

On 04/01/2013 04:01 AM, Wenchao Xia wrote:
>   Last operaton should be cancelled first.

s/operaton/operation/

[I don't care enough about US vs. UK to say whether canceled or
cancelled looks better]

> 
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
>  blockdev.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index 75416fb..a24d10e 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -954,7 +954,7 @@ void qmp_transaction(BlockdevActionList *dev_list, Error **errp)
>          dev_entry = dev_entry->next;
>  
>          states = g_malloc0(sizeof(BlkTransactionStates));
> -        QSIMPLEQ_INSERT_TAIL(&snap_bdrv_states, states, entry);
> +        QSIMPLEQ_INSERT_HEAD(&snap_bdrv_states, states, entry);

Is this a bug fix that for something that can be triggered by existing
use of the 'transaction' command even without the additions you made in
patches 1 and 2?  If so, this probably ought to come first in the
series, and you probably ought to consider enhancing the testsuite to
show why it matters.

-- 
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: 621 bytes --]

Re: [Qemu-devel] [PATCH 3/3] block: change rollback sequence in qmp_transaction

[Wenchao Xia]

于 2013-4-1 23:52, Eric Blake 写道:
> On 04/01/2013 04:01 AM, Wenchao Xia wrote:
>>    Last operaton should be cancelled first.
>
> s/operaton/operation/
>
> [I don't care enough about US vs. UK to say whether canceled or
> cancelled looks better]
>
>>
>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>> ---
>>   blockdev.c |    2 +-
>>   1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/blockdev.c b/blockdev.c
>> index 75416fb..a24d10e 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -954,7 +954,7 @@ void qmp_transaction(BlockdevActionList *dev_list, Error **errp)
>>           dev_entry = dev_entry->next;
>>
>>           states = g_malloc0(sizeof(BlkTransactionStates));
>> -        QSIMPLEQ_INSERT_TAIL(&snap_bdrv_states, states, entry);
>> +        QSIMPLEQ_INSERT_HEAD(&snap_bdrv_states, states, entry);
>
> Is this a bug fix that for something that can be triggered by existing
> use of the 'transaction' command even without the additions you made in
> patches 1 and 2?  If so, this probably ought to come first in the
> series, and you probably ought to consider enhancing the testsuite to
> show why it matters.
>
   Originally it only support backing chain, it will have no
difference about the sequence to delete it, but matters if
other types of operation are added.

-- 
Best Regards

Wenchao Xia

Re: [Qemu-devel] [PATCH 3/3] block: change rollback sequence in qmp_transaction

[Kevin Wolf]

Am 01.04.2013 um 12:01 hat Wenchao Xia geschrieben:
>   Last operaton should be cancelled first.
> 
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>

Should it? This commit message does little to convince me.

Kevin

Re: [Qemu-devel] [PATCH 3/3] block: change rollback sequence in qmp_transaction

[Wenchao Xia]

于 2013-4-2 21:59, Kevin Wolf 写道:
> Am 01.04.2013 um 12:01 hat Wenchao Xia geschrieben:
>>    Last operaton should be cancelled first.
>>
>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>
> Should it? This commit message does little to convince me.
>
> Kevin
>
   I think so, if two request are "snapshot_blkdev ide0 newimage.qcow2",
"snapshot_blkdev_internal ide0 snap0", then in rollback delete of
snap0 would be wrong.

-- 
Best Regards

Wenchao Xia

Re: [Qemu-devel] [PATCH 3/3] block: change rollback sequence in qmp_transaction

[Kevin Wolf]

Am 03.04.2013 um 07:35 hat Wenchao Xia geschrieben:
> 于 2013-4-2 21:59, Kevin Wolf 写道:
> >Am 01.04.2013 um 12:01 hat Wenchao Xia geschrieben:
> >>   Last operaton should be cancelled first.
> >>
> >>Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> >
> >Should it? This commit message does little to convince me.
> >
> >Kevin
> >
>   I think so, if two request are "snapshot_blkdev ide0 newimage.qcow2",
> "snapshot_blkdev_internal ide0 snap0", then in rollback delete of
> snap0 would be wrong.

I think this problem exists only because you didn't properly separate
the prepare and the commit stage. If any changes only took effect in a
non-failing commit stage, then you don't ever have to delete a snapshot.
Instead, you just abort the creation of the snapshot.

Kevin

Re: [Qemu-devel] [PATCH 3/3] block: change rollback sequence in qmp_transaction

[Wenchao Xia]

于 2013-4-3 17:03, Kevin Wolf 写道:
> Am 03.04.2013 um 07:35 hat Wenchao Xia geschrieben:
>> 于 2013-4-2 21:59, Kevin Wolf 写道:
>>> Am 01.04.2013 um 12:01 hat Wenchao Xia geschrieben:
>>>>    Last operaton should be cancelled first.
>>>>
>>>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>>>
>>> Should it? This commit message does little to convince me.
>>>
>>> Kevin
>>>
>>    I think so, if two request are "snapshot_blkdev ide0 newimage.qcow2",
>> "snapshot_blkdev_internal ide0 snap0", then in rollback delete of
>> snap0 would be wrong.
>
> I think this problem exists only because you didn't properly separate
> the prepare and the commit stage. If any changes only took effect in a
> non-failing commit stage, then you don't ever have to delete a snapshot.
> Instead, you just abort the creation of the snapshot.
>
> Kevin
>
   OK, I got your point, will split bdrv_snapshot_create() before adding
it in qmp_transaction.

-- 
Best Regards

Wenchao Xia