Re: [Qemu-devel] [PATCH v3 12/15] block/backup: support block job transactions

[John Snow]

On 07/09/2015 11:46 PM, 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>
> ---
>  block/backup.c            |  23 +++++++-
>  blockdev.c                | 139 
> ++++++++++++++++++++++++++++++++++++----------
>  hmp.c                     |   2 +-
>  include/block/block_int.h |   3 +-
>  qapi/block-core.json      |  14 ++++-
>  5 files changed, 147 insertions(+), 34 deletions(-)
> 
> diff --git a/block/backup.c b/block/backup.c
> index 6e24384..4f0eb17 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -226,11 +226,29 @@ static void backup_handle_dirty_bitmap(BackupBlockJob 
> *job, int ret)
>      }
>  }
>  
> +static void backup_txn_commit(BlockJob *job)
> +{
> +    BackupBlockJob *s = container_of(job, BackupBlockJob, common);
> +    if (s->sync_bitmap) {
> +        backup_handle_dirty_bitmap(s, 0);
> +    }
> +}
> +
> +static void backup_txn_abort(BlockJob *job)
> +{
> +    BackupBlockJob *s = container_of(job, BackupBlockJob, common);
> +    if (s->sync_bitmap) {
> +        backup_handle_dirty_bitmap(s, -1);
> +    }
> +}
> +

If you're going to check for sync_bitmap out here in these two functions
instead of inside backup_handle_dirty_bitmap, add an assertion into the
called function that job->sync_bitmap *will* be present.

>  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,
> +    .txn_commit     = backup_txn_commit,
> +    .txn_abort      = backup_txn_abort,
>  };
>  
>  static BlockErrorAction backup_error_action(BackupBlockJob *job,
> @@ -444,7 +462,7 @@ static void coroutine_fn backup_run(void *opaque)
>      qemu_co_rwlock_wrlock(&job->flush_rwlock);
>      qemu_co_rwlock_unlock(&job->flush_rwlock);
>  
> -    if (job->sync_bitmap) {
> +    if (!job->common.txn && job->sync_bitmap) {
>          backup_handle_dirty_bitmap(job, ret);
>      }

Can we add a little call to blockjobs, like:

bool block_cleanup_needed(BlockJob *job)
{
    return job->common.txn == NULL;
}

To make this status check look less magical?
Then, if the sync bitmap check is pushed into backup_handle, you can
just simply say:

if (blockjob_cleanup_needed(job)) {
    backup_handle_dirty_bitmap(job, ret);
}

which IMO is nicer.

>      hbitmap_free(job->bitmap);
> @@ -463,7 +481,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 +563,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);

OK, so all backup jobs will participate in the transaction -- but they
can control this based on whether or not they pass forward the txn
parameter.

>      qemu_coroutine_enter(job->common.co, job);
>      return;
>  
> diff --git a/blockdev.c b/blockdev.c
> index 116e144..30ea52d 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1586,12 +1586,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);
> @@ -1609,15 +1622,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;
> @@ -1654,12 +1672,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);
> @@ -1688,12 +1716,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;
> @@ -2578,15 +2611,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;
> @@ -2703,7 +2738,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);
> @@ -2714,19 +2749,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;
> @@ -2764,7 +2824,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);
> @@ -2773,6 +2833,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);
> +}
> +
>  #define DEFAULT_MIRROR_BUF_SIZE   (10 << 20)
>  
>  void qmp_drive_mirror(const char *device, const char *target,
> diff --git a/hmp.c b/hmp.c
> index dcc66f1..f492965 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 6eb22c7..5382422 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -642,6 +642,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.
> @@ -652,7 +653,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..d5e33fd 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.
> +#
>  # 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 +751,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 +776,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.
> +#
>  # 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 +791,8 @@
>              'sync': 'MirrorSyncMode',
>              '*speed': 'int',
>              '*on-source-error': 'BlockdevOnError',
> -            '*on-target-error': 'BlockdevOnError' } }
> +            '*on-target-error': 'BlockdevOnError',
> +            '*transactional-cancel': 'bool' } }
>  
>  ##
>  # @blockdev-snapshot-sync
> 

Thinking out loud:

I am /wondering/ if we shouldn't add "transactional-cancel" as an option
to /transaction itself/...

It seems slightly awkward to have transactional commands affixed to the
QMP commands, where one might want the option to apply to all actions.

Specifying the option for EACH action also seems slightly awkward, but
it does give you a very precise control.

I blathered to Eric Blake a little bit, and I was wondering if we
couldn't have "optional" and "mandatory" default-options for
transactions, where:

(A) Any mandatory transaction-wide options would propagate to all
actions below, which *must* support that option.
(B) Any optional transaction-wide options would propagate to all actions
below, but the action itself doesn't necessarily have to support it.
(C) The set of options the QMP commands take could be dictated by a base
structure and a derived child structure that had the optional
transaction arguments, for instance.

That way, you could:

- Specify a preference exactly once ("I want all transactions to fail
together, if possible.")
- But override it for specific actions, if desired, and
- Not have to add in explicit support for those options right away
thanks to an optional-obey mechanism.

Of course, this is a bit of an undertaking in the QAPI design arena and
perhaps it's not worth it if we don't expect to extend the transactional
mechanisms much in the future. Worth asking Markus and Eric, though,
before we add transactionally related parameters directly into specific
commands.

We might want to devise a more general purpose solution first.

--js