block: incremental backup transactions using BlockJobTxn

These structures are misnomers, somewhat.

(1) BlockTransactionState is not state for a transaction, but is rather state for a single transaction action. Rename it "BlkActionState" to be more accurate.

(2) The BdrvActionOps describes operations for the BlkActionState, above. This name might imply a 'BdrvAction' or a 'BdrvActionState', which there isn't.
Rename this to 'BlkActionOps' to match 'BlkActionState'.

Lastly, update the surrounding in-line documentation and comments to reflect the current nature of how Transactions operate.

This patch changes only comments and names, and should not affect behavior in any way.

Signed-off-by John Snow <[email protected]>

Reviewed-by Max Reitz <[email protected]>

Reviewed-by Stefan Hajnoczi <[email protected]>

Signed-off-by Stefan Hajnoczi <[email protected]>

Reviewed-by Fam Zheng <[email protected]>

Signed-off-by Fam Zheng <[email protected]>

---
 blockdev.c | 116 ++++++++++++++++++++++++++++++++++---------------------------
 1 file changed, 65 insertions(+), 51 deletions(-)

Quoted 23 lines · John Snow

These structures are misnomers, somewhat.

(1) BlockTransactionState is not state for a transaction, but is rather state for a single transaction action. Rename it "BlkActionState" to be more accurate.

(2) The BdrvActionOps describes operations for the BlkActionState, above. This name might imply a 'BdrvAction' or a 'BdrvActionState', which there isn't.
Rename this to 'BlkActionOps' to match 'BlkActionState'.

Lastly, update the surrounding in-line documentation and comments to reflect the current nature of how Transactions operate.

This patch changes only comments and names, and should not affect behavior in any way.

Signed-off-by John Snow <[email protected]>

Reviewed-by Max Reitz <[email protected]>

Reviewed-by Stefan Hajnoczi <[email protected]>

Signed-off-by Stefan Hajnoczi <[email protected]>

Reviewed-by Fam Zheng <[email protected]>

Signed-off-by Fam Zheng <[email protected]>

Quoted 4 lines

---
 blockdev.c | 116 ++++++++++++++++++++++++++++++++++---------------------------
 1 file changed, 65 insertions(+), 51 deletions(-)

just a heads up - this patch fails to apply on qemu master, so a rebase is probably in order (this series is based on qemu master, right?)

