On Thu, 09/24 17:40, 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
We could categorize commands as synchronous ones and long running ones, and
make it explicit that only long running jobs are affected by "allow_partial".
>
> Signed-off-by: John Snow <js...@redhat.com>
> ---
> 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)
Is this line too long?
> +{
> + 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,
Better as has_transaction_properties.
> + 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;
Is it better to make @props a member of BlockJobTxn instead? Or is there a new
way in QAPI to do this?
> 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);
> }
>
Fam
