[Qemu-devel] [PATCH v3 00/10] block: incremental backup transactions

[Qemu-devel] [PATCH v3 00/10] block: incremental backup transactions

[John Snow]

requires: 1429314609-29776-1-git-send-email-jsnow@redhat.com
          "[PATCH v6 00/21] block: transactionless incremental backup series"

This series adds support for incremental backup primitives in QMP
transactions. It requires my transactionless incremental backup series,
currently at v6.

Patch 1 adds basic support for add and clear transactions.
Patch 2 tests this basic support.
Patches 3-4 refactor transactions a little bit, to add clarity.
Patch 5 adds the framework for error scenarios where only
    some jobs that were launched by a transaction complete successfully,
    and we need to perform context-sensitive cleanup after the transaction
    itself has already completed.
Patches 6-7 add necessary bookkeeping information to backup job
    data structures to take advantage of this new feature.
Patch 8 modifies qmp_drive_backup to support the new feature.
Patch 9 implements the new feature for drive_backup transaction actions.
Patch 10 tests the new feature.

Notes/concerns: I've removed the code motion in this version and added some
(in my opinion) ugly forward-declarations. The series has otherwise been
minimally rebased to work with the new testing infrastructure that got changed
for v6 of the transactionless series.

===
v3:
===

Key:
[----] : patches are identical
[####] : number of functional differences between upstream/downstream patch
[down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively

001/10:[0002] [FC] 'qapi: Add transaction support to block-dirty-bitmap operations'
002/10:[0061] [FC] 'iotests: add transactional incremental backup test'
003/10:[----] [--] 'block: rename BlkTransactionState and BdrvActionOps'
004/10:[----] [--] 'block: re-add BlkTransactionState'
005/10:[0010] [FC] 'block: add transactional callbacks feature'
006/10:[----] [--] 'block: add refcount to Job object'
007/10:[----] [-C] 'block: add delayed bitmap successor cleanup'
008/10:[0051] [FC] 'qmp: Add an implementation wrapper for qmp_drive_backup'
009/10:[0001] [FC] 'block: drive_backup transaction callback support'
010/10:[0061] [FC] 'iotests: 124 - transactional failure test'

01: Removed "(Not yet implemented)" line from bitmaps.md.
    Kept R-Bs, like a selfish person would.
02: Rebased on latest transactionless series.
    Added some transaction helpers.
05: Removed superfluous deletion loop in put_blk_action_state.
08: Rebased without the preceding code motion patch.
    Re-added forward declaration for do_drive_backup.
09: Fixed commit message whitespace.
    Re-added block_job_cb forward declaration to avoid code motion patch.
10: Rebased on latest transactionless series;
    Added "wait_qmp_backup" function to complement "do_qmp_backup."
    General bike-shedding.

===
v2:
===

 01: Fixed indentation.
     Fixed QMP commands to behave with new bitmap_lookup from
       transactionless-v4.
     2.3 --> 2.4.
 02: Folded in improvements to qemu-iotest 124 from transactional-v1.
 03: NEW
 04: NEW
 05: A lot:
     Don't delete the comma in the transaction actions config
     use g_new0 instead of g_malloc0
     Phrasing: "retcode" --> "Return code"
     Use GCC attributes to mark functions as unused until future patches.
     Added some data structure documentation.
     Many structure and function renames, hopefully to improve readability.
     Use just one list for all Actions instead of two separate lists.
     Remove ActionState from the list upon deletion/decref
     And many other small tweaks.
 06: Comment phrasing.
 07: Removed errp parameter from all functions introduced by this commit.
     bdrv_dirty_bitmap_decref --> bdrv_frozen_bitmap_decref
 08: NEW
 09: _drive_backup --> do_drive_backup()
     Forward declarations removed.
 10: Rebased on top of drastically modified #05.
     Phrasing: "BackupBlockJob" instead of "BackupJob" in comments.
 11: Removed extra parameters to wait_incremental() in
       test_transaction_failure()

John Snow (10):
  qapi: Add transaction support to block-dirty-bitmap operations
  iotests: add transactional incremental backup test
  block: rename BlkTransactionState and BdrvActionOps
  block: re-add BlkTransactionState
  block: add transactional callbacks feature
  block: add refcount to Job object
  block: add delayed bitmap successor cleanup
  qmp: Add an implementation wrapper for qmp_drive_backup
  block: drive_backup transaction callback support
  iotests: 124 - transactional failure test

 block.c                    |  65 +++++-
 block/backup.c             |  29 +--
 blockdev.c                 | 566 +++++++++++++++++++++++++++++++++++++++------
 blockjob.c                 |  18 +-
 docs/bitmaps.md            |   2 +-
 include/block/block.h      |  10 +-
 include/block/block_int.h  |   8 +
 include/block/blockjob.h   |  21 ++
 qapi-schema.json           |   6 +-
 tests/qemu-iotests/124     | 174 +++++++++++++-
 tests/qemu-iotests/124.out |   4 +-
 11 files changed, 802 insertions(+), 101 deletions(-)

-- 
2.1.0

[Qemu-devel] [PATCH v3 01/10] qapi: Add transaction support to block-dirty-bitmap operations

[John Snow]

This adds two qmp commands to transactions.

block-dirty-bitmap-add allows you to create a bitmap simultaneously
alongside a new full backup to accomplish a clean synchronization
point.

block-dirty-bitmap-clear allows you to reset a bitmap back to as-if
it were new, which can also be used alongside a full backup to
accomplish a clean synchronization point.

Signed-off-by: Fam Zheng <famz@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 blockdev.c       | 100 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 docs/bitmaps.md  |   2 +-
 qapi-schema.json |   6 +++-
 3 files changed, 106 insertions(+), 2 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index b9c79ed..d162e10 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1693,6 +1693,95 @@ static void blockdev_backup_clean(BlkTransactionState *common)
     }
 }
 
+typedef struct BlockDirtyBitmapState {
+    BlkTransactionState common;
+    BdrvDirtyBitmap *bitmap;
+    BlockDriverState *bs;
+    AioContext *aio_context;
+    bool prepared;
+} BlockDirtyBitmapState;
+
+static void block_dirty_bitmap_add_prepare(BlkTransactionState *common,
+                                           Error **errp)
+{
+    Error *local_err = NULL;
+    BlockDirtyBitmapAdd *action;
+    BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
+                                             common, common);
+
+    action = common->action->block_dirty_bitmap_add;
+    /* AIO context taken within qmp_block_dirty_bitmap_add */
+    qmp_block_dirty_bitmap_add(action->node, action->name,
+                               action->has_granularity, action->granularity,
+                               &local_err);
+
+    if (!local_err) {
+        state->prepared = true;
+    } else {
+        error_propagate(errp, local_err);
+    }
+}
+
+static void block_dirty_bitmap_add_abort(BlkTransactionState *common)
+{
+    BlockDirtyBitmapAdd *action;
+    BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
+                                             common, common);
+
+    action = common->action->block_dirty_bitmap_add;
+    /* Should not be able to fail: IF the bitmap was added via .prepare(),
+     * then the node reference and bitmap name must have been valid.
+     */
+    if (state->prepared) {
+        qmp_block_dirty_bitmap_remove(action->node, action->name, &error_abort);
+    }
+}
+
+static void block_dirty_bitmap_clear_prepare(BlkTransactionState *common,
+                                             Error **errp)
+{
+    BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
+                                             common, common);
+    BlockDirtyBitmap *action;
+
+    action = common->action->block_dirty_bitmap_clear;
+    state->bitmap = block_dirty_bitmap_lookup(action->node,
+                                              action->name,
+                                              &state->bs,
+                                              &state->aio_context,
+                                              errp);
+    if (!state->bitmap) {
+        return;
+    }
+
+    if (bdrv_dirty_bitmap_frozen(state->bitmap)) {
+        error_setg(errp, "Cannot modify a frozen bitmap");
+        return;
+    } else if (!bdrv_dirty_bitmap_enabled(state->bitmap)) {
+        error_setg(errp, "Cannot clear a disabled bitmap");
+        return;
+    }
+
+    /* AioContext is released in .clean() */
+}
+
+static void block_dirty_bitmap_clear_commit(BlkTransactionState *common)
+{
+    BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
+                                             common, common);
+    bdrv_clear_dirty_bitmap(state->bitmap);
+}
+
+static void block_dirty_bitmap_clear_clean(BlkTransactionState *common)
+{
+    BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
+                                             common, common);
+
+    if (state->aio_context) {
+        aio_context_release(state->aio_context);
+    }
+}
+
 static void abort_prepare(BlkTransactionState *common, Error **errp)
 {
     error_setg(errp, "Transaction aborted using Abort action");
@@ -1733,6 +1822,17 @@ static const BdrvActionOps actions[] = {
         .abort = internal_snapshot_abort,
         .clean = internal_snapshot_clean,
     },
+    [TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_ADD] = {
+        .instance_size = sizeof(BlockDirtyBitmapState),
+        .prepare = block_dirty_bitmap_add_prepare,
+        .abort = block_dirty_bitmap_add_abort,
+    },
+    [TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_CLEAR] = {
+        .instance_size = sizeof(BlockDirtyBitmapState),
+        .prepare = block_dirty_bitmap_clear_prepare,
+        .commit = block_dirty_bitmap_clear_commit,
+        .clean = block_dirty_bitmap_clear_clean,
+    }
 };
 
 /*
diff --git a/docs/bitmaps.md b/docs/bitmaps.md
index f066b48..402dd98 100644
--- a/docs/bitmaps.md
+++ b/docs/bitmaps.md
@@ -97,7 +97,7 @@ which is included at the end of this document.
 }
 ```
 
-## Transactions (Not yet implemented)
+## Transactions
 
 * Transactional commands are forthcoming in a future version,
   and are not yet available for use. This section serves as
diff --git a/qapi-schema.json b/qapi-schema.json
index ac9594d..f6fe2b3 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1356,6 +1356,8 @@
 # abort since 1.6
 # blockdev-snapshot-internal-sync since 1.7
 # blockdev-backup since 2.3
+# block-dirty-bitmap-add since 2.4
+# block-dirty-bitmap-clear since 2.4
 ##
 { 'union': 'TransactionAction',
   'data': {
@@ -1363,7 +1365,9 @@
        'drive-backup': 'DriveBackup',
        'blockdev-backup': 'BlockdevBackup',
        'abort': 'Abort',
-       'blockdev-snapshot-internal-sync': 'BlockdevSnapshotInternal'
+       'blockdev-snapshot-internal-sync': 'BlockdevSnapshotInternal',
+       'block-dirty-bitmap-add': 'BlockDirtyBitmapAdd',
+       'block-dirty-bitmap-clear': 'BlockDirtyBitmap'
    } }
 
 ##
-- 
2.1.0

[Qemu-devel] [PATCH v3 02/10] iotests: add transactional incremental backup test

[John Snow]

Test simple usage cases for using transactions to create
and synchronize incremental backups.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/qemu-iotests/124     | 54 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/124.out |  4 ++--
 2 files changed, 56 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/124 b/tests/qemu-iotests/124
index 3ee78cd..2d50594 100644
--- a/tests/qemu-iotests/124
+++ b/tests/qemu-iotests/124
@@ -36,6 +36,23 @@ def try_remove(img):
         pass
 
 
+def transaction_action(action, **kwargs):
+    return {
+        'type': action,
+        'data': kwargs
+    }
+
+
+def transaction_bitmap_clear(node, name, **kwargs):
+    return transaction_action('block-dirty-bitmap-clear',
+                              node=node, name=name, **kwargs)
+
+
+def transaction_drive_backup(device, target, **kwargs):
+    return transaction_action('drive-backup', device=device, target=target,
+                              **kwargs)
+
+
 class Bitmap:
     def __init__(self, name, drive):
         self.name = name
@@ -264,6 +281,43 @@ class TestIncrementalBackup(iotests.QMPTestCase):
         return self.do_incremental_simple(granularity=131072)
 
 
+    def test_incremental_transaction(self):
+        '''Test: Verify backups made from transactionally created bitmaps.
+
+        Create a bitmap "before" VM execution begins, then create a second
+        bitmap AFTER writes have already occurred. Use transactions to create
+        a full backup and synchronize both bitmaps to this backup.
+        Create an incremental backup through both bitmaps and verify that
+        both backups match the current drive0 image.
+        '''
+
+        drive0 = self.drives[0]
+        bitmap0 = self.add_bitmap('bitmap0', drive0)
+        self.hmp_io_writes(drive0['id'], (('0xab', 0, 512),
+                                          ('0xfe', '16M', '256k'),
+                                          ('0x64', '32736k', '64k')))
+        bitmap1 = self.add_bitmap('bitmap1', drive0)
+
+        result = self.vm.qmp('transaction', actions=[
+            transaction_bitmap_clear(bitmap0.drive['id'], bitmap0.name),
+            transaction_bitmap_clear(bitmap1.drive['id'], bitmap1.name),
+            transaction_drive_backup(drive0['id'], drive0['backup'],
+                                     sync='full', format=drive0['fmt'])
+        ])
+        self.assert_qmp(result, 'return', {})
+        self.wait_until_completed(drive0['id'])
+        self.files.append(drive0['backup'])
+
+        self.hmp_io_writes(drive0['id'], (('0x9a', 0, 512),
+                                          ('0x55', '8M', '352k'),
+                                          ('0x78', '15872k', '1M')))
+        # Both bitmaps should be correctly in sync.
+        self.create_incremental(bitmap0)
+        self.create_incremental(bitmap1)
+        self.vm.shutdown()
+        self.check_backups()
+
+
     def test_incremental_failure(self):
         '''Test: Verify backups made after a failure are correct.
 
diff --git a/tests/qemu-iotests/124.out b/tests/qemu-iotests/124.out
index 2f7d390..594c16f 100644
--- a/tests/qemu-iotests/124.out
+++ b/tests/qemu-iotests/124.out
@@ -1,5 +1,5 @@
-.......
+........
 ----------------------------------------------------------------------
-Ran 7 tests
+Ran 8 tests
 
 OK
-- 
2.1.0

[Qemu-devel] [PATCH v3 03/10] block: rename BlkTransactionState and BdrvActionOps

[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 <jsnow@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 blockdev.c | 114 ++++++++++++++++++++++++++++++++++---------------------------
 1 file changed, 64 insertions(+), 50 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index d162e10..9c74c18 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1228,43 +1228,57 @@ static BdrvDirtyBitmap *block_dirty_bitmap_lookup(const char *node,
 
 /* New and old BlockDriverState structs for atomic group operations */
 
