Re: [Qemu-devel] [PATCH v4 11/14] block/backup: support block job transactions

[Max Reitz <mreitz@redhat.com>]

On 30.07.2015 08:39, Fam Zheng wrote:
> From: Stefan Hajnoczi <stefanha@redhat.com>
>
> Join the transaction when the 'transactional-cancel' QMP argument is
> true.
>
> This ensures that the sync bitmap is not thrown away if another block
> job in the transaction is cancelled or fails.  This is critical so
> incremental backup with multiple disks can be retried in case of
> cancellation/failure.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>   block/backup.c            |  25 ++++++--
>   blockdev.c                | 141 +++++++++++++++++++++++++++++++++++++---------
>   hmp.c                     |   2 +-
>   include/block/block_int.h |   3 +-
>   qapi/block-core.json      |  14 ++++-
>   5 files changed, 148 insertions(+), 37 deletions(-)

[...]

> diff --git a/blockdev.c b/blockdev.c
> index aba9d78..3e8b8b6 100644
> --- a/blockdev.c
> +++ b/blockdev.c

[...]

> @@ -2770,6 +2830,31 @@ out:
>       aio_context_release(aio_context);
>   }
>
> +void qmp_blockdev_backup(const char *device, const char *target,
> +                         enum MirrorSyncMode sync,
> +                         bool has_speed, int64_t speed,
> +                         bool has_on_source_error,
> +                         BlockdevOnError on_source_error,
> +                         bool has_on_target_error,
> +                         BlockdevOnError on_target_error,
> +                         bool has_transactional_cancel,
> +                         bool transactional_cancel,
> +                         Error **errp)
> +{
> +    if (has_transactional_cancel && transactional_cancel) {
> +        error_setg(errp, "Transactional cancel can only be used in the "
> +                   "'transaction' command");
> +        return;
> +    }
> +
> +    do_blockdev_backup(device, target, sync, has_speed, speed,
> +                       has_on_source_error, on_source_error,
> +                       has_on_target_error, on_target_error,
> +                       NULL, errp);
> +}
> +
> +#define DEFAULT_MIRROR_BUF_SIZE   (10 << 20)

Why?

> +
>   void qmp_drive_mirror(const char *device, const char *target,
>                         bool has_format, const char *format,
>                         bool has_node_name, const char *node_name,

[...]

> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 7b2efb8..d4674ed 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -736,6 +736,10 @@
>   #                   default 'report' (no limitations, since this applies to
>   #                   a different block device than @device).
>   #
> +# @transactional-cancel: #optional whether failure or cancellation of other
> +#                        block jobs with @transactional-cancel true causes the
> +#                        whole group to cancel. (Since 2.5)
> +#

Maybe it's worth adding "[of other block jobs] in the same transaction", 
and maybe even noting that this therefore only works with backup block 
jobs started as part of a transaction (although that should be clear then).

Max