Re: [Qemu-devel] [PATCH v4 2/3] block: Add blockdev-backup to transaction

[Max Reitz <mreitz@redhat.com>]

On 2014-12-04 at 03:29, Fam Zheng wrote:
> Also add version info for other transaction types.
>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>   blockdev.c       | 81 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   qapi-schema.json |  7 +++++
>   2 files changed, 88 insertions(+)
>
> diff --git a/blockdev.c b/blockdev.c
> index f44441a..a98a4f8 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1559,6 +1559,81 @@ static void drive_backup_clean(BlkTransactionState *common)
>       }
>   }
>   
> +typedef struct BlockdevBackupState {
> +    BlkTransactionState common;
> +    BlockDriverState *bs;
> +    BlockJob *job;
> +    AioContext *aio_context;
> +} BlockdevBackupState;
> +
> +static void blockdev_backup_prepare(BlkTransactionState *common, Error **errp)
> +{
> +    BlockdevBackupState *state = DO_UPCAST(BlockdevBackupState, common, common);
> +    BlockdevBackup *backup;
> +    BlockDriverState *bs, *target;
> +    Error *local_err = NULL;
> +
> +    assert(common->action->kind == TRANSACTION_ACTION_KIND_BLOCKDEV_BACKUP);
> +    backup = common->action->blockdev_backup;
> +
> +    bs = bdrv_find(backup->device);
> +    if (!bs) {
> +        error_set(errp, QERR_DEVICE_NOT_FOUND, backup->device);
> +        return;
> +    }
> +
> +    target = bdrv_find(backup->target);
> +    if (!target) {
> +        error_set(errp, QERR_DEVICE_NOT_FOUND, backup->target);
> +        return;
> +    }
> +
> +    /* AioContext is released in .clean() */
> +    state->aio_context = bdrv_get_aio_context(bs);
> +    if (state->aio_context != bdrv_get_aio_context(target)) {
> +        state->aio_context = NULL;
> +        error_setg(errp, "Backup between two IO threads are not implemented");

Either *Backups ore s/are/is/.

> +        return;
> +    }
> +    aio_context_acquire(state->aio_context);
> +
> +    qmp_blockdev_backup(backup->device, backup->target,
> +                        backup->sync,
> +                        backup->has_speed, backup->speed,
> +                        backup->has_on_source_error, backup->on_source_error,
> +                        backup->has_on_target_error, backup->on_target_error,
> +                        &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        state->bs = NULL;
> +        state->job = NULL;

No need for these assignments, state is 0-initialized. I wouldn't point 
that out if Stefan wouldn't just have sent a patch which removed such 
assignments in some other transaction preparation.

> +        return;
> +    }
> +
> +    state->bs = bdrv_find(backup->device);
> +    state->job = state->bs->job;
> +}
> +
> +static void blockdev_backup_abort(BlkTransactionState *common)
> +{
> +    BlockdevBackupState *state = DO_UPCAST(BlockdevBackupState, common, common);
> +    BlockDriverState *bs = state->bs;
> +
> +    /* Only cancel if it's the job we started */
> +    if (bs && bs->job && bs->job == state->job) {
> +        block_job_cancel_sync(bs->job);
> +    }
> +}
> +
> +static void blockdev_backup_clean(BlkTransactionState *common)
> +{
> +    BlockdevBackupState *state = DO_UPCAST(BlockdevBackupState, common, common);
> +
> +    if (state->aio_context) {
> +        aio_context_release(state->aio_context);
> +    }
> +}
> +
>   static void abort_prepare(BlkTransactionState *common, Error **errp)
>   {
>       error_setg(errp, "Transaction aborted using Abort action");
> @@ -1582,6 +1657,12 @@ static const BdrvActionOps actions[] = {
>           .abort = drive_backup_abort,
>           .clean = drive_backup_clean,
>       },
> +    [TRANSACTION_ACTION_KIND_BLOCKDEV_BACKUP] = {
> +        .instance_size = sizeof(BlockdevBackupState),
> +        .prepare = blockdev_backup_prepare,
> +        .abort = blockdev_backup_abort,
> +        .clean = blockdev_backup_clean,
> +    },
>       [TRANSACTION_ACTION_KIND_ABORT] = {
>           .instance_size = sizeof(BlkTransactionState),
>           .prepare = abort_prepare,
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 9ffdcf8..411d287 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -1254,11 +1254,18 @@
>   #
>   # A discriminated record of operations that can be performed with
>   # @transaction.
> +#
> +# Since 1.1
> +# drive-backup since 1.6
> +# abort since 1.6
> +# blockdev-snapshot-internal-sync since 1.7
> +# blockdev-backup since 2.3

This seems a bit hard to read... Maybe an empty line after the "Since 
1.1" would help, but I'm not sure...

>   ##
>   { 'union': 'TransactionAction',
>     'data': {
>          'blockdev-snapshot-sync': 'BlockdevSnapshot',
>          'drive-backup': 'DriveBackup',
> +       'blockdev-backup': 'BlockdevBackup',
>          'abort': 'Abort',
>          'blockdev-snapshot-internal-sync': 'BlockdevSnapshotInternal'
>      } }

So, about this patch in general: I know drive-backup works nearly the 
same way. It starts block job in prepare(), which is aborted in abort(). 
But it seems a bit like cheating to me. For me, a transaction is 
something which you can start and if any of the operations cannot be 
executed (because its preparation failed), all are aborted (that is, not 
even started). The commit() part will really do the operation, and that 
will never fail because prepare() has made sure it will not.

This isn't the case when starting block jobs in prepare(). If some other 
operation's prepare() fails, some data may have been copied already, so 
you can't really roll back the operation. It isn't so bad for 
drive-backup, because you're normally writing to a new image, so having 
overwritten that image by a bit isn't so bad. But with blockdev-backup, 
you're overwriting a block device inside qemu, so overwriting it 
partially may actually be bad (or even overwriting it completely; if a 
transaction fails, I'd expect the operations to have done nothing at all).

I understand that drive-backup does the same thing already (although I 
don't deem the impact there a bit less), so it may just be that my 
notion of transactions is wrong.

However, in this state, where is the advantage of making this an 
operation usable in a transaction? I can just start a number of 
blockdev-backup block jobs manually and cancel them if anything goes 
wrong. There's no difference, so I don't see the benefit of making it a 
transaction operation if a transaction does not actually mean "Do not do 
anything if we don't know for sure that all operations will complete 
successfully."

Max