Re: [Qemu-devel] [PATCH v10 12/14] block: add transactional properties

[John Snow <jsnow@redhat.com>]

On 11/05/2015 05:47 AM, Stefan Hajnoczi wrote:
> On Tue, Nov 03, 2015 at 12:27:19PM -0500, John Snow wrote:
>>
>>
>> On 11/03/2015 10:17 AM, Stefan Hajnoczi wrote:
>>> On Fri, Oct 23, 2015 at 07:56:50PM -0400, John Snow wrote:
>>>> @@ -1732,6 +1757,10 @@ static void block_dirty_bitmap_add_prepare(BlkActionState *common,
>>>>      BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
>>>>                                               common, common);
>>>>  
>>>> +    if (action_check_cancel_mode(common, errp) < 0) {
>>>> +        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,
>>>> @@ -1767,6 +1796,10 @@ static void block_dirty_bitmap_clear_prepare(BlkActionState *common,
>>>>                                               common, common);
>>>>      BlockDirtyBitmap *action;
>>>>  
>>>> +    if (action_check_cancel_mode(common, errp) < 0) {
>>>> +        return;
>>>> +    }
>>>> +
>>>>      action = common->action->block_dirty_bitmap_clear;
>>>>      state->bitmap = block_dirty_bitmap_lookup(action->node,
>>>>                                                action->name,
>>>
>>> Why do the bitmap add/clear actions not support err-cancel=all?
>>>
>>> I understand why other block jobs don't support it, but it's not clear
>>> why these non-block job actions cannot.
>>>
>>
>> Because they don't have a callback to invoke if the rest of the job fails.
>>
>> I could create a BlockJob for them complete with a callback to invoke,
>> but basically it's just because there's no interface to unwind them, or
>> an interface to join them with the transaction.
>>
>> They're small, synchronous non-job actions. Which makes them weird.
> 
> Funny, we've been looking at the same picture while seeing different
> things:
> https://en.wikipedia.org/wiki/Rabbit%E2%80%93duck_illusion
> 
> I think I understand your idea: the transaction should include both
> immediate actions as well as block jobs.
> 
> My mental model was different: immediate actions commit/abort along with
> the 'transaction' command.  Block jobs are separate and complete/cancel
> together in a group.
> 
> In practice I think the two end up being similar because we won't be
> able to implement immediate action commit/abort together with
> long-running block jobs because the immediate actions rely on
> quiescing/pausing the guest for atomic commit/abort.
> 
> So with your mental model the QMP client has to submit 2 'transaction'
> commands: 1 for the immediate actions, 1 for the block jobs.
> 
> In my mental model the QMP client submits 1 command but the immediate
> actions and block jobs are two separate transaction scopes.  This means
> if the block jobs fail, the client needs to be aware of the immediate
> actions that have committed.  Because of this, it becomes just as much
> client effort as submitting two separate 'transaction' commands in your
> model.
> 
> Can anyone see a practical difference?  I think I'm happy with John's
> model.
> 
> Stefan
> 

We discussed this off-list, but for the sake of the archive:

== How it is now ==

Currently, transactions have two implicit phases: the first is the
synchronous phase. If this phase completes successfully, we consider the
transaction a success. The second phase is the asynchronous phase where
jobs launched by the synchronous phase run to completion.

all synchronous commands must complete for the transaction to "succeed."
There are currently (pre-patch) no guarantees about asynchronous command
completion. As long as all synchronous actions complete, asynchronous
actions are free to succeed or fail individually.

== My Model ==

The current behavior is my "err-cancel = none" scenario: we offer no
guarantee about the success or failure of the transaction as a whole
after the synchronous portion has completed.

What I was proposing is "err-cancel = all," which to me means that _ALL_
commands in this transaction are to succeed (synchronous or not) before
_any_ actions are irrevocably committed. This means that for a
hypothetical mixed synchronous-asynchronous transaction, that even after
the transaction succeeded (it passed the synchronous phase), if an
asynchronous action later fails, all actions both synchronous and non
are rolled-back -- a kind of retroactive failure of the transaction.
This is clearly not possible in all cases, so commands that cannot
support these semantics will refuse "err-cancel = all" during the
synchronous phase.

In practice, only asynchronous actions can tolerate these semantics, but
from a user perspective, it's clear that any transaction successfully
launched with "err-cancel = all" applies to *all* actions, regardless.

== Stefan's Model ==

Stefan's model was to imply that the "err-cancel" parameter applied only
to the *asynchronous* phase, because the synchronous phase has already
reported back success to the user as a return from the qmp-transaction
command. This would mean that to Stefan, "err-cancel = all" was implying
that only the asynchronous actions had to participate in the "all or
none" behavior of the transaction -- synchronous portions were exempt.

== Equivalence ==

Both models wind up being equivalent:

In Stefan's model, you need no foreknowledge of which actions are
synchronous or not. Upon failure during the asynchronous phase you will
need to understand which actions rolled back and which ones didn't, however.

In my model, you need foreknowledge of which actions are synchronous and
which ones are not, because synchronous actions will refuse the
"err-cancel = all" parameter. There is no sifting through failure states
when the command fails.

It's mostly a matter of when you need to know the difference between the
two classes of actions. In one model, it's before. In the other, it's
after a failure.

My model also allows for an emulation of Stefan's model, using the
hypothetical "err-cancel = jobs-only" mode, which would only enforce the
transaction semantics in the asynchronous phase.


For this reason, I think the two approaches to thinking about the
problem wind up having the same effect. I would perhaps argue that my
model is more explicit -- but I'm biased. I wrote it :)

--js