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

[John Snow]

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

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)
+{
+    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,
+                     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;
         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);
 }
 
diff --git a/blockjob.c b/blockjob.c
index 91e8d3c..00146ff 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -512,7 +512,7 @@ static void block_job_txn_ref(BlockJobTxn *txn)
 
 void block_job_txn_unref(BlockJobTxn *txn)
 {
-    if (--txn->refcnt == 0) {
+    if (txn && --txn->refcnt == 0) {
         g_free(txn);
     }
 }
diff --git a/qapi-schema.json b/qapi-schema.json
index 8769099..0f28311 100644
--- a/qapi-schema.json
+++ 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',
        'abort': 'Abort',
        'blockdev-snapshot-internal-sync': 'BlockdevSnapshotInternal',
        'block-dirty-bitmap-add': 'BlockDirtyBitmapAdd',
        'block-dirty-bitmap-clear': 'BlockDirtyBitmap'
    } }
 
+{ 'struct': 'TransactionProperties',
+  'data': {
+      '*allow-partial': 'bool'
+  }
+}
+
 ##
 # @transaction
 #
@@ -1533,7 +1539,10 @@
 # Since 1.1
 ##
 { 'command': 'transaction',
-  'data': { 'actions': [ 'TransactionAction' ] } }
+  'data': { 'actions': [ 'TransactionAction' ],
+            '*properties': 'TransactionProperties'
+          }
+}
 
 ##
 # @human-monitor-command:
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 24db5c2..83742ab 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -749,19 +749,6 @@
             '*on-target-error': 'BlockdevOnError' } }
 
 ##
-# @DriveBackupTxn
-#
-# @transactional-cancel: #optional whether failure or cancellation of other
-#                        block jobs with @transactional-cancel true in the same
-#                        transaction causes the whole group to cancel. Default
-#                        is false.
-#
-# Since: 2.5
-##
-{ 'struct': 'DriveBackupTxn', 'base': 'DriveBackup',
-  'data': {'*transactional-cancel': 'bool' } }
-
-##
 # @BlockdevBackup
 #
 # @device: the name of the device which should be copied.
@@ -797,19 +784,6 @@
             '*on-target-error': 'BlockdevOnError' } }
 
 ##
-# @BlockdevBackupTxn
-#
-# @transactional-cancel: #optional whether failure or cancellation of other
-#                        block jobs with @transactional-cancel true in the same
-#                        transaction causes the whole group to cancel. Default
-#                        is false
-#
-# Since: 2.5
-##
-{ 'struct': 'BlockdevBackupTxn', 'base': 'BlockdevBackup',
-  'data': {'*transactional-cancel': 'bool' } }
-
-##
 # @blockdev-snapshot-sync
 #
 # Generates a synchronous snapshot of a block device.
diff --git a/qmp-commands.hx b/qmp-commands.hx
index c388274..7c1fed9 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1262,7 +1262,7 @@ EQMP
     },
     {
         .name       = "transaction",
-        .args_type  = "actions:q",
+        .args_type  = "actions:q,properties:q?",
         .mhandler.cmd_new = qmp_marshal_transaction,
     },
 
diff --git a/tests/qemu-iotests/124 b/tests/qemu-iotests/124
index 33d00ef..028b346 100644
--- a/tests/qemu-iotests/124
+++ b/tests/qemu-iotests/124
@@ -456,14 +456,13 @@ class TestIncrementalBackup(iotests.QMPTestCase):
         transaction = [
             transaction_drive_backup(drive0['id'], target0, sync='incremental',
                                      format=drive0['fmt'], mode='existing',
-                                     bitmap=dr0bm0.name,
-                                     transactional_cancel=True),
+                                     bitmap=dr0bm0.name),
             transaction_drive_backup(drive1['id'], target1, sync='incremental',
                                      format=drive1['fmt'], mode='existing',
-                                     bitmap=dr1bm0.name,
-                                     transactional_cancel=True),
+                                     bitmap=dr1bm0.name)
         ]
-        result = self.vm.qmp('transaction', actions=transaction)
+        result = self.vm.qmp('transaction', actions=transaction,
+                             properties={'allow-partial':False} )
         self.assert_qmp(result, 'return', {})
 
         # Observe that drive0's backup is cancelled and drive1 completes with
@@ -485,7 +484,8 @@ class TestIncrementalBackup(iotests.QMPTestCase):
         target1 = self.prepare_backup(dr1bm0)
 
         # Re-run the exact same transaction.
-        result = self.vm.qmp('transaction', actions=transaction)
+        result = self.vm.qmp('transaction', actions=transaction,
+                             properties={'allow-partial':False})
         self.assert_qmp(result, 'return', {})
 
         # Both should complete successfully this time.
-- 
2.4.3