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

[Fam Zheng <famz@redhat.com>]

On Mon, 08/03 20:26, Max Reitz wrote:
> 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?

Superfluous, removing.

> 
> >+
> >  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).

OK, adding "in the same transaction".

Fam