-typedef struct BlkTransactionState BlkTransactionState;
+typedef struct BlkActionState BlkActionState;
 
-/* Only prepare() may fail. In a single transaction, only one of commit() or
-   abort() will be called, clean() will always be called if it present. */
-typedef struct BdrvActionOps {
-    /* Size of state struct, in bytes. */
+/**
+ * BlkActionOps:
+ * Table of operations that define an Action.
+ *
+ * @instance_size: Size of state struct, in bytes.
+ * @prepare: Prepare the work, must NOT be NULL.
+ * @commit: Commit the changes, can be NULL.
+ * @abort: Abort the changes on fail, can be NULL.
+ * @clean: Clean up resources after all transaction actions have called
+ *         commit() or abort(). Can be NULL.
+ *
+ * Only prepare() may fail. In a single transaction, only one of commit() or
+ * abort() will be called. clean() will always be called if it is present.
+ */
+typedef struct BlkActionOps {
     size_t instance_size;
-    /* Prepare the work, must NOT be NULL. */
-    void (*prepare)(BlkTransactionState *common, Error **errp);
-    /* Commit the changes, can be NULL. */
-    void (*commit)(BlkTransactionState *common);
-    /* Abort the changes on fail, can be NULL. */
-    void (*abort)(BlkTransactionState *common);
-    /* Clean up resource in the end, can be NULL. */
-    void (*clean)(BlkTransactionState *common);
-} BdrvActionOps;
+    void (*prepare)(BlkActionState *common, Error **errp);
+    void (*commit)(BlkActionState *common);
+    void (*abort)(BlkActionState *common);
+    void (*clean)(BlkActionState *common);
+} BlkActionOps;
 
-/*
- * This structure must be arranged as first member in child type, assuming
- * that compiler will also arrange it to the same address with parent instance.
- * Later it will be used in free().
+/**
+ * BlkActionState:
+ * Describes one Action's state within a Transaction.
+ *
+ * @action: QAPI-defined enum identifying which Action to perform.
+ * @ops: Table of ActionOps this Action can perform.
+ * @entry: List membership for all Actions in this Transaction.
+ *
+ * This structure must be arranged as first member in a subclassed type,
+ * assuming that the compiler will also arrange it to the same offsets as the
+ * base class.
  */
