From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34290) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZlgIy-0002I7-CF for qemu-devel@nongnu.org; Mon, 12 Oct 2015 12:50:38 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZlgIw-000422-8z for qemu-devel@nongnu.org; Mon, 12 Oct 2015 12:50:32 -0400 References: <1443130823-10723-1-git-send-email-jsnow@redhat.com> From: John Snow Message-ID: <561BE4CC.9070705@redhat.com> Date: Mon, 12 Oct 2015 12:50:20 -0400 MIME-Version: 1.0 In-Reply-To: <1443130823-10723-1-git-send-email-jsnow@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [RFC] transactions: add transaction-wide property List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-block@nongnu.org Cc: famz@redhat.com, armbru@redhat.com, qemu-devel@nongnu.org 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 > --- > 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. >