[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v5 11/14] block/backup: support block job transa
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH v5 11/14] block/backup: support block job transactions |
Date: |
Fri, 11 Sep 2015 15:26:32 -0600 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.2.0 |
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
- [Qemu-devel] [PATCH v5 09/14] block: Add block job transactions, (continued)
- [Qemu-devel] [PATCH v5 09/14] block: Add block job transactions, Fam Zheng, 2015/09/07
- [Qemu-devel] [PATCH v5 10/14] blockdev: make BlockJobTxn available to qmp 'transaction', Fam Zheng, 2015/09/07
- [Qemu-devel] [PATCH v5 12/14] iotests: 124 - transactional failure test, Fam Zheng, 2015/09/07
- [Qemu-devel] [PATCH v5 11/14] block/backup: support block job transactions, Fam Zheng, 2015/09/07
- [Qemu-devel] [PATCH v5 13/14] qmp-commands.hx: Update the supported 'transaction' operations, Fam Zheng, 2015/09/07
- [Qemu-devel] [PATCH v5 14/14] tests: add BlockJobTxn unit test, Fam Zheng, 2015/09/07