(I'll review from your github branch for now)

+    block_job_ref(job);
     finish(job, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
         return -EBUSY;

blockdev.c +4 -0

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

Quoted 12 lines · John Snow

blockdev.c +4 -0

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

blockdev.c static void block_dirty_bitmap_clear_prepare(BlkActionState *common, · line 1805

Quoted 10 lines

blockdev.c +4 -0

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

Quoted 26 lines · Stefan Hajnoczi

Quoted 12 lines · John Snow

blockdev.c +4 -0

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

blockdev.c static void block_dirty_bitmap_clear_prepare(BlkActionState *common, · line 1805

Quoted 10 lines

blockdev.c +4 -0

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

Quoted 3 lines

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.

Quoted 33 lines · John Snow

Quoted 26 lines · Stefan Hajnoczi

Quoted 12 lines · John Snow

blockdev.c +4 -0

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

blockdev.c static void block_dirty_bitmap_clear_prepare(BlkActionState *common, · line 1805

Quoted 10 lines

blockdev.c +4 -0

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

Quoted 3 lines

I understand why other block jobs don't support it, but it's not clear why these non-block job actions cannot.

Quoted 7 lines

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

Quoted 68 lines · Stefan Hajnoczi

Quoted 33 lines · John Snow

Quoted 26 lines · Stefan Hajnoczi

Quoted 12 lines · John Snow

blockdev.c +4 -0

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

blockdev.c static void block_dirty_bitmap_clear_prepare(BlkActionState *common, · line 1805

Quoted 10 lines

blockdev.c +4 -0

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

Quoted 3 lines

I understand why other block jobs don't support it, but it's not clear why these non-block job actions cannot.

Quoted 7 lines

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.

Quoted 5 lines

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

Quoted 136 lines · John Snow

Quoted 68 lines · Stefan Hajnoczi

Quoted 33 lines · John Snow

Quoted 26 lines · Stefan Hajnoczi

Quoted 12 lines · John Snow

blockdev.c +4 -0

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

blockdev.c static void block_dirty_bitmap_clear_prepare(BlkActionState *common, · line 1805

Quoted 10 lines

blockdev.c +4 -0

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

Quoted 3 lines

I understand why other block jobs don't support it, but it's not clear why these non-block job actions cannot.

Quoted 7 lines

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.

Quoted 5 lines

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.

Quoted 8 lines

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

I haven't followed this topic, and my opinions are therefore quite uninformed. Here goes anway.

In John's model, you need to know whether an action is synchronous. If you get it wrong, your attempt to err-cancel=all will fail. Sounds like a nice early failure to me.

In Stefan's model, you "need to understand which actions rolled back" to make sense of a failure. How? Are exactly the asynchronous ones rolled back? What happens if you get it wrong?

Quoted 68 lines · John Snow

Quoted 33 lines · Stefan Hajnoczi

Quoted 26 lines · John Snow

Quoted 12 lines

blockdev.c +4 -0

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

blockdev.c static void block_dirty_bitmap_clear_prepare(BlkActionState *common, · line 1805

Quoted 10 lines

blockdev.c +4 -0

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

Quoted 3 lines

I understand why other block jobs don't support it, but it's not clear why these non-block job actions cannot.

Quoted 7 lines

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.

Quoted 5 lines

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.

Am 05.11.2015 um 19:52 hat John Snow geschrieben:

Quoted 86 lines · Stefan Hajnoczi

Quoted 68 lines · John Snow

Quoted 33 lines · Stefan Hajnoczi

Quoted 26 lines · John Snow

Quoted 12 lines

blockdev.c +4 -0

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

blockdev.c static void block_dirty_bitmap_clear_prepare(BlkActionState *common, · line 1805

Quoted 10 lines

blockdev.c +4 -0

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

Quoted 3 lines

I understand why other block jobs don't support it, but it's not clear why these non-block job actions cannot.

Quoted 7 lines

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.

Quoted 5 lines

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.

Quoted 4 lines

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.

You're overcomplicating this. All actions are currently synchronous and what you consider asynchronous transaction actions aren't actually part of the transaction at all. The action is "start block job X", not "run block job X".

Quoted 6 lines

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

Quoted 10 lines

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.

Is this possible in any case? You're losing transaction semantics the lastest when you drop the BQL that the monitor holds. At least atomicity and isolation aren't given any more.

You can try to undo some parts of what you did later one, but if any involved BDS was used in the meantime by anything other than the block job, you don't have transactional behaviour any more.

And isn't the management tool perfectly capable of cleaning up all the block jobs without magic happening in qemu if one of them fails? Do we actually need atomic failure later on? And if so, do we need atomic failure only of block jobs started in the same transaction? Why?

Am 05.11.2015 um 19:52 hat John Snow geschrieben:

Quoted 86 lines · Stefan Hajnoczi

Quoted 68 lines · John Snow

Quoted 33 lines · Stefan Hajnoczi

Quoted 26 lines · John Snow

Quoted 12 lines

blockdev.c +4 -0

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

blockdev.c static void block_dirty_bitmap_clear_prepare(BlkActionState *common, · line 1805

Quoted 10 lines

blockdev.c +4 -0

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

Quoted 3 lines

I understand why other block jobs don't support it, but it's not clear why these non-block job actions cannot.

Quoted 7 lines

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.

Quoted 5 lines

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.

Quoted 4 lines

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.

You're overcomplicating this. All actions are currently synchronous and what you consider asynchronous transaction actions aren't actually part of the transaction at all. The action is "start block job X", not "run block job X".

Quoted 6 lines

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

Quoted 10 lines

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.

Is this possible in any case? You're losing transaction semantics the lastest when you drop the BQL that the monitor holds. At least atomicity and isolation aren't given any more.

You can try to undo some parts of what you did later one, but if any involved BDS was used in the meantime by anything other than the block job, you don't have transactional behaviour any more.

And isn't the management tool perfectly capable of cleaning up all the block jobs without magic happening in qemu if one of them fails? Do we actually need atomic failure later on? And if so, do we need atomic failure only of block jobs started in the same transaction? Why?

Quoted 92 lines · Kevin Wolf

Am 05.11.2015 um 19:52 hat John Snow geschrieben:

Quoted 86 lines · Stefan Hajnoczi

Quoted 68 lines · John Snow

Quoted 33 lines · Stefan Hajnoczi

Quoted 26 lines · John Snow

Quoted 12 lines

(quoted diff) +4 -0

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

Review static void block_dirty_bitmap_clear_prepare(BlkActionState *common, · line 1805

Quoted 10 lines

(quoted diff) +4 -0

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

Quoted 3 lines

I understand why other block jobs don't support it, but it's not clear why these non-block job actions cannot.

Quoted 7 lines

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.

Quoted 5 lines

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.

Quoted 4 lines

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.

Quoted 5 lines

You're overcomplicating this. All actions are currently synchronous and what you consider asynchronous transaction actions aren't actually part of the transaction at all. The action is "start block job X", not "run block job X".

"OK," except the entire goal of the series was to allow the "aren't actually part of the transaction at all" parts to live and die together based on the transaction they were launched from. This really implies they belong to a second asynchronous phase of the transaction.

Otherwise, why would totally unrelated jobs fail because a different one did?

I realize this is an /extension/ of the existing semantics vs a "fix," and part of the confusion is how I and everyone else was looking at it differently. How could this happen, though? Let's look at our transaction documentation:

"Operations that are currently supported:" [...] "- drive-backup" [...] "If there is any failure
performing any of the operations, all operations for the group are abandoned."

Where do we say "Except drive-backup, though, because technically the drive-backup action only starts the job, and we don't really consider this to be part of the transaction" ? We've never actually defined this behavior as part of our API as far as I can see.

Regardless: the net effect is still the same. We want jobs launched by transactions to (optionally!) cancel themselves on failure. The complications only arise because people want the exact semantic meaning to be precise for the QMP API.

The concept is simple, the language to describe it has been muddy.

Quoted 17 lines

Quoted 6 lines

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

Quoted 10 lines

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.

Quoted 4 lines

Is this possible in any case? You're losing transaction semantics the lastest when you drop the BQL that the monitor holds. At least atomicity and isolation aren't given any more.

It might be possible in at least *some* cases. I currently don't even attempt it, though. This is why some transaction actions just refuse this parameter if it shows up.

It'd be pretty easy to undo a bitmap-add or a bitmap-clear, as long as I gave these actions a conditional success callback.

The "undo" semantics of the jobs are somewhat different. I am not suggesting we can teleport back in time to before we tried to do the backup, but we can at least abort the backup and make it like you never issued the command -- which is important for incremental backups.

The rollback behavior for each action needs to be spelled out in our documentation ... as well as categorizing which actions complete before qmp_transactions return and which may have lingering effects (like drive-backup.)

Quoted 9 lines

You can try to undo some parts of what you did later one, but if any involved BDS was used in the meantime by anything other than the block job, you don't have transactional behaviour any more.

And isn't the management tool perfectly capable of cleaning up all the block jobs without magic happening in qemu if one of them fails? Do we actually need atomic failure later on? And if so, do we need atomic failure only of block jobs started in the same transaction? Why?

There's always a debate about who is responsible for cleaning things up on failures: QEMU or the management tool? Luckily: it's an optional parameter, so the tool can decide at run-time about what semantics it wants.

IMO: It's a little late in this series to question if we need this or not, but I'll oblige.

The original objection from Stefan to the incremental backup

https://soundcloud.com/tothejazz/gyms-flappy-the-happy-sealtransaction
semantics was that if two incremental backup jobs launched by a

transaction had only partial success, the management tool would have to take on extra state and possibly issue some corrective actions to QEMU in order to undo the damage.

QEMU, however, could just unravel the failure fairly trivially and allow the backup commands to maintain a simple binary success/failure state. This was agreed to be the better, less complicated management scenario.

In the case that incremental backups partially complete, you'll have one bitmap deleted and a different bitmap merged back onto the BDS. The management tool can at this point absolutely not delete the one incremental that got made and needs to leave it in place before re-issuing the incremental backup. It's then responsible for either squashing the two incremental backups it made, or otherwise managing the disparity in the number of actual backup files.

For QEMU's trouble, we don't delete incremental backup data until after the backup operation, so it's trivial to hold onto the data until we know everything's OK.

We decided it was nicest for QEMU to just roll back all of the jobs in this case. If the management tool disagrees, it can just roll with the original semantics.

I still believe strongly that this is the right way to go. It's flexible, allows for either party to manage the failure, the parameter is completely non-ambiguous, provides for early failure if the expected semantics are not possible, and the code is already here and mostly reviewed... except for the API names.

--js

Add both transactional properties to the QMP transactional interface, and add the BlockJobTxn that we create as a result of the err-cancel property to the BlkActionState structure.

[split up from a patch originally by Stefan and Fam. --js]

Signed-off-by Stefan Hajnoczi <[email protected]>

Signed-off-by Fam Zheng <[email protected]>

Signed-off-by John Snow <[email protected]>

Signed-off-by John Snow <[email protected]>

---
 blockdev.c       | 78 +++++++++++++++++++++++++++++++++++++++++++++++++++++---
 qapi-schema.json | 48 +++++++++++++++++++++++++++++++---
 qmp-commands.hx  |  2 +-
 3 files changed, 120 insertions(+), 8 deletions(-)

qapi-schema.json +1 -0

+##
 # @transaction
 #

Quoted 10 lines · John Snow

Add both transactional properties to the QMP transactional interface, and add the BlockJobTxn that we create as a result of the err-cancel property to the BlkActionState structure.

[split up from a patch originally by Stefan and Fam. --js]

Signed-off-by Stefan Hajnoczi <[email protected]>

Signed-off-by Fam Zheng <[email protected]>

Signed-off-by John Snow <[email protected]>

Signed-off-by John Snow <[email protected]>

Double S-o-b looks odd.

Quoted 11 lines

---
 blockdev.c       | 78 +++++++++++++++++++++++++++++++++++++++++++++++++++++---
 qapi-schema.json | 48 +++++++++++++++++++++++++++++++---
 qmp-commands.hx  |  2 +-
 3 files changed, 120 insertions(+), 8 deletions(-)

qapi-schema.json +1 -0

+##
 # @transaction
 #

qapi-schema.json line 1593

Quoted 10 lines

qapi-schema.json +4 -2

 # Executes a number of transactionable QMP commands atomically. If any
 # operation fails, then the entire set of actions will be abandoned and the
 # appropriate error returned.
 #
-#  List of:
-#  @TransactionAction: information needed for the respective operation
+# @actions: List of @TransactionAction;
+#           information needed for the respective operations.
+#
+# @properties: Optional structure of additional options to control the

Elsewhere, we've spelled it '#optional'; Marc-Andre has patches that rely on that spelling to turn it .json into documentation.

But otherwise, looks good on first glance.

Quoted 15 lines · Eric Blake

qapi-schema.json line 1593

Quoted 10 lines · John Snow

Add both transactional properties to the QMP transactional interface, and add the BlockJobTxn that we create as a result of the err-cancel property to the BlkActionState structure.

qapi-schema.json +4 -0

+# @actions: List of @TransactionAction;
+#           information needed for the respective operations.
+#
+# @properties: Optional structure of additional options to control the

Elsewhere, we've spelled it '#optional'; Marc-Andre has patches that rely on that spelling to turn it .json into documentation.

Quoted 2 lines

But otherwise, looks good on first glance.

Do you mean:

"# @properties: #optional structure of "... ?

Welcome to V10!

Where'd 8 and 9 go? Private off-list missives from Fam. Now you, I, and everyone on qemu-devel are staring at V10.

What's new in V10?

I replaced the per-action "transactional-cancel" parameter with a per-transaction paremeter named "err-cancel" which is implemented as an enum in case we want to add new behaviors in the future, such as a "jobs only" cancel mode.

For now, it's "all" or "none", and if you use it with actions that do not support the latent transactional cancel, you will receive an error for your troubles.

This version primarily changed patches 10,11 and replaced it with patches 10-12 that are cut a little differently.

This is based on top of the work by Stefan Hajnoczi and Fam Zheng.

Recap: motivation for block job transactions
--------------------------------------------
If an incremental backup block job fails then we reclaim the bitmap so the job can be retried. The problem comes when multiple jobs are started as part of a qmp 'transaction' command. We need to group these jobs in a transaction so that either all jobs complete successfully or all bitmaps are reclaimed.

Without transactions, there is a case where some jobs complete successfully and throw away their bitmaps, making it impossible to retry the backup by rerunning the command if one of the jobs fails.

How does this implementation work?
----------------------------------
These patches add a BlockJobTxn object with the following API:

  txn = block_job_txn_new();
  block_job_txn_add_job(txn, job1);
  block_job_txn_add_job(txn, job2);

The jobs either both complete successfully or they both fail/cancel. If the user cancels job1 then job2 will also be cancelled and vice versa.

Jobs objects stay alive waiting for other jobs to complete, even if the coroutines have returned. They can be cancelled by the user during this time. Job blockers are still in effect and no other block job can run on this device in the meantime (since QEMU currently only allows 1 job per device). This is the main drawback to this approach but reasonable since you probably don't want to run other jobs/operations until you're sure the backup was successful (you won't be able to retry a failed backup if there's a new job running).

[History]

v10: Replaced per-action parameter with per-transaction properties.

     Patches 10,11 were split into 10-12.

v9: this version fixes a reference count problem with job->bs,
in patch 05.

v8: Rebase on to master.
Minor fixes addressing John Snow's comments.

v7: Add Eric's rev-by in 1, 11.
Add Max's rev-by in 4, 5, 9, 10, 11.
Add John's rev-by in 5, 6, 8.
Fix wording for 6. [John]
Fix comment of block_job_txn_add_job() in 9. [Max] Remove superfluous hunks, and document default value in 11. [Eric] Update Makefile dep in 14. [Max]

________________________________________________________________________________

For convenience, this branch is available at:
https://github.com/jnsnow/qemu.git branch block-transpop
https://github.com/jnsnow/qemu/tree/block-transpop

This version is tagged block-transpop-v10:
https://github.com/jnsnow/qemu/releases/tag/block-transpop-v10

Fam Zheng (6):
backup: Extract dirty bitmap handling as a separate function
blockjob: Introduce reference count and fix reference to job->bs
blockjob: Add .commit and .abort block job actions
blockjob: Add "completed" and "ret" in BlockJob
blockjob: Simplify block_job_finish_sync
block: Add block job transactions

John Snow (7):
qapi: Add transaction support to block-dirty-bitmap operations
iotests: add transactional incremental backup test
block: rename BlkTransactionState and BdrvActionOps block/backup: Rely on commit/abort for cleanup
block: Add BlockJobTxn support to backup_run
block: add transactional properties
iotests: 124 - transactional failure test

Stefan Hajnoczi (1):
tests: add BlockJobTxn unit test

 block.c                    |  19 +-
 block/backup.c             |  50 ++++--
 block/mirror.c             |   2 +-
 blockdev.c                 | 430 ++++++++++++++++++++++++++++++++++-----------
 blockjob.c                 | 188 ++++++++++++++++----
 docs/bitmaps.md            |   6 +-
 include/block/block.h      |   2 +-
 include/block/block_int.h  |   6 +-
 include/block/blockjob.h   |  85 ++++++++-
 qapi-schema.json           |  54 +++++-
 qemu-img.c                 |   3 -
 qmp-commands.hx            |   2 +-
 tests/Makefile             |   3 +
 tests/qemu-iotests/124     | 182 ++++++++++++++++++-
 tests/qemu-iotests/124.out |   4 +-
 tests/test-blockjob-txn.c  | 244 +++++++++++++++++++++++++
 16 files changed, 1108 insertions(+), 172 deletions(-)
 create mode 100644 tests/test-blockjob-txn.c

Welcome to V10!

Where'd 8 and 9 go? Private off-list missives from Fam. Now you, I, and everyone on qemu-devel are staring at V10.

What's new in V10?

I replaced the per-action "transactional-cancel" parameter with a per-transaction paremeter named "err-cancel" which is implemented as an enum in case we want to add new behaviors in the future, such as a "jobs only" cancel mode.

For now, it's "all" or "none", and if you use it with actions that do not support the latent transactional cancel, you will receive an error for your troubles.

Quoted 7 lines · John Snow

Welcome to V10!

Where'd 8 and 9 go? Private off-list missives from Fam. Now you, I, and everyone on qemu-devel are staring at V10.

What's new in V10?

Quoted 8 lines

I replaced the per-action "transactional-cancel" parameter with a per-transaction paremeter named "err-cancel" which is implemented as an enum in case we want to add new behaviors in the future, such as a "jobs only" cancel mode.

For now, it's "all" or "none", and if you use it with actions that do not support the latent transactional cancel, you will receive an error for your troubles.

I left comments on a few patches.

The "err-cancel" and "ActionCancelMode" naming does not describe the concept fully, since successful block jobs will also behave differently (waiting for each other to finish before fully completing). "blockjob-transactions" is the best name I can think of that describes the full concept rather than focus on just cancellation.

Besides that I'm happy with the QMP interface.

Quoted 17 lines · Stefan Hajnoczi

Quoted 7 lines · John Snow

Welcome to V10!

Where'd 8 and 9 go? Private off-list missives from Fam. Now you, I, and everyone on qemu-devel are staring at V10.

What's new in V10?

Quoted 8 lines

I replaced the per-action "transactional-cancel" parameter with a per-transaction paremeter named "err-cancel" which is implemented as an enum in case we want to add new behaviors in the future, such as a "jobs only" cancel mode.

For now, it's "all" or "none", and if you use it with actions that do not support the latent transactional cancel, you will receive an error for your troubles.

Quoted 8 lines

I left comments on a few patches.

The "err-cancel" and "ActionCancelMode" naming does not describe the concept fully, since successful block jobs will also behave differently (waiting for each other to finish before fully completing). "blockjob-transactions" is the best name I can think of that describes the full concept rather than focus on just cancellation.

There's two hard problems in Computer Science...

I mean, cancellation certainly is the biggest change in net behavior, though we are fiddling with the timings of the completion notices.

OK, so continuing on my inability to name this feature ...

blockjob-transactions = { individual, grouped, jobs-only }

where we'd support "individual" as the default,
"grouped" is the forced cancellation mode, and
jobs-only is a hypothetical future mode that adds jobs to the group, and ignoring non-jobs' inability to join the transaction.

Still, it seems weird to have:
transaction 'properties': {'blockjob-transactions': 'grouped'}, it feels redundant to me.

How about:

completion-mode = { individual, grouped }

Avoids repeating "Transaction" and does not hyperfocus on cancellation.

Quoted 2 lines

Besides that I'm happy with the QMP interface.