-struct BlkTransactionState {
+struct BlkActionState {
     TransactionAction *action;
-    const BdrvActionOps *ops;
-    QSIMPLEQ_ENTRY(BlkTransactionState) entry;
+    const BlkActionOps *ops;
+    QSIMPLEQ_ENTRY(BlkActionState) entry;
 };
 
 /* internal snapshot private data */
 typedef struct InternalSnapshotState {
-    BlkTransactionState common;
+    BlkActionState common;
     BlockDriverState *bs;
     AioContext *aio_context;
     QEMUSnapshotInfo sn;
 } InternalSnapshotState;
 
-static void internal_snapshot_prepare(BlkTransactionState *common,
+static void internal_snapshot_prepare(BlkActionState *common,
                                       Error **errp)
 {
     Error *local_err = NULL;
@@ -1358,7 +1372,7 @@ static void internal_snapshot_prepare(BlkTransactionState *common,
     state->bs = bs;
 }
 
-static void internal_snapshot_abort(BlkTransactionState *common)
+static void internal_snapshot_abort(BlkActionState *common)
 {
     InternalSnapshotState *state =
                              DO_UPCAST(InternalSnapshotState, common, common);
@@ -1381,7 +1395,7 @@ static void internal_snapshot_abort(BlkTransactionState *common)
     }
 }
 
-static void internal_snapshot_clean(BlkTransactionState *common)
+static void internal_snapshot_clean(BlkActionState *common)
 {
     InternalSnapshotState *state = DO_UPCAST(InternalSnapshotState,
                                              common, common);
@@ -1393,13 +1407,13 @@ static void internal_snapshot_clean(BlkTransactionState *common)
 
 /* external snapshot private data */
 typedef struct ExternalSnapshotState {
-    BlkTransactionState common;
+    BlkActionState common;
     BlockDriverState *old_bs;
     BlockDriverState *new_bs;
     AioContext *aio_context;
 } ExternalSnapshotState;
 
-static void external_snapshot_prepare(BlkTransactionState *common,
+static void external_snapshot_prepare(BlkActionState *common,
                                       Error **errp)
 {
     BlockDriver *drv;
@@ -1520,7 +1534,7 @@ static void external_snapshot_prepare(BlkTransactionState *common,
     }
 }
 
-static void external_snapshot_commit(BlkTransactionState *common)
+static void external_snapshot_commit(BlkActionState *common)
 {
     ExternalSnapshotState *state =
                              DO_UPCAST(ExternalSnapshotState, common, common);
@@ -1538,7 +1552,7 @@ static void external_snapshot_commit(BlkTransactionState *common)
     aio_context_release(state->aio_context);
 }
 
-static void external_snapshot_abort(BlkTransactionState *common)
+static void external_snapshot_abort(BlkActionState *common)
 {
     ExternalSnapshotState *state =
                              DO_UPCAST(ExternalSnapshotState, common, common);
@@ -1551,13 +1565,13 @@ static void external_snapshot_abort(BlkTransactionState *common)
 }
 
 typedef struct DriveBackupState {
-    BlkTransactionState common;
+    BlkActionState common;
     BlockDriverState *bs;
     AioContext *aio_context;
     BlockJob *job;
 } DriveBackupState;
 
-static void drive_backup_prepare(BlkTransactionState *common, Error **errp)
+static void drive_backup_prepare(BlkActionState *common, Error **errp)
 {
     DriveBackupState *state = DO_UPCAST(DriveBackupState, common, common);
     BlockDriverState *bs;
@@ -1597,7 +1611,7 @@ static void drive_backup_prepare(BlkTransactionState *common, Error **errp)
     state->job = state->bs->job;
 }
 
-static void drive_backup_abort(BlkTransactionState *common)
+static void drive_backup_abort(BlkActionState *common)
 {
     DriveBackupState *state = DO_UPCAST(DriveBackupState, common, common);
     BlockDriverState *bs = state->bs;
@@ -1608,7 +1622,7 @@ static void drive_backup_abort(BlkTransactionState *common)
     }
 }
 
-static void drive_backup_clean(BlkTransactionState *common)
+static void drive_backup_clean(BlkActionState *common)
 {
     DriveBackupState *state = DO_UPCAST(DriveBackupState, common, common);
 
@@ -1618,13 +1632,13 @@ static void drive_backup_clean(BlkTransactionState *common)
 }
 
 typedef struct BlockdevBackupState {
-    BlkTransactionState common;
+    BlkActionState common;
     BlockDriverState *bs;
     BlockJob *job;
     AioContext *aio_context;
 } BlockdevBackupState;
 
-static void blockdev_backup_prepare(BlkTransactionState *common, Error **errp)
+static void blockdev_backup_prepare(BlkActionState *common, Error **errp)
 {
     BlockdevBackupState *state = DO_UPCAST(BlockdevBackupState, common, common);
     BlockdevBackup *backup;
@@ -1673,7 +1687,7 @@ static void blockdev_backup_prepare(BlkTransactionState *common, Error **errp)
     state->job = state->bs->job;
 }
 
-static void blockdev_backup_abort(BlkTransactionState *common)
+static void blockdev_backup_abort(BlkActionState *common)
 {
     BlockdevBackupState *state = DO_UPCAST(BlockdevBackupState, common, common);
     BlockDriverState *bs = state->bs;
@@ -1684,7 +1698,7 @@ static void blockdev_backup_abort(BlkTransactionState *common)
     }
 }
 
-static void blockdev_backup_clean(BlkTransactionState *common)
+static void blockdev_backup_clean(BlkActionState *common)
 {
     BlockdevBackupState *state = DO_UPCAST(BlockdevBackupState, common, common);
 
@@ -1694,14 +1708,14 @@ static void blockdev_backup_clean(BlkTransactionState *common)
 }
 
 typedef struct BlockDirtyBitmapState {
-    BlkTransactionState common;
+    BlkActionState common;
     BdrvDirtyBitmap *bitmap;
     BlockDriverState *bs;
     AioContext *aio_context;
     bool prepared;
 } BlockDirtyBitmapState;
 
-static void block_dirty_bitmap_add_prepare(BlkTransactionState *common,
+static void block_dirty_bitmap_add_prepare(BlkActionState *common,
                                            Error **errp)
 {
     Error *local_err = NULL;
@@ -1722,7 +1736,7 @@ static void block_dirty_bitmap_add_prepare(BlkTransactionState *common,
     }
 }
 
-static void block_dirty_bitmap_add_abort(BlkTransactionState *common)
+static void block_dirty_bitmap_add_abort(BlkActionState *common)
 {
     BlockDirtyBitmapAdd *action;
     BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
@@ -1737,7 +1751,7 @@ static void block_dirty_bitmap_add_abort(BlkTransactionState *common)
     }
 }
 
-static void block_dirty_bitmap_clear_prepare(BlkTransactionState *common,
+static void block_dirty_bitmap_clear_prepare(BlkActionState *common,
                                              Error **errp)
 {
     BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
@@ -1765,14 +1779,14 @@ static void block_dirty_bitmap_clear_prepare(BlkTransactionState *common,
     /* AioContext is released in .clean() */
 }
 
-static void block_dirty_bitmap_clear_commit(BlkTransactionState *common)
+static void block_dirty_bitmap_clear_commit(BlkActionState *common)
 {
     BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
                                              common, common);
     bdrv_clear_dirty_bitmap(state->bitmap);
 }
 
-static void block_dirty_bitmap_clear_clean(BlkTransactionState *common)
+static void block_dirty_bitmap_clear_clean(BlkActionState *common)
 {
     BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
                                              common, common);
@@ -1782,17 +1796,17 @@ static void block_dirty_bitmap_clear_clean(BlkTransactionState *common)
     }
 }
 
-static void abort_prepare(BlkTransactionState *common, Error **errp)
+static void abort_prepare(BlkActionState *common, Error **errp)
 {
     error_setg(errp, "Transaction aborted using Abort action");
 }
 
-static void abort_commit(BlkTransactionState *common)
+static void abort_commit(BlkActionState *common)
 {
     g_assert_not_reached(); /* this action never succeeds */
 }
 
-static const BdrvActionOps actions[] = {
+static const BlkActionOps actions[] = {
     [TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_SYNC] = {
         .instance_size = sizeof(ExternalSnapshotState),
         .prepare  = external_snapshot_prepare,
@@ -1812,7 +1826,7 @@ static const BdrvActionOps actions[] = {
         .clean = blockdev_backup_clean,
     },
     [TRANSACTION_ACTION_KIND_ABORT] = {
-        .instance_size = sizeof(BlkTransactionState),
+        .instance_size = sizeof(BlkActionState),
         .prepare = abort_prepare,
         .commit = abort_commit,
     },
@@ -1842,10 +1856,10 @@ static const BdrvActionOps actions[] = {
 void qmp_transaction(TransactionActionList *dev_list, Error **errp)
 {
     TransactionActionList *dev_entry = dev_list;
-    BlkTransactionState *state, *next;
+    BlkActionState *state, *next;
     Error *local_err = NULL;
 
-    QSIMPLEQ_HEAD(snap_bdrv_states, BlkTransactionState) snap_bdrv_states;
+    QSIMPLEQ_HEAD(snap_bdrv_states, BlkActionState) snap_bdrv_states;
     QSIMPLEQ_INIT(&snap_bdrv_states);
 
     /* drain all i/o before any operations */
@@ -1854,7 +1868,7 @@ void qmp_transaction(TransactionActionList *dev_list, Error **errp)
     /* We don't do anything in this loop that commits us to the operations */
     while (NULL != dev_entry) {
         TransactionAction *dev_info = NULL;
-        const BdrvActionOps *ops;
+        const BlkActionOps *ops;
 
         dev_info = dev_entry->value;
         dev_entry = dev_entry->next;
-- 
2.1.0

[Qemu-devel] [PATCH v3 04/10] block: re-add BlkTransactionState

[John Snow]

Now that the structure formerly known as BlkTransactionState has been
renamed to something sensible (BlkActionState), re-introduce an actual
BlkTransactionState that actually manages state for the entire Transaction.

In the process, convert the old QSIMPLEQ list of actions into a QTAILQ,
to let us more efficiently delete items in arbitrary order, which will
be more important in the future when some actions will expire at the end
of the transaction, but others may persist until all callbacks triggered
by the transaction are recollected.

Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 blockdev.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 59 insertions(+), 7 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 9c74c18..2ab63ed 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1253,6 +1253,19 @@ typedef struct BlkActionOps {
 } BlkActionOps;
 
 /**
+ * BlkTransactionState:
+ * Object to track the job completion info for jobs launched
+ * by a transaction group.
+ *
+ * @jobs: A reference count that tracks how many jobs still need to complete.
+ * @actions: A list of all Actions in the Transaction.
+ */
+typedef struct BlkTransactionState {
+    int jobs;
+    QTAILQ_HEAD(actions, BlkActionState) actions;
+} BlkTransactionState;
+
+/**
  * BlkActionState:
  * Describes one Action's state within a Transaction.
  *
@@ -1267,9 +1280,45 @@ typedef struct BlkActionOps {
 struct BlkActionState {
     TransactionAction *action;
     const BlkActionOps *ops;
-    QSIMPLEQ_ENTRY(BlkActionState) entry;
+    QTAILQ_ENTRY(BlkActionState) entry;
 };
 
+static BlkTransactionState *new_blk_transaction_state(void)
+{
+    BlkTransactionState *bts = g_new0(BlkTransactionState, 1);
+
+    /* The qmp_transaction function itself can be considered a pending job
+     * that should complete before pending action callbacks are executed,
+     * so increment the jobs remaining refcount to indicate this. */
+    bts->jobs = 1;
+    QTAILQ_INIT(&bts->actions);
+    return bts;
+}
+
+static void destroy_blk_transaction_state(BlkTransactionState *bts)
+{
+    BlkActionState *bas, *bas_next;
+
+    /* The list should in normal cases be empty,
+     * but in case someone really just wants to kibosh the whole deal: */
+    QTAILQ_FOREACH_SAFE(bas, &bts->actions, entry, bas_next) {
+        QTAILQ_REMOVE(&bts->actions, bas, entry);
+        g_free(bas);
+    }
+
+    g_free(bts);
+}
+
+static BlkTransactionState *transaction_job_complete(BlkTransactionState *bts)
+{
+    bts->jobs--;
+    if (bts->jobs == 0) {
+        destroy_blk_transaction_state(bts);
+        return NULL;
+    }
+    return bts;
+}
+
 /* internal snapshot private data */
 typedef struct InternalSnapshotState {
     BlkActionState common;
@@ -1857,10 +1906,10 @@ void qmp_transaction(TransactionActionList *dev_list, Error **errp)
 {
     TransactionActionList *dev_entry = dev_list;
     BlkActionState *state, *next;
+    BlkTransactionState *bts;
     Error *local_err = NULL;
 
-    QSIMPLEQ_HEAD(snap_bdrv_states, BlkActionState) snap_bdrv_states;
-    QSIMPLEQ_INIT(&snap_bdrv_states);
+    bts = new_blk_transaction_state();
 
     /* drain all i/o before any operations */
     bdrv_drain_all();
@@ -1881,7 +1930,7 @@ void qmp_transaction(TransactionActionList *dev_list, Error **errp)
         state = g_malloc0(ops->instance_size);
         state->ops = ops;
         state->action = dev_info;
-        QSIMPLEQ_INSERT_TAIL(&snap_bdrv_states, state, entry);
+        QTAILQ_INSERT_TAIL(&bts->actions, state, entry);
 
         state->ops->prepare(state, &local_err);
         if (local_err) {
@@ -1890,7 +1939,7 @@ void qmp_transaction(TransactionActionList *dev_list, Error **errp)
         }
     }
 
-    QSIMPLEQ_FOREACH(state, &snap_bdrv_states, entry) {
+    QTAILQ_FOREACH(state, &bts->actions, entry) {
         if (state->ops->commit) {
             state->ops->commit(state);
         }
@@ -1901,18 +1950,21 @@ void qmp_transaction(TransactionActionList *dev_list, Error **errp)
 
 delete_and_fail:
     /* failure, and it is all-or-none; roll back all operations */
-    QSIMPLEQ_FOREACH(state, &snap_bdrv_states, entry) {
+    QTAILQ_FOREACH(state, &bts->actions, entry) {
         if (state->ops->abort) {
             state->ops->abort(state);
         }
     }
 exit:
-    QSIMPLEQ_FOREACH_SAFE(state, &snap_bdrv_states, entry, next) {
+    QTAILQ_FOREACH_SAFE(state, &bts->actions, entry, next) {
         if (state->ops->clean) {
             state->ops->clean(state);
         }
+        QTAILQ_REMOVE(&bts->actions, state, entry);
         g_free(state);
     }
+
+    transaction_job_complete(bts);
 }
 
 
-- 
2.1.0

[Qemu-devel] [PATCH v3 05/10] block: add transactional callbacks feature

[John Snow]

The goal here is to add a new method to transactions that allows
developers to specify a callback that will get invoked only once
all jobs spawned by a transaction are completed, allowing developers
the chance to perform actions conditionally pending complete success,
partial failure, or complete failure.

In order to register the new callback to be invoked, a user must request
a callback pointer and closure by calling new_action_cb_wrapper, which
creates a wrapper around an opaque pointer and callback that would have
originally been passed to e.g. backup_start().

The function will return a function pointer and a new opaque pointer to
be passed instead. The transaction system will effectively intercept the
original callbacks and perform book-keeping on the transaction after it
has delivered the original enveloped callback.

This means that Transaction Action callback methods will be called after
all callbacks triggered by all Actions in the Transactional group have
been received.

This feature has no knowledge of any jobs spawned by Actions that do not
inform the system via new_action_cb_wrapper().

For an example of how to use the feature, please skip ahead to:
'block: drive_backup transaction callback support' which serves as an example
for how to hook up a post-transaction callback to the Drive Backup action.


Note 1: Defining a callback method alone is not sufficient to have the new
        method invoked. You must call new_action_cb_wrapper() AND ensure the
        callback it returns is the one used as the callback for the job
        launched by the action.

Note 2: You can use this feature for any system that registers completions of
        an asynchronous task via a callback of the form
        (void *opaque, int ret), not just block job callbacks.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 blockdev.c | 183 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 179 insertions(+), 4 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 2ab63ed..31ccb1b 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1240,6 +1240,8 @@ typedef struct BlkActionState BlkActionState;
  * @abort: Abort the changes on fail, can be NULL.
  * @clean: Clean up resources after all transaction actions have called
  *         commit() or abort(). Can be NULL.
+ * @cb: Executed after all jobs launched by actions in the transaction finish,
+ *      but only if requested by new_action_cb_wrapper() prior to clean().
  *
  * Only prepare() may fail. In a single transaction, only one of commit() or
  * abort() will be called. clean() will always be called if it is present.
@@ -1250,6 +1252,7 @@ typedef struct BlkActionOps {
     void (*commit)(BlkActionState *common);
     void (*abort)(BlkActionState *common);
     void (*clean)(BlkActionState *common);
+    void (*cb)(BlkActionState *common);
 } BlkActionOps;
 
 /**
@@ -1258,19 +1261,46 @@ typedef struct BlkActionOps {
  * by a transaction group.
  *
  * @jobs: A reference count that tracks how many jobs still need to complete.
+ * @status: A cumulative return code for all actions that have reported
+ *          a return code via callback in the transaction.
  * @actions: A list of all Actions in the Transaction.
+ *           However, once the transaction has completed, it will be only a list
+ *           of transactions that have registered a post-transaction callback.
  */
 typedef struct BlkTransactionState {
     int jobs;
+    int status;
     QTAILQ_HEAD(actions, BlkActionState) actions;
 } BlkTransactionState;
 
+typedef void (CallbackFn)(void *opaque, int ret);
+
+/**
+ * BlkActionCallbackData:
+ * Necessary state for intercepting and
+ * re-delivering a callback triggered by an Action.
+ *
+ * @opaque: The data to be given to the encapsulated callback when
+ *          a job launched by an Action completes.
+ * @ret: The status code that was delivered to the encapsulated callback.
+ * @callback: The encapsulated callback to invoke upon completion of
+ *            the Job launched by the Action.
+ */
+typedef struct BlkActionCallbackData {
+    void *opaque;
+    int ret;
+    CallbackFn *callback;
+} BlkActionCallbackData;
+
 /**
  * BlkActionState:
  * Describes one Action's state within a Transaction.
  *
  * @action: QAPI-defined enum identifying which Action to perform.
  * @ops: Table of ActionOps this Action can perform.
+ * @transaction: A pointer back to the Transaction this Action belongs to.
+ * @cb_data: Information on this Action's encapsulated callback, if any.
+ * @refcount: reference count, allowing access to this state beyond clean().
  * @entry: List membership for all Actions in this Transaction.
  *
  * This structure must be arranged as first member in a subclassed type,
@@ -1280,6 +1310,9 @@ typedef struct BlkTransactionState {
 struct BlkActionState {
     TransactionAction *action;
     const BlkActionOps *ops;
+    BlkTransactionState *transaction;
+    BlkActionCallbackData *cb_data;
+    int refcount;
     QTAILQ_ENTRY(BlkActionState) entry;
 };
 
@@ -1295,6 +1328,18 @@ static BlkTransactionState *new_blk_transaction_state(void)
     return bts;
 }
 
+static BlkActionState *put_blk_action_state(BlkActionState *state)
+{
+    state->refcount--;
+    if (state->refcount == 0) {
+        QTAILQ_REMOVE(&state->transaction->actions, state, entry);
+        g_free(state->cb_data);
+        g_free(state);
+        return NULL;
+    }
+    return state;
+}
+
 static void destroy_blk_transaction_state(BlkTransactionState *bts)
 {
     BlkActionState *bas, *bas_next;
@@ -1302,23 +1347,151 @@ static void destroy_blk_transaction_state(BlkTransactionState *bts)
     /* The list should in normal cases be empty,
      * but in case someone really just wants to kibosh the whole deal: */
     QTAILQ_FOREACH_SAFE(bas, &bts->actions, entry, bas_next) {
-        QTAILQ_REMOVE(&bts->actions, bas, entry);
-        g_free(bas);
+        put_blk_action_state(bas);
     }
 
     g_free(bts);
 }
 
+static void transaction_run_cb_action_ops(BlkTransactionState *bts)
+{
+    BlkActionState *bas, *bas_next;
+
+    QTAILQ_FOREACH_SAFE(bas, &bts->actions, entry, bas_next) {
+        if (bas->ops->cb) {
+            bas->ops->cb(bas);
+        }
+
+        g_free(bas->cb_data);
+        bas->cb_data = NULL;
+        put_blk_action_state(bas);
+    }
+}
+
 static BlkTransactionState *transaction_job_complete(BlkTransactionState *bts)
 {
     bts->jobs--;
     if (bts->jobs == 0) {
+        transaction_run_cb_action_ops(bts);
         destroy_blk_transaction_state(bts);
         return NULL;
     }
     return bts;
 }
 
+static void transaction_job_cancel(BlkActionState *bas)
+{
+    assert(bas->transaction->jobs > 1);
+    transaction_job_complete(bas->transaction);
+}
+
+/**
+ * Intercept a callback that was issued due to a transactional action.
+ */
+static void transaction_action_callback(void *opaque, int ret)
+{
+    BlkActionState *common = opaque;
+    BlkActionCallbackData *cb_data = common->cb_data;
+    CallbackFn *callback = cb_data->callback;
+
+    /* Prepare data for ops->cb() */
+    cb_data->callback = NULL;
+    cb_data->ret = ret;
+
+    /* Cumulative return code will track the first error discovered */
+    if (!common->transaction->status) {
+        common->transaction->status = ret;
+    }
+
+    /* Deliver the intercepted callback prior to marking it as completed */
+    callback(cb_data->opaque, cb_data->ret);
+    transaction_job_complete(common->transaction);
+}
+
+/**
+ * Create a new transactional callback wrapper.
+ *
+ * Given a callback and a closure, generate a new
+ * callback and closure that will invoke the
+ * given callback with the given closure.
+ *
+ * After all wrappers in the transactional group have
+ * been processed, each action's .cb() method will be
+ * invoked.
+ *
+ * @common The transaction action state to set a callback for.
+ * @opaque A closure intended for the encapsulated callback.
+ * @callback The callback we are encapsulating.
+ * @new_opaque[out]: The closure to be used instead of @opaque.
+ *
+ * @return The callback to be used instead of @callback.
+ */
+__attribute__((__unused__))
+static CallbackFn *new_action_cb_wrapper(BlkActionState *common,
+                                         void *opaque,
+                                         CallbackFn *callback,
+                                         void **new_opaque)
+{
+    BlkActionCallbackData *cb_data = g_new0(BlkActionCallbackData, 1);
+    assert(new_opaque);
+
+    /* Stash the original callback information */
+    cb_data->opaque = opaque;
+    cb_data->callback = callback;
+    common->cb_data = cb_data;
+
+    /* The BAS itself will serve as our new closure */
+    *new_opaque = common;
+    common->refcount++;
+
+    /* Inform the transaction to expect one more return */
+    common->transaction->jobs++;
+
+    /* Lastly, the actual callback function to handle the interception. */
+    return transaction_action_callback;
+}
+
+/**
+ * Undo any actions performed by the above call.
+ */
+__attribute__((__unused__))
+static void cancel_action_cb_wrapper(BlkActionState *common)
+{
+    /* Stage 0: Wrapper was never created: */
+    if (common->cb_data == NULL) {
+        assert(common->refcount == 1);
+        return;
+    }
+
+    /* Stage 1: Callback was created, but the job never launched.
+     * (We assume that the job cannot possibly be running, because
+     *  we assume it has been synchronously canceled if it was.)
+     *
+     * We also assume that any cleanup normally handled by cb()
+     * is not needed, because it should have been prepared after
+     * the job launched.
+     */
+    if (common->cb_data && common->cb_data->callback) {
+        transaction_job_cancel(common);
+        goto cleanup;
+    }
+
+    /* Stage 2: Job already completed, or was canceled.
+     *
+     * Force an error in the callback data and just invoke the completion
+     * handler to perform appropriate cleanup for us.
+     */
+    if (common->cb_data && !common->cb_data->callback) {
+        common->cb_data->ret = -ECANCELED;
+        common->ops->cb(common);
+    }
+
+ cleanup:
+    g_free(common->cb_data);
+    common->cb_data = NULL;
+    put_blk_action_state(common);
+}
+
 /* internal snapshot private data */
 typedef struct InternalSnapshotState {
     BlkActionState common;
@@ -1928,8 +2101,10 @@ void qmp_transaction(TransactionActionList *dev_list, Error **errp)
         assert(ops->instance_size > 0);
 
         state = g_malloc0(ops->instance_size);
+        state->refcount = 1;
         state->ops = ops;
         state->action = dev_info;
+        state->transaction = bts;
         QTAILQ_INSERT_TAIL(&bts->actions, state, entry);
 
         state->ops->prepare(state, &local_err);
@@ -1960,10 +2135,10 @@ exit:
         if (state->ops->clean) {
             state->ops->clean(state);
         }
-        QTAILQ_REMOVE(&bts->actions, state, entry);
-        g_free(state);
+        put_blk_action_state(state);
     }
 
+    /* The implicit 'job' of the transaction itself is complete: */
     transaction_job_complete(bts);
 }
 
-- 
2.1.0

[Qemu-devel] [PATCH v3 06/10] block: add refcount to Job object

[John Snow]

If we want to get at the job after the life of the job,
we'll need a refcount for this object.

This may occur for example if we wish to inspect the actions
taken by a particular job after a transactional group of jobs
runs, and further actions are required.

Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 blockjob.c               | 18 ++++++++++++++++--
 include/block/blockjob.h | 21 +++++++++++++++++++++
 2 files changed, 37 insertions(+), 2 deletions(-)

diff --git a/blockjob.c b/blockjob.c
index ba2255d..d620082 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -35,6 +35,19 @@
 #include "qemu/timer.h"
 #include "qapi-event.h"
 
+void block_job_incref(BlockJob *job)
+{
+    job->refcount++;
+}
+
+void block_job_decref(BlockJob *job)
+{
+    job->refcount--;
+    if (job->refcount == 0) {
+        g_free(job);
+    }
+}
+
 void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs,
                        int64_t speed, BlockCompletionFunc *cb,
                        void *opaque, Error **errp)
@@ -57,6 +70,7 @@ void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs,
     job->cb            = cb;
     job->opaque        = opaque;
     job->busy          = true;
+    job->refcount      = 1;
     bs->job = job;
 
     /* Only set speed when necessary to avoid NotSupported error */
@@ -68,7 +82,7 @@ void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs,
             bs->job = NULL;
             bdrv_op_unblock_all(bs, job->blocker);
             error_free(job->blocker);
-            g_free(job);
+            block_job_decref(job);
             error_propagate(errp, local_err);
             return NULL;
         }
@@ -85,7 +99,7 @@ void block_job_completed(BlockJob *job, int ret)
     bs->job = NULL;
     bdrv_op_unblock_all(bs, job->blocker);
     error_free(job->blocker);
-    g_free(job);
+    block_job_decref(job);
 }
 
 void block_job_set_speed(BlockJob *job, int64_t speed, Error **errp)
diff --git a/include/block/blockjob.h b/include/block/blockjob.h
index b6d4ebb..dcc0596 100644
--- a/include/block/blockjob.h
+++ b/include/block/blockjob.h
@@ -116,6 +116,9 @@ struct BlockJob {
 
     /** The opaque value that is passed to the completion function.  */
     void *opaque;
+
+    /** A reference count, allowing for post-job actions in e.g. transactions */
+    int refcount;
 };
 
 /**
@@ -141,6 +144,24 @@ void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs,
                        void *opaque, Error **errp);
 
 /**
+ * block_job_incref:
+ * @job: The job to pick up a handle to
+ *
+ * Increment the refcount on @job, to be able to use it asynchronously
+ * from the job it is being used for. Put down the reference when done
+ * with @block_job_unref.
+ */
+void block_job_incref(BlockJob *job);
+
+/**
+ * block_job_decref:
+ * @job: The job to unreference and delete.
+ *
+ * Decrement the refcount on @job and delete it if there are no more references.
+ */
+void block_job_decref(BlockJob *job);
+
+/**
  * block_job_sleep_ns:
  * @job: The job that calls the function.
  * @clock: The clock to sleep on.
-- 
2.1.0

[Qemu-devel] [PATCH v3 07/10] block: add delayed bitmap successor cleanup

[John Snow]

Allow bitmap successors to carry reference counts.

We can in a later patch use this ability to clean up the dirty bitmap
according to both the individual job's success and the success of all
jobs in the transaction group.

The code for cleaning up a bitmap is also moved from backup_run to
backup_complete.

Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 block.c               | 65 ++++++++++++++++++++++++++++++++++++++++++++++-----
 block/backup.c        | 20 ++++++----------
 include/block/block.h | 10 ++++----
 3 files changed, 70 insertions(+), 25 deletions(-)

diff --git a/block.c b/block.c
index b29aafe..0e7308c 100644
--- a/block.c
+++ b/block.c
@@ -51,6 +51,12 @@
 #include <windows.h>
 #endif
 
+typedef enum BitmapSuccessorAction {
+    SUCCESSOR_ACTION_UNDEFINED = 0,
+    SUCCESSOR_ACTION_ABDICATE,
+    SUCCESSOR_ACTION_RECLAIM
+} BitmapSuccessorAction;
+
 /**
  * A BdrvDirtyBitmap can be in three possible states:
  * (1) successor is NULL and disabled is false: full r/w mode
@@ -65,6 +71,8 @@ struct BdrvDirtyBitmap {
     char *name;                 /* Optional non-empty unique ID */
     int64_t size;               /* Size of the bitmap (Number of sectors) */
     bool disabled;              /* Bitmap is read-only */
+    int successor_refcount;     /* Number of active handles to the successor */
+    BitmapSuccessorAction act;  /* Action to take on successor upon release */
     QLIST_ENTRY(BdrvDirtyBitmap) list;
 };
 
@@ -5540,6 +5548,7 @@ int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
 
     /* Install the successor and freeze the parent */
     bitmap->successor = child;
+    bitmap->successor_refcount = 1;
     return 0;
 }
 
@@ -5547,9 +5556,9 @@ int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
  * For a bitmap with a successor, yield our name to the successor,
  * delete the old bitmap, and return a handle to the new bitmap.
  */
-BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs,
-                                            BdrvDirtyBitmap *bitmap,
-                                            Error **errp)
+static BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs,
+                                                   BdrvDirtyBitmap *bitmap,
+                                                   Error **errp)
 {
     char *name;
     BdrvDirtyBitmap *successor = bitmap->successor;
@@ -5574,9 +5583,9 @@ BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs,
  * we may wish to re-join the parent and child/successor.
  * The merged parent will be un-frozen, but not explicitly re-enabled.
  */
-BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
-                                           BdrvDirtyBitmap *parent,
-                                           Error **errp)
+static BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
+                                                  BdrvDirtyBitmap *parent,
+                                                  Error **errp)
 {
     BdrvDirtyBitmap *successor = parent->successor;
 
@@ -5595,6 +5604,50 @@ BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
     return parent;
 }
 
