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

[Wenchao Xia <xiawenc@linux.vnet.ibm.com>]

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