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

[John Snow]

On 10/20/2015 03:26 AM, Fam Zheng wrote:
> 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".
> 

Sounds like (1). I am not particularly fond of changing behavior based
on the /type/ of action, silently without the user's knowledge.
Currently there's no way to introspect what "kind" of action each action
is, so I'd rather not start playing those kind of games.

I'd rather be explicit and fail the command, or...

Something like:

sync-cancel: {none, jobs, all}

where "all" would fail if you tried to use a command that didn't support
it, "jobs" would apply it only to jobs (a weak preference), and "none"
is the current behavior (things fail or complete independent of each other.)

We'd still need a way to introspect which actions were asynchronous jobs
and which ones were synchronous tasks.

Still, I like the idea of transactional properties as I guess everyone
can tell by now...

>>
>> 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)
> 
> Is this line too long?
> 

Probably. It's just a quick POC RFC ...

>> +{
>> +    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.
> 

Yes.

>> +                     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?
> 

Eric Blake is developing a new "box" property of QMP commands such that
any arguments are put into a structure and passed as a box instead of
top-level commands.

My intent here is just to pass that box forward without (necessarily)
needing knowledge of what it contains.

Thinking to the future, I was envisioning this as not just a property of
the BlockJobTxn, but a property of the *transaction itself*, which is
why it's a separate argument here -- these are not properties of the
BlockJobTxn, they're arguments/properties of the Transaction.

--js

>>          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
>