+static BdrvDirtyBitmap *bdrv_free_bitmap_successor(BlockDriverState *bs,
+                                                   BdrvDirtyBitmap *parent)
+{
+    assert(!parent->successor_refcount);
+
+    switch (parent->act) {
+    case SUCCESSOR_ACTION_RECLAIM:
+        return bdrv_reclaim_dirty_bitmap(bs, parent, NULL);
+    case SUCCESSOR_ACTION_ABDICATE:
+        return bdrv_dirty_bitmap_abdicate(bs, parent, NULL);
+    case SUCCESSOR_ACTION_UNDEFINED:
+    default:
+        g_assert_not_reached();
+    }
+}
+
+BdrvDirtyBitmap *bdrv_frozen_bitmap_decref(BlockDriverState *bs,
+                                           BdrvDirtyBitmap *parent,
+                                           int ret)
+{
+    assert(bdrv_dirty_bitmap_frozen(parent));
+    assert(parent->successor);
+
+    if (ret) {
+        parent->act = SUCCESSOR_ACTION_RECLAIM;
+    } else if (parent->act != SUCCESSOR_ACTION_RECLAIM) {
+        parent->act = SUCCESSOR_ACTION_ABDICATE;
+    }
+
+    parent->successor_refcount--;
+    if (parent->successor_refcount == 0) {
+        return bdrv_free_bitmap_successor(bs, parent);
+    }
+    return parent;
+}
+
+void bdrv_dirty_bitmap_incref(BdrvDirtyBitmap *parent)
+{
+    assert(bdrv_dirty_bitmap_frozen(parent));
+    assert(parent->successor);
+
+    parent->successor_refcount++;
+}
+
 /**
  * Truncates _all_ bitmaps attached to a BDS.
  */
diff --git a/block/backup.c b/block/backup.c
index a297df6..62f8d2b 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -240,6 +240,12 @@ static void backup_complete(BlockJob *job, void *opaque)
 
     bdrv_unref(s->target);
 
+    if (s->sync_bitmap) {
+        BdrvDirtyBitmap *bm;
+        bm = bdrv_frozen_bitmap_decref(job->bs, s->sync_bitmap, data->ret);
+        assert(bm);
+    }
+
     block_job_completed(job, data->ret);
     g_free(data);
 }
@@ -428,18 +434,6 @@ static void coroutine_fn backup_run(void *opaque)
     qemu_co_rwlock_wrlock(&job->flush_rwlock);
     qemu_co_rwlock_unlock(&job->flush_rwlock);
 
-    if (job->sync_bitmap) {
-        BdrvDirtyBitmap *bm;
-        if (ret < 0) {
-            /* Merge the successor back into the parent, delete nothing. */
-            bm = bdrv_reclaim_dirty_bitmap(bs, job->sync_bitmap, NULL);
-            assert(bm);
-        } else {
-            /* Everything is fine, delete this bitmap and install the backup. */
-            bm = bdrv_dirty_bitmap_abdicate(bs, job->sync_bitmap, NULL);
-            assert(bm);
-        }
-    }
     hbitmap_free(job->bitmap);
 
     bdrv_iostatus_disable(target);
@@ -543,6 +537,6 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target,
 
  error:
     if (sync_bitmap) {
-        bdrv_reclaim_dirty_bitmap(bs, sync_bitmap, NULL);
+        bdrv_frozen_bitmap_decref(bs, sync_bitmap, -ECANCELED);
     }
 }
diff --git a/include/block/block.h b/include/block/block.h
index 36bdf04..7b548b1 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -456,12 +456,10 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
 int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
                                        BdrvDirtyBitmap *bitmap,
                                        Error **errp);
-BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs,
-                                            BdrvDirtyBitmap *bitmap,
-                                            Error **errp);
-BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
-                                           BdrvDirtyBitmap *bitmap,
-                                           Error **errp);
+BdrvDirtyBitmap *bdrv_frozen_bitmap_decref(BlockDriverState *bs,
+                                           BdrvDirtyBitmap *parent,
+                                           int ret);
+void bdrv_dirty_bitmap_incref(BdrvDirtyBitmap *parent);
 BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState *bs,
                                         const char *name);
 void bdrv_dirty_bitmap_make_anon(BdrvDirtyBitmap *bitmap);
-- 
2.1.0

[Qemu-devel] [PATCH v3 08/10] qmp: Add an implementation wrapper for qmp_drive_backup

[John Snow]

We'd like to be able to specify the callback given to backup_start
manually in the case of transactions, so split apart qmp_drive_backup
into an implementation and a wrapper.

Switch drive_backup_prepare to use the new wrapper, but don't overload
the callback and closure yet.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 blockdev.c | 78 +++++++++++++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 59 insertions(+), 19 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 31ccb1b..eed23b8 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1793,6 +1793,19 @@ typedef struct DriveBackupState {
     BlockJob *job;
 } DriveBackupState;
 
+static void do_drive_backup(const char *device, const char *target,
+                            bool has_format, const char *format,
+                            enum MirrorSyncMode sync,
+                            bool has_mode, enum NewImageMode mode,
+                            bool has_speed, int64_t speed,
+                            bool has_bitmap, const char *bitmap,
+                            bool has_on_source_error,
+                            BlockdevOnError on_source_error,
+                            bool has_on_target_error,
+                            BlockdevOnError on_target_error,
+                            BlockCompletionFunc *cb, void *opaque,
+                            Error **errp);
+
 static void drive_backup_prepare(BlkActionState *common, Error **errp)
 {
     DriveBackupState *state = DO_UPCAST(DriveBackupState, common, common);
@@ -1815,15 +1828,16 @@ static void drive_backup_prepare(BlkActionState *common, Error **errp)
     state->aio_context = bdrv_get_aio_context(bs);
     aio_context_acquire(state->aio_context);
 
-    qmp_drive_backup(backup->device, backup->target,
-                     backup->has_format, backup->format,
-                     backup->sync,
-                     backup->has_mode, backup->mode,
-                     backup->has_speed, backup->speed,
-                     backup->has_bitmap, backup->bitmap,
-                     backup->has_on_source_error, backup->on_source_error,
-                     backup->has_on_target_error, backup->on_target_error,
-                     &local_err);
+    do_drive_backup(backup->device, backup->target,
+                    backup->has_format, backup->format,
+                    backup->sync,
+                    backup->has_mode, backup->mode,
+                    backup->has_speed, backup->speed,
+                    backup->has_bitmap, backup->bitmap,
+                    backup->has_on_source_error, backup->on_source_error,
+                    backup->has_on_target_error, backup->on_target_error,
+                    NULL, NULL,
+                    &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
         return;
@@ -2765,15 +2779,18 @@ out:
     aio_context_release(aio_context);
 }
 
-void qmp_drive_backup(const char *device, const char *target,
-                      bool has_format, const char *format,
-                      enum MirrorSyncMode sync,
-                      bool has_mode, enum NewImageMode mode,
-                      bool has_speed, int64_t speed,
-                      bool has_bitmap, const char *bitmap,
-                      bool has_on_source_error, BlockdevOnError on_source_error,
-                      bool has_on_target_error, BlockdevOnError on_target_error,
-                      Error **errp)
+static void do_drive_backup(const char *device, const char *target,
+                            bool has_format, const char *format,
+                            enum MirrorSyncMode sync,
+                            bool has_mode, enum NewImageMode mode,
+                            bool has_speed, int64_t speed,
+                            bool has_bitmap, const char *bitmap,
+                            bool has_on_source_error,
+                            BlockdevOnError on_source_error,
+                            bool has_on_target_error,
+                            BlockdevOnError on_target_error,
+                            BlockCompletionFunc *cb, void *opaque,
+                            Error **errp)
 {
     BlockBackend *blk;
     BlockDriverState *bs;
@@ -2887,9 +2904,16 @@ void qmp_drive_backup(const char *device, const char *target,
         }
     }
 
+    /* If we are not supplied with callback override info, use our defaults */
+    if (cb == NULL) {
+        cb = block_job_cb;
+    }
+    if (opaque == NULL) {
+        opaque = bs;
+    }
     backup_start(bs, target_bs, speed, sync, bmap,
                  on_source_error, on_target_error,
-                 block_job_cb, bs, &local_err);
+                 cb, opaque, &local_err);
     if (local_err != NULL) {
         bdrv_unref(target_bs);
         error_propagate(errp, local_err);
@@ -2900,6 +2924,22 @@ out:
     aio_context_release(aio_context);
 }
 
+void qmp_drive_backup(const char *device, const char *target,
+                      bool has_format, const char *format,
+                      enum MirrorSyncMode sync,
+                      bool has_mode, enum NewImageMode mode,
+                      bool has_speed, int64_t speed,
+                      bool has_bitmap, const char *bitmap,
+                      bool has_on_source_error, BlockdevOnError on_source_error,
+                      bool has_on_target_error, BlockdevOnError on_target_error,
+                      Error **errp)
+{
+    do_drive_backup(device, target, has_format, format, sync, has_mode, mode,
+                    has_speed, speed, has_bitmap, bitmap, has_on_source_error,
+                    on_source_error, has_on_target_error, on_target_error,
+                    NULL, NULL, errp);
+}
+
 BlockDeviceInfoList *qmp_query_named_block_nodes(Error **errp)
 {
     return bdrv_named_nodes_list();
-- 
2.1.0

[Qemu-devel] [PATCH v3 09/10] block: drive_backup transaction callback support

[John Snow]

This patch actually implements the transactional callback system
for the drive_backup action.

(1) We manually pick up a reference to the bitmap if present to allow
    its cleanup to be delayed until after all drive_backup jobs launched
    by the transaction have fully completed.

(2) We create a functional closure that envelops the original drive_backup
    callback, to be able to intercept the completion status and return code
    for the job.

(3) We add the drive_backup_cb method for the drive_backup action, which
    unpacks the completion information and invokes the final cleanup.

(4) backup_transaction_complete will perform the final cleanup on the
    backup job.

(5) In the case of transaction cancellation, drive_backup_cb is still
    responsible for cleaning up the mess we may have already made.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 block/backup.c            |  9 ++++++++
 blockdev.c                | 53 ++++++++++++++++++++++++++++++++++++++++++++---
 include/block/block_int.h |  8 +++++++
 3 files changed, 67 insertions(+), 3 deletions(-)

diff --git a/block/backup.c b/block/backup.c
index 62f8d2b..2b24eb6 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -233,6 +233,15 @@ typedef struct {
     int ret;
 } BackupCompleteData;
 
+void backup_transaction_complete(BlockJob *job, int ret)
+{
+    BackupBlockJob *s = container_of(job, BackupBlockJob, common);
+
+    if (s->sync_bitmap) {
+        bdrv_frozen_bitmap_decref(job->bs, s->sync_bitmap, ret);
+    }
+}
+
 static void backup_complete(BlockJob *job, void *opaque)
 {
     BackupBlockJob *s = container_of(job, BackupBlockJob, common);
diff --git a/blockdev.c b/blockdev.c
index eed23b8..7b12705 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1426,7 +1426,6 @@ static void transaction_action_callback(void *opaque, int ret)
  *
  * @return The callback to be used instead of @callback.
  */
-__attribute__((__unused__))
 static CallbackFn *new_action_cb_wrapper(BlkActionState *common,
                                          void *opaque,
                                          CallbackFn *callback,
@@ -1454,7 +1453,6 @@ static CallbackFn *new_action_cb_wrapper(BlkActionState *common,
 /**
  * Undo any actions performed by the above call.
  */
-__attribute__((__unused__))
 static void cancel_action_cb_wrapper(BlkActionState *common)
 {
     /* Stage 0: Wrapper was never created: */
@@ -1805,6 +1803,7 @@ static void do_drive_backup(const char *device, const char *target,
                             BlockdevOnError on_target_error,
                             BlockCompletionFunc *cb, void *opaque,
                             Error **errp);
+static void block_job_cb(void *opaque, int ret);
 
 static void drive_backup_prepare(BlkActionState *common, Error **errp)
 {
@@ -1813,6 +1812,9 @@ static void drive_backup_prepare(BlkActionState *common, Error **errp)
     BlockBackend *blk;
     DriveBackup *backup;
     Error *local_err = NULL;
+    CallbackFn *cb;
+    void *opaque;
+    BdrvDirtyBitmap *bmap = NULL;
 
     assert(common->action->kind == TRANSACTION_ACTION_KIND_DRIVE_BACKUP);
     backup = common->action->drive_backup;
@@ -1824,6 +1826,19 @@ static void drive_backup_prepare(BlkActionState *common, Error **errp)
     }
     bs = blk_bs(blk);
 
+    /* BackupBlockJob is opaque to us, so look up the bitmap ourselves */
+    if (backup->has_bitmap) {
+        bmap = bdrv_find_dirty_bitmap(bs, backup->bitmap);
+        if (!bmap) {
+            error_setg(errp, "Bitmap '%s' could not be found", backup->bitmap);
+            return;
+        }
+    }
+
+    /* Create our transactional callback wrapper,
+       and register that we'd like to call .cb() later. */
+    cb = new_action_cb_wrapper(common, bs, block_job_cb, &opaque);
+
     /* AioContext is released in .clean() */
     state->aio_context = bdrv_get_aio_context(bs);
     aio_context_acquire(state->aio_context);
@@ -1836,7 +1851,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,
-                    NULL, NULL,
+                    cb, opaque,
                     &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
@@ -1845,6 +1860,12 @@ static void drive_backup_prepare(BlkActionState *common, Error **errp)
 
     state->bs = bs;
     state->job = state->bs->job;
+    /* Keep the job alive until .cb(), too:
+     * References are only incremented after the job launches successfully. */
+    block_job_incref(state->job);
+    if (bmap) {
+        bdrv_dirty_bitmap_incref(bmap);
+    }
 }
 
 static void drive_backup_abort(BlkActionState *common)
@@ -1856,6 +1877,10 @@ static void drive_backup_abort(BlkActionState *common)
     if (bs && bs->job && bs->job == state->job) {
         block_job_cancel_sync(bs->job);
     }
+
+    /* Undo any callback actions we may have done. Putting down references
+     * obtained during prepare() is handled by cb(). */
+    cancel_action_cb_wrapper(common);
 }
 
 static void drive_backup_clean(BlkActionState *common)
@@ -1867,6 +1892,27 @@ static void drive_backup_clean(BlkActionState *common)
     }
 }
 
+static void drive_backup_cb(BlkActionState *common)
+{
+    BlkActionCallbackData *cb_data = common->cb_data;
+    BlockDriverState *bs = cb_data->opaque;
+    DriveBackupState *state = DO_UPCAST(DriveBackupState, common, common);
+
+    assert(state->bs == bs);
+    if (bs->job) {
+        assert(state->job == bs->job);
+    }
+
+    state->aio_context = bdrv_get_aio_context(bs);
+    aio_context_acquire(state->aio_context);
+
+    /* Note: We also have the individual job's return code in cb_data->ret */
+    backup_transaction_complete(state->job, common->transaction->status);
+    block_job_decref(state->job);
+
+    aio_context_release(state->aio_context);
+}
+
 typedef struct BlockdevBackupState {
     BlkActionState common;
     BlockDriverState *bs;
@@ -2054,6 +2100,7 @@ static const BlkActionOps actions[] = {
         .prepare = drive_backup_prepare,
         .abort = drive_backup_abort,
         .clean = drive_backup_clean,
+        .cb = drive_backup_cb,
     },
     [TRANSACTION_ACTION_KIND_BLOCKDEV_BACKUP] = {
         .instance_size = sizeof(BlockdevBackupState),
diff --git a/include/block/block_int.h b/include/block/block_int.h
index e0d5561..0d26713 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -619,6 +619,14 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target,
                   BlockCompletionFunc *cb, void *opaque,
                   Error **errp);
 
+/*
+ * backup_transaction_complete:
+ * @job: The BackupBlockJob that was associated with a transaction.
+ * @ret: The collective return code for the entire transaction.
+ *       Expects zero for success, non-zero for an error otherwise.
+ */
+void backup_transaction_complete(BlockJob *job, int ret);
+
 void blk_dev_change_media_cb(BlockBackend *blk, bool load);
 bool blk_dev_has_removable_media(BlockBackend *blk);
 void blk_dev_eject_request(BlockBackend *blk, bool force);
-- 
2.1.0

[Qemu-devel] [PATCH v3 10/10] iotests: 124 - transactional failure test

[John Snow]

Use a transaction to request an incremental backup across two drives.
Coerce one of the jobs to fail, and then re-run the transaction.

Verify that no bitmap data was lost due to the partial transaction
failure.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/qemu-iotests/124     | 120 ++++++++++++++++++++++++++++++++++++++++++++-
 tests/qemu-iotests/124.out |   4 +-
 2 files changed, 121 insertions(+), 3 deletions(-)

diff --git a/tests/qemu-iotests/124 b/tests/qemu-iotests/124
index 2d50594..772edd4 100644
--- a/tests/qemu-iotests/124
+++ b/tests/qemu-iotests/124
@@ -139,9 +139,12 @@ class TestIncrementalBackup(iotests.QMPTestCase):
     def do_qmp_backup(self, error='Input/output error', **kwargs):
         res = self.vm.qmp('drive-backup', **kwargs)
         self.assert_qmp(res, 'return', {})
+        return self.wait_qmp_backup(kwargs['device'], error)
 
+
+    def wait_qmp_backup(self, device, error='Input/output error'):
         event = self.vm.event_wait(name="BLOCK_JOB_COMPLETED",
-                                   match={'data': {'device': kwargs['device']}})
+                                   match={'data': {'device': device}})
         self.assertIsNotNone(event)
 
         try:
@@ -375,6 +378,121 @@ class TestIncrementalBackup(iotests.QMPTestCase):
         self.check_backups()
 
 
+    def test_transaction_failure(self):
+        '''Test: Verify backups made from a transaction that partially fails.
+
+        Add a second drive with its own unique pattern, and add a bitmap to each
+        drive. Use blkdebug to interfere with the backup on just one drive and
+        attempt to create a coherent incremental backup across both drives.
+
+        verify a failure in one but not both, then delete the failed stubs and
+        re-run the same transaction.
+
+        verify that both incrementals are created successfully.
+        '''
+
+        # Create a second drive, with pattern:
+        drive1 = self.add_node('drive1')
+        self.img_create(drive1['file'], drive1['fmt'])
+        io_write_patterns(drive1['file'], (('0x14', 0, 512),
+                                           ('0x5d', '1M', '32k'),
+                                           ('0xcd', '32M', '124k')))
+
+        # Create a blkdebug interface to this img as 'drive1'
+        result = self.vm.qmp('blockdev-add', options={
+            'id': drive1['id'],
+            'driver': drive1['fmt'],
+            'file': {
+                'driver': 'blkdebug',
+                'image': {
+                    'driver': 'file',
+                    'filename': drive1['file']
+                },
+                'set-state': [{
+                    'event': 'flush_to_disk',
+                    'state': 1,
+                    'new_state': 2
+                }],
+                'inject-error': [{
+                    'event': 'read_aio',
+                    'errno': 5,
+                    'state': 2,
+                    'immediately': False,
+                    'once': True
+                }],
+            }
+        })
+        self.assert_qmp(result, 'return', {})
+
+        # Create bitmaps and full backups for both drives
+        drive0 = self.drives[0]
+        dr0bm0 = self.add_bitmap('bitmap0', drive0)
+        dr1bm0 = self.add_bitmap('bitmap0', drive1)
+        self.create_anchor_backup(drive0)
+        self.create_anchor_backup(drive1)
+        self.assert_no_active_block_jobs()
+        self.assertFalse(self.vm.get_qmp_events(wait=False))
+
+        # Emulate some writes
+        self.hmp_io_writes(drive0['id'], (('0xab', 0, 512),
+                                          ('0xfe', '16M', '256k'),
+                                          ('0x64', '32736k', '64k')))
+        self.hmp_io_writes(drive1['id'], (('0xba', 0, 512),
+                                          ('0xef', '16M', '256k'),
+                                          ('0x46', '32736k', '64k')))
+
+        # Create incremental backup targets
+        target0 = self.prepare_backup(dr0bm0)
+        target1 = self.prepare_backup(dr1bm0)
+
+        # Ask for a new incremental backup per-each drive,
+        # expecting drive1's backup to fail:
+        transaction = [
+            transaction_drive_backup(drive0['id'], target0, sync='dirty-bitmap',
+                                     format=drive0['fmt'], mode='existing',
+                                     bitmap=dr0bm0.name),
+            transaction_drive_backup(drive1['id'], target1, sync='dirty-bitmap',
+                                     format=drive1['fmt'], mode='existing',
+                                     bitmap=dr1bm0.name),
+        ]
+        result = self.vm.qmp('transaction', actions=transaction)
+        self.assert_qmp(result, 'return', {})
+
+        # Observe that drive0's backup completes, but drive1's does not.
+        # Consume drive1's error and ensure all pending actions are completed.
+        self.assertTrue(self.wait_qmp_backup(drive0['id']))
+        self.assertFalse(self.wait_qmp_backup(drive1['id']))
+        error = self.vm.event_wait('BLOCK_JOB_ERROR')
+        self.assert_qmp(error, 'data', {'device': drive1['id'],
+                                        'action': 'report',
+                                        'operation': 'read'})
+        self.assertFalse(self.vm.get_qmp_events(wait=False))
+        self.assert_no_active_block_jobs()
+
+        # Delete drive0's successful target and eliminate our record of the
+        # unsuccessful drive1 target. Then re-run the same transaction.
+        dr0bm0.del_target()
+        dr1bm0.del_target()
+        target0 = self.prepare_backup(dr0bm0)
+        target1 = self.prepare_backup(dr1bm0)
+
+        # Re-run the exact same transaction.
+        result = self.vm.qmp('transaction', actions=transaction)
+        self.assert_qmp(result, 'return', {})
+
+        # Both should complete successfully this time.
+        self.assertTrue(self.wait_qmp_backup(drive0['id']))
+        self.assertTrue(self.wait_qmp_backup(drive1['id']))
+        self.make_reference_backup(dr0bm0)
+        self.make_reference_backup(dr1bm0)
+        self.assertFalse(self.vm.get_qmp_events(wait=False))
+        self.assert_no_active_block_jobs()
+
+        # And the images should of course validate.
+        self.vm.shutdown()
+        self.check_backups()
+
+
     def test_sync_dirty_bitmap_missing(self):
         self.assert_no_active_block_jobs()
         self.files.append(self.err_img)
diff --git a/tests/qemu-iotests/124.out b/tests/qemu-iotests/124.out
index 594c16f..dae404e 100644
--- a/tests/qemu-iotests/124.out
+++ b/tests/qemu-iotests/124.out
@@ -1,5 +1,5 @@
-........
+.........
 ----------------------------------------------------------------------
-Ran 8 tests
+Ran 9 tests
 
 OK
-- 
2.1.0

Re: [Qemu-devel] [PATCH v3 01/10] qapi: Add transaction support to block-dirty-bitmap operations

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1227 bytes --]

On 04/22/2015 06:04 PM, John Snow wrote:
> This adds two qmp commands to transactions.
> 
> block-dirty-bitmap-add allows you to create a bitmap simultaneously
> alongside a new full backup to accomplish a clean synchronization
> point.
> 
> block-dirty-bitmap-clear allows you to reset a bitmap back to as-if
> it were new, which can also be used alongside a full backup to
> accomplish a clean synchronization point.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> Signed-off-by: John Snow <jsnow@redhat.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---

> +++ b/docs/bitmaps.md
> @@ -97,7 +97,7 @@ which is included at the end of this document.
>  }
>  ```
>  
> -## Transactions (Not yet implemented)
> +## Transactions
>  
>  * Transactional commands are forthcoming in a future version,
>    and are not yet available for use. This section serves as

You got the section heading, but missed the next paragraph.

But (as you just proved in this attempt) deleting even more stale docs
doesn't invalidate my R-b on the code :)

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v3 02/10] iotests: add transactional incremental backup test

[Max Reitz]

On 23.04.2015 02:04, John Snow wrote:
> Test simple usage cases for using transactions to create
> and synchronize incremental backups.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   tests/qemu-iotests/124     | 54 ++++++++++++++++++++++++++++++++++++++++++++++
>   tests/qemu-iotests/124.out |  4 ++--
>   2 files changed, 56 insertions(+), 2 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>

Re: [Qemu-devel] [PATCH v3 05/10] block: add transactional callbacks feature

[Max Reitz]

On 23.04.2015 02:04, John Snow wrote:
> The goal here is to add a new method to transactions that allows
> developers to specify a callback that will get invoked only once
> all jobs spawned by a transaction are completed, allowing developers
> the chance to perform actions conditionally pending complete success,
> partial failure, or complete failure.
>
> In order to register the new callback to be invoked, a user must request
> a callback pointer and closure by calling new_action_cb_wrapper, which
> creates a wrapper around an opaque pointer and callback that would have
> originally been passed to e.g. backup_start().
>
> The function will return a function pointer and a new opaque pointer to
> be passed instead. The transaction system will effectively intercept the
> original callbacks and perform book-keeping on the transaction after it
> has delivered the original enveloped callback.
>
> This means that Transaction Action callback methods will be called after
> all callbacks triggered by all Actions in the Transactional group have
> been received.
>
> This feature has no knowledge of any jobs spawned by Actions that do not
> inform the system via new_action_cb_wrapper().
>
> For an example of how to use the feature, please skip ahead to:
> 'block: drive_backup transaction callback support' which serves as an example
> for how to hook up a post-transaction callback to the Drive Backup action.
>
>
> Note 1: Defining a callback method alone is not sufficient to have the new
>          method invoked. You must call new_action_cb_wrapper() AND ensure the
>          callback it returns is the one used as the callback for the job
>          launched by the action.
>
> Note 2: You can use this feature for any system that registers completions of
>          an asynchronous task via a callback of the form
>          (void *opaque, int ret), not just block job callbacks.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   blockdev.c | 183 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
>   1 file changed, 179 insertions(+), 4 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>

Re: [Qemu-devel] [PATCH v3 08/10] qmp: Add an implementation wrapper for qmp_drive_backup

[Max Reitz]

On 23.04.2015 02:04, John Snow wrote:
> We'd like to be able to specify the callback given to backup_start
> manually in the case of transactions, so split apart qmp_drive_backup
> into an implementation and a wrapper.
>
> Switch drive_backup_prepare to use the new wrapper, but don't overload
> the callback and closure yet.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   blockdev.c | 78 +++++++++++++++++++++++++++++++++++++++++++++++---------------
>   1 file changed, 59 insertions(+), 19 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>

Re: [Qemu-devel] [PATCH v3 09/10] block: drive_backup transaction callback support

[Max Reitz]

On 23.04.2015 02:04, John Snow wrote:
> This patch actually implements the transactional callback system
> for the drive_backup action.
>
> (1) We manually pick up a reference to the bitmap if present to allow
>      its cleanup to be delayed until after all drive_backup jobs launched
>      by the transaction have fully completed.
>
> (2) We create a functional closure that envelops the original drive_backup
>      callback, to be able to intercept the completion status and return code
>      for the job.
>
> (3) We add the drive_backup_cb method for the drive_backup action, which
>      unpacks the completion information and invokes the final cleanup.
>
> (4) backup_transaction_complete will perform the final cleanup on the
>      backup job.
>
> (5) In the case of transaction cancellation, drive_backup_cb is still
>      responsible for cleaning up the mess we may have already made.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   block/backup.c            |  9 ++++++++
>   blockdev.c                | 53 ++++++++++++++++++++++++++++++++++++++++++++---
>   include/block/block_int.h |  8 +++++++
>   3 files changed, 67 insertions(+), 3 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>

Re: [Qemu-devel] [PATCH v3 10/10] iotests: 124 - transactional failure test

[Max Reitz]

On 23.04.2015 02:04, John Snow wrote:
> Use a transaction to request an incremental backup across two drives.
> Coerce one of the jobs to fail, and then re-run the transaction.
>
> Verify that no bitmap data was lost due to the partial transaction
> failure.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   tests/qemu-iotests/124     | 120 ++++++++++++++++++++++++++++++++++++++++++++-
>   tests/qemu-iotests/124.out |   4 +-
>   2 files changed, 121 insertions(+), 3 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>

Re: [Qemu-devel] [Qemu-block] [PATCH v3 01/10] qapi: Add transaction support to block-dirty-bitmap operations

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1888 bytes --]

On Wed, Apr 22, 2015 at 08:04:44PM -0400, John Snow wrote:
> +static void block_dirty_bitmap_clear_prepare(BlkTransactionState *common,
> +                                             Error **errp)
> +{
> +    BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
> +                                             common, common);
> +    BlockDirtyBitmap *action;
> +
> +    action = common->action->block_dirty_bitmap_clear;
> +    state->bitmap = block_dirty_bitmap_lookup(action->node,
> +                                              action->name,
> +                                              &state->bs,
> +                                              &state->aio_context,
> +                                              errp);
> +    if (!state->bitmap) {
> +        return;
> +    }
> +
> +    if (bdrv_dirty_bitmap_frozen(state->bitmap)) {
> +        error_setg(errp, "Cannot modify a frozen bitmap");
> +        return;
> +    } else if (!bdrv_dirty_bitmap_enabled(state->bitmap)) {
> +        error_setg(errp, "Cannot clear a disabled bitmap");
> +        return;
> +    }
> +
> +    /* AioContext is released in .clean() */
> +}
> +
> +static void block_dirty_bitmap_clear_commit(BlkTransactionState *common)
> +{
> +    BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
> +                                             common, common);
> +    bdrv_clear_dirty_bitmap(state->bitmap);
> +}

These semantics don't work in this example:

[block-dirty-bitmap-clear,
 drive-backup]

Since drive-backup starts the blockjob in .prepare() but
block-dirty-bitmap-clear only clears the bitmap in .commit() the order
is wrong.

.prepare() has to do something non-destructive, like stashing away the
HBitmap and replacing it with an empty one.  Then .commit() can discard
the old bitmap while .abort() can move the old bitmap back to undo the
operation.

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH v3 01/10] qapi: Add transaction support to block-dirty-bitmap operations

[John Snow]

On 05/07/2015 10:54 AM, Stefan Hajnoczi wrote:
> On Wed, Apr 22, 2015 at 08:04:44PM -0400, John Snow wrote:
>> +static void block_dirty_bitmap_clear_prepare(BlkTransactionState
>> *common, +                                             Error
>> **errp) +{ +    BlockDirtyBitmapState *state =
>> DO_UPCAST(BlockDirtyBitmapState, +
>> common, common); +    BlockDirtyBitmap *action; + +    action =
>> common->action->block_dirty_bitmap_clear; +    state->bitmap =
>> block_dirty_bitmap_lookup(action->node, +
>> action->name, +
>> &state->bs, +
>> &state->aio_context, +
>> errp); +    if (!state->bitmap) { +        return; +    } + +
>> if (bdrv_dirty_bitmap_frozen(state->bitmap)) { +
>> error_setg(errp, "Cannot modify a frozen bitmap"); +
>> return; +    } else if
>> (!bdrv_dirty_bitmap_enabled(state->bitmap)) { +
>> error_setg(errp, "Cannot clear a disabled bitmap"); +
>> return; +    } + +    /* AioContext is released in .clean() */ 
>> +} + +static void
>> block_dirty_bitmap_clear_commit(BlkTransactionState *common) +{ +
>> BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState, +
>> common, common); +    bdrv_clear_dirty_bitmap(state->bitmap); +}
> 
> These semantics don't work in this example:
> 
> [block-dirty-bitmap-clear, drive-backup]
> 
> Since drive-backup starts the blockjob in .prepare() but 
> block-dirty-bitmap-clear only clears the bitmap in .commit() the
> order is wrong.
> 
> .prepare() has to do something non-destructive, like stashing away
> the HBitmap and replacing it with an empty one.  Then .commit() can
> discard the old bitmap while .abort() can move the old bitmap back
> to undo the operation.
> 
> Stefan
> 

Hmm, that's sort of gross. That means that any transactional command
*ever* destined to be used with drive-backup in any conceivable way
needs to move a lot more of its action forward to .prepare().

That sort of defeats the premise of .prepare() and .commit(), no? And
all because drive-backup jumped the gun.

That's going to get hard to maintain as we add more transactions.

--js

Re: [Qemu-devel] [Qemu-block] [PATCH v3 01/10] qapi: Add transaction support to block-dirty-bitmap operations

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 3201 bytes --]

On Thu, May 07, 2015 at 01:22:26PM -0400, John Snow wrote:
> 
> 
> On 05/07/2015 10:54 AM, Stefan Hajnoczi wrote:
> > On Wed, Apr 22, 2015 at 08:04:44PM -0400, John Snow wrote:
> >> +static void block_dirty_bitmap_clear_prepare(BlkTransactionState
> >> *common, +                                             Error
> >> **errp) +{ +    BlockDirtyBitmapState *state =
> >> DO_UPCAST(BlockDirtyBitmapState, +
> >> common, common); +    BlockDirtyBitmap *action; + +    action =
> >> common->action->block_dirty_bitmap_clear; +    state->bitmap =
> >> block_dirty_bitmap_lookup(action->node, +
> >> action->name, +
> >> &state->bs, +
> >> &state->aio_context, +
> >> errp); +    if (!state->bitmap) { +        return; +    } + +
> >> if (bdrv_dirty_bitmap_frozen(state->bitmap)) { +
> >> error_setg(errp, "Cannot modify a frozen bitmap"); +
> >> return; +    } else if
> >> (!bdrv_dirty_bitmap_enabled(state->bitmap)) { +
> >> error_setg(errp, "Cannot clear a disabled bitmap"); +
> >> return; +    } + +    /* AioContext is released in .clean() */ 
> >> +} + +static void
> >> block_dirty_bitmap_clear_commit(BlkTransactionState *common) +{ +
> >> BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState, +
> >> common, common); +    bdrv_clear_dirty_bitmap(state->bitmap); +}
> > 
> > These semantics don't work in this example:
> > 
> > [block-dirty-bitmap-clear, drive-backup]
> > 
> > Since drive-backup starts the blockjob in .prepare() but 
> > block-dirty-bitmap-clear only clears the bitmap in .commit() the
> > order is wrong.
> > 
> > .prepare() has to do something non-destructive, like stashing away
> > the HBitmap and replacing it with an empty one.  Then .commit() can
> > discard the old bitmap while .abort() can move the old bitmap back
> > to undo the operation.
> > 
> > Stefan
> > 
> 
> Hmm, that's sort of gross. That means that any transactional command
> *ever* destined to be used with drive-backup in any conceivable way
> needs to move a lot more of its action forward to .prepare().
> 
> That sort of defeats the premise of .prepare() and .commit(), no? And
> all because drive-backup jumped the gun.

No it doesn't.  Actions have to appear atomic to the qmp_transaction
caller.  Both approaches achieve that so they are both correct in
isolation.

The ambiguity is whether "commit the changes" for .commit() means
"changes take effect" or "discard stashed state, making undo
impossible".

I think the "discard stashed state, making undo impossible"
interpretation is good because .commit() is not allowed to fail.  That
function should only do things that never fail.

> That's going to get hard to maintain as we add more transactions.

Yes, we need to be consistent and stick to one of the interpretations in
order to guarantee ordering.

Unfortunately, there is already an inconsistency:

1. internal_snapshot - snapshot taken in .prepare()
2. external_snapshot - BDS node appended in .commit()
3. drive_backup - block job started in .prepare()
4. blockdev_backup - block job started in .prepare()

external_snapshot followed by internal_snapshot acts like the reverse
ordering!

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH v3 01/10] qapi: Add transaction support to block-dirty-bitmap operations

[Max Reitz]

On 08.05.2015 15:14, Stefan Hajnoczi wrote:
> On Thu, May 07, 2015 at 01:22:26PM -0400, John Snow wrote:
>>
>> On 05/07/2015 10:54 AM, Stefan Hajnoczi wrote:
>>> On Wed, Apr 22, 2015 at 08:04:44PM -0400, John Snow wrote:
>>>> +static void block_dirty_bitmap_clear_prepare(BlkTransactionState
>>>> *common, +                                             Error
>>>> **errp) +{ +    BlockDirtyBitmapState *state =
>>>> DO_UPCAST(BlockDirtyBitmapState, +
>>>> common, common); +    BlockDirtyBitmap *action; + +    action =
>>>> common->action->block_dirty_bitmap_clear; +    state->bitmap =
>>>> block_dirty_bitmap_lookup(action->node, +
>>>> action->name, +
>>>> &state->bs, +
>>>> &state->aio_context, +
>>>> errp); +    if (!state->bitmap) { +        return; +    } + +
>>>> if (bdrv_dirty_bitmap_frozen(state->bitmap)) { +
>>>> error_setg(errp, "Cannot modify a frozen bitmap"); +
>>>> return; +    } else if
>>>> (!bdrv_dirty_bitmap_enabled(state->bitmap)) { +
>>>> error_setg(errp, "Cannot clear a disabled bitmap"); +
>>>> return; +    } + +    /* AioContext is released in .clean() */
>>>> +} + +static void
>>>> block_dirty_bitmap_clear_commit(BlkTransactionState *common) +{ +
>>>> BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState, +
>>>> common, common); +    bdrv_clear_dirty_bitmap(state->bitmap); +}
>>> These semantics don't work in this example:
>>>
>>> [block-dirty-bitmap-clear, drive-backup]
>>>
>>> Since drive-backup starts the blockjob in .prepare() but
>>> block-dirty-bitmap-clear only clears the bitmap in .commit() the
>>> order is wrong.

Well, "starts the block job" is technically correct, but the block job 
doesn't run until later. If it were to really start in prepare, that 
would be wrong. Actually, the block job is initialized and yields, 
allowing the code handling the QMP transaction command to continue. I 
think in your example that means that the block job won't actually run 
until after block-dirty-bitmap-clear has been committed.

Max

>>>
>>> .prepare() has to do something non-destructive, like stashing away
>>> the HBitmap and replacing it with an empty one.  Then .commit() can
>>> discard the old bitmap while .abort() can move the old bitmap back
>>> to undo the operation.
>>>
>>> Stefan
>>>
>> Hmm, that's sort of gross. That means that any transactional command
>> *ever* destined to be used with drive-backup in any conceivable way
>> needs to move a lot more of its action forward to .prepare().
>>
>> That sort of defeats the premise of .prepare() and .commit(), no? And
>> all because drive-backup jumped the gun.
> No it doesn't.  Actions have to appear atomic to the qmp_transaction
> caller.  Both approaches achieve that so they are both correct in
> isolation.
>
> The ambiguity is whether "commit the changes" for .commit() means
> "changes take effect" or "discard stashed state, making undo
> impossible".
>
> I think the "discard stashed state, making undo impossible"
> interpretation is good because .commit() is not allowed to fail.  That
> function should only do things that never fail.
>
>> That's going to get hard to maintain as we add more transactions.
> Yes, we need to be consistent and stick to one of the interpretations in
> order to guarantee ordering.
>
> Unfortunately, there is already an inconsistency:
>
> 1. internal_snapshot - snapshot taken in .prepare()
> 2. external_snapshot - BDS node appended in .commit()
> 3. drive_backup - block job started in .prepare()
> 4. blockdev_backup - block job started in .prepare()
>
> external_snapshot followed by internal_snapshot acts like the reverse
> ordering!
>
> Stefan

Re: [Qemu-devel] [Qemu-block] [PATCH v3 01/10] qapi: Add transaction support to block-dirty-bitmap operations

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1732 bytes --]

On 05/08/2015 07:14 AM, Stefan Hajnoczi wrote:

> No it doesn't.  Actions have to appear atomic to the qmp_transaction
> caller.  Both approaches achieve that so they are both correct in
> isolation.
> 
> The ambiguity is whether "commit the changes" for .commit() means
> "changes take effect" or "discard stashed state, making undo
> impossible".
> 
> I think the "discard stashed state, making undo impossible"
> interpretation is good because .commit() is not allowed to fail.  That
> function should only do things that never fail.
> 
>> That's going to get hard to maintain as we add more transactions.
> 
> Yes, we need to be consistent and stick to one of the interpretations in
> order to guarantee ordering.
> 
> Unfortunately, there is already an inconsistency:
> 
> 1. internal_snapshot - snapshot taken in .prepare()
> 2. external_snapshot - BDS node appended in .commit()
> 3. drive_backup - block job started in .prepare()
> 4. blockdev_backup - block job started in .prepare()
> 
> external_snapshot followed by internal_snapshot acts like the reverse
> ordering!

Is that fatal, though?  Let's see if I'm understanding the problem
correctly: if you start with a.qcow2, then
 external_snapshot followed by internal_snapshot
should create b.qcow2 then the internal snapshot inside b.qcow2, while
 internal_snapshot followed by external_snapshot
should create the internal snapshot inside a.qcow2, then create b.qcow2

But since we create the BDS node later than the internal snapshot is
taken, both sequences currently cause the internal snapshot to live in
a.qcow2.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH v3 01/10] qapi: Add transaction support to block-dirty-bitmap operations

[John Snow]

On 05/08/2015 09:17 AM, Max Reitz wrote:
> On 08.05.2015 15:14, Stefan Hajnoczi wrote:
>> On Thu, May 07, 2015 at 01:22:26PM -0400, John Snow wrote:
>>>
>>> On 05/07/2015 10:54 AM, Stefan Hajnoczi wrote:
>>>> On Wed, Apr 22, 2015 at 08:04:44PM -0400, John Snow wrote:
>>>>> +static void block_dirty_bitmap_clear_prepare(BlkTransactionState
>>>>> *common, +                                             Error
>>>>> **errp) +{ +    BlockDirtyBitmapState *state =
>>>>> DO_UPCAST(BlockDirtyBitmapState, +
>>>>> common, common); +    BlockDirtyBitmap *action; + +    action =
>>>>> common->action->block_dirty_bitmap_clear; +    state->bitmap =
>>>>> block_dirty_bitmap_lookup(action->node, +
>>>>> action->name, +
>>>>> &state->bs, +
>>>>> &state->aio_context, +
>>>>> errp); +    if (!state->bitmap) { +        return; +    } + +
>>>>> if (bdrv_dirty_bitmap_frozen(state->bitmap)) { +
>>>>> error_setg(errp, "Cannot modify a frozen bitmap"); +
>>>>> return; +    } else if
>>>>> (!bdrv_dirty_bitmap_enabled(state->bitmap)) { +
>>>>> error_setg(errp, "Cannot clear a disabled bitmap"); +
>>>>> return; +    } + +    /* AioContext is released in .clean() */
>>>>> +} + +static void
>>>>> block_dirty_bitmap_clear_commit(BlkTransactionState *common) +{ +
>>>>> BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState, +
>>>>> common, common); +    bdrv_clear_dirty_bitmap(state->bitmap); +}
>>>> These semantics don't work in this example:
>>>>
>>>> [block-dirty-bitmap-clear, drive-backup]
>>>>
>>>> Since drive-backup starts the blockjob in .prepare() but
>>>> block-dirty-bitmap-clear only clears the bitmap in .commit() the
>>>> order is wrong.
> 
> Well, "starts the block job" is technically correct, but the block job
> doesn't run until later. If it were to really start in prepare, that
> would be wrong. Actually, the block job is initialized and yields,
> allowing the code handling the QMP transaction command to continue. I
> think in your example that means that the block job won't actually run
> until after block-dirty-bitmap-clear has been committed.
> 
> Max
> 

The important thing is that we get the contents of the drive as it was,
according to the bitmap as it was, when we start the job.

So if clear doesn't actually modify the bitmap until the commit() phase,
but drive-backup actually goes until the first yield() in prepare...

We're actually going to see an assertion() failure, likely. drive-backup
will freeze the bitmap with a successor, but then the clear transaction
will try to "commit" the changes and try to modify a frozen bitmap.

Oops.

What we (The royal we...) need to figure out is how we want to solve
this problem.

>>>>
>>>> .prepare() has to do something non-destructive, like stashing away
>>>> the HBitmap and replacing it with an empty one.  Then .commit() can
>>>> discard the old bitmap while .abort() can move the old bitmap back
>>>> to undo the operation.
>>>>
>>>> Stefan
>>>>
>>> Hmm, that's sort of gross. That means that any transactional command
>>> *ever* destined to be used with drive-backup in any conceivable way
>>> needs to move a lot more of its action forward to .prepare().
>>>
>>> That sort of defeats the premise of .prepare() and .commit(), no? And
>>> all because drive-backup jumped the gun.
>> No it doesn't.  Actions have to appear atomic to the qmp_transaction
>> caller.  Both approaches achieve that so they are both correct in
>> isolation.
>>
>> The ambiguity is whether "commit the changes" for .commit() means
>> "changes take effect" or "discard stashed state, making undo
>> impossible".
>>
>> I think the "discard stashed state, making undo impossible"
>> interpretation is good because .commit() is not allowed to fail.  That
>> function should only do things that never fail.
>>

To be clear, you are favoring drive-backup's interpretation of the
prepare and commit phases. I had been operating under the other
interpretation.

I think I like the semantics of my interpretation better, but have to
admit it's a lot harder programmatically to enforce "commit cannot fail"
for all of the transactions we support under mine, so your
interpretation is probably the right way to go for sanity's sake -- we
just need to add a bit of documentation to make it clear.

I suppose in this case clear() isn't too hard to modify -- just rip the
Hbitmap out of it and replace it with a new empty one. Don't free() the
old one until commit(). Should be a fairly inexpensive operation.

I will have to re-audit all the existing transactions to make sure these
semantics are consistent, though.

>>> That's going to get hard to maintain as we add more transactions.
>> Yes, we need to be consistent and stick to one of the interpretations in
>> order to guarantee ordering.
>>
>> Unfortunately, there is already an inconsistency:
>>
>> 1. internal_snapshot - snapshot taken in .prepare()
>> 2. external_snapshot - BDS node appended in .commit()
>> 3. drive_backup - block job started in .prepare()
>> 4. blockdev_backup - block job started in .prepare()
>>
>> external_snapshot followed by internal_snapshot acts like the reverse
>> ordering!
>>
>> Stefan
> 

What a mess!

--js

Re: [Qemu-devel] [Qemu-block] [PATCH v3 01/10] qapi: Add transaction support to block-dirty-bitmap operations

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2151 bytes --]

On Fri, May 08, 2015 at 08:29:09AM -0600, Eric Blake wrote:
> On 05/08/2015 07:14 AM, Stefan Hajnoczi wrote:
> 
> > No it doesn't.  Actions have to appear atomic to the qmp_transaction
> > caller.  Both approaches achieve that so they are both correct in
> > isolation.
> > 
> > The ambiguity is whether "commit the changes" for .commit() means
> > "changes take effect" or "discard stashed state, making undo
> > impossible".
> > 
> > I think the "discard stashed state, making undo impossible"
> > interpretation is good because .commit() is not allowed to fail.  That
> > function should only do things that never fail.
> > 
> >> That's going to get hard to maintain as we add more transactions.
> > 
> > Yes, we need to be consistent and stick to one of the interpretations in
> > order to guarantee ordering.
> > 
> > Unfortunately, there is already an inconsistency:
> > 
> > 1. internal_snapshot - snapshot taken in .prepare()
> > 2. external_snapshot - BDS node appended in .commit()
> > 3. drive_backup - block job started in .prepare()
> > 4. blockdev_backup - block job started in .prepare()
> > 
> > external_snapshot followed by internal_snapshot acts like the reverse
> > ordering!
> 
> Is that fatal, though?

Yes, ordering is critical when add-bitmap or clear-bitmap are combined
with drive-backup.  Typically the drive-backup must happen after
add-bitmap or clear-bitmap.

There is probably no one who uses external and internal snapshots
together in a single 'transaction' command, so my example is contrived
but it's the same problem.

> Let's see if I'm understanding the problem
> correctly: if you start with a.qcow2, then
>  external_snapshot followed by internal_snapshot
> should create b.qcow2 then the internal snapshot inside b.qcow2, while
>  internal_snapshot followed by external_snapshot
> should create the internal snapshot inside a.qcow2, then create b.qcow2
> 
> But since we create the BDS node later than the internal snapshot is
> taken, both sequences currently cause the internal snapshot to live in
> a.qcow2.

Right.  Ordering is not honored :(.

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH v3 02/10] iotests: add transactional incremental backup test

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 450 bytes --]

On Wed, Apr 22, 2015 at 08:04:45PM -0400, John Snow wrote:
> Test simple usage cases for using transactions to create
> and synchronize incremental backups.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  tests/qemu-iotests/124     | 54 ++++++++++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/124.out |  4 ++--
>  2 files changed, 56 insertions(+), 2 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH v3 03/10] block: rename BlkTransactionState and BdrvActionOps

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1051 bytes --]

On Wed, Apr 22, 2015 at 08:04:46PM -0400, John Snow wrote:
> 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 <jsnow@redhat.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> ---
>  blockdev.c | 114 ++++++++++++++++++++++++++++++++++---------------------------
>  1 file changed, 64 insertions(+), 50 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH v3 01/10] qapi: Add transaction support to block-dirty-bitmap operations

[Kevin Wolf]

[-- Attachment #1: Type: text/plain, Size: 1909 bytes --]

Am 11.05.2015 um 15:10 hat Stefan Hajnoczi geschrieben:
> On Fri, May 08, 2015 at 08:29:09AM -0600, Eric Blake wrote:
> > On 05/08/2015 07:14 AM, Stefan Hajnoczi wrote:
> > 
> > > No it doesn't.  Actions have to appear atomic to the qmp_transaction
> > > caller.  Both approaches achieve that so they are both correct in
> > > isolation.
> > > 
> > > The ambiguity is whether "commit the changes" for .commit() means
> > > "changes take effect" or "discard stashed state, making undo
> > > impossible".
> > > 
> > > I think the "discard stashed state, making undo impossible"
> > > interpretation is good because .commit() is not allowed to fail.  That
> > > function should only do things that never fail.
> > > 
> > >> That's going to get hard to maintain as we add more transactions.
> > > 
> > > Yes, we need to be consistent and stick to one of the interpretations in
> > > order to guarantee ordering.
> > > 
> > > Unfortunately, there is already an inconsistency:
> > > 
> > > 1. internal_snapshot - snapshot taken in .prepare()
> > > 2. external_snapshot - BDS node appended in .commit()
> > > 3. drive_backup - block job started in .prepare()
> > > 4. blockdev_backup - block job started in .prepare()
> > > 
> > > external_snapshot followed by internal_snapshot acts like the reverse
> > > ordering!
> > 
> > Is that fatal, though?
> 
> Yes, ordering is critical when add-bitmap or clear-bitmap are combined
> with drive-backup.  Typically the drive-backup must happen after
> add-bitmap or clear-bitmap.

Is there a reason to include add-bitmap/clear-bitmap in the transaction
rather than preparing everything before the transaction and then only
starting the backup (possibly of multiple disks) in a transaction?

The original assumption was that there are no interdependencies between
different transaction commands and the order is undefined.

Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]