* [Qemu-devel] [PATCH v5 01/10] qapi: Add transaction support to block-dirty-bitmap operations
2015-06-04 21:46 [Qemu-devel] [PATCH v5 00/10] block: incremental backup transactions John Snow
@ 2015-06-04 21:46 ` John Snow
2015-06-05 12:12 ` Stefan Hajnoczi
2015-06-04 21:46 ` [Qemu-devel] [PATCH v5 02/10] iotests: add transactional incremental backup test John Snow
` (8 subsequent siblings)
9 siblings, 1 reply; 17+ messages in thread
From: John Snow @ 2015-06-04 21:46 UTC (permalink / raw)
To: qemu-block
Cc: kwolf, famz, John Snow, qemu-devel, mreitz, vsementsov, stefanha
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>
---
block.c | 19 +++++++-
blockdev.c | 114 +++++++++++++++++++++++++++++++++++++++++++++-
docs/bitmaps.md | 6 +--
include/block/block.h | 1 -
include/block/block_int.h | 3 ++
qapi-schema.json | 6 ++-
6 files changed, 139 insertions(+), 10 deletions(-)
diff --git a/block.c b/block.c
index 2b9ceae..a481654 100644
--- a/block.c
+++ b/block.c
@@ -3329,10 +3329,25 @@ void bdrv_reset_dirty_bitmap(BdrvDirtyBitmap *bitmap,
hbitmap_reset(bitmap->bitmap, cur_sector, nr_sectors);
}
-void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap)
+void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap **out)
{
assert(bdrv_dirty_bitmap_enabled(bitmap));
- hbitmap_reset(bitmap->bitmap, 0, bitmap->size);
+ if (!out) {
+ hbitmap_reset(bitmap->bitmap, 0, bitmap->size);
+ } else {
+ HBitmap *backup = bitmap->bitmap;
+ bitmap->bitmap = hbitmap_alloc(bitmap->size,
+ hbitmap_granularity(backup));
+ *out = backup;
+ }
+}
+
+void bdrv_undo_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap *in)
+{
+ HBitmap *tmp = bitmap->bitmap;
+ assert(bdrv_dirty_bitmap_enabled(bitmap));
+ bitmap->bitmap = in;
+ hbitmap_free(tmp);
}
void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector,
diff --git a/blockdev.c b/blockdev.c
index 5eaf77e..a62cc4b 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1694,6 +1694,106 @@ static void blockdev_backup_clean(BlkTransactionState *common)
}
}
+typedef struct BlockDirtyBitmapState {
+ BlkTransactionState common;
+ BdrvDirtyBitmap *bitmap;
+ BlockDriverState *bs;
+ AioContext *aio_context;
+ HBitmap *backup;
+ 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 and released 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;
+ }
+
+ bdrv_clear_dirty_bitmap(state->bitmap, &state->backup);
+ /* AioContext is released in .clean() */
+}
+
+static void block_dirty_bitmap_clear_abort(BlkTransactionState *common)
+{
+ BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
+ common, common);
+
+ bdrv_undo_clear_dirty_bitmap(state->bitmap, state->backup);
+}
+
+static void block_dirty_bitmap_clear_commit(BlkTransactionState *common)
+{
+ BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
+ common, common);
+
+ hbitmap_free(state->backup);
+}
+
+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");
@@ -1734,6 +1834,18 @@ 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,
+ .abort = block_dirty_bitmap_clear_abort,
+ .clean = block_dirty_bitmap_clear_clean,
+ }
};
/*
@@ -2107,7 +2219,7 @@ void qmp_block_dirty_bitmap_clear(const char *node, const char *name,
goto out;
}
- bdrv_clear_dirty_bitmap(bitmap);
+ bdrv_clear_dirty_bitmap(bitmap, NULL);
out:
aio_context_release(aio_context);
diff --git a/docs/bitmaps.md b/docs/bitmaps.md
index f066b48..a60fee1 100644
--- a/docs/bitmaps.md
+++ b/docs/bitmaps.md
@@ -97,11 +97,7 @@ which is included at the end of this document.
}
```
-## Transactions (Not yet implemented)
-
-* Transactional commands are forthcoming in a future version,
- and are not yet available for use. This section serves as
- documentation of intent for their design and usage.
+## Transactions
### Justification
diff --git a/include/block/block.h b/include/block/block.h
index f7680b6..3d62d3e 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -482,7 +482,6 @@ void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap,
int64_t cur_sector, int nr_sectors);
void bdrv_reset_dirty_bitmap(BdrvDirtyBitmap *bitmap,
int64_t cur_sector, int nr_sectors);
-void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap);
void bdrv_dirty_iter_init(BdrvDirtyBitmap *bitmap, struct HBitmapIter *hbi);
void bdrv_set_dirty_iter(struct HBitmapIter *hbi, int64_t offset);
int64_t bdrv_get_dirty_count(BdrvDirtyBitmap *bitmap);
diff --git a/include/block/block_int.h b/include/block/block_int.h
index f004378..5af712f 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -641,4 +641,7 @@ void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector, int nr_sectors);
void bdrv_reset_dirty(BlockDriverState *bs, int64_t cur_sector,
int nr_sectors);
+void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap **out);
+void bdrv_undo_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap *in);
+
#endif /* BLOCK_INT_H */
diff --git a/qapi-schema.json b/qapi-schema.json
index 0662a9b..bbd4b3a 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1471,6 +1471,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': {
@@ -1478,7 +1480,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
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [Qemu-devel] [PATCH v5 01/10] qapi: Add transaction support to block-dirty-bitmap operations
2015-06-04 21:46 ` [Qemu-devel] [PATCH v5 01/10] qapi: Add transaction support to block-dirty-bitmap operations John Snow
@ 2015-06-05 12:12 ` Stefan Hajnoczi
0 siblings, 0 replies; 17+ messages in thread
From: Stefan Hajnoczi @ 2015-06-05 12:12 UTC (permalink / raw)
To: John Snow; +Cc: kwolf, famz, qemu-block, qemu-devel, mreitz, vsementsov
[-- Attachment #1: Type: text/plain, Size: 992 bytes --]
On Thu, Jun 04, 2015 at 05:46:03PM -0400, 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>
> ---
> block.c | 19 +++++++-
> blockdev.c | 114 +++++++++++++++++++++++++++++++++++++++++++++-
> docs/bitmaps.md | 6 +--
> include/block/block.h | 1 -
> include/block/block_int.h | 3 ++
> qapi-schema.json | 6 ++-
> 6 files changed, 139 insertions(+), 10 deletions(-)
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 17+ messages in thread
* [Qemu-devel] [PATCH v5 02/10] iotests: add transactional incremental backup test
2015-06-04 21:46 [Qemu-devel] [PATCH v5 00/10] block: incremental backup transactions John Snow
2015-06-04 21:46 ` [Qemu-devel] [PATCH v5 01/10] qapi: Add transaction support to block-dirty-bitmap operations John Snow
@ 2015-06-04 21:46 ` John Snow
2015-06-04 21:46 ` [Qemu-devel] [PATCH v5 03/10] block: rename BlkTransactionState and BdrvActionOps John Snow
` (7 subsequent siblings)
9 siblings, 0 replies; 17+ messages in thread
From: John Snow @ 2015-06-04 21:46 UTC (permalink / raw)
To: qemu-block
Cc: kwolf, famz, John Snow, qemu-devel, mreitz, vsementsov, stefanha
Test simple usage cases for using transactions to create
and synchronize incremental backups.
Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@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
^ permalink raw reply related [flat|nested] 17+ messages in thread* [Qemu-devel] [PATCH v5 03/10] block: rename BlkTransactionState and BdrvActionOps
2015-06-04 21:46 [Qemu-devel] [PATCH v5 00/10] block: incremental backup transactions John Snow
2015-06-04 21:46 ` [Qemu-devel] [PATCH v5 01/10] qapi: Add transaction support to block-dirty-bitmap operations John Snow
2015-06-04 21:46 ` [Qemu-devel] [PATCH v5 02/10] iotests: add transactional incremental backup test John Snow
@ 2015-06-04 21:46 ` John Snow
2015-06-04 21:46 ` [Qemu-devel] [PATCH v5 04/10] block: re-add BlkTransactionState John Snow
` (6 subsequent siblings)
9 siblings, 0 replies; 17+ messages in thread
From: John Snow @ 2015-06-04 21:46 UTC (permalink / raw)
To: qemu-block
Cc: kwolf, famz, John Snow, qemu-devel, mreitz, vsementsov, stefanha
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>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
---
blockdev.c | 116 ++++++++++++++++++++++++++++++++++---------------------------
1 file changed, 65 insertions(+), 51 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index a62cc4b..6df575d 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;
@@ -1359,7 +1373,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);
@@ -1382,7 +1396,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);
@@ -1394,13 +1408,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;
@@ -1521,7 +1535,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);
@@ -1539,7 +1553,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);
@@ -1552,13 +1566,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;
@@ -1598,7 +1612,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;
@@ -1609,7 +1623,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);
@@ -1619,13 +1633,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;
@@ -1674,7 +1688,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;
@@ -1685,7 +1699,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);
@@ -1695,7 +1709,7 @@ static void blockdev_backup_clean(BlkTransactionState *common)
}
typedef struct BlockDirtyBitmapState {
- BlkTransactionState common;
+ BlkActionState common;
BdrvDirtyBitmap *bitmap;
BlockDriverState *bs;
AioContext *aio_context;
@@ -1703,7 +1717,7 @@ typedef struct BlockDirtyBitmapState {
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;
@@ -1724,7 +1738,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,
@@ -1739,7 +1753,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,
@@ -1768,7 +1782,7 @@ static void block_dirty_bitmap_clear_prepare(BlkTransactionState *common,
/* AioContext is released in .clean() */
}
-static void block_dirty_bitmap_clear_abort(BlkTransactionState *common)
+static void block_dirty_bitmap_clear_abort(BlkActionState *common)
{
BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
common, common);
@@ -1776,7 +1790,7 @@ static void block_dirty_bitmap_clear_abort(BlkTransactionState *common)
bdrv_undo_clear_dirty_bitmap(state->bitmap, state->backup);
}
-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);
@@ -1784,7 +1798,7 @@ static void block_dirty_bitmap_clear_commit(BlkTransactionState *common)
hbitmap_free(state->backup);
}
-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);
@@ -1794,17 +1808,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,
@@ -1824,7 +1838,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,
},
@@ -1855,10 +1869,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 */
@@ -1867,7 +1881,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
^ permalink raw reply related [flat|nested] 17+ messages in thread* [Qemu-devel] [PATCH v5 04/10] block: re-add BlkTransactionState
2015-06-04 21:46 [Qemu-devel] [PATCH v5 00/10] block: incremental backup transactions John Snow
` (2 preceding siblings ...)
2015-06-04 21:46 ` [Qemu-devel] [PATCH v5 03/10] block: rename BlkTransactionState and BdrvActionOps John Snow
@ 2015-06-04 21:46 ` John Snow
2015-06-05 17:27 ` Max Reitz
2015-06-04 21:46 ` [Qemu-devel] [PATCH v5 05/10] block: add transactional callbacks feature John Snow
` (5 subsequent siblings)
9 siblings, 1 reply; 17+ messages in thread
From: John Snow @ 2015-06-04 21:46 UTC (permalink / raw)
To: qemu-block
Cc: kwolf, famz, John Snow, qemu-devel, mreitz, vsementsov, stefanha
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>
---
blockdev.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-------
1 file changed, 56 insertions(+), 7 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index 6df575d..dea57b4 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.
+ *
+ * @refcnt: A reference count for this object.
+ * @actions: A list of all Actions in the Transaction.
+ */
+typedef struct BlkTransactionState {
+ int refcnt;
+ QTAILQ_HEAD(actions, BlkActionState) actions;
+} BlkTransactionState;
+
+/**
* BlkActionState:
* Describes one Action's state within a Transaction.
*
@@ -1267,9 +1280,42 @@ 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);
+
+ bts->refcnt = 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->refcnt--;
+ if (bts->refcnt == 0) {
+ destroy_blk_transaction_state(bts);
+ return NULL;
+ }
+ return bts;
+}
+
/* internal snapshot private data */
typedef struct InternalSnapshotState {
BlkActionState common;
@@ -1870,10 +1916,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();
@@ -1894,7 +1940,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) {
@@ -1903,7 +1949,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);
}
@@ -1914,18 +1960,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
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [Qemu-devel] [PATCH v5 04/10] block: re-add BlkTransactionState
2015-06-04 21:46 ` [Qemu-devel] [PATCH v5 04/10] block: re-add BlkTransactionState John Snow
@ 2015-06-05 17:27 ` Max Reitz
0 siblings, 0 replies; 17+ messages in thread
From: Max Reitz @ 2015-06-05 17:27 UTC (permalink / raw)
To: John Snow, qemu-block; +Cc: kwolf, famz, qemu-devel, vsementsov, stefanha
On 04.06.2015 23:46, John Snow wrote:
> 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>
> ---
> blockdev.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-------
> 1 file changed, 56 insertions(+), 7 deletions(-)
Reviewed-by: Max Reitz <mreitz@redhat.com>
^ permalink raw reply [flat|nested] 17+ messages in thread
* [Qemu-devel] [PATCH v5 05/10] block: add transactional callbacks feature
2015-06-04 21:46 [Qemu-devel] [PATCH v5 00/10] block: incremental backup transactions John Snow
` (3 preceding siblings ...)
2015-06-04 21:46 ` [Qemu-devel] [PATCH v5 04/10] block: re-add BlkTransactionState John Snow
@ 2015-06-04 21:46 ` John Snow
2015-06-05 12:45 ` Stefan Hajnoczi
2015-06-04 21:46 ` [Qemu-devel] [PATCH v5 06/10] block: add delayed bitmap successor cleanup John Snow
` (4 subsequent siblings)
9 siblings, 1 reply; 17+ messages in thread
From: John Snow @ 2015-06-04 21:46 UTC (permalink / raw)
To: qemu-block
Cc: kwolf, famz, John Snow, qemu-devel, mreitz, vsementsov, stefanha
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 dea57b4..9db8202 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.
*
* @refcnt: A reference count for this object.
+ * @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 refcnt;
+ 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;
};
@@ -1292,6 +1325,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;
@@ -1299,23 +1344,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->refcnt--;
if (bts->refcnt == 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->refcnt > 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->refcnt++;
+
+ /* 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;
@@ -1938,8 +2111,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);
@@ -1970,10 +2145,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
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [Qemu-devel] [PATCH v5 05/10] block: add transactional callbacks feature
2015-06-04 21:46 ` [Qemu-devel] [PATCH v5 05/10] block: add transactional callbacks feature John Snow
@ 2015-06-05 12:45 ` Stefan Hajnoczi
0 siblings, 0 replies; 17+ messages in thread
From: Stefan Hajnoczi @ 2015-06-05 12:45 UTC (permalink / raw)
To: John Snow; +Cc: kwolf, famz, qemu-block, qemu-devel, mreitz, vsementsov
[-- Attachment #1: Type: text/plain, Size: 3974 bytes --]
On Thu, Jun 04, 2015 at 05:46:07PM -0400, 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(-)
>
> diff --git a/blockdev.c b/blockdev.c
> index dea57b4..9db8202 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.
> *
> * @refcnt: A reference count for this object.
> + * @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 refcnt;
> + int status;
> QTAILQ_HEAD(actions, BlkActionState) actions;
> } BlkTransactionState;
>
> +typedef void (CallbackFn)(void *opaque, int ret);
Please use BlockCompletionFunc from aio.h:
typedef void BlockCompletionFunc(void *opaque, int ret);
[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 17+ messages in thread
* [Qemu-devel] [PATCH v5 06/10] block: add delayed bitmap successor cleanup
2015-06-04 21:46 [Qemu-devel] [PATCH v5 00/10] block: incremental backup transactions John Snow
` (4 preceding siblings ...)
2015-06-04 21:46 ` [Qemu-devel] [PATCH v5 05/10] block: add transactional callbacks feature John Snow
@ 2015-06-04 21:46 ` John Snow
2015-06-05 12:56 ` Stefan Hajnoczi
2015-06-04 21:46 ` [Qemu-devel] [PATCH v5 07/10] qmp: Add an implementation wrapper for qmp_drive_backup John Snow
` (3 subsequent siblings)
9 siblings, 1 reply; 17+ messages in thread
From: John Snow @ 2015-06-04 21:46 UTC (permalink / raw)
To: qemu-block
Cc: kwolf, famz, John Snow, qemu-devel, mreitz, vsementsov, stefanha
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.
As a consequence of moving the bitmap successor cleanup actions behind
the frozen bitmap decref operation, the previous operations are made
static, their errp parameter deleted, and some user-facing errors are
replaced by assertions.
"Also," The code for cleaning up a bitmap is also moved from backup_run to
backup_complete.
Signed-off-by: John Snow <jsnow@redhat.com>
---
block.c | 78 +++++++++++++++++++++++++++++++++++++++------------
block/backup.c | 20 +++++--------
include/block/block.h | 10 +++----
3 files changed, 71 insertions(+), 37 deletions(-)
diff --git a/block.c b/block.c
index a481654..1eb81ac 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;
};
@@ -3156,6 +3164,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;
}
@@ -3163,19 +3172,13 @@ 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)
{
char *name;
BdrvDirtyBitmap *successor = bitmap->successor;
- if (successor == NULL) {
- error_setg(errp, "Cannot relinquish control if "
- "there's no successor present");
- return NULL;
- }
-
+ assert(successor);
name = bitmap->name;
bitmap->name = NULL;
successor->name = name;
@@ -3190,19 +3193,13 @@ 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)
{
BdrvDirtyBitmap *successor = parent->successor;
- if (!successor) {
- error_setg(errp, "Cannot reclaim a successor when none is present");
- return NULL;
- }
-
+ assert(successor);
if (!hbitmap_merge(parent->bitmap, successor->bitmap)) {
- error_setg(errp, "Merging of parent and successor bitmap failed");
return NULL;
}
bdrv_release_dirty_bitmap(bs, successor);
@@ -3211,6 +3208,51 @@ BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
return parent;
}
+static BdrvDirtyBitmap *bdrv_free_bitmap_successor(BlockDriverState *bs,
+ BdrvDirtyBitmap *parent)
+{
+ assert(!parent->successor_refcount);
+ assert(parent->successor);
+
+ switch (parent->act) {
+ case SUCCESSOR_ACTION_RECLAIM:
+ return bdrv_reclaim_dirty_bitmap(bs, parent);
+ case SUCCESSOR_ACTION_ABDICATE:
+ return bdrv_dirty_bitmap_abdicate(bs, parent);
+ 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_frozen_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 d3f648d..4ac0be8 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 3d62d3e..b7892d2 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -459,12 +459,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_frozen_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
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [Qemu-devel] [PATCH v5 06/10] block: add delayed bitmap successor cleanup
2015-06-04 21:46 ` [Qemu-devel] [PATCH v5 06/10] block: add delayed bitmap successor cleanup John Snow
@ 2015-06-05 12:56 ` Stefan Hajnoczi
2015-06-05 15:56 ` John Snow
0 siblings, 1 reply; 17+ messages in thread
From: Stefan Hajnoczi @ 2015-06-05 12:56 UTC (permalink / raw)
To: John Snow; +Cc: kwolf, famz, qemu-block, qemu-devel, mreitz, vsementsov
[-- Attachment #1: Type: text/plain, Size: 1282 bytes --]
On Thu, Jun 04, 2015 at 05:46:08PM -0400, John Snow wrote:
> @@ -3190,19 +3193,13 @@ 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)
> {
> BdrvDirtyBitmap *successor = parent->successor;
>
> - if (!successor) {
> - error_setg(errp, "Cannot reclaim a successor when none is present");
> - return NULL;
> - }
> -
> + assert(successor);
> if (!hbitmap_merge(parent->bitmap, successor->bitmap)) {
> - error_setg(errp, "Merging of parent and successor bitmap failed");
> return NULL;
> }
Is this reachable? If the bitmap size and granularity match then
hbitmap_merge() does not fail.
This should probably be covered with an assertion instead to show that
this is not allowed to happen.
[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [Qemu-devel] [PATCH v5 06/10] block: add delayed bitmap successor cleanup
2015-06-05 12:56 ` Stefan Hajnoczi
@ 2015-06-05 15:56 ` John Snow
0 siblings, 0 replies; 17+ messages in thread
From: John Snow @ 2015-06-05 15:56 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: kwolf, famz, qemu-block, qemu-devel, mreitz, vsementsov
On 06/05/2015 08:56 AM, Stefan Hajnoczi wrote:
> On Thu, Jun 04, 2015 at 05:46:08PM -0400, John Snow wrote:
>> @@ -3190,19 +3193,13 @@ 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) { BdrvDirtyBitmap *successor =
>> parent->successor;
>>
>> - if (!successor) { - error_setg(errp, "Cannot reclaim
>> a successor when none is present"); - return NULL; - }
>> - + assert(successor); if (!hbitmap_merge(parent->bitmap,
>> successor->bitmap)) { - error_setg(errp, "Merging of
>> parent and successor bitmap failed"); return NULL; }
>
> Is this reachable? If the bitmap size and granularity match then
> hbitmap_merge() does not fail.
>
> This should probably be covered with an assertion instead to show
> that this is not allowed to happen.
>
You're right, I was thinking "What if something changes under my feet?
This layer does not know about the HBitmap internals," but for now
they do match and an assertion is appropriate to guard against
accidental changes.
--js
^ permalink raw reply [flat|nested] 17+ messages in thread
* [Qemu-devel] [PATCH v5 07/10] qmp: Add an implementation wrapper for qmp_drive_backup
2015-06-04 21:46 [Qemu-devel] [PATCH v5 00/10] block: incremental backup transactions John Snow
` (5 preceding siblings ...)
2015-06-04 21:46 ` [Qemu-devel] [PATCH v5 06/10] block: add delayed bitmap successor cleanup John Snow
@ 2015-06-04 21:46 ` John Snow
2015-06-05 17:31 ` Max Reitz
2015-06-04 21:46 ` [Qemu-devel] [PATCH v5 08/10] block: drive_backup transaction callback support John Snow
` (2 subsequent siblings)
9 siblings, 1 reply; 17+ messages in thread
From: John Snow @ 2015-06-04 21:46 UTC (permalink / raw)
To: qemu-block
Cc: kwolf, famz, John Snow, qemu-devel, mreitz, vsementsov, stefanha
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 | 76 ++++++++++++++++++++++++++++++++++++++++++++++----------------
1 file changed, 57 insertions(+), 19 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index 9db8202..4cb4179 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1791,6 +1791,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);
@@ -1813,15 +1826,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;
@@ -2775,15 +2789,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;
@@ -2897,9 +2914,14 @@ 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;
+ 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);
@@ -2910,6 +2932,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(errp);
--
2.1.0
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [Qemu-devel] [PATCH v5 07/10] qmp: Add an implementation wrapper for qmp_drive_backup
2015-06-04 21:46 ` [Qemu-devel] [PATCH v5 07/10] qmp: Add an implementation wrapper for qmp_drive_backup John Snow
@ 2015-06-05 17:31 ` Max Reitz
0 siblings, 0 replies; 17+ messages in thread
From: Max Reitz @ 2015-06-05 17:31 UTC (permalink / raw)
To: John Snow, qemu-block; +Cc: kwolf, famz, qemu-devel, vsementsov, stefanha
On 04.06.2015 23:46, 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 | 76 ++++++++++++++++++++++++++++++++++++++++++++++----------------
> 1 file changed, 57 insertions(+), 19 deletions(-)
Reviewed-by: Max Reitz <mreitz@redhat.com>
^ permalink raw reply [flat|nested] 17+ messages in thread
* [Qemu-devel] [PATCH v5 08/10] block: drive_backup transaction callback support
2015-06-04 21:46 [Qemu-devel] [PATCH v5 00/10] block: incremental backup transactions John Snow
` (6 preceding siblings ...)
2015-06-04 21:46 ` [Qemu-devel] [PATCH v5 07/10] qmp: Add an implementation wrapper for qmp_drive_backup John Snow
@ 2015-06-04 21:46 ` John Snow
2015-06-04 21:46 ` [Qemu-devel] [PATCH v5 09/10] iotests: 124 - transactional failure test John Snow
2015-06-04 21:46 ` [Qemu-devel] [PATCH v5 10/10] qmp-commands.hx: Update the supported 'transaction' operations John Snow
9 siblings, 0 replies; 17+ messages in thread
From: John Snow @ 2015-06-04 21:46 UTC (permalink / raw)
To: qemu-block
Cc: kwolf, famz, John Snow, qemu-devel, mreitz, vsementsov, stefanha
This patch actually implements the transactional callback system
for the drive_backup action.
(1) 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.
(2) We inform the BlockJob layer that this job is involved in a transaction,
which just increments a reference on the bitmap.
(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_action_complete will perform the final cleanup on the
opaque object (a BdrvDirtyBitmap, here) returned earlier.
(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 | 20 ++++++++++++++++++++
blockdev.c | 33 ++++++++++++++++++++++++++++++---
include/block/block_int.h | 16 ++++++++++++++++
3 files changed, 66 insertions(+), 3 deletions(-)
diff --git a/block/backup.c b/block/backup.c
index 4ac0be8..e681f1b 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -233,6 +233,26 @@ typedef struct {
int ret;
} BackupCompleteData;
+void *backup_action_start(BlockJob *job)
+{
+ BackupBlockJob *s = container_of(job, BackupBlockJob, common);
+
+ if (s->sync_bitmap) {
+ bdrv_frozen_bitmap_incref(s->sync_bitmap);
+ }
+
+ return s->sync_bitmap;
+}
+
+void backup_action_complete(BlockDriverState *bs, void *opaque, int ret)
+{
+ BdrvDirtyBitmap *bitmap = opaque;
+
+ if (bitmap) {
+ bdrv_frozen_bitmap_decref(bs, 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 4cb4179..905bf84 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1423,7 +1423,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,
@@ -1451,7 +1450,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: */
@@ -1789,6 +1787,7 @@ typedef struct DriveBackupState {
BlockDriverState *bs;
AioContext *aio_context;
BlockJob *job;
+ void *opaque;
} DriveBackupState;
static void do_drive_backup(const char *device, const char *target,
@@ -1803,6 +1802,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)
{
@@ -1811,6 +1811,8 @@ static void drive_backup_prepare(BlkActionState *common, Error **errp)
BlockBackend *blk;
DriveBackup *backup;
Error *local_err = NULL;
+ CallbackFn *cb;
+ void *opaque;
assert(common->action->kind == TRANSACTION_ACTION_KIND_DRIVE_BACKUP);
backup = common->action->drive_backup;
@@ -1826,6 +1828,10 @@ static void drive_backup_prepare(BlkActionState *common, Error **errp)
state->aio_context = bdrv_get_aio_context(bs);
aio_context_acquire(state->aio_context);
+ /* 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);
+
do_drive_backup(backup->device, backup->target,
backup->has_format, backup->format,
backup->sync,
@@ -1834,7 +1840,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);
@@ -1843,6 +1849,7 @@ static void drive_backup_prepare(BlkActionState *common, Error **errp)
state->bs = bs;
state->job = state->bs->job;
+ state->opaque = backup_action_start(state->job);
}
static void drive_backup_abort(BlkActionState *common)
@@ -1854,6 +1861,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)
@@ -1863,6 +1874,21 @@ static void drive_backup_clean(BlkActionState *common)
if (state->aio_context) {
aio_context_release(state->aio_context);
}
+ state->job = NULL;
+}
+
+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);
+
+ state->aio_context = bdrv_get_aio_context(bs);
+ aio_context_acquire(state->aio_context);
+ assert(state->bs == bs);
+
+ backup_action_complete(bs, state->opaque, common->transaction->status);
+ aio_context_release(state->aio_context);
}
typedef struct BlockdevBackupState {
@@ -2063,6 +2089,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 5af712f..5ca5f15 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -630,6 +630,22 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target,
BlockCompletionFunc *cb, void *opaque,
Error **errp);
+/*
+ * backup_action_start:
+ * @job: A BackupBlockJob we wish to specify is part of a transaction.
+ * Allows cleanup mechanisms to occur later via backup_action_complete.
+ */
+void *backup_action_start(BlockJob *job);
+
+/*
+ * backup_transaction_complete:
+ * @bs: The BlockDriverState associated with the backup job.
+ * @opaque: As returned from backup_action_start.
+ * @ret: The collective return code for the entire transaction.
+ * Expects zero for success, non-zero for an error otherwise.
+ */
+void backup_action_complete(BlockDriverState *bs, void *opaque, 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
^ permalink raw reply related [flat|nested] 17+ messages in thread* [Qemu-devel] [PATCH v5 09/10] iotests: 124 - transactional failure test
2015-06-04 21:46 [Qemu-devel] [PATCH v5 00/10] block: incremental backup transactions John Snow
` (7 preceding siblings ...)
2015-06-04 21:46 ` [Qemu-devel] [PATCH v5 08/10] block: drive_backup transaction callback support John Snow
@ 2015-06-04 21:46 ` John Snow
2015-06-04 21:46 ` [Qemu-devel] [PATCH v5 10/10] qmp-commands.hx: Update the supported 'transaction' operations John Snow
9 siblings, 0 replies; 17+ messages in thread
From: John Snow @ 2015-06-04 21:46 UTC (permalink / raw)
To: qemu-block
Cc: kwolf, famz, John Snow, qemu-devel, mreitz, vsementsov, stefanha
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>
Reviewed-by: Max Reitz <mreitz@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
^ permalink raw reply related [flat|nested] 17+ messages in thread* [Qemu-devel] [PATCH v5 10/10] qmp-commands.hx: Update the supported 'transaction' operations
2015-06-04 21:46 [Qemu-devel] [PATCH v5 00/10] block: incremental backup transactions John Snow
` (8 preceding siblings ...)
2015-06-04 21:46 ` [Qemu-devel] [PATCH v5 09/10] iotests: 124 - transactional failure test John Snow
@ 2015-06-04 21:46 ` John Snow
9 siblings, 0 replies; 17+ messages in thread
From: John Snow @ 2015-06-04 21:46 UTC (permalink / raw)
To: qemu-block
Cc: kwolf, famz, Kashyap Chamarthy, John Snow, qemu-devel, mreitz,
vsementsov, stefanha
From: Kashyap Chamarthy <kchamart@redhat.com>
Although the canonical source of reference for QMP commands is
qapi-schema.json, for consistency's sake, update qmp-commands.hx to
state the list of supported transactionable operations, namely:
drive-backup
blockdev-backup
blockdev-snapshot-internal-sync
abort
block-dirty-bitmap-add
block-dirty-bitmap-clear
Signed-off-by: Kashyap Chamarthy <kchamart@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
---
qmp-commands.hx | 21 ++++++++++++++++-----
1 file changed, 16 insertions(+), 5 deletions(-)
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 14e109e..a6029a2 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1238,11 +1238,22 @@ SQMP
transaction
-----------
-Atomically operate on one or more block devices. The only supported operations
-for now are drive-backup, internal and external snapshotting. A list of
-dictionaries is accepted, that contains the actions to be performed.
-If there is any failure performing any of the operations, all operations
-for the group are abandoned.
+Atomically operate on one or more block devices. Operations that are
+currently supported:
+
+ - drive-backup
+ - blockdev-backup
+ - blockdev-snapshot-sync
+ - blockdev-snapshot-internal-sync
+ - abort
+ - block-dirty-bitmap-add
+ - block-dirty-bitmap-clear
+
+Refer to the qemu/qapi-schema.json file for minimum required QEMU
+versions for these operations. A list of dictionaries is accepted,
+that contains the actions to be performed. If there is any failure
+performing any of the operations, all operations for the group are
+abandoned.
For external snapshots, the dictionary contains the device, the file to use for
the new snapshot, and the format. The default format, if not specified, is
--
2.1.0
^ permalink raw reply related [flat|nested] 17+ messages in thread