A little bit of cross-talk with my "state of the union" reply and this
review from Eric.
Sorry, everyone!
On 10/20/2015 04:12 PM, Eric Blake wrote:
> On 09/24/2015 03: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.
>
> The classic example is the 'Abort' transaction, which always fails. Or
> put another way, if you run a transaction that includes both the
> creation of a new bitmap, and the abort action, what does the abort do
> to the bitmap.
>
>>
>> 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
>
> I'm not sure I have an opinion on this one yet.
>
As stated in my other mail, I'm leaning towards (1) with a design that
allows us to switch to (2) per-action as we feel like it.
>>
>> 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(-)
>>
>> @@ -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");
>
> Should the error message say 'allow-partial' to match the QMP?
>
It sure should.
>>
>> +/**
>> + * 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;
>> +}
>
> If *props is NULL on entry, then allow_partial is true on exit but
> has_allow_partial is still false. I guess that means we're relying on
> the rest of the code ignoring has_allow_partial because they used this
> function to set defaults.
>
> Yeah, having default support built into qapi would make this nicer. I
> don't know if we'll have enough time for qapi defaults to make it in 2.5
> (the queue is already quite big for merely getting boxed qmp callback
> functions in place). But that's all internal, and won't matter if it
> doesn't get added until 2.6, without affecting what behavior the
> external user sees.
>
Yes, this is goofy. I will change it to a "false by default" phrasing
for the property to avoid this.
>> +
>> /*
>> * '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,
>
> Name this has_props, to be more reminiscent of other qapi naming.
>
Yes.
>> + struct TransactionProperties *props,
>> + Error **errp)
>> {
>
>> - 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();
>> + }
>>
>
>> }
>> + if (!hasTransactionProperties) {
>> + g_free(props);
>
> qapi_free_TransactionProperties(props) is probably better.
>
>
>> +++ 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',
>
> I like that we no longer need the special subclasses just for transactions.
>
>> 'abort': 'Abort',
>> 'blockdev-snapshot-internal-sync': 'BlockdevSnapshotInternal',
>> 'block-dirty-bitmap-add': 'BlockDirtyBitmapAdd',
>> 'block-dirty-bitmap-clear': 'BlockDirtyBitmap'
>> } }
>>
>> +{ 'struct': 'TransactionProperties',
>> + 'data': {
>> + '*allow-partial': 'bool'
>> + }
>> +}
>
> Missing documentation, but of course this is RFC. Overall, I like the
> idea, and think it's on the right track.
>
>> @@ -1533,7 +1539,10 @@
>> # Since 1.1
>> ##
>> { 'command': 'transaction',
>> - 'data': { 'actions': [ 'TransactionAction' ] } }
>> + 'data': { 'actions': [ 'TransactionAction' ],
>> + '*properties': 'TransactionProperties'
>> + }
>> +}
>
> I guess part of the RFC is to figure out the QMP representation we want.
> As written, this requires:
>
> { "command":"transaction", "arguments":{
> "actions":[ {...},{...} ],
> "properties":{"allow-partial":true} } }
>
> while on the C side, new properties don't change the signature of
> qmp_transaction() any further (because it is all buried behind the
> has_props/props parameters). But where we are headed with my qapi
> patches that ARE posted is the ability to declare a boxed command:
>
> { 'struct': 'TransactionSet',
> 'data': { 'actions': [ 'TransactionAction' ],
> '*allow-partial': 'bool' } }
> { 'command': 'transaction', 'boxed': true, 'data': 'TransactionSet' }
>
> Which gives a QMP representation with fewer {}:
>
> { "command":"transaction", "arguments":{
> "actions":[ {...},{...} ],
> "allow-partial": true } }
>
> and a C signature of:
>
> void qmp_transaction(TransactionSet *txn, Error **errp)
>
This is definitely nicer, from an end-user perspective. For a machine,
maybe it doesn't matter so much.
> so that you then access txn->actions and txn->allow_partial, rather than
> having a different parameter for every C struct member, and where the
> signature is now stable when you add further qapi extensions.
>
> It all boils down to what is easier to use, and whether we want the
> extra {} in the QMP representation. I'm not too fussed either way.
>
Nor I.
I think in the case of Transactions, I liked the idea of the properties
being a very clearly optional structure that was not merged top-level
with the rest of the command. I kind of enjoyed that "boxed" look from
the QAPI view, as well as what that does for the C bits.
Transactions have a very interesting structure that's somewhat disparate
from other QMP commands, and this structure here (to me) helps highlight
the structure.
Though I'm not really married to it.
Thanks for the look-see.
--js
