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

[Kevin Wolf <kwolf@redhat.com>]

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