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

[Eric Blake]

On 09/07/2015 01:34 AM, Fam Zheng wrote:
> From: Stefan Hajnoczi <address@hidden>
> 
> 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 <address@hidden>
> Signed-off-by: Fam Zheng <address@hidden>
> ---

Interface review:

> +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;
> +    }

Hmm. It might be nicer if we had two separate qapi structs:

# used in 'blockdev-backup'
{ 'struct':'BlockdevBackup', 'data': { device ... on-target-error } }

# used in 'transaction'
{ 'struct':'BlockdevBackupTxn', 'base':'BlockdevBackup',
  'data': { 'transactional-cancel':'bool' } }

so that the user can't abuse the boolean from the wrong context.

[Side notes...
Furthermore, with pending changes coming down the qapi pipeline, we will
generate the C struct so that you can safely do
'(BlockdevBackup*)blockdev_backup_txn' to go from the child back to the
base class.
https://lists.gnu.org/archive/html/qemu-devel/2015-09/msg02583.html

> +
> +    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);
> +}

And with other changes coming down the pipe, you could write a function as:

void do_blockdev_backup(BlockdevBackup *args, Error **errp)

by adding 'box':true' to 'blockdev-backup' and make it a bit easier to
pass around all the data without breaking it into multiple parameters.
https://lists.gnu.org/archive/html/qemu-devel/2015-09/msg02599.html

But we're not there yet - it may have to be a simplification we apply
after the fact. :)
...]

> +++ b/qapi/block-core.json
> @@ -736,6 +736,11 @@
>  #                   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 in the 
> same
> +#                        transaction causes the whole group to cancel.
> +#                        (Since 2.5)

Mention default false.

> +#
>  # Note that @on-source-error and @on-target-error only affect background I/O.
>  # If an error occurs during a guest write request, the device's rerror/werror
>  # actions will be used.
> @@ -747,7 +752,8 @@
>              'sync': 'MirrorSyncMode', '*mode': 'NewImageMode',
>              '*speed': 'int', '*bitmap': 'str',
>              '*on-source-error': 'BlockdevOnError',
> -            '*on-target-error': 'BlockdevOnError' } }
> +            '*on-target-error': 'BlockdevOnError',
> +            '*transactional-cancel': 'bool' } }

See my above comments about the idea of using a parent and child class,
rather than exposing this outside of transaction, especially since you
didn't document that it can't be used outside of a transaction.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature