diff mbox

[v5,11/14] block/backup: support block job transactions

Message ID 1441611277-24596-12-git-send-email-famz@redhat.com
State New
Headers show

Commit Message

Fam Zheng Sept. 7, 2015, 7:34 a.m. UTC 
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                | 139 ++++++++++++++++++++++++++++++++++++----------
 hmp.c                     |   2 +-
 include/block/block_int.h |   3 +-
 qapi/block-core.json      |  16 +++++-
 5 files changed, 148 insertions(+), 37 deletions(-)

Comments

Max Reitz Sept. 11, 2015, 6:50 p.m. UTC | #1 
On 07.09.2015 09:34, 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                | 139 ++++++++++++++++++++++++++++++++++++----------
>  hmp.c                     |   2 +-
>  include/block/block_int.h |   3 +-
>  qapi/block-core.json      |  16 +++++-
>  5 files changed, 148 insertions(+), 37 deletions(-)
> 
> diff --git a/block/backup.c b/block/backup.c
> index 9776d9c..3a3dccc 100644
> --- a/block/backup.c
> +++ b/block/backup.c

[snip]

> @@ -545,6 +559,7 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target,
>                         sync_bitmap : NULL;
>      job->common.len = len;
>      job->common.co = qemu_coroutine_create(backup_run);
> +    block_job_txn_add_job(txn, &job->common);

You're calling this for every single job, so the reference count of txn
will probably be job count + 1. However, block_job_txn_unref() is only
called for one block job by block_job_completed_txn_{abort,success}(). I
think you need to call block_job_txn_unref() for every completed block
job, otherwise it will be leaked if there is more than a single job in
the txn.

Max

>      qemu_coroutine_enter(job->common.co, job);
>      return;
>
Eric Blake Sept. 11, 2015, 9:26 p.m. UTC | #2 
On 09/07/2015 01:34 AM, 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>
> ---

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.
Fam Zheng Sept. 15, 2015, 5:08 a.m. UTC | #3 
On Fri, 09/11 15:26, Eric Blake wrote:
> On 09/07/2015 01:34 AM, 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>
> > ---
> 
> 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.

Very good point, will do that in v6.

Fam
diff mbox

Patch

diff --git a/block/backup.c b/block/backup.c
index 9776d9c..3a3dccc 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -226,11 +226,29 @@  static void backup_cleanup_sync_bitmap(BackupBlockJob *job, int ret)
     }
 }
 
+static void backup_commit(BlockJob *job)
+{
+    BackupBlockJob *s = container_of(job, BackupBlockJob, common);
+    if (s->sync_bitmap) {
+        backup_cleanup_sync_bitmap(s, 0);
+    }
+}
+
+static void backup_abort(BlockJob *job)
+{
+    BackupBlockJob *s = container_of(job, BackupBlockJob, common);
+    if (s->sync_bitmap) {
+        backup_cleanup_sync_bitmap(s, -1);
+    }
+}
+
 static const BlockJobDriver backup_job_driver = {
     .instance_size  = sizeof(BackupBlockJob),
     .job_type       = BLOCK_JOB_TYPE_BACKUP,
     .set_speed      = backup_set_speed,
     .iostatus_reset = backup_iostatus_reset,
+    .commit         = backup_commit,
+    .abort          = backup_abort,
 };
 
 static BlockErrorAction backup_error_action(BackupBlockJob *job,
@@ -443,10 +461,6 @@  static void coroutine_fn backup_run(void *opaque)
     /* wait until pending backup_do_cow() calls have completed */
     qemu_co_rwlock_wrlock(&job->flush_rwlock);
     qemu_co_rwlock_unlock(&job->flush_rwlock);
-
-    if (job->sync_bitmap) {
-        backup_cleanup_sync_bitmap(job, ret);
-    }
     hbitmap_free(job->bitmap);
 
     bdrv_iostatus_disable(target);
@@ -463,7 +477,7 @@  void backup_start(BlockDriverState *bs, BlockDriverState *target,
                   BlockdevOnError on_source_error,
                   BlockdevOnError on_target_error,
                   BlockCompletionFunc *cb, void *opaque,
-                  Error **errp)
+                  BlockJobTxn *txn, Error **errp)
 {
     int64_t len;
 
@@ -545,6 +559,7 @@  void backup_start(BlockDriverState *bs, BlockDriverState *target,
                        sync_bitmap : NULL;
     job->common.len = len;
     job->common.co = qemu_coroutine_create(backup_run);
+    block_job_txn_add_job(txn, &job->common);
     qemu_coroutine_enter(job->common.co, job);
     return;
 
diff --git a/blockdev.c b/blockdev.c
index 3d5e5cd..db1fdce 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1592,12 +1592,25 @@  typedef struct DriveBackupState {
     BlockJob *job;
 } DriveBackupState;
 
+static void do_drive_backup(const char *device, const char *target,
+                            bool has_format, const char *format,
+                            enum MirrorSyncMode sync,
+                            bool has_mode, enum NewImageMode mode,
+                            bool has_speed, int64_t speed,
+                            bool has_bitmap, const char *bitmap,
+                            bool has_on_source_error,
+                            BlockdevOnError on_source_error,
+                            bool has_on_target_error,
+                            BlockdevOnError on_target_error,
+                            BlockJobTxn *txn, Error **errp);
+
 static void drive_backup_prepare(BlkActionState *common, Error **errp)
 {
     DriveBackupState *state = DO_UPCAST(DriveBackupState, common, common);
     BlockDriverState *bs;
     BlockBackend *blk;
     DriveBackup *backup;
+    BlockJobTxn *txn = NULL;
     Error *local_err = NULL;
 
     assert(common->action->kind == TRANSACTION_ACTION_KIND_DRIVE_BACKUP);
@@ -1615,15 +1628,20 @@  static void drive_backup_prepare(BlkActionState *common, Error **errp)
     state->aio_context = bdrv_get_aio_context(bs);
     aio_context_acquire(state->aio_context);
 
-    qmp_drive_backup(backup->device, backup->target,
-                     backup->has_format, backup->format,
-                     backup->sync,
-                     backup->has_mode, backup->mode,
-                     backup->has_speed, backup->speed,
-                     backup->has_bitmap, backup->bitmap,
-                     backup->has_on_source_error, backup->on_source_error,
-                     backup->has_on_target_error, backup->on_target_error,
-                     &local_err);
+    if (backup->has_transactional_cancel &&
+        backup->transactional_cancel) {
+        txn = common->block_job_txn;
+    }
+
+    do_drive_backup(backup->device, backup->target,
+                    backup->has_format, backup->format,
+                    backup->sync,
+                    backup->has_mode, backup->mode,
+                    backup->has_speed, backup->speed,
+                    backup->has_bitmap, backup->bitmap,
+                    backup->has_on_source_error, backup->on_source_error,
+                    backup->has_on_target_error, backup->on_target_error,
+                    txn, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
         return;
@@ -1660,12 +1678,22 @@  typedef struct BlockdevBackupState {
     AioContext *aio_context;
 } BlockdevBackupState;
 
+static void do_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,
+                               BlockJobTxn *txn, Error **errp);
+
 static void blockdev_backup_prepare(BlkActionState *common, Error **errp)
 {
     BlockdevBackupState *state = DO_UPCAST(BlockdevBackupState, common, common);
     BlockdevBackup *backup;
     BlockDriverState *bs, *target;
     BlockBackend *blk;
+    BlockJobTxn *txn = NULL;
     Error *local_err = NULL;
 
     assert(common->action->kind == TRANSACTION_ACTION_KIND_BLOCKDEV_BACKUP);
@@ -1694,12 +1722,17 @@  static void blockdev_backup_prepare(BlkActionState *common, Error **errp)
     }
     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 (backup->has_transactional_cancel &&
+        backup->transactional_cancel) {
+        txn = common->block_job_txn;
+    }
+
+    do_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,
+                       txn, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
         return;
@@ -2582,15 +2615,17 @@  out:
     aio_context_release(aio_context);
 }
 
-void qmp_drive_backup(const char *device, const char *target,
-                      bool has_format, const char *format,
-                      enum MirrorSyncMode sync,
-                      bool has_mode, enum NewImageMode mode,
-                      bool has_speed, int64_t speed,
-                      bool has_bitmap, const char *bitmap,
-                      bool has_on_source_error, BlockdevOnError on_source_error,
-                      bool has_on_target_error, BlockdevOnError on_target_error,
-                      Error **errp)
+static void do_drive_backup(const char *device, const char *target,
+                            bool has_format, const char *format,
+                            enum MirrorSyncMode sync,
+                            bool has_mode, enum NewImageMode mode,
+                            bool has_speed, int64_t speed,
+                            bool has_bitmap, const char *bitmap,
+                            bool has_on_source_error,
+                            BlockdevOnError on_source_error,
+                            bool has_on_target_error,
+                            BlockdevOnError on_target_error,
+                            BlockJobTxn *txn, Error **errp)
 {
     BlockBackend *blk;
     BlockDriverState *bs;
@@ -2707,7 +2742,7 @@  void qmp_drive_backup(const char *device, const char *target,
 
     backup_start(bs, target_bs, speed, sync, bmap,
                  on_source_error, on_target_error,
-                 block_job_cb, bs, &local_err);
+                 block_job_cb, bs, txn, &local_err);
     if (local_err != NULL) {
         bdrv_unref(target_bs);
         error_propagate(errp, local_err);
@@ -2718,19 +2753,44 @@  out:
     aio_context_release(aio_context);
 }
 
+void qmp_drive_backup(const char *device, const char *target,
+                      bool has_format, const char *format,
+                      enum MirrorSyncMode sync,
+                      bool has_mode, enum NewImageMode mode,
+                      bool has_speed, int64_t speed,
+                      bool has_bitmap, const char *bitmap,
+                      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;
+    }
+
+    return do_drive_backup(device, target, has_format, format, sync,
+                           has_mode, mode, has_speed, speed,
+                           has_bitmap, bitmap,
+                           has_on_source_error, on_source_error,
+                           has_on_target_error, on_target_error,
+                           NULL, errp);
+}
+
 BlockDeviceInfoList *qmp_query_named_block_nodes(Error **errp)
 {
     return bdrv_named_nodes_list(errp);
 }
 
-void qmp_blockdev_backup(const char *device, const char *target,
+void do_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,
-                         Error **errp)
+                         BlockJobTxn *txn, Error **errp)
 {
     BlockBackend *blk;
     BlockDriverState *bs;
@@ -2768,7 +2828,7 @@  void qmp_blockdev_backup(const char *device, const char *target,
     bdrv_ref(target_bs);
     bdrv_set_aio_context(target_bs, aio_context);
     backup_start(bs, target_bs, speed, sync, NULL, on_source_error,
-                 on_target_error, block_job_cb, bs, &local_err);
+                 on_target_error, block_job_cb, bs, txn, &local_err);
     if (local_err != NULL) {
         bdrv_unref(target_bs);
         error_propagate(errp, local_err);
@@ -2777,6 +2837,29 @@  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);
+}
+
 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/hmp.c b/hmp.c
index 3f807b7..11edea5 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1090,7 +1090,7 @@  void hmp_drive_backup(Monitor *mon, const QDict *qdict)
     qmp_drive_backup(device, filename, !!format, format,
                      full ? MIRROR_SYNC_MODE_FULL : MIRROR_SYNC_MODE_TOP,
                      true, mode, false, 0, false, NULL,
-                     false, 0, false, 0, &err);
+                     false, 0, false, 0, false, false, &err);
     hmp_handle_error(mon, &err);
 }
 
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 281d790..6fd07a2 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -643,6 +643,7 @@  void mirror_start(BlockDriverState *bs, BlockDriverState *target,
  * @on_target_error: The action to take upon error writing to the target.
  * @cb: Completion function for the job.
  * @opaque: Opaque pointer value passed to @cb.
+ * @txn: Transaction that this job is part of (may be NULL).
  *
  * Start a backup operation on @bs.  Clusters in @bs are written to @target
  * until the job is cancelled or manually completed.
@@ -653,7 +654,7 @@  void backup_start(BlockDriverState *bs, BlockDriverState *target,
                   BlockdevOnError on_source_error,
                   BlockdevOnError on_target_error,
                   BlockCompletionFunc *cb, void *opaque,
-                  Error **errp);
+                  BlockJobTxn *txn, Error **errp);
 
 void blk_dev_change_media_cb(BlockBackend *blk, bool load);
 bool blk_dev_has_removable_media(BlockBackend *blk);
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 7b2efb8..ddd6592 100644
--- a/qapi/block-core.json
+++ 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)
+#
 # 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' } }
 
 ##
 # @BlockdevBackup
@@ -771,6 +777,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)
+#
 # 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.
@@ -782,7 +793,8 @@ 
             'sync': 'MirrorSyncMode',
             '*speed': 'int',
             '*on-source-error': 'BlockdevOnError',
-            '*on-target-error': 'BlockdevOnError' } }
+            '*on-target-error': 'BlockdevOnError',
+            '*transactional-cancel': 'bool' } }
 
 ##
 # @blockdev-snapshot-sync