Re: [Qemu-block] [Qemu-devel] [RFC] transactions: add transaction-wide property

[John Snow]

Ping -- any consensus on how we should implement the "do-or-die"
argument for transactions that start block jobs? :)

This patch may look a little hokey in how it boxes arguments, but I can
re-do it on top of Eric Blake's very official way of boxing arguments,
when the QAPI dust settles.

--js

On 09/24/2015 05:40 PM, John Snow wrote:
> This replaces the per-action property as in Fam's series.
> Instead, we have a transaction-wide property that is shared
> with each action.
> 
> At the action level, if a property supplied transaction-wide
> is disagreeable, we return error and the transaction is aborted.
> 
> RFC:
> 
> Where this makes sense: Any transactional actions that aren't
> prepared to accept this new paradigm of transactional behavior
> can error_setg and return.
> 
> Where this may not make sense: consider the transactions which
> do not use BlockJobs to perform their actions, i.e. they are
> synchronous during the transactional phase. Because they either
> fail or succeed so early, we might say that of course they can
> support this property ...
> 
> ...however, consider the case where we create a new bitmap and
> perform a full backup, using allow_partial=false. If the backup
> fails, we might well expect the bitmap to be deleted because a
> member of the transaction ultimately/eventually failed. However,
> the bitmap creation was not undone because it does not have a
> pending/delayed abort/commit action -- those are only for jobs
> in this implementation.
> 
> How do we fix this?
> 
> (1) We just say "No, you can't use the new block job transaction
>     completion mechanic in conjunction with these commands,"
> 
> (2) We make Bitmap creation/resetting small, synchronous blockjobs
>     that can join the BlockJobTxn
> 
> Signed-off-by: John Snow <address@hidden>
> ---
>  blockdev.c             | 87 
> ++++++++++++++++++++++++++++++++++++--------------
>  blockjob.c             |  2 +-
>  qapi-schema.json       | 15 +++++++--
>  qapi/block-core.json   | 26 ---------------
>  qmp-commands.hx        |  2 +-
>  tests/qemu-iotests/124 | 12 +++----
>  6 files changed, 83 insertions(+), 61 deletions(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index 45a9fe7..02b1a83 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1061,7 +1061,7 @@ static void blockdev_do_action(int kind, void *data, 
> Error **errp)
>      action.data = data;
>      list.value = &action;
>      list.next = NULL;
> -    qmp_transaction(&list, errp);
> +    qmp_transaction(&list, false, NULL, errp);
>  }
>  
>  void qmp_blockdev_snapshot_sync(bool has_device, const char *device,
> @@ -1286,6 +1286,7 @@ struct BlkActionState {
>      TransactionAction *action;
>      const BlkActionOps *ops;
>      BlockJobTxn *block_job_txn;
> +    TransactionProperties *txn_props;
>      QSIMPLEQ_ENTRY(BlkActionState) entry;
>  };
>  
> @@ -1322,6 +1323,12 @@ static void internal_snapshot_prepare(BlkActionState 
> *common,
>      name = internal->name;
>  
>      /* 2. check for validation */
> +    if (!common->txn_props->allow_partial) {
> +        error_setg(errp,
> +                   "internal_snapshot does not support allow_partial = 
> false");
> +        return;
> +    }
> +
>      blk = blk_by_name(device);
>      if (!blk) {
>          error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
> @@ -1473,6 +1480,12 @@ static void external_snapshot_prepare(BlkActionState 
> *common,
>      }
>  
>      /* start processing */
> +    if (!common->txn_props->allow_partial) {
> +        error_setg(errp,
> +                   "external_snapshot does not support allow_partial = 
> false");
> +        return;
> +    }
> +
>      state->old_bs = bdrv_lookup_bs(has_device ? device : NULL,
>                                     has_node_name ? node_name : NULL,
>                                     &local_err);
> @@ -1603,14 +1616,11 @@ static void drive_backup_prepare(BlkActionState 
> *common, Error **errp)
>      DriveBackupState *state = DO_UPCAST(DriveBackupState, common, common);
>      BlockDriverState *bs;
>      BlockBackend *blk;
> -    DriveBackupTxn *backup_txn;
>      DriveBackup *backup;
> -    BlockJobTxn *txn = NULL;
>      Error *local_err = NULL;
>  
>      assert(common->action->kind == TRANSACTION_ACTION_KIND_DRIVE_BACKUP);
> -    backup_txn = common->action->drive_backup;
> -    backup = backup_txn->base;
> +    backup = common->action->drive_backup->base;
>  
>      blk = blk_by_name(backup->device);
>      if (!blk) {
> @@ -1624,11 +1634,6 @@ static void drive_backup_prepare(BlkActionState 
> *common, Error **errp)
>      state->aio_context = bdrv_get_aio_context(bs);
>      aio_context_acquire(state->aio_context);
>  
> -    if (backup_txn->has_transactional_cancel &&
> -        backup_txn->transactional_cancel) {
> -        txn = common->block_job_txn;
> -    }
> -
>      do_drive_backup(backup->device, backup->target,
>                      backup->has_format, backup->format,
>                      backup->sync,
> @@ -1637,7 +1642,7 @@ static void drive_backup_prepare(BlkActionState 
> *common, Error **errp)
>                      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);
> +                    common->block_job_txn, &local_err);
>      if (local_err) {
>          error_propagate(errp, local_err);
>          return;
> @@ -1686,16 +1691,13 @@ static void do_blockdev_backup(const char *device, 
> const char *target,
>  static void blockdev_backup_prepare(BlkActionState *common, Error **errp)
>  {
>      BlockdevBackupState *state = DO_UPCAST(BlockdevBackupState, common, 
> common);
> -    BlockdevBackupTxn *backup_txn;
>      BlockdevBackup *backup;
>      BlockDriverState *bs, *target;
>      BlockBackend *blk;
> -    BlockJobTxn *txn = NULL;
>      Error *local_err = NULL;
>  
>      assert(common->action->kind == TRANSACTION_ACTION_KIND_BLOCKDEV_BACKUP);
> -    backup_txn = common->action->blockdev_backup;
> -    backup = backup_txn->base;
> +    backup = common->action->blockdev_backup->base;
>  
>      blk = blk_by_name(backup->device);
>      if (!blk) {
> @@ -1720,17 +1722,12 @@ static void blockdev_backup_prepare(BlkActionState 
> *common, Error **errp)
>      }
>      aio_context_acquire(state->aio_context);
>  
> -    if (backup_txn->has_transactional_cancel &&
> -        backup_txn->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);
> +                       common->block_job_txn, &local_err);
>      if (local_err) {
>          error_propagate(errp, local_err);
>          return;
> @@ -1777,6 +1774,13 @@ static void 
> block_dirty_bitmap_add_prepare(BlkActionState *common,
>      BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
>                                               common, common);
>  
> +    if (!common->txn_props->allow_partial) {
> +        error_setg(errp,
> +                   "block_dirty_bitmap_add does not support "
> +                   "allow_partial = false");
> +        return;
> +    }
> +
>      action = common->action->block_dirty_bitmap_add;
>      /* AIO context taken and released within qmp_block_dirty_bitmap_add */
>      qmp_block_dirty_bitmap_add(action->node, action->name,
> @@ -1812,6 +1816,13 @@ static void 
> block_dirty_bitmap_clear_prepare(BlkActionState *common,
>                                               common, common);
>      BlockDirtyBitmap *action;
>  
> +    if (!common->txn_props->allow_partial) {
> +        error_setg(errp,
> +                   "block_dirty_bitmap_clear does not support "
> +                   "allow_partial = false");
> +        return;
> +    }
> +
>      action = common->action->block_dirty_bitmap_clear;
>      state->bitmap = block_dirty_bitmap_lookup(action->node,
>                                                action->name,
> @@ -1914,21 +1925,45 @@ static const BlkActionOps actions[] = {
>      }
>  };
>  
> +/**
> + * Allocate a TransactionProperties structure if necessary, and fill
> + * that structure with desired defaults if they are unset.
> + */
> +static TransactionProperties 
> *get_transaction_properties(TransactionProperties *props)
> +{
> +    if (!props) {
> +        props = g_new0(TransactionProperties, 1);
> +    }
> +
> +    if (!props->has_allow_partial) {
> +        props->allow_partial = true;
> +    }
> +
> +    return props;
> +}
> +
>  /*
>   * 'Atomic' group operations.  The operations are performed as a set, and if
>   * any fail then we roll back all operations in the group.
>   */
> -void qmp_transaction(TransactionActionList *dev_list, Error **errp)
> +void qmp_transaction(TransactionActionList *dev_list,
> +                     bool hasTransactionProperties,
> +                     struct TransactionProperties *props,
> +                     Error **errp)
>  {
>      TransactionActionList *dev_entry = dev_list;
> -    BlockJobTxn *block_job_txn;
> +    BlockJobTxn *block_job_txn = NULL;
>      BlkActionState *state, *next;
>      Error *local_err = NULL;
>  
>      QSIMPLEQ_HEAD(snap_bdrv_states, BlkActionState) snap_bdrv_states;
>      QSIMPLEQ_INIT(&snap_bdrv_states);
>  
> -    block_job_txn = block_job_txn_new();
> +    /* Does this transaction get *canceled* as a group on failure? */
> +    props = get_transaction_properties(props);
> +    if (props->allow_partial == false) {
> +        block_job_txn = block_job_txn_new();
> +    }
>  
>      /* drain all i/o before any operations */
>      bdrv_drain_all();
> @@ -1950,6 +1985,7 @@ void qmp_transaction(TransactionActionList *dev_list, 
> Error **errp)
>          state->ops = ops;
>          state->action = dev_info;
>          state->block_job_txn = block_job_txn;
> +        state->txn_props = props;
>          QSIMPLEQ_INSERT_TAIL(&snap_bdrv_states, state, entry);
>  
>          state->ops->prepare(state, &local_err);
> @@ -1982,6 +2018,9 @@ exit:
>          }
>          g_free(state);
>      }
> +    if (!hasTransactionProperties) {
> +        g_free(props);
> +    }
>      block_job_txn_unref(block_job_txn);
>  }
>  
> diff --git a/blockjob.c b/blockjob.c
> index 91e8d3c..00146ff 100644
> --- a/blockjob.c
> +++ b/blockjob.c
> @@ -512,7 +512,7 @@ static void block_job_txn_ref(BlockJobTxn *txn)
>  
>  void block_job_txn_unref(BlockJobTxn *txn)
>  {
> -    if (--txn->refcnt == 0) {
> +    if (txn && --txn->refcnt == 0) {
>          g_free(txn);
>      }
>  }
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 8769099..0f28311 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -1505,14 +1505,20 @@
>  { 'union': 'TransactionAction',
>    'data': {
>         'blockdev-snapshot-sync': 'BlockdevSnapshot',
> -       'drive-backup': 'DriveBackupTxn',
> -       'blockdev-backup': 'BlockdevBackupTxn',
> +       'drive-backup': 'DriveBackup',
> +       'blockdev-backup': 'BlockdevBackup',
>         'abort': 'Abort',
>         'blockdev-snapshot-internal-sync': 'BlockdevSnapshotInternal',
>         'block-dirty-bitmap-add': 'BlockDirtyBitmapAdd',
>         'block-dirty-bitmap-clear': 'BlockDirtyBitmap'
>     } }
>  
> +{ 'struct': 'TransactionProperties',
> +  'data': {
> +      '*allow-partial': 'bool'
> +  }
> +}
> +
>  ##
>  # @transaction
>  #
> @@ -1533,7 +1539,10 @@
>  # Since 1.1
>  ##
>  { 'command': 'transaction',
> -  'data': { 'actions': [ 'TransactionAction' ] } }
> +  'data': { 'actions': [ 'TransactionAction' ],
> +            '*properties': 'TransactionProperties'
> +          }
> +}
>  
>  ##
>  # @human-monitor-command:
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 24db5c2..83742ab 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -749,19 +749,6 @@
>              '*on-target-error': 'BlockdevOnError' } }
>  
>  ##
> -# @DriveBackupTxn
> -#
> -# @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. 
> Default
> -#                        is false.
> -#
> -# Since: 2.5
> -##
> -{ 'struct': 'DriveBackupTxn', 'base': 'DriveBackup',
> -  'data': {'*transactional-cancel': 'bool' } }
> -
> -##
>  # @BlockdevBackup
>  #
>  # @device: the name of the device which should be copied.
> @@ -797,19 +784,6 @@
>              '*on-target-error': 'BlockdevOnError' } }
>  
>  ##
> -# @BlockdevBackupTxn
> -#
> -# @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. 
> Default
> -#                        is false
> -#
> -# Since: 2.5
> -##
> -{ 'struct': 'BlockdevBackupTxn', 'base': 'BlockdevBackup',
> -  'data': {'*transactional-cancel': 'bool' } }
> -
> -##
>  # @blockdev-snapshot-sync
>  #
>  # Generates a synchronous snapshot of a block device.
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index c388274..7c1fed9 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -1262,7 +1262,7 @@ EQMP
>      },
>      {
>          .name       = "transaction",
> -        .args_type  = "actions:q",
> +        .args_type  = "actions:q,properties:q?",
>          .mhandler.cmd_new = qmp_marshal_transaction,
>      },
>  
> diff --git a/tests/qemu-iotests/124 b/tests/qemu-iotests/124
> index 33d00ef..028b346 100644
> --- a/tests/qemu-iotests/124
> +++ b/tests/qemu-iotests/124
> @@ -456,14 +456,13 @@ class TestIncrementalBackup(iotests.QMPTestCase):
>          transaction = [
>              transaction_drive_backup(drive0['id'], target0, 
> sync='incremental',
>                                       format=drive0['fmt'], mode='existing',
> -                                     bitmap=dr0bm0.name,
> -                                     transactional_cancel=True),
> +                                     bitmap=dr0bm0.name),
>              transaction_drive_backup(drive1['id'], target1, 
> sync='incremental',
>                                       format=drive1['fmt'], mode='existing',
> -                                     bitmap=dr1bm0.name,
> -                                     transactional_cancel=True),
> +                                     bitmap=dr1bm0.name)
>          ]
> -        result = self.vm.qmp('transaction', actions=transaction)
> +        result = self.vm.qmp('transaction', actions=transaction,
> +                             properties={'allow-partial':False} )
>          self.assert_qmp(result, 'return', {})
>  
>          # Observe that drive0's backup is cancelled and drive1 completes with
> @@ -485,7 +484,8 @@ class TestIncrementalBackup(iotests.QMPTestCase):
>          target1 = self.prepare_backup(dr1bm0)
>  
>          # Re-run the exact same transaction.
> -        result = self.vm.qmp('transaction', actions=transaction)
> +        result = self.vm.qmp('transaction', actions=transaction,
> +                             properties={'allow-partial':False})
>          self.assert_qmp(result, 'return', {})
>  
>          # Both should complete successfully this time.
>