* [Qemu-devel] [PATCH v7 00/14] block: incremental backup transactions using BlockJobTxn
@ 2015-09-22 2:46 Fam Zheng
2015-09-22 2:46 ` [Qemu-devel] [PATCH v7 01/14] qapi: Add transaction support to block-dirty-bitmap operations Fam Zheng
` (14 more replies)
0 siblings, 15 replies; 30+ messages in thread
From: Fam Zheng @ 2015-09-22 2:46 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Jeff Cody, Max Reitz, vsementsov, stefanha, John Snow
v7: Add Eric's rev-by in 1, 11.
Add Max's rev-by in 4, 5, 9, 10, 11.
Add John's rev-by in 5, 6, 8.
Fix wording for 6. [John]
Fix comment of block_job_txn_add_job() in 9. [Max]
Remove superfluous hunks, and document default value in 11. [Eric]
Update Makefile dep in 14. [Max]
v6: Address comments from Max and Eric (many thanks for reviewing!):
Add Max's reviews.
Don't leak txn. [Max]
Unusual comment ending "**/" -> "*/". [Eric]
Fix the stale block_job_txn_prepare_to_complete comment. [Max]
Move "block_job_txn_unref" to below FOREACH block. [Max]
Split "base" and "txn" in qapi schema, so that transactional-cancel is only
visible in transaction. [Eric]
v5: Address comments from Max Reitz:
2.4 -> 2.5 in qapi docs.
Don't leak txn object.
English syntax fixes.
Really leave the "cancelled" status of failed job.
Remove a superfluous added line.
This is based on top of the work by Stefan Hajnoczi and John Snow.
Recap: motivation for block job transactions
--------------------------------------------
If an incremental backup block job fails then we reclaim the bitmap so
the job can be retried. The problem comes when multiple jobs are started as
part of a qmp 'transaction' command. We need to group these jobs in a
transaction so that either all jobs complete successfully or all bitmaps are
reclaimed.
Without transactions, there is a case where some jobs complete successfully and
throw away their bitmaps, making it impossible to retry the backup by rerunning
the command if one of the jobs fails.
How does this implementation work?
----------------------------------
These patches add a BlockJobTxn object with the following API:
txn = block_job_txn_new();
block_job_txn_add_job(txn, job1);
block_job_txn_add_job(txn, job2);
The jobs either both complete successfully or they both fail/cancel. If the
user cancels job1 then job2 will also be cancelled and vice versa.
Jobs objects stay alive waiting for other jobs to complete, even if the
coroutines have returned. They can be cancelled by the user during this time.
Job blockers are still in effect and no other block job can run on this device
in the meantime (since QEMU currently only allows 1 job per device). This is
the main drawback to this approach but reasonable since you probably don't want
to run other jobs/operations until you're sure the backup was successful (you
won't be able to retry a failed backup if there's a new job running).
Fam Zheng (6):
backup: Extract dirty bitmap handling as a separate function
blockjob: Introduce reference count
blockjob: Add .commit and .abort block job actions
blockjob: Add "completed" and "ret" in BlockJob
blockjob: Simplify block_job_finish_sync
block: Add block job transactions
John Snow (4):
qapi: Add transaction support to block-dirty-bitmap operations
iotests: add transactional incremental backup test
block: rename BlkTransactionState and BdrvActionOps
iotests: 124 - transactional failure test
Kashyap Chamarthy (1):
qmp-commands.hx: Update the supported 'transaction' operations
Stefan Hajnoczi (3):
blockdev: make BlockJobTxn available to qmp 'transaction'
block/backup: support block job transactions
tests: add BlockJobTxn unit test
block.c | 19 ++-
block/backup.c | 50 +++++--
block/mirror.c | 2 +-
blockdev.c | 354 +++++++++++++++++++++++++++++++++++----------
blockjob.c | 185 +++++++++++++++++++----
docs/bitmaps.md | 6 +-
include/block/block.h | 2 +-
include/block/block_int.h | 6 +-
include/block/blockjob.h | 85 ++++++++++-
qapi-schema.json | 10 +-
qapi/block-core.json | 27 +++-
qmp-commands.hx | 21 ++-
tests/Makefile | 3 +
tests/qemu-iotests/124 | 182 ++++++++++++++++++++++-
tests/qemu-iotests/124.out | 4 +-
tests/test-blockjob-txn.c | 244 +++++++++++++++++++++++++++++++
16 files changed, 1056 insertions(+), 144 deletions(-)
create mode 100644 tests/test-blockjob-txn.c
--
2.4.3
^ permalink raw reply [flat|nested] 30+ messages in thread
* [Qemu-devel] [PATCH v7 01/14] qapi: Add transaction support to block-dirty-bitmap operations
2015-09-22 2:46 [Qemu-devel] [PATCH v7 00/14] block: incremental backup transactions using BlockJobTxn Fam Zheng
@ 2015-09-22 2:46 ` Fam Zheng
2015-09-22 2:46 ` [Qemu-devel] [PATCH v7 02/14] iotests: add transactional incremental backup test Fam Zheng
` (13 subsequent siblings)
14 siblings, 0 replies; 30+ messages in thread
From: Fam Zheng @ 2015-09-22 2:46 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Jeff Cody, Max Reitz, vsementsov, stefanha, John Snow
From: John Snow <jsnow@redhat.com>
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: John Snow <jsnow@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Fam Zheng <famz@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 6268e37..65cacbb 100644
--- a/block.c
+++ b/block.c
@@ -3578,10 +3578,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_all(bitmap->bitmap);
+ if (!out) {
+ hbitmap_reset_all(bitmap->bitmap);
+ } 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 32b04b4..6cd18ad 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1708,6 +1708,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");
@@ -1748,6 +1848,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,
+ }
};
/*
@@ -2128,7 +2240,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 fa87f07..9fd8ea6 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 ef67353..ca56021 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -503,7 +503,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 14ad4c3..281d790 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -664,4 +664,7 @@ void blk_dev_resize_cb(BlockBackend *blk);
void bdrv_set_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 821362d..583e036 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1499,6 +1499,8 @@
# abort since 1.6
# blockdev-snapshot-internal-sync since 1.7
# blockdev-backup since 2.3
+# block-dirty-bitmap-add since 2.5
+# block-dirty-bitmap-clear since 2.5
##
{ 'union': 'TransactionAction',
'data': {
@@ -1506,7 +1508,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.4.3
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [Qemu-devel] [PATCH v7 02/14] iotests: add transactional incremental backup test
2015-09-22 2:46 [Qemu-devel] [PATCH v7 00/14] block: incremental backup transactions using BlockJobTxn Fam Zheng
2015-09-22 2:46 ` [Qemu-devel] [PATCH v7 01/14] qapi: Add transaction support to block-dirty-bitmap operations Fam Zheng
@ 2015-09-22 2:46 ` Fam Zheng
2015-09-22 2:46 ` [Qemu-devel] [PATCH v7 03/14] block: rename BlkTransactionState and BdrvActionOps Fam Zheng
` (12 subsequent siblings)
14 siblings, 0 replies; 30+ messages in thread
From: Fam Zheng @ 2015-09-22 2:46 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Jeff Cody, Max Reitz, vsementsov, stefanha, John Snow
From: John Snow <jsnow@redhat.com>
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>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
Signed-off-by: Fam Zheng <famz@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 9ccd118..9c1977e 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.4.3
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [Qemu-devel] [PATCH v7 03/14] block: rename BlkTransactionState and BdrvActionOps
2015-09-22 2:46 [Qemu-devel] [PATCH v7 00/14] block: incremental backup transactions using BlockJobTxn Fam Zheng
2015-09-22 2:46 ` [Qemu-devel] [PATCH v7 01/14] qapi: Add transaction support to block-dirty-bitmap operations Fam Zheng
2015-09-22 2:46 ` [Qemu-devel] [PATCH v7 02/14] iotests: add transactional incremental backup test Fam Zheng
@ 2015-09-22 2:46 ` Fam Zheng
2015-09-22 2:46 ` [Qemu-devel] [PATCH v7 04/14] backup: Extract dirty bitmap handling as a separate function Fam Zheng
` (11 subsequent siblings)
14 siblings, 0 replies; 30+ messages in thread
From: Fam Zheng @ 2015-09-22 2:46 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Jeff Cody, Max Reitz, vsementsov, stefanha, John Snow
From: John Snow <jsnow@redhat.com>
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>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
Signed-off-by: Fam Zheng <famz@redhat.com>
---
blockdev.c | 116 ++++++++++++++++++++++++++++++++++---------------------------
1 file changed, 65 insertions(+), 51 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index 6cd18ad..796bc64 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1246,43 +1246,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;
@@ -1378,7 +1392,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);
@@ -1401,7 +1415,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);
@@ -1413,13 +1427,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)
{
int flags, ret;
@@ -1534,7 +1548,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);
@@ -1552,7 +1566,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);
@@ -1565,13 +1579,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;
@@ -1612,7 +1626,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;
@@ -1623,7 +1637,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);
@@ -1633,13 +1647,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;
@@ -1688,7 +1702,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;
@@ -1699,7 +1713,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);
@@ -1709,7 +1723,7 @@ static void blockdev_backup_clean(BlkTransactionState *common)
}
typedef struct BlockDirtyBitmapState {
- BlkTransactionState common;
+ BlkActionState common;
BdrvDirtyBitmap *bitmap;
BlockDriverState *bs;
AioContext *aio_context;
@@ -1717,7 +1731,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;
@@ -1738,7 +1752,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,
@@ -1753,7 +1767,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,
@@ -1782,7 +1796,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);
@@ -1790,7 +1804,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);
@@ -1798,7 +1812,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);
@@ -1808,17 +1822,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,
@@ -1838,7 +1852,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,
},
@@ -1869,10 +1883,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 */
@@ -1881,7 +1895,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.4.3
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [Qemu-devel] [PATCH v7 04/14] backup: Extract dirty bitmap handling as a separate function
2015-09-22 2:46 [Qemu-devel] [PATCH v7 00/14] block: incremental backup transactions using BlockJobTxn Fam Zheng
` (2 preceding siblings ...)
2015-09-22 2:46 ` [Qemu-devel] [PATCH v7 03/14] block: rename BlkTransactionState and BdrvActionOps Fam Zheng
@ 2015-09-22 2:46 ` Fam Zheng
2015-09-22 2:46 ` [Qemu-devel] [PATCH v7 05/14] blockjob: Introduce reference count Fam Zheng
` (10 subsequent siblings)
14 siblings, 0 replies; 30+ messages in thread
From: Fam Zheng @ 2015-09-22 2:46 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Jeff Cody, Max Reitz, vsementsov, stefanha, John Snow
This will be reused by the coming new transactional completion code.
Signed-off-by: Fam Zheng <famz@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
block/backup.c | 27 +++++++++++++++++----------
1 file changed, 17 insertions(+), 10 deletions(-)
diff --git a/block/backup.c b/block/backup.c
index 965654d..609b199 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -211,6 +211,22 @@ static void backup_iostatus_reset(BlockJob *job)
bdrv_iostatus_reset(s->target);
}
+static void backup_cleanup_sync_bitmap(BackupBlockJob *job, int ret)
+{
+ BdrvDirtyBitmap *bm;
+ BlockDriverState *bs = job->common.bs;
+
+ if (ret < 0 || block_job_is_cancelled(&job->common)) {
+ /* 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);
+ }
+}
+
static const BlockJobDriver backup_job_driver = {
.instance_size = sizeof(BackupBlockJob),
.job_type = BLOCK_JOB_TYPE_BACKUP,
@@ -430,16 +446,7 @@ static void coroutine_fn backup_run(void *opaque)
qemu_co_rwlock_unlock(&job->flush_rwlock);
if (job->sync_bitmap) {
- BdrvDirtyBitmap *bm;
- if (ret < 0 || block_job_is_cancelled(&job->common)) {
- /* 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);
- }
+ backup_cleanup_sync_bitmap(job, ret);
}
hbitmap_free(job->bitmap);
--
2.4.3
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [Qemu-devel] [PATCH v7 05/14] blockjob: Introduce reference count
2015-09-22 2:46 [Qemu-devel] [PATCH v7 00/14] block: incremental backup transactions using BlockJobTxn Fam Zheng
` (3 preceding siblings ...)
2015-09-22 2:46 ` [Qemu-devel] [PATCH v7 04/14] backup: Extract dirty bitmap handling as a separate function Fam Zheng
@ 2015-09-22 2:46 ` Fam Zheng
2015-09-22 2:46 ` [Qemu-devel] [PATCH v7 06/14] blockjob: Add .commit and .abort block job actions Fam Zheng
` (9 subsequent siblings)
14 siblings, 0 replies; 30+ messages in thread
From: Fam Zheng @ 2015-09-22 2:46 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Jeff Cody, Max Reitz, vsementsov, stefanha, John Snow
So that block_job_complete_sync can be simplified.
Reviewed-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>
Signed-off-by: Fam Zheng <famz@redhat.com>
---
block/mirror.c | 2 +-
blockjob.c | 22 ++++++++++++++--------
include/block/blockjob.h | 18 +++++++++++++++---
3 files changed, 30 insertions(+), 12 deletions(-)
diff --git a/block/mirror.c b/block/mirror.c
index a258926..3472ad4 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -736,7 +736,7 @@ static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target,
s->dirty_bitmap = bdrv_create_dirty_bitmap(bs, granularity, NULL, errp);
if (!s->dirty_bitmap) {
g_free(s->replaces);
- block_job_release(bs);
+ block_job_unref(&s->common);
return;
}
bdrv_set_enable_write_cache(s->target, true);
diff --git a/blockjob.c b/blockjob.c
index 62bb906..ec12887 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -58,6 +58,7 @@ void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs,
job->cb = cb;
job->opaque = opaque;
job->busy = true;
+ job->refcnt = 1;
bs->job = job;
/* Only set speed when necessary to avoid NotSupported error */
@@ -66,7 +67,7 @@ void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs,
block_job_set_speed(job, speed, &local_err);
if (local_err) {
- block_job_release(bs);
+ block_job_unref(job);
error_propagate(errp, local_err);
return NULL;
}
@@ -74,14 +75,19 @@ void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs,
return job;
}
-void block_job_release(BlockDriverState *bs)
+void block_job_ref(BlockJob *job)
{
- BlockJob *job = bs->job;
+ ++job->refcnt;
+}
- bs->job = NULL;
- bdrv_op_unblock_all(bs, job->blocker);
- error_free(job->blocker);
- g_free(job);
+void block_job_unref(BlockJob *job)
+{
+ if (--job->refcnt == 0) {
+ job->bs->job = NULL;
+ bdrv_op_unblock_all(job->bs, job->blocker);
+ error_free(job->blocker);
+ g_free(job);
+ }
}
void block_job_completed(BlockJob *job, int ret)
@@ -90,7 +96,7 @@ void block_job_completed(BlockJob *job, int ret)
assert(bs->job == job);
job->cb(job->opaque, ret);
- block_job_release(bs);
+ block_job_unref(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 dd9d5e6..3e7ad21 100644
--- a/include/block/blockjob.h
+++ b/include/block/blockjob.h
@@ -122,6 +122,9 @@ struct BlockJob {
/** The opaque value that is passed to the completion function. */
void *opaque;
+
+ /** Reference count of the block job */
+ int refcnt;
};
/**
@@ -166,12 +169,21 @@ void block_job_sleep_ns(BlockJob *job, QEMUClockType type, int64_t ns);
void block_job_yield(BlockJob *job);
/**
- * block_job_release:
+ * block_job_ref:
* @bs: The block device.
*
- * Release job resources when an error occurred or job completed.
+ * Grab a reference to the block job. Should be paired with block_job_unref.
*/
-void block_job_release(BlockDriverState *bs);
+void block_job_ref(BlockJob *job);
+
+/**
+ * block_job_unref:
+ * @bs: The block device.
+ *
+ * Release reference to the block job and release resources if it is the last
+ * reference.
+ */
+void block_job_unref(BlockJob *job);
/**
* block_job_completed:
--
2.4.3
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [Qemu-devel] [PATCH v7 06/14] blockjob: Add .commit and .abort block job actions
2015-09-22 2:46 [Qemu-devel] [PATCH v7 00/14] block: incremental backup transactions using BlockJobTxn Fam Zheng
` (4 preceding siblings ...)
2015-09-22 2:46 ` [Qemu-devel] [PATCH v7 05/14] blockjob: Introduce reference count Fam Zheng
@ 2015-09-22 2:46 ` Fam Zheng
2015-09-22 2:46 ` [Qemu-devel] [PATCH v7 07/14] blockjob: Add "completed" and "ret" in BlockJob Fam Zheng
` (8 subsequent siblings)
14 siblings, 0 replies; 30+ messages in thread
From: Fam Zheng @ 2015-09-22 2:46 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Jeff Cody, Max Reitz, vsementsov, stefanha, John Snow
Reviewed-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>
Signed-off-by: Fam Zheng <famz@redhat.com>
---
include/block/blockjob.h | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
diff --git a/include/block/blockjob.h b/include/block/blockjob.h
index 3e7ad21..aee39a4 100644
--- a/include/block/blockjob.h
+++ b/include/block/blockjob.h
@@ -50,6 +50,26 @@ typedef struct BlockJobDriver {
* manually.
*/
void (*complete)(BlockJob *job, Error **errp);
+
+ /**
+ * If the callback is not NULL, it will be invoked when all the jobs
+ * belonging to the same transaction complete; or upon this job's
+ * completion if it is not in a transaction. Skipped if NULL.
+ *
+ * All jobs will complete with a call to either .commit() or .abort() but
+ * never both.
+ */
+ void (*commit)(BlockJob *job);
+
+ /**
+ * If the callback is not NULL, it will be invoked when any job in the
+ * same transaction fails; or upon this job's failure (due to error or
+ * cancellation) if it is not in a transaction. Skipped if NULL.
+ *
+ * All jobs will complete with a call to either .commit() or .abort() but
+ * never both.
+ */
+ void (*abort)(BlockJob *job);
} BlockJobDriver;
/**
--
2.4.3
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [Qemu-devel] [PATCH v7 07/14] blockjob: Add "completed" and "ret" in BlockJob
2015-09-22 2:46 [Qemu-devel] [PATCH v7 00/14] block: incremental backup transactions using BlockJobTxn Fam Zheng
` (5 preceding siblings ...)
2015-09-22 2:46 ` [Qemu-devel] [PATCH v7 06/14] blockjob: Add .commit and .abort block job actions Fam Zheng
@ 2015-09-22 2:46 ` Fam Zheng
2015-09-22 2:46 ` [Qemu-devel] [PATCH v7 08/14] blockjob: Simplify block_job_finish_sync Fam Zheng
` (7 subsequent siblings)
14 siblings, 0 replies; 30+ messages in thread
From: Fam Zheng @ 2015-09-22 2:46 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Jeff Cody, Max Reitz, vsementsov, stefanha, John Snow
They are set when block_job_completed is called.
Signed-off-by: Fam Zheng <famz@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
blockjob.c | 3 +++
include/block/blockjob.h | 9 +++++++++
2 files changed, 12 insertions(+)
diff --git a/blockjob.c b/blockjob.c
index ec12887..293b62a 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -95,6 +95,9 @@ void block_job_completed(BlockJob *job, int ret)
BlockDriverState *bs = job->bs;
assert(bs->job == job);
+ assert(!job->completed);
+ job->completed = true;
+ job->ret = ret;
job->cb(job->opaque, ret);
block_job_unref(job);
}
diff --git a/include/block/blockjob.h b/include/block/blockjob.h
index aee39a4..a77799f 100644
--- a/include/block/blockjob.h
+++ b/include/block/blockjob.h
@@ -145,6 +145,15 @@ struct BlockJob {
/** Reference count of the block job */
int refcnt;
+
+ /* True if this job has reported completion by calling block_job_completed.
+ */
+ bool completed;
+
+ /* ret code passed to block_job_completed.
+ */
+ int ret;
+
};
/**
--
2.4.3
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [Qemu-devel] [PATCH v7 08/14] blockjob: Simplify block_job_finish_sync
2015-09-22 2:46 [Qemu-devel] [PATCH v7 00/14] block: incremental backup transactions using BlockJobTxn Fam Zheng
` (6 preceding siblings ...)
2015-09-22 2:46 ` [Qemu-devel] [PATCH v7 07/14] blockjob: Add "completed" and "ret" in BlockJob Fam Zheng
@ 2015-09-22 2:46 ` Fam Zheng
2015-09-22 2:46 ` [Qemu-devel] [PATCH v7 09/14] block: Add block job transactions Fam Zheng
` (6 subsequent siblings)
14 siblings, 0 replies; 30+ messages in thread
From: Fam Zheng @ 2015-09-22 2:46 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Jeff Cody, Max Reitz, vsementsov, stefanha, John Snow
With job->completed and job->ret to replace BlockFinishData.
Signed-off-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>
---
blockjob.c | 27 ++++++---------------------
1 file changed, 6 insertions(+), 21 deletions(-)
diff --git a/blockjob.c b/blockjob.c
index 293b62a..36c18e0 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -185,43 +185,28 @@ struct BlockFinishData {
int ret;
};
-static void block_job_finish_cb(void *opaque, int ret)
-{
- struct BlockFinishData *data = opaque;
-
- data->cancelled = block_job_is_cancelled(data->job);
- data->ret = ret;
- data->cb(data->opaque, ret);
-}
-
static int block_job_finish_sync(BlockJob *job,
void (*finish)(BlockJob *, Error **errp),
Error **errp)
{
- struct BlockFinishData data;
BlockDriverState *bs = job->bs;
Error *local_err = NULL;
+ int ret;
assert(bs->job == job);
- /* Set up our own callback to store the result and chain to
- * the original callback.
- */
- data.job = job;
- data.cb = job->cb;
- data.opaque = job->opaque;
- data.ret = -EINPROGRESS;
- job->cb = block_job_finish_cb;
- job->opaque = &data;
+ block_job_ref(job);
finish(job, &local_err);
if (local_err) {
error_propagate(errp, local_err);
return -EBUSY;
}
- while (data.ret == -EINPROGRESS) {
+ while (!job->completed) {
aio_poll(bdrv_get_aio_context(bs), true);
}
- return (data.cancelled && data.ret == 0) ? -ECANCELED : data.ret;
+ ret = (job->cancelled && job->ret == 0) ? -ECANCELED : job->ret;
+ block_job_unref(job);
+ return ret;
}
/* A wrapper around block_job_cancel() taking an Error ** parameter so it may be
--
2.4.3
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [Qemu-devel] [PATCH v7 09/14] block: Add block job transactions
2015-09-22 2:46 [Qemu-devel] [PATCH v7 00/14] block: incremental backup transactions using BlockJobTxn Fam Zheng
` (7 preceding siblings ...)
2015-09-22 2:46 ` [Qemu-devel] [PATCH v7 08/14] blockjob: Simplify block_job_finish_sync Fam Zheng
@ 2015-09-22 2:46 ` Fam Zheng
2015-09-22 19:09 ` John Snow
2015-09-22 2:46 ` [Qemu-devel] [PATCH v7 10/14] blockdev: make BlockJobTxn available to qmp 'transaction' Fam Zheng
` (5 subsequent siblings)
14 siblings, 1 reply; 30+ messages in thread
From: Fam Zheng @ 2015-09-22 2:46 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Jeff Cody, Max Reitz, vsementsov, stefanha, John Snow
Sometimes block jobs must execute as a transaction group. Finishing
jobs wait until all other jobs are ready to complete successfully.
Failure or cancellation of one job cancels the other jobs in the group.
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
[Rewrite the implementation which is now contained in block_job_completed.
--Fam]
Signed-off-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
blockjob.c | 135 ++++++++++++++++++++++++++++++++++++++++++++++-
include/block/block.h | 1 +
include/block/blockjob.h | 38 +++++++++++++
3 files changed, 172 insertions(+), 2 deletions(-)
diff --git a/blockjob.c b/blockjob.c
index 36c18e0..91e8d3c 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -36,6 +36,19 @@
#include "qemu/timer.h"
#include "qapi-event.h"
+/* Transactional group of block jobs */
+struct BlockJobTxn {
+
+ /* Is this txn being cancelled? */
+ bool aborting;
+
+ /* List of jobs */
+ QLIST_HEAD(, BlockJob) jobs;
+
+ /* Reference count */
+ int refcnt;
+};
+
void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs,
int64_t speed, BlockCompletionFunc *cb,
void *opaque, Error **errp)
@@ -90,6 +103,86 @@ void block_job_unref(BlockJob *job)
}
}
+static void block_job_completed_single(BlockJob *job)
+{
+ if (!job->ret) {
+ if (job->driver->commit) {
+ job->driver->commit(job);
+ }
+ } else {
+ if (job->driver->abort) {
+ job->driver->abort(job);
+ }
+ }
+ job->cb(job->opaque, job->ret);
+ if (job->txn) {
+ block_job_txn_unref(job->txn);
+ }
+ block_job_unref(job);
+}
+
+static void block_job_completed_txn_abort(BlockJob *job)
+{
+ AioContext *ctx;
+ BlockJobTxn *txn = job->txn;
+ BlockJob *other_job, *next;
+
+ if (txn->aborting) {
+ /*
+ * We are cancelled by another job, which will handle everything.
+ */
+ return;
+ }
+ txn->aborting = true;
+ /* We are the first failed job. Cancel other jobs. */
+ QLIST_FOREACH(other_job, &txn->jobs, txn_list) {
+ ctx = bdrv_get_aio_context(other_job->bs);
+ aio_context_acquire(ctx);
+ }
+ QLIST_FOREACH(other_job, &txn->jobs, txn_list) {
+ if (other_job == job || other_job->completed) {
+ /* Other jobs are "effectively" cancelled by us, set the status for
+ * them; this job, however, may or may not be cancelled, depending
+ * on the caller, so leave it. */
+ if (other_job != job) {
+ other_job->cancelled = true;
+ }
+ continue;
+ }
+ block_job_cancel_sync(other_job);
+ assert(other_job->completed);
+ }
+ QLIST_FOREACH_SAFE(other_job, &txn->jobs, txn_list, next) {
+ ctx = bdrv_get_aio_context(other_job->bs);
+ block_job_completed_single(other_job);
+ aio_context_release(ctx);
+ }
+}
+
+static void block_job_completed_txn_success(BlockJob *job)
+{
+ AioContext *ctx;
+ BlockJobTxn *txn = job->txn;
+ BlockJob *other_job, *next;
+ /*
+ * Successful completion, see if there are other running jobs in this
+ * txn.
+ */
+ QLIST_FOREACH(other_job, &txn->jobs, txn_list) {
+ if (!other_job->completed) {
+ return;
+ }
+ }
+ /* We are the last completed job, commit the transaction. */
+ QLIST_FOREACH_SAFE(other_job, &txn->jobs, txn_list, next) {
+ ctx = bdrv_get_aio_context(other_job->bs);
+ aio_context_acquire(ctx);
+ assert(other_job->ret == 0);
+ block_job_completed_single(other_job);
+ aio_context_release(ctx);
+ }
+}
+
void block_job_completed(BlockJob *job, int ret)
{
BlockDriverState *bs = job->bs;
@@ -98,8 +191,13 @@ void block_job_completed(BlockJob *job, int ret)
assert(!job->completed);
job->completed = true;
job->ret = ret;
- job->cb(job->opaque, ret);
- block_job_unref(job);
+ if (!job->txn) {
+ block_job_completed_single(job);
+ } else if (ret < 0 || block_job_is_cancelled(job)) {
+ block_job_completed_txn_abort(job);
+ } else {
+ block_job_completed_txn_success(job);
+ }
}
void block_job_set_speed(BlockJob *job, int64_t speed, Error **errp)
@@ -398,3 +496,36 @@ void block_job_defer_to_main_loop(BlockJob *job,
qemu_bh_schedule(data->bh);
}
+
+BlockJobTxn *block_job_txn_new(void)
+{
+ BlockJobTxn *txn = g_new0(BlockJobTxn, 1);
+ QLIST_INIT(&txn->jobs);
+ txn->refcnt = 1;
+ return txn;
+}
+
+static void block_job_txn_ref(BlockJobTxn *txn)
+{
+ txn->refcnt++;
+}
+
+void block_job_txn_unref(BlockJobTxn *txn)
+{
+ if (--txn->refcnt == 0) {
+ g_free(txn);
+ }
+}
+
+void block_job_txn_add_job(BlockJobTxn *txn, BlockJob *job)
+{
+ if (!txn) {
+ return;
+ }
+
+ assert(!job->txn);
+ job->txn = txn;
+
+ QLIST_INSERT_HEAD(&txn->jobs, job, txn_list);
+ block_job_txn_ref(txn);
+}
diff --git a/include/block/block.h b/include/block/block.h
index ca56021..94548cb 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -14,6 +14,7 @@ typedef struct BlockDriver BlockDriver;
typedef struct BlockJob BlockJob;
typedef struct BdrvChild BdrvChild;
typedef struct BdrvChildRole BdrvChildRole;
+typedef struct BlockJobTxn BlockJobTxn;
typedef struct BlockDriverInfo {
/* in bytes, 0 if irrelevant */
diff --git a/include/block/blockjob.h b/include/block/blockjob.h
index a77799f..fa77189 100644
--- a/include/block/blockjob.h
+++ b/include/block/blockjob.h
@@ -154,6 +154,9 @@ struct BlockJob {
*/
int ret;
+ /** Non-NULL if this job is part of a transaction */
+ BlockJobTxn *txn;
+ QLIST_ENTRY(BlockJob) txn_list;
};
/**
@@ -397,4 +400,39 @@ void block_job_defer_to_main_loop(BlockJob *job,
BlockJobDeferToMainLoopFn *fn,
void *opaque);
+/**
+ * block_job_txn_new:
+ *
+ * Allocate and return a new block job transaction. Jobs can be added to the
+ * transaction using block_job_txn_add_job().
+ *
+ * The transaction is automatically freed when the last job completes or is
+ * cancelled.
+ *
+ * All jobs in the transaction either complete successfully or fail/cancel as a
+ * group. Jobs wait for each other before completing. Cancelling one job
+ * cancels all jobs in the transaction.
+ */
+BlockJobTxn *block_job_txn_new(void);
+
+/**
+ * block_job_txn_unref:
+ *
+ * Release a reference that was previously acquired with block_job_txn_add_job
+ * or block_job_txn_new. If it's the last reference to the object, it will be
+ * freed.
+ */
+void block_job_txn_unref(BlockJobTxn *txn);
+
+/**
+ * block_job_txn_add_job:
+ * @txn: The transaction (may be NULL)
+ * @job: Job to add to the transaction
+ *
+ * Add @job to the transaction. The @job must not already be in a transaction.
+ * The caller must call either block_job_txn_unref() or block_job_completed()
+ * to release the reference that is automatically grabbed here.
+ */
+void block_job_txn_add_job(BlockJobTxn *txn, BlockJob *job);
+
#endif
--
2.4.3
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [Qemu-devel] [PATCH v7 10/14] blockdev: make BlockJobTxn available to qmp 'transaction'
2015-09-22 2:46 [Qemu-devel] [PATCH v7 00/14] block: incremental backup transactions using BlockJobTxn Fam Zheng
` (8 preceding siblings ...)
2015-09-22 2:46 ` [Qemu-devel] [PATCH v7 09/14] block: Add block job transactions Fam Zheng
@ 2015-09-22 2:46 ` Fam Zheng
2015-09-22 19:13 ` John Snow
2015-09-22 2:46 ` [Qemu-devel] [PATCH v7 11/14] block/backup: support block job transactions Fam Zheng
` (4 subsequent siblings)
14 siblings, 1 reply; 30+ messages in thread
From: Fam Zheng @ 2015-09-22 2:46 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Jeff Cody, Max Reitz, vsementsov, stefanha, John Snow
From: Stefan Hajnoczi <stefanha@redhat.com>
Provide a BlockJobTxn to actions executed in a qmp 'transaction'
command. This allows actions to make their block jobs either complete
as a group or fail/cancel together.
The next patch adds the first user.
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Signed-off-by: Fam Zheng <famz@redhat.com>
---
blockdev.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/blockdev.c b/blockdev.c
index 796bc64..ed50cb4 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1285,6 +1285,7 @@ typedef struct BlkActionOps {
struct BlkActionState {
TransactionAction *action;
const BlkActionOps *ops;
+ BlockJobTxn *block_job_txn;
QSIMPLEQ_ENTRY(BlkActionState) entry;
};
@@ -1883,12 +1884,15 @@ static const BlkActionOps actions[] = {
void qmp_transaction(TransactionActionList *dev_list, Error **errp)
{
TransactionActionList *dev_entry = dev_list;
+ BlockJobTxn *block_job_txn;
BlkActionState *state, *next;
Error *local_err = NULL;
QSIMPLEQ_HEAD(snap_bdrv_states, BlkActionState) snap_bdrv_states;
QSIMPLEQ_INIT(&snap_bdrv_states);
+ block_job_txn = block_job_txn_new();
+
/* drain all i/o before any operations */
bdrv_drain_all();
@@ -1908,6 +1912,7 @@ void qmp_transaction(TransactionActionList *dev_list, Error **errp)
state = g_malloc0(ops->instance_size);
state->ops = ops;
state->action = dev_info;
+ state->block_job_txn = block_job_txn;
QSIMPLEQ_INSERT_TAIL(&snap_bdrv_states, state, entry);
state->ops->prepare(state, &local_err);
@@ -1940,6 +1945,7 @@ exit:
}
g_free(state);
}
+ block_job_txn_unref(block_job_txn);
}
--
2.4.3
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [Qemu-devel] [PATCH v7 11/14] block/backup: support block job transactions
2015-09-22 2:46 [Qemu-devel] [PATCH v7 00/14] block: incremental backup transactions using BlockJobTxn Fam Zheng
` (9 preceding siblings ...)
2015-09-22 2:46 ` [Qemu-devel] [PATCH v7 10/14] blockdev: make BlockJobTxn available to qmp 'transaction' Fam Zheng
@ 2015-09-22 2:46 ` Fam Zheng
2015-09-22 16:03 ` Eric Blake
2015-09-22 21:08 ` John Snow
2015-09-22 2:46 ` [Qemu-devel] [PATCH v7 12/14] iotests: 124 - transactional failure test Fam Zheng
` (3 subsequent siblings)
14 siblings, 2 replies; 30+ messages in thread
From: Fam Zheng @ 2015-09-22 2:46 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Jeff Cody, Max Reitz, vsementsov, stefanha, John Snow
From: Stefan Hajnoczi <stefanha@redhat.com>
Join the transaction when the 'transactional-cancel' QMP argument is
true.
This ensures that the sync bitmap is not thrown away if another block
job in the transaction is cancelled or fails. This is critical so
incremental backup with multiple disks can be retried in case of
cancellation/failure.
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Fam Zheng <famz@redhat.com>
---
block/backup.c | 25 +++++++--
blockdev.c | 132 +++++++++++++++++++++++++++++++++++-----------
include/block/block_int.h | 3 +-
qapi-schema.json | 4 +-
qapi/block-core.json | 27 +++++++++-
5 files changed, 152 insertions(+), 39 deletions(-)
diff --git a/block/backup.c b/block/backup.c
index 609b199..5880116 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -227,11 +227,29 @@ static void backup_cleanup_sync_bitmap(BackupBlockJob *job, int ret)
}
}
+static void backup_commit(BlockJob *job)
+{
+ BackupBlockJob *s = container_of(job, BackupBlockJob, common);
+ if (s->sync_bitmap) {
+ backup_cleanup_sync_bitmap(s, 0);
+ }
+}
+
+static void backup_abort(BlockJob *job)
+{
+ BackupBlockJob *s = container_of(job, BackupBlockJob, common);
+ if (s->sync_bitmap) {
+ backup_cleanup_sync_bitmap(s, -1);
+ }
+}
+
static const BlockJobDriver backup_job_driver = {
.instance_size = sizeof(BackupBlockJob),
.job_type = BLOCK_JOB_TYPE_BACKUP,
.set_speed = backup_set_speed,
.iostatus_reset = backup_iostatus_reset,
+ .commit = backup_commit,
+ .abort = backup_abort,
};
static BlockErrorAction backup_error_action(BackupBlockJob *job,
@@ -444,10 +462,6 @@ static void coroutine_fn backup_run(void *opaque)
/* wait until pending backup_do_cow() calls have completed */
qemu_co_rwlock_wrlock(&job->flush_rwlock);
qemu_co_rwlock_unlock(&job->flush_rwlock);
-
- if (job->sync_bitmap) {
- backup_cleanup_sync_bitmap(job, ret);
- }
hbitmap_free(job->bitmap);
bdrv_iostatus_disable(target);
@@ -464,7 +478,7 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target,
BlockdevOnError on_source_error,
BlockdevOnError on_target_error,
BlockCompletionFunc *cb, void *opaque,
- Error **errp)
+ BlockJobTxn *txn, Error **errp)
{
int64_t len;
@@ -546,6 +560,7 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target,
sync_bitmap : NULL;
job->common.len = len;
job->common.co = qemu_coroutine_create(backup_run);
+ block_job_txn_add_job(txn, &job->common);
qemu_coroutine_enter(job->common.co, job);
return;
diff --git a/blockdev.c b/blockdev.c
index ed50cb4..45a9fe7 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1586,16 +1586,31 @@ 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,
+ BlockJobTxn *txn, Error **errp);
+
static void drive_backup_prepare(BlkActionState *common, Error **errp)
{
DriveBackupState *state = DO_UPCAST(DriveBackupState, common, common);
BlockDriverState *bs;
BlockBackend *blk;
+ DriveBackupTxn *backup_txn;
DriveBackup *backup;
+ BlockJobTxn *txn = NULL;
Error *local_err = NULL;
assert(common->action->kind == TRANSACTION_ACTION_KIND_DRIVE_BACKUP);
- backup = common->action->drive_backup;
+ backup_txn = common->action->drive_backup;
+ backup = backup_txn->base;
blk = blk_by_name(backup->device);
if (!blk) {
@@ -1609,15 +1624,20 @@ 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);
+ if (backup_txn->has_transactional_cancel &&
+ backup_txn->transactional_cancel) {
+ txn = common->block_job_txn;
+ }
+
+ do_drive_backup(backup->device, backup->target,
+ backup->has_format, backup->format,
+ backup->sync,
+ 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,
+ txn, &local_err);
if (local_err) {
error_propagate(errp, local_err);
return;
@@ -1654,16 +1674,28 @@ typedef struct BlockdevBackupState {
AioContext *aio_context;
} BlockdevBackupState;
+static void do_blockdev_backup(const char *device, const char *target,
+ enum MirrorSyncMode sync,
+ bool has_speed, int64_t speed,
+ bool has_on_source_error,
+ BlockdevOnError on_source_error,
+ bool has_on_target_error,
+ BlockdevOnError on_target_error,
+ BlockJobTxn *txn, Error **errp);
+
static void blockdev_backup_prepare(BlkActionState *common, Error **errp)
{
BlockdevBackupState *state = DO_UPCAST(BlockdevBackupState, common, common);
+ BlockdevBackupTxn *backup_txn;
BlockdevBackup *backup;
BlockDriverState *bs, *target;
BlockBackend *blk;
+ BlockJobTxn *txn = NULL;
Error *local_err = NULL;
assert(common->action->kind == TRANSACTION_ACTION_KIND_BLOCKDEV_BACKUP);
- backup = common->action->blockdev_backup;
+ backup_txn = common->action->blockdev_backup;
+ backup = backup_txn->base;
blk = blk_by_name(backup->device);
if (!blk) {
@@ -1688,12 +1720,17 @@ static void blockdev_backup_prepare(BlkActionState *common, Error **errp)
}
aio_context_acquire(state->aio_context);
- qmp_blockdev_backup(backup->device, backup->target,
- backup->sync,
- backup->has_speed, backup->speed,
- backup->has_on_source_error, backup->on_source_error,
- backup->has_on_target_error, backup->on_target_error,
- &local_err);
+ if (backup_txn->has_transactional_cancel &&
+ backup_txn->transactional_cancel) {
+ txn = common->block_job_txn;
+ }
+
+ do_blockdev_backup(backup->device, backup->target,
+ backup->sync,
+ backup->has_speed, backup->speed,
+ backup->has_on_source_error, backup->on_source_error,
+ backup->has_on_target_error, backup->on_target_error,
+ txn, &local_err);
if (local_err) {
error_propagate(errp, local_err);
return;
@@ -2573,15 +2610,17 @@ 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,
+ BlockJobTxn *txn, Error **errp)
{
BlockBackend *blk;
BlockDriverState *bs;
@@ -2696,7 +2735,7 @@ void qmp_drive_backup(const char *device, const char *target,
backup_start(bs, target_bs, speed, sync, bmap,
on_source_error, on_target_error,
- block_job_cb, bs, &local_err);
+ block_job_cb, bs, txn, &local_err);
if (local_err != NULL) {
bdrv_unref(target_bs);
error_propagate(errp, local_err);
@@ -2707,19 +2746,37 @@ 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)
+{
+ return 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, errp);
+}
+
BlockDeviceInfoList *qmp_query_named_block_nodes(Error **errp)
{
return bdrv_named_nodes_list(errp);
}
-void qmp_blockdev_backup(const char *device, const char *target,
+void do_blockdev_backup(const char *device, const char *target,
enum MirrorSyncMode sync,
bool has_speed, int64_t speed,
bool has_on_source_error,
BlockdevOnError on_source_error,
bool has_on_target_error,
BlockdevOnError on_target_error,
- Error **errp)
+ BlockJobTxn *txn, Error **errp)
{
BlockBackend *blk;
BlockDriverState *bs;
@@ -2757,7 +2814,7 @@ void qmp_blockdev_backup(const char *device, const char *target,
bdrv_ref(target_bs);
bdrv_set_aio_context(target_bs, aio_context);
backup_start(bs, target_bs, speed, sync, NULL, on_source_error,
- on_target_error, block_job_cb, bs, &local_err);
+ on_target_error, block_job_cb, bs, txn, &local_err);
if (local_err != NULL) {
bdrv_unref(target_bs);
error_propagate(errp, local_err);
@@ -2766,6 +2823,21 @@ out:
aio_context_release(aio_context);
}
+void qmp_blockdev_backup(const char *device, const char *target,
+ enum MirrorSyncMode sync,
+ bool has_speed, int64_t speed,
+ bool has_on_source_error,
+ BlockdevOnError on_source_error,
+ bool has_on_target_error,
+ BlockdevOnError on_target_error,
+ Error **errp)
+{
+ do_blockdev_backup(device, target, sync, has_speed, speed,
+ has_on_source_error, on_source_error,
+ has_on_target_error, on_target_error,
+ NULL, errp);
+}
+
void qmp_drive_mirror(const char *device, const char *target,
bool has_format, const char *format,
bool has_node_name, const char *node_name,
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 281d790..6fd07a2 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -643,6 +643,7 @@ void mirror_start(BlockDriverState *bs, BlockDriverState *target,
* @on_target_error: The action to take upon error writing to the target.
* @cb: Completion function for the job.
* @opaque: Opaque pointer value passed to @cb.
+ * @txn: Transaction that this job is part of (may be NULL).
*
* Start a backup operation on @bs. Clusters in @bs are written to @target
* until the job is cancelled or manually completed.
@@ -653,7 +654,7 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target,
BlockdevOnError on_source_error,
BlockdevOnError on_target_error,
BlockCompletionFunc *cb, void *opaque,
- Error **errp);
+ BlockJobTxn *txn, Error **errp);
void blk_dev_change_media_cb(BlockBackend *blk, bool load);
bool blk_dev_has_removable_media(BlockBackend *blk);
diff --git a/qapi-schema.json b/qapi-schema.json
index 583e036..6641be3 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1505,8 +1505,8 @@
{ 'union': 'TransactionAction',
'data': {
'blockdev-snapshot-sync': 'BlockdevSnapshot',
- 'drive-backup': 'DriveBackup',
- 'blockdev-backup': 'BlockdevBackup',
+ 'drive-backup': 'DriveBackupTxn',
+ 'blockdev-backup': 'BlockdevBackupTxn',
'abort': 'Abort',
'blockdev-snapshot-internal-sync': 'BlockdevSnapshotInternal',
'block-dirty-bitmap-add': 'BlockDirtyBitmapAdd',
diff --git a/qapi/block-core.json b/qapi/block-core.json
index bb2189e..24db5c2 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -735,7 +735,6 @@
# @on-target-error: #optional the action to take on an error on the target,
# default 'report' (no limitations, since this applies to
# a different block device than @device).
-#
# Note that @on-source-error and @on-target-error only affect background I/O.
# If an error occurs during a guest write request, the device's rerror/werror
# actions will be used.
@@ -750,6 +749,19 @@
'*on-target-error': 'BlockdevOnError' } }
##
+# @DriveBackupTxn
+#
+# @transactional-cancel: #optional whether failure or cancellation of other
+# block jobs with @transactional-cancel true in the same
+# transaction causes the whole group to cancel. Default
+# is false.
+#
+# Since: 2.5
+##
+{ 'struct': 'DriveBackupTxn', 'base': 'DriveBackup',
+ 'data': {'*transactional-cancel': 'bool' } }
+
+##
# @BlockdevBackup
#
# @device: the name of the device which should be copied.
@@ -785,6 +797,19 @@
'*on-target-error': 'BlockdevOnError' } }
##
+# @BlockdevBackupTxn
+#
+# @transactional-cancel: #optional whether failure or cancellation of other
+# block jobs with @transactional-cancel true in the same
+# transaction causes the whole group to cancel. Default
+# is false
+#
+# Since: 2.5
+##
+{ 'struct': 'BlockdevBackupTxn', 'base': 'BlockdevBackup',
+ 'data': {'*transactional-cancel': 'bool' } }
+
+##
# @blockdev-snapshot-sync
#
# Generates a synchronous snapshot of a block device.
--
2.4.3
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [Qemu-devel] [PATCH v7 12/14] iotests: 124 - transactional failure test
2015-09-22 2:46 [Qemu-devel] [PATCH v7 00/14] block: incremental backup transactions using BlockJobTxn Fam Zheng
` (10 preceding siblings ...)
2015-09-22 2:46 ` [Qemu-devel] [PATCH v7 11/14] block/backup: support block job transactions Fam Zheng
@ 2015-09-22 2:46 ` Fam Zheng
2015-09-22 2:46 ` [Qemu-devel] [PATCH v7 13/14] qmp-commands.hx: Update the supported 'transaction' operations Fam Zheng
` (2 subsequent siblings)
14 siblings, 0 replies; 30+ messages in thread
From: Fam Zheng @ 2015-09-22 2:46 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Jeff Cody, Max Reitz, vsementsov, stefanha, John Snow
From: John Snow <jsnow@redhat.com>
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.
To support the 'transactional-cancel' QMP argument name it's necessary
for transaction_action() to convert underscores in Python argument names
to hyphens for QMP argument names.
Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Fam Zheng <famz@redhat.com>
---
tests/qemu-iotests/124 | 130 ++++++++++++++++++++++++++++++++++++++++++++-
tests/qemu-iotests/124.out | 4 +-
2 files changed, 130 insertions(+), 4 deletions(-)
diff --git a/tests/qemu-iotests/124 b/tests/qemu-iotests/124
index 9c1977e..33d00ef 100644
--- a/tests/qemu-iotests/124
+++ b/tests/qemu-iotests/124
@@ -39,7 +39,7 @@ def try_remove(img):
def transaction_action(action, **kwargs):
return {
'type': action,
- 'data': kwargs
+ 'data': dict((k.replace('_', '-'), v) for k, v in kwargs.iteritems())
}
@@ -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.assertNotEqual(event, None)
try:
@@ -156,6 +159,12 @@ class TestIncrementalBackup(iotests.QMPTestCase):
return False
+ def wait_qmp_backup_cancelled(self, device):
+ event = self.vm.event_wait(name='BLOCK_JOB_CANCELLED',
+ match={'data': {'device': device}})
+ self.assertNotEqual(event, None)
+
+
def create_anchor_backup(self, drive=None):
if drive is None:
drive = self.drives[-1]
@@ -375,6 +384,123 @@ 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='incremental',
+ format=drive0['fmt'], mode='existing',
+ bitmap=dr0bm0.name,
+ transactional_cancel=True),
+ transaction_drive_backup(drive1['id'], target1, sync='incremental',
+ format=drive1['fmt'], mode='existing',
+ bitmap=dr1bm0.name,
+ transactional_cancel=True),
+ ]
+ result = self.vm.qmp('transaction', actions=transaction)
+ self.assert_qmp(result, 'return', {})
+
+ # Observe that drive0's backup is cancelled and drive1 completes with
+ # an error.
+ self.wait_qmp_backup_cancelled(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.4.3
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [Qemu-devel] [PATCH v7 13/14] qmp-commands.hx: Update the supported 'transaction' operations
2015-09-22 2:46 [Qemu-devel] [PATCH v7 00/14] block: incremental backup transactions using BlockJobTxn Fam Zheng
` (11 preceding siblings ...)
2015-09-22 2:46 ` [Qemu-devel] [PATCH v7 12/14] iotests: 124 - transactional failure test Fam Zheng
@ 2015-09-22 2:46 ` Fam Zheng
2015-09-30 18:56 ` John Snow
2015-09-22 2:46 ` [Qemu-devel] [PATCH v7 14/14] tests: add BlockJobTxn unit test Fam Zheng
2015-09-22 20:48 ` [Qemu-devel] [PATCH v7 00/14] block: incremental backup transactions using BlockJobTxn John Snow
14 siblings, 1 reply; 30+ messages in thread
From: Fam Zheng @ 2015-09-22 2:46 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Jeff Cody, Max Reitz, vsementsov, stefanha, John Snow
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>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Fam Zheng <famz@redhat.com>
---
qmp-commands.hx | 21 ++++++++++++++++-----
1 file changed, 16 insertions(+), 5 deletions(-)
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 66f0300..365c874 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1265,11 +1265,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.4.3
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [Qemu-devel] [PATCH v7 14/14] tests: add BlockJobTxn unit test
2015-09-22 2:46 [Qemu-devel] [PATCH v7 00/14] block: incremental backup transactions using BlockJobTxn Fam Zheng
` (12 preceding siblings ...)
2015-09-22 2:46 ` [Qemu-devel] [PATCH v7 13/14] qmp-commands.hx: Update the supported 'transaction' operations Fam Zheng
@ 2015-09-22 2:46 ` Fam Zheng
2015-09-22 23:33 ` John Snow
2015-09-22 20:48 ` [Qemu-devel] [PATCH v7 00/14] block: incremental backup transactions using BlockJobTxn John Snow
14 siblings, 1 reply; 30+ messages in thread
From: Fam Zheng @ 2015-09-22 2:46 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Jeff Cody, Max Reitz, vsementsov, stefanha, John Snow
From: Stefan Hajnoczi <stefanha@redhat.com>
The BlockJobTxn unit test verifies that both single jobs and pairs of
jobs behave as a transaction group. Either all jobs complete
successfully or the group is cancelled.
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
tests/Makefile | 3 +
tests/test-blockjob-txn.c | 244 ++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 247 insertions(+)
create mode 100644 tests/test-blockjob-txn.c
diff --git a/tests/Makefile b/tests/Makefile
index 4063639..e887a61 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -47,6 +47,8 @@ check-unit-y += tests/test-thread-pool$(EXESUF)
gcov-files-test-thread-pool-y = thread-pool.c
gcov-files-test-hbitmap-y = util/hbitmap.c
check-unit-y += tests/test-hbitmap$(EXESUF)
+gcov-files-test-hbitmap-y = blockjob.c
+check-unit-y += tests/test-blockjob-txn$(EXESUF)
check-unit-y += tests/test-x86-cpuid$(EXESUF)
# all code tested by test-x86-cpuid is inside topology.h
gcov-files-test-x86-cpuid-y =
@@ -307,6 +309,7 @@ tests/test-coroutine$(EXESUF): tests/test-coroutine.o $(test-block-obj-y)
tests/test-aio$(EXESUF): tests/test-aio.o $(test-block-obj-y)
tests/test-rfifolock$(EXESUF): tests/test-rfifolock.o $(test-util-obj-y)
tests/test-throttle$(EXESUF): tests/test-throttle.o $(test-block-obj-y)
+tests/test-blockjob-txn$(EXESUF): tests/test-blockjob-txn.o $(test-block-obj-y) $(test-util-obj-y)
tests/test-thread-pool$(EXESUF): tests/test-thread-pool.o $(test-block-obj-y)
tests/test-iov$(EXESUF): tests/test-iov.o $(test-util-obj-y)
tests/test-hbitmap$(EXESUF): tests/test-hbitmap.o $(test-util-obj-y)
diff --git a/tests/test-blockjob-txn.c b/tests/test-blockjob-txn.c
new file mode 100644
index 0000000..000f70e
--- /dev/null
+++ b/tests/test-blockjob-txn.c
@@ -0,0 +1,244 @@
+/*
+ * Blockjob transactions tests
+ *
+ * Copyright Red Hat, Inc. 2015
+ *
+ * Authors:
+ * Stefan Hajnoczi <stefanha@redhat.com>
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2 or later.
+ * See the COPYING.LIB file in the top-level directory.
+ */
+
+#include <glib.h>
+#include "qapi/error.h"
+#include "qemu/main-loop.h"
+#include "block/blockjob.h"
+
+typedef struct {
+ BlockJob common;
+ unsigned int iterations;
+ bool use_timer;
+ int rc;
+ int *result;
+} TestBlockJob;
+
+static const BlockJobDriver test_block_job_driver = {
+ .instance_size = sizeof(TestBlockJob),
+};
+
+static void test_block_job_complete(BlockJob *job, void *opaque)
+{
+ BlockDriverState *bs = job->bs;
+ int rc = (intptr_t)opaque;
+
+ if (block_job_is_cancelled(job)) {
+ rc = -ECANCELED;
+ }
+
+ block_job_completed(job, rc);
+ bdrv_unref(bs);
+}
+
+static void coroutine_fn test_block_job_run(void *opaque)
+{
+ TestBlockJob *s = opaque;
+ BlockJob *job = &s->common;
+
+ while (s->iterations--) {
+ if (s->use_timer) {
+ block_job_sleep_ns(job, QEMU_CLOCK_REALTIME, 0);
+ } else {
+ block_job_yield(job);
+ }
+
+ if (block_job_is_cancelled(job)) {
+ break;
+ }
+ }
+
+ block_job_defer_to_main_loop(job, test_block_job_complete,
+ (void *)(intptr_t)s->rc);
+}
+
+typedef struct {
+ TestBlockJob *job;
+ int *result;
+} TestBlockJobCBData;
+
+static void test_block_job_cb(void *opaque, int ret)
+{
+ TestBlockJobCBData *data = opaque;
+ if (!ret && block_job_is_cancelled(&data->job->common)) {
+ ret = -ECANCELED;
+ }
+ *data->result = ret;
+ g_free(data);
+}
+
+/* Create a block job that completes with a given return code after a given
+ * number of event loop iterations. The return code is stored in the given
+ * result pointer.
+ *
+ * The event loop iterations can either be handled automatically with a 0 delay
+ * timer, or they can be stepped manually by entering the coroutine.
+ */
+static BlockJob *test_block_job_start(unsigned int iterations,
+ bool use_timer,
+ int rc, int *result)
+{
+ BlockDriverState *bs;
+ TestBlockJob *s;
+ TestBlockJobCBData *data;
+
+ data = g_new0(TestBlockJobCBData, 1);
+ bs = bdrv_new();
+ s = block_job_create(&test_block_job_driver, bs, 0, test_block_job_cb,
+ data, &error_abort);
+ s->iterations = iterations;
+ s->use_timer = use_timer;
+ s->rc = rc;
+ s->result = result;
+ s->common.co = qemu_coroutine_create(test_block_job_run);
+ data->job = s;
+ data->result = result;
+ qemu_coroutine_enter(s->common.co, s);
+ return &s->common;
+}
+
+static void test_single_job(int expected)
+{
+ BlockJob *job;
+ BlockJobTxn *txn;
+ int result = -EINPROGRESS;
+
+ txn = block_job_txn_new();
+ job = test_block_job_start(1, true, expected, &result);
+ block_job_txn_add_job(txn, job);
+
+ if (expected == -ECANCELED) {
+ block_job_cancel(job);
+ }
+
+ while (result == -EINPROGRESS) {
+ aio_poll(qemu_get_aio_context(), true);
+ }
+ g_assert_cmpint(result, ==, expected);
+}
+
+static void test_single_job_success(void)
+{
+ test_single_job(0);
+}
+
+static void test_single_job_failure(void)
+{
+ test_single_job(-EIO);
+}
+
+static void test_single_job_cancel(void)
+{
+ test_single_job(-ECANCELED);
+}
+
+static void test_pair_jobs(int expected1, int expected2)
+{
+ BlockJob *job1;
+ BlockJob *job2;
+ BlockJobTxn *txn;
+ int result1 = -EINPROGRESS;
+ int result2 = -EINPROGRESS;
+
+ txn = block_job_txn_new();
+ job1 = test_block_job_start(1, true, expected1, &result1);
+ block_job_txn_add_job(txn, job1);
+ job2 = test_block_job_start(2, true, expected2, &result2);
+ block_job_txn_add_job(txn, job2);
+
+ if (expected1 == -ECANCELED) {
+ block_job_cancel(job1);
+ }
+ if (expected2 == -ECANCELED) {
+ block_job_cancel(job2);
+ }
+
+ while (result1 == -EINPROGRESS || result2 == -EINPROGRESS) {
+ aio_poll(qemu_get_aio_context(), true);
+ }
+
+ /* Failure or cancellation of one job cancels the other job */
+ if (expected1 != 0) {
+ expected2 = -ECANCELED;
+ } else if (expected2 != 0) {
+ expected1 = -ECANCELED;
+ }
+
+ g_assert_cmpint(result1, ==, expected1);
+ g_assert_cmpint(result2, ==, expected2);
+}
+
+static void test_pair_jobs_success(void)
+{
+ test_pair_jobs(0, 0);
+}
+
+static void test_pair_jobs_failure(void)
+{
+ /* Test both orderings. The two jobs run for a different number of
+ * iterations so the code path is different depending on which job fails
+ * first.
+ */
+ test_pair_jobs(-EIO, 0);
+ test_pair_jobs(0, -EIO);
+}
+
+static void test_pair_jobs_cancel(void)
+{
+ test_pair_jobs(-ECANCELED, 0);
+ test_pair_jobs(0, -ECANCELED);
+}
+
+static void test_pair_jobs_fail_cancel_race(void)
+{
+ BlockJob *job1;
+ BlockJob *job2;
+ BlockJobTxn *txn;
+ int result1 = -EINPROGRESS;
+ int result2 = -EINPROGRESS;
+
+ txn = block_job_txn_new();
+ job1 = test_block_job_start(1, true, -ECANCELED, &result1);
+ block_job_txn_add_job(txn, job1);
+ job2 = test_block_job_start(2, false, 0, &result2);
+ block_job_txn_add_job(txn, job2);
+
+ block_job_cancel(job1);
+
+ /* Now make job2 finish before the main loop kicks jobs. This simulates
+ * the race between a pending kick and another job completing.
+ */
+ block_job_enter(job2);
+ block_job_enter(job2);
+
+ while (result1 == -EINPROGRESS || result2 == -EINPROGRESS) {
+ aio_poll(qemu_get_aio_context(), true);
+ }
+
+ g_assert_cmpint(result1, ==, -ECANCELED);
+ g_assert_cmpint(result2, ==, -ECANCELED);
+}
+
+int main(int argc, char **argv)
+{
+ qemu_init_main_loop(&error_abort);
+
+ g_test_init(&argc, &argv, NULL);
+ g_test_add_func("/single/success", test_single_job_success);
+ g_test_add_func("/single/failure", test_single_job_failure);
+ g_test_add_func("/single/cancel", test_single_job_cancel);
+ g_test_add_func("/pair/success", test_pair_jobs_success);
+ g_test_add_func("/pair/failure", test_pair_jobs_failure);
+ g_test_add_func("/pair/cancel", test_pair_jobs_cancel);
+ g_test_add_func("/pair/fail-cancel-race", test_pair_jobs_fail_cancel_race);
+ return g_test_run();
+}
--
2.4.3
^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH v7 11/14] block/backup: support block job transactions
2015-09-22 2:46 ` [Qemu-devel] [PATCH v7 11/14] block/backup: support block job transactions Fam Zheng
@ 2015-09-22 16:03 ` Eric Blake
2015-09-22 21:08 ` John Snow
1 sibling, 0 replies; 30+ messages in thread
From: Eric Blake @ 2015-09-22 16:03 UTC (permalink / raw)
To: Fam Zheng, qemu-devel
Cc: Kevin Wolf, Jeff Cody, Max Reitz, vsementsov, stefanha, John Snow
[-- Attachment #1: Type: text/plain, Size: 1340 bytes --]
On 09/21/2015 08:46 PM, Fam Zheng wrote:
> From: Stefan Hajnoczi <stefanha@redhat.com>
>
> Join the transaction when the 'transactional-cancel' QMP argument is
> true.
>
> This ensures that the sync bitmap is not thrown away if another block
> job in the transaction is cancelled or fails. This is critical so
> incremental backup with multiple disks can be retried in case of
> cancellation/failure.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
> +++ b/qapi/block-core.json
> @@ -735,7 +735,6 @@
> # @on-target-error: #optional the action to take on an error on the target,
> # default 'report' (no limitations, since this applies to
> # a different block device than @device).
> -#
> # Note that @on-source-error and @on-target-error only affect background I/O.
> # If an error occurs during a guest write request, the device's rerror/werror
> # actions will be used.
Spurious line deletion when addressing review comment. Maintainer can
touch it up for the pull request, so my R-b still stands.
--
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 --]
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH v7 09/14] block: Add block job transactions
2015-09-22 2:46 ` [Qemu-devel] [PATCH v7 09/14] block: Add block job transactions Fam Zheng
@ 2015-09-22 19:09 ` John Snow
2015-09-24 8:29 ` Fam Zheng
0 siblings, 1 reply; 30+ messages in thread
From: John Snow @ 2015-09-22 19:09 UTC (permalink / raw)
To: Fam Zheng, qemu-devel
Cc: Kevin Wolf, Jeff Cody, vsementsov, stefanha, Max Reitz
On 09/21/2015 10:46 PM, Fam Zheng wrote:
> Sometimes block jobs must execute as a transaction group. Finishing
> jobs wait until all other jobs are ready to complete successfully.
> Failure or cancellation of one job cancels the other jobs in the group.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> [Rewrite the implementation which is now contained in block_job_completed.
> --Fam]
> Signed-off-by: Fam Zheng <famz@redhat.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> ---
> blockjob.c | 135 ++++++++++++++++++++++++++++++++++++++++++++++-
> include/block/block.h | 1 +
> include/block/blockjob.h | 38 +++++++++++++
> 3 files changed, 172 insertions(+), 2 deletions(-)
>
> diff --git a/blockjob.c b/blockjob.c
> index 36c18e0..91e8d3c 100644
> --- a/blockjob.c
> +++ b/blockjob.c
> @@ -36,6 +36,19 @@
> #include "qemu/timer.h"
> #include "qapi-event.h"
>
> +/* Transactional group of block jobs */
> +struct BlockJobTxn {
> +
> + /* Is this txn being cancelled? */
> + bool aborting;
> +
> + /* List of jobs */
> + QLIST_HEAD(, BlockJob) jobs;
> +
> + /* Reference count */
> + int refcnt;
> +};
> +
> void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs,
> int64_t speed, BlockCompletionFunc *cb,
> void *opaque, Error **errp)
> @@ -90,6 +103,86 @@ void block_job_unref(BlockJob *job)
> }
> }
>
> +static void block_job_completed_single(BlockJob *job)
> +{
> + if (!job->ret) {
> + if (job->driver->commit) {
> + job->driver->commit(job);
> + }
> + } else {
> + if (job->driver->abort) {
> + job->driver->abort(job);
> + }
> + }
> + job->cb(job->opaque, job->ret);
> + if (job->txn) {
> + block_job_txn_unref(job->txn);
> + }
> + block_job_unref(job);
> +}
> +
> +static void block_job_completed_txn_abort(BlockJob *job)
> +{
> + AioContext *ctx;
> + BlockJobTxn *txn = job->txn;
> + BlockJob *other_job, *next;
> +
> + if (txn->aborting) {
> + /*
> + * We are cancelled by another job, which will handle everything.
> + */
> + return;
> + }
> + txn->aborting = true;
> + /* We are the first failed job. Cancel other jobs. */
> + QLIST_FOREACH(other_job, &txn->jobs, txn_list) {
> + ctx = bdrv_get_aio_context(other_job->bs);
> + aio_context_acquire(ctx);
> + }
> + QLIST_FOREACH(other_job, &txn->jobs, txn_list) {
> + if (other_job == job || other_job->completed) {
> + /* Other jobs are "effectively" cancelled by us, set the status for
> + * them; this job, however, may or may not be cancelled, depending
> + * on the caller, so leave it. */
> + if (other_job != job) {
> + other_job->cancelled = true;
> + }
> + continue;
> + }
> + block_job_cancel_sync(other_job);
> + assert(other_job->completed);
> + }
> + QLIST_FOREACH_SAFE(other_job, &txn->jobs, txn_list, next) {
> + ctx = bdrv_get_aio_context(other_job->bs);
> + block_job_completed_single(other_job);
> + aio_context_release(ctx);
> + }
> +}
> +
> +static void block_job_completed_txn_success(BlockJob *job)
> +{
> + AioContext *ctx;
> + BlockJobTxn *txn = job->txn;
> + BlockJob *other_job, *next;
> + /*
> + * Successful completion, see if there are other running jobs in this
> + * txn.
> + */
> + QLIST_FOREACH(other_job, &txn->jobs, txn_list) {
> + if (!other_job->completed) {
> + return;
> + }
> + }
> + /* We are the last completed job, commit the transaction. */
> + QLIST_FOREACH_SAFE(other_job, &txn->jobs, txn_list, next) {
> + ctx = bdrv_get_aio_context(other_job->bs);
> + aio_context_acquire(ctx);
> + assert(other_job->ret == 0);
Sorry for being a dense noggin about this, but is it documented anywhere
(through an assertion or otherwise) that we will never return a
positive, non-zero return code for a block job?
I just don't want to get into a situation where, in the future, someone
decides to do so and then mysteriously something breaks a version or two
later.
> + block_job_completed_single(other_job);
> + aio_context_release(ctx);
> + }
> +}
> +
> void block_job_completed(BlockJob *job, int ret)
> {
> BlockDriverState *bs = job->bs;
> @@ -98,8 +191,13 @@ void block_job_completed(BlockJob *job, int ret)
> assert(!job->completed);
> job->completed = true;
> job->ret = ret;
> - job->cb(job->opaque, ret);
> - block_job_unref(job);
> + if (!job->txn) {
> + block_job_completed_single(job);
> + } else if (ret < 0 || block_job_is_cancelled(job)) {
> + block_job_completed_txn_abort(job);
> + } else {
> + block_job_completed_txn_success(job);
> + }
> }
>
> void block_job_set_speed(BlockJob *job, int64_t speed, Error **errp)
> @@ -398,3 +496,36 @@ void block_job_defer_to_main_loop(BlockJob *job,
>
> qemu_bh_schedule(data->bh);
> }
> +
> +BlockJobTxn *block_job_txn_new(void)
> +{
> + BlockJobTxn *txn = g_new0(BlockJobTxn, 1);
> + QLIST_INIT(&txn->jobs);
> + txn->refcnt = 1;
> + return txn;
> +}
> +
> +static void block_job_txn_ref(BlockJobTxn *txn)
> +{
> + txn->refcnt++;
> +}
> +
> +void block_job_txn_unref(BlockJobTxn *txn)
> +{
> + if (--txn->refcnt == 0) {
> + g_free(txn);
> + }
> +}
> +
> +void block_job_txn_add_job(BlockJobTxn *txn, BlockJob *job)
> +{
> + if (!txn) {
> + return;
> + }
> +
> + assert(!job->txn);
> + job->txn = txn;
> +
> + QLIST_INSERT_HEAD(&txn->jobs, job, txn_list);
> + block_job_txn_ref(txn);
> +}
> diff --git a/include/block/block.h b/include/block/block.h
> index ca56021..94548cb 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -14,6 +14,7 @@ typedef struct BlockDriver BlockDriver;
> typedef struct BlockJob BlockJob;
> typedef struct BdrvChild BdrvChild;
> typedef struct BdrvChildRole BdrvChildRole;
> +typedef struct BlockJobTxn BlockJobTxn;
>
> typedef struct BlockDriverInfo {
> /* in bytes, 0 if irrelevant */
> diff --git a/include/block/blockjob.h b/include/block/blockjob.h
> index a77799f..fa77189 100644
> --- a/include/block/blockjob.h
> +++ b/include/block/blockjob.h
> @@ -154,6 +154,9 @@ struct BlockJob {
> */
> int ret;
>
> + /** Non-NULL if this job is part of a transaction */
> + BlockJobTxn *txn;
> + QLIST_ENTRY(BlockJob) txn_list;
> };
>
> /**
> @@ -397,4 +400,39 @@ void block_job_defer_to_main_loop(BlockJob *job,
> BlockJobDeferToMainLoopFn *fn,
> void *opaque);
>
> +/**
> + * block_job_txn_new:
> + *
> + * Allocate and return a new block job transaction. Jobs can be added to the
> + * transaction using block_job_txn_add_job().
> + *
> + * The transaction is automatically freed when the last job completes or is
> + * cancelled.
> + *
> + * All jobs in the transaction either complete successfully or fail/cancel as a
> + * group. Jobs wait for each other before completing. Cancelling one job
> + * cancels all jobs in the transaction.
> + */
> +BlockJobTxn *block_job_txn_new(void);
> +
> +/**
> + * block_job_txn_unref:
> + *
> + * Release a reference that was previously acquired with block_job_txn_add_job
> + * or block_job_txn_new. If it's the last reference to the object, it will be
> + * freed.
> + */
> +void block_job_txn_unref(BlockJobTxn *txn);
> +
> +/**
> + * block_job_txn_add_job:
> + * @txn: The transaction (may be NULL)
> + * @job: Job to add to the transaction
> + *
> + * Add @job to the transaction. The @job must not already be in a transaction.
> + * The caller must call either block_job_txn_unref() or block_job_completed()
> + * to release the reference that is automatically grabbed here.
> + */
> +void block_job_txn_add_job(BlockJobTxn *txn, BlockJob *job);
> +
> #endif
>
--
—js
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH v7 10/14] blockdev: make BlockJobTxn available to qmp 'transaction'
2015-09-22 2:46 ` [Qemu-devel] [PATCH v7 10/14] blockdev: make BlockJobTxn available to qmp 'transaction' Fam Zheng
@ 2015-09-22 19:13 ` John Snow
0 siblings, 0 replies; 30+ messages in thread
From: John Snow @ 2015-09-22 19:13 UTC (permalink / raw)
To: Fam Zheng, qemu-devel
Cc: Kevin Wolf, Jeff Cody, vsementsov, stefanha, Max Reitz
On 09/21/2015 10:46 PM, Fam Zheng wrote:
> From: Stefan Hajnoczi <stefanha@redhat.com>
>
> Provide a BlockJobTxn to actions executed in a qmp 'transaction'
> command. This allows actions to make their block jobs either complete
> as a group or fail/cancel together.
>
> The next patch adds the first user.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> Reviewed-by: Fam Zheng <famz@redhat.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
> blockdev.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/blockdev.c b/blockdev.c
> index 796bc64..ed50cb4 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1285,6 +1285,7 @@ typedef struct BlkActionOps {
> struct BlkActionState {
> TransactionAction *action;
> const BlkActionOps *ops;
> + BlockJobTxn *block_job_txn;
> QSIMPLEQ_ENTRY(BlkActionState) entry;
> };
>
I know it's obvious, but for consistency we should update the
documentation sitting right above this structure.
> @@ -1883,12 +1884,15 @@ static const BlkActionOps actions[] = {
> void qmp_transaction(TransactionActionList *dev_list, Error **errp)
> {
> TransactionActionList *dev_entry = dev_list;
> + BlockJobTxn *block_job_txn;
> BlkActionState *state, *next;
> Error *local_err = NULL;
>
> QSIMPLEQ_HEAD(snap_bdrv_states, BlkActionState) snap_bdrv_states;
> QSIMPLEQ_INIT(&snap_bdrv_states);
>
> + block_job_txn = block_job_txn_new();
> +
> /* drain all i/o before any operations */
> bdrv_drain_all();
>
> @@ -1908,6 +1912,7 @@ void qmp_transaction(TransactionActionList *dev_list, Error **errp)
> state = g_malloc0(ops->instance_size);
> state->ops = ops;
> state->action = dev_info;
> + state->block_job_txn = block_job_txn;
> QSIMPLEQ_INSERT_TAIL(&snap_bdrv_states, state, entry);
>
> state->ops->prepare(state, &local_err);
> @@ -1940,6 +1945,7 @@ exit:
> }
> g_free(state);
> }
> + block_job_txn_unref(block_job_txn);
> }
>
>
>
With or without my recommendation:
Reviewed-by: John Snow <jsnow@redhat.com>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH v7 00/14] block: incremental backup transactions using BlockJobTxn
2015-09-22 2:46 [Qemu-devel] [PATCH v7 00/14] block: incremental backup transactions using BlockJobTxn Fam Zheng
` (13 preceding siblings ...)
2015-09-22 2:46 ` [Qemu-devel] [PATCH v7 14/14] tests: add BlockJobTxn unit test Fam Zheng
@ 2015-09-22 20:48 ` John Snow
14 siblings, 0 replies; 30+ messages in thread
From: John Snow @ 2015-09-22 20:48 UTC (permalink / raw)
To: Fam Zheng, qemu-devel
Cc: Kevin Wolf, Jeff Cody, vsementsov, stefanha, Max Reitz
On 09/21/2015 10:46 PM, Fam Zheng wrote:
> v7: Add Eric's rev-by in 1, 11.
> Add Max's rev-by in 4, 5, 9, 10, 11.
> Add John's rev-by in 5, 6, 8.
> Fix wording for 6. [John]
> Fix comment of block_job_txn_add_job() in 9. [Max]
> Remove superfluous hunks, and document default value in 11. [Eric]
> Update Makefile dep in 14. [Max]
>
> v6: Address comments from Max and Eric (many thanks for reviewing!):
> Add Max's reviews.
> Don't leak txn. [Max]
> Unusual comment ending "**/" -> "*/". [Eric]
> Fix the stale block_job_txn_prepare_to_complete comment. [Max]
> Move "block_job_txn_unref" to below FOREACH block. [Max]
> Split "base" and "txn" in qapi schema, so that transactional-cancel is only
> visible in transaction. [Eric]
>
> v5: Address comments from Max Reitz:
> 2.4 -> 2.5 in qapi docs.
> Don't leak txn object.
> English syntax fixes.
> Really leave the "cancelled" status of failed job.
> Remove a superfluous added line.
>
> This is based on top of the work by Stefan Hajnoczi and John Snow.
>
> Recap: motivation for block job transactions
> --------------------------------------------
> If an incremental backup block job fails then we reclaim the bitmap so
> the job can be retried. The problem comes when multiple jobs are started as
> part of a qmp 'transaction' command. We need to group these jobs in a
> transaction so that either all jobs complete successfully or all bitmaps are
> reclaimed.
>
> Without transactions, there is a case where some jobs complete successfully and
> throw away their bitmaps, making it impossible to retry the backup by rerunning
> the command if one of the jobs fails.
>
> How does this implementation work?
> ----------------------------------
> These patches add a BlockJobTxn object with the following API:
>
> txn = block_job_txn_new();
> block_job_txn_add_job(txn, job1);
> block_job_txn_add_job(txn, job2);
>
> The jobs either both complete successfully or they both fail/cancel. If the
> user cancels job1 then job2 will also be cancelled and vice versa.
>
> Jobs objects stay alive waiting for other jobs to complete, even if the
> coroutines have returned. They can be cancelled by the user during this time.
> Job blockers are still in effect and no other block job can run on this device
> in the meantime (since QEMU currently only allows 1 job per device). This is
> the main drawback to this approach but reasonable since you probably don't want
> to run other jobs/operations until you're sure the backup was successful (you
> won't be able to retry a failed backup if there's a new job running).
>
>
> Fam Zheng (6):
> backup: Extract dirty bitmap handling as a separate function
> blockjob: Introduce reference count
> blockjob: Add .commit and .abort block job actions
> blockjob: Add "completed" and "ret" in BlockJob
> blockjob: Simplify block_job_finish_sync
> block: Add block job transactions
>
> John Snow (4):
> qapi: Add transaction support to block-dirty-bitmap operations
> iotests: add transactional incremental backup test
> block: rename BlkTransactionState and BdrvActionOps
> iotests: 124 - transactional failure test
>
> Kashyap Chamarthy (1):
> qmp-commands.hx: Update the supported 'transaction' operations
>
> Stefan Hajnoczi (3):
> blockdev: make BlockJobTxn available to qmp 'transaction'
> block/backup: support block job transactions
> tests: add BlockJobTxn unit test
>
> block.c | 19 ++-
> block/backup.c | 50 +++++--
> block/mirror.c | 2 +-
> blockdev.c | 354 +++++++++++++++++++++++++++++++++++----------
> blockjob.c | 185 +++++++++++++++++++----
> docs/bitmaps.md | 6 +-
> include/block/block.h | 2 +-
> include/block/block_int.h | 6 +-
> include/block/blockjob.h | 85 ++++++++++-
> qapi-schema.json | 10 +-
> qapi/block-core.json | 27 +++-
> qmp-commands.hx | 21 ++-
> tests/Makefile | 3 +
> tests/qemu-iotests/124 | 182 ++++++++++++++++++++++-
> tests/qemu-iotests/124.out | 4 +-
> tests/test-blockjob-txn.c | 244 +++++++++++++++++++++++++++++++
> 16 files changed, 1056 insertions(+), 144 deletions(-)
> create mode 100644 tests/test-blockjob-txn.c
>
Reminder: needed as a new patch -- an update to the documentation
concerning the transactional-cancel argument added in patch 11/14 for
docs/bitmaps.md.
--js
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH v7 11/14] block/backup: support block job transactions
2015-09-22 2:46 ` [Qemu-devel] [PATCH v7 11/14] block/backup: support block job transactions Fam Zheng
2015-09-22 16:03 ` Eric Blake
@ 2015-09-22 21:08 ` John Snow
2015-09-22 22:34 ` Eric Blake
1 sibling, 1 reply; 30+ messages in thread
From: John Snow @ 2015-09-22 21:08 UTC (permalink / raw)
To: Fam Zheng, qemu-devel
Cc: Kevin Wolf, Jeff Cody, Markus Armbruster, Max Reitz, vsementsov,
stefanha
Eric, Markus: I've CC'd you for some crazy what-if QAPI questions after
my R-B on this patch.
On 09/21/2015 10:46 PM, Fam Zheng wrote:
> From: Stefan Hajnoczi <stefanha@redhat.com>
>
> Join the transaction when the 'transactional-cancel' QMP argument is
> true.
>
> This ensures that the sync bitmap is not thrown away if another block
> job in the transaction is cancelled or fails. This is critical so
> incremental backup with multiple disks can be retried in case of
> cancellation/failure.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
> block/backup.c | 25 +++++++--
> blockdev.c | 132 +++++++++++++++++++++++++++++++++++-----------
> include/block/block_int.h | 3 +-
> qapi-schema.json | 4 +-
> qapi/block-core.json | 27 +++++++++-
> 5 files changed, 152 insertions(+), 39 deletions(-)
>
> diff --git a/block/backup.c b/block/backup.c
> index 609b199..5880116 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -227,11 +227,29 @@ static void backup_cleanup_sync_bitmap(BackupBlockJob *job, int ret)
> }
> }
>
> +static void backup_commit(BlockJob *job)
> +{
> + BackupBlockJob *s = container_of(job, BackupBlockJob, common);
> + if (s->sync_bitmap) {
> + backup_cleanup_sync_bitmap(s, 0);
> + }
> +}
> +
> +static void backup_abort(BlockJob *job)
> +{
> + BackupBlockJob *s = container_of(job, BackupBlockJob, common);
> + if (s->sync_bitmap) {
> + backup_cleanup_sync_bitmap(s, -1);
> + }
> +}
> +
> static const BlockJobDriver backup_job_driver = {
> .instance_size = sizeof(BackupBlockJob),
> .job_type = BLOCK_JOB_TYPE_BACKUP,
> .set_speed = backup_set_speed,
> .iostatus_reset = backup_iostatus_reset,
> + .commit = backup_commit,
> + .abort = backup_abort,
> };
>
> static BlockErrorAction backup_error_action(BackupBlockJob *job,
> @@ -444,10 +462,6 @@ static void coroutine_fn backup_run(void *opaque)
> /* wait until pending backup_do_cow() calls have completed */
> qemu_co_rwlock_wrlock(&job->flush_rwlock);
> qemu_co_rwlock_unlock(&job->flush_rwlock);
> -
> - if (job->sync_bitmap) {
> - backup_cleanup_sync_bitmap(job, ret);
> - }
> hbitmap_free(job->bitmap);
>
> bdrv_iostatus_disable(target);
> @@ -464,7 +478,7 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target,
> BlockdevOnError on_source_error,
> BlockdevOnError on_target_error,
> BlockCompletionFunc *cb, void *opaque,
> - Error **errp)
> + BlockJobTxn *txn, Error **errp)
> {
> int64_t len;
>
> @@ -546,6 +560,7 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target,
> sync_bitmap : NULL;
> job->common.len = len;
> job->common.co = qemu_coroutine_create(backup_run);
> + block_job_txn_add_job(txn, &job->common);
> qemu_coroutine_enter(job->common.co, job);
> return;
>
So a backup job will join a transaction unconditionally if it is given one.
> diff --git a/blockdev.c b/blockdev.c
> index ed50cb4..45a9fe7 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1586,16 +1586,31 @@ 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,
> + BlockJobTxn *txn, Error **errp);
> +
> static void drive_backup_prepare(BlkActionState *common, Error **errp)
> {
> DriveBackupState *state = DO_UPCAST(DriveBackupState, common, common);
> BlockDriverState *bs;
> BlockBackend *blk;
> + DriveBackupTxn *backup_txn;
> DriveBackup *backup;
> + BlockJobTxn *txn = NULL;
> Error *local_err = NULL;
>
> assert(common->action->kind == TRANSACTION_ACTION_KIND_DRIVE_BACKUP);
> - backup = common->action->drive_backup;
> + backup_txn = common->action->drive_backup;
> + backup = backup_txn->base;
>
> blk = blk_by_name(backup->device);
> if (!blk) {
> @@ -1609,15 +1624,20 @@ 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);
> + if (backup_txn->has_transactional_cancel &&
> + backup_txn->transactional_cancel) {
> + txn = common->block_job_txn;
> + }
> +
> + do_drive_backup(backup->device, backup->target,
> + backup->has_format, backup->format,
> + backup->sync,
> + 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,
> + txn, &local_err);
> if (local_err) {
> error_propagate(errp, local_err);
> return;
So far so good. "has_transactional_cancel" can be set per-each command
so we have a fine-grained control over which commands in a transaction
we want to fail or die together, which is nice.
> @@ -1654,16 +1674,28 @@ typedef struct BlockdevBackupState {
> AioContext *aio_context;
> } BlockdevBackupState;
>
> +static void do_blockdev_backup(const char *device, const char *target,
> + enum MirrorSyncMode sync,
> + bool has_speed, int64_t speed,
> + bool has_on_source_error,
> + BlockdevOnError on_source_error,
> + bool has_on_target_error,
> + BlockdevOnError on_target_error,
> + BlockJobTxn *txn, Error **errp);
> +
> static void blockdev_backup_prepare(BlkActionState *common, Error **errp)
> {
> BlockdevBackupState *state = DO_UPCAST(BlockdevBackupState, common, common);
> + BlockdevBackupTxn *backup_txn;
> BlockdevBackup *backup;
> BlockDriverState *bs, *target;
> BlockBackend *blk;
> + BlockJobTxn *txn = NULL;
> Error *local_err = NULL;
>
> assert(common->action->kind == TRANSACTION_ACTION_KIND_BLOCKDEV_BACKUP);
> - backup = common->action->blockdev_backup;
> + backup_txn = common->action->blockdev_backup;
> + backup = backup_txn->base;
>
> blk = blk_by_name(backup->device);
> if (!blk) {
> @@ -1688,12 +1720,17 @@ static void blockdev_backup_prepare(BlkActionState *common, Error **errp)
> }
> aio_context_acquire(state->aio_context);
>
> - qmp_blockdev_backup(backup->device, backup->target,
> - backup->sync,
> - backup->has_speed, backup->speed,
> - backup->has_on_source_error, backup->on_source_error,
> - backup->has_on_target_error, backup->on_target_error,
> - &local_err);
> + if (backup_txn->has_transactional_cancel &&
> + backup_txn->transactional_cancel) {
> + txn = common->block_job_txn;
> + }
> +
> + do_blockdev_backup(backup->device, backup->target,
> + backup->sync,
> + backup->has_speed, backup->speed,
> + backup->has_on_source_error, backup->on_source_error,
> + backup->has_on_target_error, backup->on_target_error,
> + txn, &local_err);
Might as well take care of blockdev-backup while we're at it, sure.
> if (local_err) {
> error_propagate(errp, local_err);
> return;
> @@ -2573,15 +2610,17 @@ 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,
> + BlockJobTxn *txn, Error **errp)
> {
> BlockBackend *blk;
> BlockDriverState *bs;
> @@ -2696,7 +2735,7 @@ void qmp_drive_backup(const char *device, const char *target,
>
> backup_start(bs, target_bs, speed, sync, bmap,
> on_source_error, on_target_error,
> - block_job_cb, bs, &local_err);
> + block_job_cb, bs, txn, &local_err);
> if (local_err != NULL) {
> bdrv_unref(target_bs);
> error_propagate(errp, local_err);
> @@ -2707,19 +2746,37 @@ 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)
> +{
> + return 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, errp);
> +}
> +
> BlockDeviceInfoList *qmp_query_named_block_nodes(Error **errp)
> {
> return bdrv_named_nodes_list(errp);
> }
>
> -void qmp_blockdev_backup(const char *device, const char *target,
> +void do_blockdev_backup(const char *device, const char *target,
> enum MirrorSyncMode sync,
> bool has_speed, int64_t speed,
> bool has_on_source_error,
> BlockdevOnError on_source_error,
> bool has_on_target_error,
> BlockdevOnError on_target_error,
> - Error **errp)
> + BlockJobTxn *txn, Error **errp)
> {
> BlockBackend *blk;
> BlockDriverState *bs;
> @@ -2757,7 +2814,7 @@ void qmp_blockdev_backup(const char *device, const char *target,
> bdrv_ref(target_bs);
> bdrv_set_aio_context(target_bs, aio_context);
> backup_start(bs, target_bs, speed, sync, NULL, on_source_error,
> - on_target_error, block_job_cb, bs, &local_err);
> + on_target_error, block_job_cb, bs, txn, &local_err);
> if (local_err != NULL) {
> bdrv_unref(target_bs);
> error_propagate(errp, local_err);
> @@ -2766,6 +2823,21 @@ out:
> aio_context_release(aio_context);
> }
>
> +void qmp_blockdev_backup(const char *device, const char *target,
> + enum MirrorSyncMode sync,
> + bool has_speed, int64_t speed,
> + bool has_on_source_error,
> + BlockdevOnError on_source_error,
> + bool has_on_target_error,
> + BlockdevOnError on_target_error,
> + Error **errp)
> +{
> + do_blockdev_backup(device, target, sync, has_speed, speed,
> + has_on_source_error, on_source_error,
> + has_on_target_error, on_target_error,
> + NULL, errp);
> +}
> +
> void qmp_drive_mirror(const char *device, const char *target,
> bool has_format, const char *format,
> bool has_node_name, const char *node_name,
All looks good so far.
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 281d790..6fd07a2 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -643,6 +643,7 @@ void mirror_start(BlockDriverState *bs, BlockDriverState *target,
> * @on_target_error: The action to take upon error writing to the target.
> * @cb: Completion function for the job.
> * @opaque: Opaque pointer value passed to @cb.
> + * @txn: Transaction that this job is part of (may be NULL).
> *
> * Start a backup operation on @bs. Clusters in @bs are written to @target
> * until the job is cancelled or manually completed.
> @@ -653,7 +654,7 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target,
> BlockdevOnError on_source_error,
> BlockdevOnError on_target_error,
> BlockCompletionFunc *cb, void *opaque,
> - Error **errp);
> + BlockJobTxn *txn, Error **errp);
>
> void blk_dev_change_media_cb(BlockBackend *blk, bool load);
> bool blk_dev_has_removable_media(BlockBackend *blk);
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 583e036..6641be3 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -1505,8 +1505,8 @@
> { 'union': 'TransactionAction',
> 'data': {
> 'blockdev-snapshot-sync': 'BlockdevSnapshot',
> - 'drive-backup': 'DriveBackup',
> - 'blockdev-backup': 'BlockdevBackup',
> + 'drive-backup': 'DriveBackupTxn',
> + 'blockdev-backup': 'BlockdevBackupTxn',
> 'abort': 'Abort',
> 'blockdev-snapshot-internal-sync': 'BlockdevSnapshotInternal',
> 'block-dirty-bitmap-add': 'BlockDirtyBitmapAdd',
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index bb2189e..24db5c2 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -735,7 +735,6 @@
> # @on-target-error: #optional the action to take on an error on the target,
> # default 'report' (no limitations, since this applies to
> # a different block device than @device).
> -#
(Is this intentional?)
> # Note that @on-source-error and @on-target-error only affect background I/O.
> # If an error occurs during a guest write request, the device's rerror/werror
> # actions will be used.
> @@ -750,6 +749,19 @@
> '*on-target-error': 'BlockdevOnError' } }
>
> ##
> +# @DriveBackupTxn
> +#
> +# @transactional-cancel: #optional whether failure or cancellation of other
> +# block jobs with @transactional-cancel true in the same
> +# transaction causes the whole group to cancel. Default
> +# is false.
> +#
> +# Since: 2.5
> +##
> +{ 'struct': 'DriveBackupTxn', 'base': 'DriveBackup',
> + 'data': {'*transactional-cancel': 'bool' } }
> +
> +##
> # @BlockdevBackup
> #
> # @device: the name of the device which should be copied.
> @@ -785,6 +797,19 @@
> '*on-target-error': 'BlockdevOnError' } }
>
> ##
> +# @BlockdevBackupTxn
> +#
> +# @transactional-cancel: #optional whether failure or cancellation of other
> +# block jobs with @transactional-cancel true in the same
> +# transaction causes the whole group to cancel. Default
> +# is false
> +#
> +# Since: 2.5
> +##
> +{ 'struct': 'BlockdevBackupTxn', 'base': 'BlockdevBackup',
> + 'data': {'*transactional-cancel': 'bool' } }
> +
> +##
> # @blockdev-snapshot-sync
> #
> # Generates a synchronous snapshot of a block device.
>
Going to go ahead and say "OK, looks good:"
Reviewed-by: John Snow <jsnow@redhat.com>
Good, that's out of the way. Let's take a deep breath now and read some
crazy what-if scenarios. I still have some idle questions before 2.5
freeze hits us... Eric, Markus:
The current design of this patch is such that the blockdev-backup and
drive-backup QMP commands are extended for use in the transaction
action. This means that as part of the arguments for the action, we can
specify "transactional-cancel" as on/off, defaulting to off. This
maintains backwards compatible behavior.
As an example of a lone command (for simplicity...)
{ "execute": "transaction",
"arguments": {
"actions": [
{"type": "drive-backup",
"data": {"device": "drive0",
"target": "/path/to/new_full_backup.img",
"sync": "full", "format": "qcow2",
"transactional-cancel": true
}
}
]
}
}
This means that this command should fail if any of the other block jobs
in the transaction (that have also set transactional-cancel(!)) fail.
This is nice because it allows us to specify per-action which actions
should live or die on the success of the transaction as a whole.
What I was wondering is if we wanted to pidgeon-hole ourselves like this
by adding arguments per-action and instead opt for a more generic,
transaction-wide setting.
In my mind, the transactional cancel has nothing to do with each
/specific/ action, but has more to do with a property of transactions
and actions in general.
So I was thinking of two things:
(1) Transactional cancel, while false by default, could be toggled to
true by default for an entire transaction all-at-once
{ "execute": "transaction",
"arguments": {
"actions": [ ... ],
"transactional-cancel": true
}
}
This would toggle the default state for all actions to "fail if anything
else in the transaction does."
Of course, as of right now, not all actions actually support this
feature, but they might need to in the future -- and as more and more
actions use this feature, it might be nice to have the global setting.
If "transactional-cancel" was specified and is true (i.e. not the
default) and actions are provided that do not support the argument
explicitly, the transaction command itself should fail up-front.
(2) Overridable per-action settings as a property of "action"
Instead of adding the argument per-each action, bake it in as a property
of the action itself. The original idea behind this was to decouple it
from the QMP arguments definition, but this patch here also accomplishes
this -- at the expense of subclassing each QMP argument set manually.
Something like this is what I was originally thinking of doing:
{ "execute": "transaction",
"arguments": {
"actions": [
{ "type": "drive-backup",
"data": { ... },
"properties": { "transactional-cancel": true }
}
]
}
}
In light of how Fam and Eric solved the problem of "not polluting the
QMP arguments" for this patch, #2 is perhaps less pressing or interesting.
A benefit, though, is that we can define a set of generic transactional
action properties that all actions can inspect to adjust their behavior
without us needing to specify additional argument subclasses in the QAPI
every time.
Mostly, at this point, I want to ask if we are OK with adding the
"transactional-cancel" argument ad-hoc per-action forever, or if we'd
like to allow a global setting or move the property "up the stack" to
some extent.
Maybe the answer is "no," but I wanted to throw the idea out there
before we committed to an API.
Sorry for the wall'o'text :)
--js
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH v7 11/14] block/backup: support block job transactions
2015-09-22 21:08 ` John Snow
@ 2015-09-22 22:34 ` Eric Blake
2015-09-22 23:27 ` John Snow
0 siblings, 1 reply; 30+ messages in thread
From: Eric Blake @ 2015-09-22 22:34 UTC (permalink / raw)
To: John Snow, Fam Zheng, qemu-devel
Cc: Kevin Wolf, Jeff Cody, Markus Armbruster, Max Reitz, vsementsov,
stefanha
[-- Attachment #1: Type: text/plain, Size: 6461 bytes --]
On 09/22/2015 03:08 PM, John Snow wrote:
> Eric, Markus: I've CC'd you for some crazy what-if QAPI questions after
> my R-B on this patch.
>
> The current design of this patch is such that the blockdev-backup and
> drive-backup QMP commands are extended for use in the transaction
> action. This means that as part of the arguments for the action, we can
> specify "transactional-cancel" as on/off, defaulting to off. This
> maintains backwards compatible behavior.
>
>
> As an example of a lone command (for simplicity...)
>
> { "execute": "transaction",
> "arguments": {
> "actions": [
> {"type": "drive-backup",
> "data": {"device": "drive0",
> "target": "/path/to/new_full_backup.img",
> "sync": "full", "format": "qcow2",
> "transactional-cancel": true
> }
> }
> ]
> }
> }
>
> This means that this command should fail if any of the other block jobs
> in the transaction (that have also set transactional-cancel(!)) fail.
Just wondering - is there ever a case where someone would want to create
a transaction with multiple sub-groups? That is, I want actions A, B,
C, D to all occur at the same point in time, but with grouping [A,B] and
[C, D] (so that if A fails B gets cancelled, but C and D can still
continue)? Worse, is there a series of actions to accomplish something
that cannot happen unless it is interleaved across multiple such subgroups?
Here's hoping that, for design simplicity, we only ever need one
subgroup per 'transaction' (auto-cancel semantics applies to all
commands in the group that opted in, with no way to further refine into
sub-groups of commands).
>
> This is nice because it allows us to specify per-action which actions
> should live or die on the success of the transaction as a whole.
>
> What I was wondering is if we wanted to pidgeon-hole ourselves like this
> by adding arguments per-action and instead opt for a more generic,
> transaction-wide setting.
>
> In my mind, the transactional cancel has nothing to do with each
> /specific/ action, but has more to do with a property of transactions
> and actions in general.
>
>
> So I was thinking of two things:
>
> (1) Transactional cancel, while false by default, could be toggled to
> true by default for an entire transaction all-at-once
>
> { "execute": "transaction",
> "arguments": {
> "actions": [ ... ],
> "transactional-cancel": true
> }
> }
>
> This would toggle the default state for all actions to "fail if anything
> else in the transaction does."
Or worded in another way - what is the use case for having a batch of
actions where some commands are grouped-cancel, but the remaining
actions are independent? Is there ever a case where you would supply
"transactional-cancel":true to one action, but not all of them? If not,
then this idea of hoisting the bool to the transaction arguments level
makes more sense (it's an all-or-none switch, not a per-action switch).
Qapi-wise, here's how you would do this:
{ 'command': 'transaction',
'data': { 'actions': [ 'TransactionAction' ],
'*transactional-cancel': 'bool' } }
>
> Of course, as of right now, not all actions actually support this
> feature, but they might need to in the future -- and as more and more
> actions use this feature, it might be nice to have the global setting.
>
> If "transactional-cancel" was specified and is true (i.e. not the
> default) and actions are provided that do not support the argument
> explicitly, the transaction command itself should fail up-front.
This actually makes sense to me, and might be simpler to implement.
>
> (2) Overridable per-action settings as a property of "action"
>
> Instead of adding the argument per-each action, bake it in as a property
> of the action itself. The original idea behind this was to decouple it
> from the QMP arguments definition, but this patch here also accomplishes
> this -- at the expense of subclassing each QMP argument set manually.
>
> Something like this is what I was originally thinking of doing:
>
> { "execute": "transaction",
> "arguments": {
> "actions": [
> { "type": "drive-backup",
> "data": { ... },
> "properties": { "transactional-cancel": true }
> }
> ]
> }
> }
>
> In light of how Fam and Eric solved the problem of "not polluting the
> QMP arguments" for this patch, #2 is perhaps less pressing or interesting.
>
> A benefit, though, is that we can define a set of generic transactional
> action properties that all actions can inspect to adjust their behavior
> without us needing to specify additional argument subclasses in the QAPI
> every time.
And here's how we'd do it in qapi. We'd have to convert
TransactionAction from simple union into flat union (here, using syntax
that doesn't work on qemu.git mainline, but requires my v5 00/46
mega-patch of introspection followups - hmm, it's still verbose, so
maybe we want to invent yet more sugar to avoid having to declare all
those wrapper types):
{ 'enum': 'TransactionActionKind', 'data': [
'blockdev-snapshot-sync', 'drive-backup', 'blockdev-backup',
'abort', 'blockdev-snapshot-internal-sync' ] }
{ 'struct': 'TransactionProperties',
'data': { '*transactional-cancel': 'bool' } }
{ 'struct': 'BlockdevSnapshotSyncWrapper',
'data': { 'data': 'BlockdevSnapshotSync' } }
{ 'union': 'TransactionAction',
'base': { 'type': 'TransactionActionKind',
'*properties': 'TransactionProperties'},
'discriminator': 'type',
'data': { 'blockdev-snapshot-sync': 'BlockdevSnapshotSyncWrapper',
... } }
>
> Mostly, at this point, I want to ask if we are OK with adding the
> "transactional-cancel" argument ad-hoc per-action forever, or if we'd
> like to allow a global setting or move the property "up the stack" to
> some extent.
>
> Maybe the answer is "no," but I wanted to throw the idea out there
> before we committed to an API.
No, it's a good question. And adding it once at the 'transaction' layer
or in the 'TransactionAction' union may indeed make more sense from the
design perspective, rather than ad-hoc adding to each (growing) member
of the union.
--
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 --]
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH v7 11/14] block/backup: support block job transactions
2015-09-22 22:34 ` Eric Blake
@ 2015-09-22 23:27 ` John Snow
2015-09-23 11:09 ` Markus Armbruster
0 siblings, 1 reply; 30+ messages in thread
From: John Snow @ 2015-09-22 23:27 UTC (permalink / raw)
To: Eric Blake, Fam Zheng, qemu-devel
Cc: Kevin Wolf, Jeff Cody, Markus Armbruster, Max Reitz, vsementsov,
stefanha
On 09/22/2015 06:34 PM, Eric Blake wrote:
> On 09/22/2015 03:08 PM, John Snow wrote:
>> Eric, Markus: I've CC'd you for some crazy what-if QAPI questions
>> after my R-B on this patch.
>>
>
>> The current design of this patch is such that the blockdev-backup
>> and drive-backup QMP commands are extended for use in the
>> transaction action. This means that as part of the arguments for
>> the action, we can specify "transactional-cancel" as on/off,
>> defaulting to off. This maintains backwards compatible behavior.
>>
>>
>> As an example of a lone command (for simplicity...)
>>
>> { "execute": "transaction", "arguments": { "actions": [ {"type":
>> "drive-backup", "data": {"device": "drive0", "target":
>> "/path/to/new_full_backup.img", "sync": "full", "format":
>> "qcow2", "transactional-cancel": true } } ] } }
>>
>> This means that this command should fail if any of the other
>> block jobs in the transaction (that have also set
>> transactional-cancel(!)) fail.
>
> Just wondering - is there ever a case where someone would want to
> create a transaction with multiple sub-groups? That is, I want
> actions A, B, C, D to all occur at the same point in time, but with
> grouping [A,B] and [C, D] (so that if A fails B gets cancelled, but
> C and D can still
Only two sub-groups:
(A) actions that live and die together
(B) actions that proceed no matter what.
The only reason we even allow these two is because the default
behavior has been B historically, so we need to allow that to continue on.
I don't think we need to architect multiple subgroups of "live and die
together" semantics.
> continue)? Worse, is there a series of actions to accomplish
> something that cannot happen unless it is interleaved across
> multiple such subgroups?
>
Not sure we'll need to address this.
> Here's hoping that, for design simplicity, we only ever need one
> subgroup per 'transaction' (auto-cancel semantics applies to all
> commands in the group that opted in, with no way to further refine
> into sub-groups of commands).
>
I think this is correct.
>>
>> This is nice because it allows us to specify per-action which
>> actions should live or die on the success of the transaction as a
>> whole.
>>
>> What I was wondering is if we wanted to pidgeon-hole ourselves
>> like this by adding arguments per-action and instead opt for a
>> more generic, transaction-wide setting.
>>
>> In my mind, the transactional cancel has nothing to do with each
>> /specific/ action, but has more to do with a property of
>> transactions and actions in general.
>>
>>
>> So I was thinking of two things:
>>
>> (1) Transactional cancel, while false by default, could be
>> toggled to true by default for an entire transaction all-at-once
>>
>> { "execute": "transaction", "arguments": { "actions": [ ... ],
>> "transactional-cancel": true } }
>>
>> This would toggle the default state for all actions to "fail if
>> anything else in the transaction does."
>
> Or worded in another way - what is the use case for having a batch
> of actions where some commands are grouped-cancel, but the
> remaining actions are independent? Is there ever a case where you
> would supply "transactional-cancel":true to one action, but not all
> of them? If not, then this idea of hoisting the bool to the
> transaction arguments level makes more sense (it's an all-or-none
> switch, not a per-action switch).
>
> Qapi-wise, here's how you would do this:
>
> { 'command': 'transaction', 'data': { 'actions': [
> 'TransactionAction' ], '*transactional-cancel': 'bool' } }
>
Right. If there's no need for us to specify per-action settings at
all, this becomes the obvious "correct" solution.
If we do want to be able to specify this sub-grouping per-action (for
whatever reason), this might still be nice as a convenience feature.
>>
>> Of course, as of right now, not all actions actually support
>> this feature, but they might need to in the future -- and as more
>> and more actions use this feature, it might be nice to have the
>> global setting.
>>
>> If "transactional-cancel" was specified and is true (i.e. not
>> the default) and actions are provided that do not support the
>> argument explicitly, the transaction command itself should fail
>> up-front.
>
> This actually makes sense to me, and might be simpler to
> implement.
>
I'm not sure how to implement this, per-se, but in my mind it's
something like:
- A property of the transaction gets set (transactional-cancel) in
qmp_transaction.
- Each action has to prove that it has retrieved this value
- drive-backup of course actually uses it,
- other commands might fetch it to intentionally ignore it
(i.e. if cancel y/n would have no effect)
- If an action does not fetch this property, the transactional setup
flags an error and aborts the transaction with an error
- e.g. "Attempted to use property unsupported by action ..."
With perhaps an allowance that, as long as the property is set to
default, actions aren't required to check it.
>>
>> (2) Overridable per-action settings as a property of "action"
>>
>> Instead of adding the argument per-each action, bake it in as a
>> property of the action itself. The original idea behind this was
>> to decouple it from the QMP arguments definition, but this patch
>> here also accomplishes this -- at the expense of subclassing each
>> QMP argument set manually.
>>
>> Something like this is what I was originally thinking of doing:
>>
>> { "execute": "transaction", "arguments": { "actions": [ { "type":
>> "drive-backup", "data": { ... }, "properties": {
>> "transactional-cancel": true } } ] } }
>>
>> In light of how Fam and Eric solved the problem of "not polluting
>> the QMP arguments" for this patch, #2 is perhaps less pressing or
>> interesting.
>>
>> A benefit, though, is that we can define a set of generic
>> transactional action properties that all actions can inspect to
>> adjust their behavior without us needing to specify additional
>> argument subclasses in the QAPI every time.
>
> And here's how we'd do it in qapi. We'd have to convert
> TransactionAction from simple union into flat union (here, using
> syntax that doesn't work on qemu.git mainline, but requires my v5
> 00/46 mega-patch of introspection followups - hmm, it's still
> verbose, so maybe we want to invent yet more sugar to avoid having
> to declare all those wrapper types):
>
> { 'enum': 'TransactionActionKind', 'data': [
> 'blockdev-snapshot-sync', 'drive-backup', 'blockdev-backup',
> 'abort', 'blockdev-snapshot-internal-sync' ] } { 'struct':
> 'TransactionProperties', 'data': { '*transactional-cancel': 'bool'
> } } { 'struct': 'BlockdevSnapshotSyncWrapper', 'data': { 'data':
> 'BlockdevSnapshotSync' } } { 'union': 'TransactionAction', 'base':
> { 'type': 'TransactionActionKind', '*properties':
> 'TransactionProperties'}, 'discriminator': 'type', 'data': {
> 'blockdev-snapshot-sync': 'BlockdevSnapshotSyncWrapper', ... } }
>
>>
>> Mostly, at this point, I want to ask if we are OK with adding
>> the "transactional-cancel" argument ad-hoc per-action forever, or
>> if we'd like to allow a global setting or move the property "up
>> the stack" to some extent.
>>
>> Maybe the answer is "no," but I wanted to throw the idea out
>> there before we committed to an API.
>
> No, it's a good question. And adding it once at the 'transaction'
> layer or in the 'TransactionAction' union may indeed make more
> sense from the design perspective, rather than ad-hoc adding to
> each (growing) member of the union.
>
#2 was just another method of hoisting the QAPI arguments that are
transaction-specific away from the QMP arguments, and won't
necessarily be needed in conjunction with #1.
--js
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH v7 14/14] tests: add BlockJobTxn unit test
2015-09-22 2:46 ` [Qemu-devel] [PATCH v7 14/14] tests: add BlockJobTxn unit test Fam Zheng
@ 2015-09-22 23:33 ` John Snow
0 siblings, 0 replies; 30+ messages in thread
From: John Snow @ 2015-09-22 23:33 UTC (permalink / raw)
To: Fam Zheng, qemu-devel
Cc: Kevin Wolf, Jeff Cody, vsementsov, stefanha, Max Reitz
On 09/21/2015 10:46 PM, Fam Zheng wrote:
> From: Stefan Hajnoczi <stefanha@redhat.com>
>
> The BlockJobTxn unit test verifies that both single jobs and pairs of
> jobs behave as a transaction group. Either all jobs complete
> successfully or the group is cancelled.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> ---
> tests/Makefile | 3 +
> tests/test-blockjob-txn.c | 244 ++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 247 insertions(+)
> create mode 100644 tests/test-blockjob-txn.c
>
> diff --git a/tests/Makefile b/tests/Makefile
> index 4063639..e887a61 100644
> --- a/tests/Makefile
> +++ b/tests/Makefile
> @@ -47,6 +47,8 @@ check-unit-y += tests/test-thread-pool$(EXESUF)
> gcov-files-test-thread-pool-y = thread-pool.c
> gcov-files-test-hbitmap-y = util/hbitmap.c
> check-unit-y += tests/test-hbitmap$(EXESUF)
> +gcov-files-test-hbitmap-y = blockjob.c
> +check-unit-y += tests/test-blockjob-txn$(EXESUF)
> check-unit-y += tests/test-x86-cpuid$(EXESUF)
> # all code tested by test-x86-cpuid is inside topology.h
> gcov-files-test-x86-cpuid-y =
> @@ -307,6 +309,7 @@ tests/test-coroutine$(EXESUF): tests/test-coroutine.o $(test-block-obj-y)
> tests/test-aio$(EXESUF): tests/test-aio.o $(test-block-obj-y)
> tests/test-rfifolock$(EXESUF): tests/test-rfifolock.o $(test-util-obj-y)
> tests/test-throttle$(EXESUF): tests/test-throttle.o $(test-block-obj-y)
> +tests/test-blockjob-txn$(EXESUF): tests/test-blockjob-txn.o $(test-block-obj-y) $(test-util-obj-y)
> tests/test-thread-pool$(EXESUF): tests/test-thread-pool.o $(test-block-obj-y)
> tests/test-iov$(EXESUF): tests/test-iov.o $(test-util-obj-y)
> tests/test-hbitmap$(EXESUF): tests/test-hbitmap.o $(test-util-obj-y)
> diff --git a/tests/test-blockjob-txn.c b/tests/test-blockjob-txn.c
> new file mode 100644
> index 0000000..000f70e
> --- /dev/null
> +++ b/tests/test-blockjob-txn.c
> @@ -0,0 +1,244 @@
> +/*
> + * Blockjob transactions tests
> + *
> + * Copyright Red Hat, Inc. 2015
> + *
> + * Authors:
> + * Stefan Hajnoczi <stefanha@redhat.com>
> + *
> + * This work is licensed under the terms of the GNU LGPL, version 2 or later.
> + * See the COPYING.LIB file in the top-level directory.
> + */
> +
> +#include <glib.h>
> +#include "qapi/error.h"
> +#include "qemu/main-loop.h"
> +#include "block/blockjob.h"
> +
> +typedef struct {
> + BlockJob common;
> + unsigned int iterations;
> + bool use_timer;
> + int rc;
> + int *result;
> +} TestBlockJob;
> +
> +static const BlockJobDriver test_block_job_driver = {
> + .instance_size = sizeof(TestBlockJob),
> +};
> +
> +static void test_block_job_complete(BlockJob *job, void *opaque)
> +{
> + BlockDriverState *bs = job->bs;
> + int rc = (intptr_t)opaque;
> +
> + if (block_job_is_cancelled(job)) {
> + rc = -ECANCELED;
> + }
> +
> + block_job_completed(job, rc);
> + bdrv_unref(bs);
> +}
> +
> +static void coroutine_fn test_block_job_run(void *opaque)
> +{
> + TestBlockJob *s = opaque;
> + BlockJob *job = &s->common;
> +
> + while (s->iterations--) {
> + if (s->use_timer) {
> + block_job_sleep_ns(job, QEMU_CLOCK_REALTIME, 0);
> + } else {
> + block_job_yield(job);
> + }
> +
> + if (block_job_is_cancelled(job)) {
> + break;
> + }
> + }
> +
> + block_job_defer_to_main_loop(job, test_block_job_complete,
> + (void *)(intptr_t)s->rc);
> +}
> +
> +typedef struct {
> + TestBlockJob *job;
> + int *result;
> +} TestBlockJobCBData;
> +
> +static void test_block_job_cb(void *opaque, int ret)
> +{
> + TestBlockJobCBData *data = opaque;
> + if (!ret && block_job_is_cancelled(&data->job->common)) {
> + ret = -ECANCELED;
> + }
> + *data->result = ret;
> + g_free(data);
> +}
> +
> +/* Create a block job that completes with a given return code after a given
> + * number of event loop iterations. The return code is stored in the given
> + * result pointer.
> + *
> + * The event loop iterations can either be handled automatically with a 0 delay
> + * timer, or they can be stepped manually by entering the coroutine.
> + */
> +static BlockJob *test_block_job_start(unsigned int iterations,
> + bool use_timer,
> + int rc, int *result)
> +{
> + BlockDriverState *bs;
> + TestBlockJob *s;
> + TestBlockJobCBData *data;
> +
> + data = g_new0(TestBlockJobCBData, 1);
> + bs = bdrv_new();
> + s = block_job_create(&test_block_job_driver, bs, 0, test_block_job_cb,
> + data, &error_abort);
> + s->iterations = iterations;
> + s->use_timer = use_timer;
> + s->rc = rc;
> + s->result = result;
> + s->common.co = qemu_coroutine_create(test_block_job_run);
> + data->job = s;
> + data->result = result;
> + qemu_coroutine_enter(s->common.co, s);
> + return &s->common;
> +}
> +
> +static void test_single_job(int expected)
> +{
> + BlockJob *job;
> + BlockJobTxn *txn;
> + int result = -EINPROGRESS;
> +
> + txn = block_job_txn_new();
> + job = test_block_job_start(1, true, expected, &result);
> + block_job_txn_add_job(txn, job);
> +
> + if (expected == -ECANCELED) {
> + block_job_cancel(job);
> + }
> +
> + while (result == -EINPROGRESS) {
> + aio_poll(qemu_get_aio_context(), true);
> + }
> + g_assert_cmpint(result, ==, expected);
> +}
> +
> +static void test_single_job_success(void)
> +{
> + test_single_job(0);
> +}
> +
> +static void test_single_job_failure(void)
> +{
> + test_single_job(-EIO);
> +}
> +
> +static void test_single_job_cancel(void)
> +{
> + test_single_job(-ECANCELED);
> +}
> +
> +static void test_pair_jobs(int expected1, int expected2)
> +{
> + BlockJob *job1;
> + BlockJob *job2;
> + BlockJobTxn *txn;
> + int result1 = -EINPROGRESS;
> + int result2 = -EINPROGRESS;
> +
> + txn = block_job_txn_new();
> + job1 = test_block_job_start(1, true, expected1, &result1);
> + block_job_txn_add_job(txn, job1);
> + job2 = test_block_job_start(2, true, expected2, &result2);
> + block_job_txn_add_job(txn, job2);
> +
> + if (expected1 == -ECANCELED) {
> + block_job_cancel(job1);
> + }
> + if (expected2 == -ECANCELED) {
> + block_job_cancel(job2);
> + }
> +
> + while (result1 == -EINPROGRESS || result2 == -EINPROGRESS) {
> + aio_poll(qemu_get_aio_context(), true);
> + }
> +
> + /* Failure or cancellation of one job cancels the other job */
> + if (expected1 != 0) {
> + expected2 = -ECANCELED;
> + } else if (expected2 != 0) {
> + expected1 = -ECANCELED;
> + }
> +
> + g_assert_cmpint(result1, ==, expected1);
> + g_assert_cmpint(result2, ==, expected2);
> +}
> +
> +static void test_pair_jobs_success(void)
> +{
> + test_pair_jobs(0, 0);
> +}
> +
> +static void test_pair_jobs_failure(void)
> +{
> + /* Test both orderings. The two jobs run for a different number of
> + * iterations so the code path is different depending on which job fails
> + * first.
> + */
> + test_pair_jobs(-EIO, 0);
> + test_pair_jobs(0, -EIO);
> +}
> +
> +static void test_pair_jobs_cancel(void)
> +{
> + test_pair_jobs(-ECANCELED, 0);
> + test_pair_jobs(0, -ECANCELED);
> +}
> +
> +static void test_pair_jobs_fail_cancel_race(void)
> +{
> + BlockJob *job1;
> + BlockJob *job2;
> + BlockJobTxn *txn;
> + int result1 = -EINPROGRESS;
> + int result2 = -EINPROGRESS;
> +
> + txn = block_job_txn_new();
> + job1 = test_block_job_start(1, true, -ECANCELED, &result1);
> + block_job_txn_add_job(txn, job1);
> + job2 = test_block_job_start(2, false, 0, &result2);
> + block_job_txn_add_job(txn, job2);
> +
> + block_job_cancel(job1);
> +
> + /* Now make job2 finish before the main loop kicks jobs. This simulates
> + * the race between a pending kick and another job completing.
> + */
> + block_job_enter(job2);
> + block_job_enter(job2);
> +
> + while (result1 == -EINPROGRESS || result2 == -EINPROGRESS) {
> + aio_poll(qemu_get_aio_context(), true);
> + }
> +
> + g_assert_cmpint(result1, ==, -ECANCELED);
> + g_assert_cmpint(result2, ==, -ECANCELED);
> +}
> +
> +int main(int argc, char **argv)
> +{
> + qemu_init_main_loop(&error_abort);
> +
> + g_test_init(&argc, &argv, NULL);
> + g_test_add_func("/single/success", test_single_job_success);
> + g_test_add_func("/single/failure", test_single_job_failure);
> + g_test_add_func("/single/cancel", test_single_job_cancel);
> + g_test_add_func("/pair/success", test_pair_jobs_success);
> + g_test_add_func("/pair/failure", test_pair_jobs_failure);
> + g_test_add_func("/pair/cancel", test_pair_jobs_cancel);
> + g_test_add_func("/pair/fail-cancel-race", test_pair_jobs_fail_cancel_race);
> + return g_test_run();
> +}
>
Reviewed-by: John Snow <jsnow@redhat.com>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH v7 11/14] block/backup: support block job transactions
2015-09-22 23:27 ` John Snow
@ 2015-09-23 11:09 ` Markus Armbruster
2015-09-23 16:14 ` John Snow
0 siblings, 1 reply; 30+ messages in thread
From: Markus Armbruster @ 2015-09-23 11:09 UTC (permalink / raw)
To: John Snow
Cc: Kevin Wolf, Fam Zheng, Jeff Cody, qemu-devel, Max Reitz,
vsementsov, stefanha
John, your MUA turned the QMP examples to mush. You may want to teach
it manners.
John Snow <jsnow@redhat.com> writes:
> On 09/22/2015 06:34 PM, Eric Blake wrote:
>> On 09/22/2015 03:08 PM, John Snow wrote:
>>> Eric, Markus: I've CC'd you for some crazy what-if QAPI questions
>>> after my R-B on this patch.
>>>
>>
>>> The current design of this patch is such that the blockdev-backup
>>> and drive-backup QMP commands are extended for use in the
>>> transaction action. This means that as part of the arguments for
>>> the action, we can specify "transactional-cancel" as on/off,
>>> defaulting to off. This maintains backwards compatible behavior.
>>>
>>>
>>> As an example of a lone command (for simplicity...)
>>>
>>> { "execute": "transaction",
>>> "arguments": {
>>> "actions": [
>>> {"type": "drive-backup",
>>> "data": {"device": "drive0",
>>> "target": "/path/to/new_full_backup.img",
>>> "sync": "full", "format": "qcow2",
>>> "transactional-cancel": true
>>> }
>>> }
>>> ]
>>> }
>>> }
>>>
>>> This means that this command should fail if any of the other
>>> block jobs in the transaction (that have also set
>>> transactional-cancel(!)) fail.
>>
>> Just wondering - is there ever a case where someone would want to
>> create a transaction with multiple sub-groups? That is, I want
>> actions A, B, C, D to all occur at the same point in time, but with
>> grouping [A,B] and [C, D] (so that if A fails B gets cancelled, but
>> C and D can still
You make my head hurt.
> Only two sub-groups:
>
> (A) actions that live and die together
> (B) actions that proceed no matter what.
>
> The only reason we even allow these two is because the default
> behavior has been B historically, so we need to allow that to continue on.
>
> I don't think we need to architect multiple subgroups of "live and die
> together" semantics.
>
>> continue)? Worse, is there a series of actions to accomplish
>> something that cannot happen unless it is interleaved across
>> multiple such subgroups?
>>
>
> Not sure we'll need to address this.
>
>> Here's hoping that, for design simplicity, we only ever need one
>> subgroup per 'transaction' (auto-cancel semantics applies to all
>> commands in the group that opted in, with no way to further refine
>> into sub-groups of commands).
>>
>
> I think this is correct.
Puh!
>>>
>>> This is nice because it allows us to specify per-action which
>>> actions should live or die on the success of the transaction as a
>>> whole.
>>>
>>> What I was wondering is if we wanted to pidgeon-hole ourselves
>>> like this by adding arguments per-action and instead opt for a
>>> more generic, transaction-wide setting.
>>>
>>> In my mind, the transactional cancel has nothing to do with each
>>> /specific/ action, but has more to do with a property of
>>> transactions and actions in general.
>>>
>>>
>>> So I was thinking of two things:
>>>
>>> (1) Transactional cancel, while false by default, could be
>>> toggled to true by default for an entire transaction all-at-once
>>>
>>> { "execute": "transaction",
>>> "arguments": {
>>> "actions": [ ... ],
>>> "transactional-cancel": true
>>> }
>>> }
>>>
>>> This would toggle the default state for all actions to "fail if
>>> anything else in the transaction does."
>>
>> Or worded in another way - what is the use case for having a batch
>> of actions where some commands are grouped-cancel, but the
>> remaining actions are independent? Is there ever a case where you
>> would supply "transactional-cancel":true to one action, but not all
>> of them? If not, then this idea of hoisting the bool to the
>> transaction arguments level makes more sense (it's an all-or-none
>> switch, not a per-action switch).
>>
>> Qapi-wise, here's how you would do this:
>>
>> { 'command': 'transaction',
>> 'data': { 'actions': [ 'TransactionAction' ],
>> '*transactional-cancel': 'bool' } }
>>
>
> Right. If there's no need for us to specify per-action settings at
> all, this becomes the obvious "correct" solution.
>
> If we do want to be able to specify this sub-grouping per-action (for
> whatever reason), this might still be nice as a convenience feature.
A common flag is semantically simpler than a per-action flag. As
always, the more complex solution needs to be justified with an actual
use case.
A common @transactional-cancel defaulting to false suffices for backward
compatibility.
We think users will generally want to set it to true for all actions.
Again, a common flag suffices.
Unless someone comes up with actual use case for a per-action flag,
let's stick to the simpler common flag.
>>> Of course, as of right now, not all actions actually support this
>>>feature, but they might need to in the future -- and as more and more
>>>actions use this feature, it might be nice to have the global
>>>setting.
Uh-oh, you mean the flag is currently per-action because only some kinds
of actions actually implement it being set?
>>> If "transactional-cancel" was specified and is true (i.e. not
>>> the default) and actions are provided that do not support the
>>> argument explicitly, the transaction command itself should fail
>>> up-front.
>>
>> This actually makes sense to me, and might be simpler to
>> implement.
Yes, that's the stupidest solution that could possibly work, and
therefore the one that should be tried first.
> I'm not sure how to implement this, per-se, but in my mind it's
> something like:
>
> - A property of the transaction gets set (transactional-cancel) in
> qmp_transaction.
> - Each action has to prove that it has retrieved this value
> - drive-backup of course actually uses it,
> - other commands might fetch it to intentionally ignore it
> (i.e. if cancel y/n would have no effect)
> - If an action does not fetch this property, the transactional setup
> flags an error and aborts the transaction with an error
> - e.g. "Attempted to use property unsupported by action ..."
>
> With perhaps an allowance that, as long as the property is set to
> default, actions aren't required to check it.
Do we really need code to detect commands that don't know about the
flag? As long as the number of transaction-capable commands is small,
why not simple make them all aware of the flag? Make the ones that
don't implement it fail, which nicely fails the whole transaction.
[...]
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH v7 11/14] block/backup: support block job transactions
2015-09-23 11:09 ` Markus Armbruster
@ 2015-09-23 16:14 ` John Snow
2015-09-24 6:34 ` Markus Armbruster
0 siblings, 1 reply; 30+ messages in thread
From: John Snow @ 2015-09-23 16:14 UTC (permalink / raw)
To: Markus Armbruster
Cc: Kevin Wolf, Fam Zheng, Jeff Cody, qemu-devel, Max Reitz,
vsementsov, stefanha
On 09/23/2015 07:09 AM, Markus Armbruster wrote:
> John, your MUA turned the QMP examples to mush. You may want to teach
> it manners.
>
Ugh, sorry. I apparently can't trust
> John Snow <jsnow@redhat.com> writes:
>
>> On 09/22/2015 06:34 PM, Eric Blake wrote:
>>> On 09/22/2015 03:08 PM, John Snow wrote:
>>>> Eric, Markus: I've CC'd you for some crazy what-if QAPI questions
>>>> after my R-B on this patch.
>>>>
>>>
>>>> The current design of this patch is such that the blockdev-backup
>>>> and drive-backup QMP commands are extended for use in the
>>>> transaction action. This means that as part of the arguments for
>>>> the action, we can specify "transactional-cancel" as on/off,
>>>> defaulting to off. This maintains backwards compatible behavior.
>>>>
>>>>
>>>> As an example of a lone command (for simplicity...)
>>>>
>>>> { "execute": "transaction",
>>>> "arguments": {
>>>> "actions": [
>>>> {"type": "drive-backup",
>>>> "data": {"device": "drive0",
>>>> "target": "/path/to/new_full_backup.img",
>>>> "sync": "full", "format": "qcow2",
>>>> "transactional-cancel": true
>>>> }
>>>> }
>>>> ]
>>>> }
>>>> }
>>>>
>>>> This means that this command should fail if any of the other
>>>> block jobs in the transaction (that have also set
>>>> transactional-cancel(!)) fail.
>>>
>>> Just wondering - is there ever a case where someone would want to
>>> create a transaction with multiple sub-groups? That is, I want
>>> actions A, B, C, D to all occur at the same point in time, but with
>>> grouping [A,B] and [C, D] (so that if A fails B gets cancelled, but
>>> C and D can still
>
> You make my head hurt.
>
>> Only two sub-groups:
>>
>> (A) actions that live and die together
>> (B) actions that proceed no matter what.
>>
>> The only reason we even allow these two is because the default
>> behavior has been B historically, so we need to allow that to continue on.
>>
>> I don't think we need to architect multiple subgroups of "live and die
>> together" semantics.
>>
>>> continue)? Worse, is there a series of actions to accomplish
>>> something that cannot happen unless it is interleaved across
>>> multiple such subgroups?
>>>
>>
>> Not sure we'll need to address this.
>>
>>> Here's hoping that, for design simplicity, we only ever need one
>>> subgroup per 'transaction' (auto-cancel semantics applies to all
>>> commands in the group that opted in, with no way to further refine
>>> into sub-groups of commands).
>>>
>>
>> I think this is correct.
>
> Puh!
>
"I think this is correct*"
"*If we do want to allow per-action specificity of this option."
>>>>
>>>> This is nice because it allows us to specify per-action which
>>>> actions should live or die on the success of the transaction as a
>>>> whole.
>>>>
>>>> What I was wondering is if we wanted to pidgeon-hole ourselves
>>>> like this by adding arguments per-action and instead opt for a
>>>> more generic, transaction-wide setting.
>>>>
>>>> In my mind, the transactional cancel has nothing to do with each
>>>> /specific/ action, but has more to do with a property of
>>>> transactions and actions in general.
>>>>
>>>>
>>>> So I was thinking of two things:
>>>>
>>>> (1) Transactional cancel, while false by default, could be
>>>> toggled to true by default for an entire transaction all-at-once
>>>>
>>>> { "execute": "transaction",
>>>> "arguments": {
>>>> "actions": [ ... ],
>>>> "transactional-cancel": true
>>>> }
>>>> }
>>>>
>>>> This would toggle the default state for all actions to "fail if
>>>> anything else in the transaction does."
>>>
>>> Or worded in another way - what is the use case for having a batch
>>> of actions where some commands are grouped-cancel, but the
>>> remaining actions are independent? Is there ever a case where you
>>> would supply "transactional-cancel":true to one action, but not all
>>> of them? If not, then this idea of hoisting the bool to the
>>> transaction arguments level makes more sense (it's an all-or-none
>>> switch, not a per-action switch).
>>>
>>> Qapi-wise, here's how you would do this:
>>>
>>> { 'command': 'transaction',
>>> 'data': { 'actions': [ 'TransactionAction' ],
>>> '*transactional-cancel': 'bool' } }
>>>
>>
>> Right. If there's no need for us to specify per-action settings at
>> all, this becomes the obvious "correct" solution.
>>
>> If we do want to be able to specify this sub-grouping per-action (for
>> whatever reason), this might still be nice as a convenience feature.
>
> A common flag is semantically simpler than a per-action flag. As
> always, the more complex solution needs to be justified with an actual
> use case.
>
> A common @transactional-cancel defaulting to false suffices for backward
> compatibility.
>
> We think users will generally want to set it to true for all actions.
> Again, a common flag suffices.
>
> Unless someone comes up with actual use case for a per-action flag,
> let's stick to the simpler common flag.
>
>>>> Of course, as of right now, not all actions actually support this
>>>> feature, but they might need to in the future -- and as more and more
>>>> actions use this feature, it might be nice to have the global
>>>> setting.
>
> Uh-oh, you mean the flag is currently per-action because only some kinds
> of actions actually implement it being set?
>
More like: This is the first time we've ever needed it, and we've only
been considering how drive-backup will behave under this flag.
We are implementing this property for the first time for (essentially)
one command. Other commands don't support the behavior yet because we've
just added it.
I am intervening and asking how we want to express the property.
>>>> If "transactional-cancel" was specified and is true (i.e. not
>>>> the default) and actions are provided that do not support the
>>>> argument explicitly, the transaction command itself should fail
>>>> up-front.
>>>
>>> This actually makes sense to me, and might be simpler to
>>> implement.
>
> Yes, that's the stupidest solution that could possibly work, and
> therefore the one that should be tried first.
>
>> I'm not sure how to implement this, per-se, but in my mind it's
>> something like:
>>
>> - A property of the transaction gets set (transactional-cancel) in
>> qmp_transaction.
>> - Each action has to prove that it has retrieved this value
>> - drive-backup of course actually uses it,
>> - other commands might fetch it to intentionally ignore it
>> (i.e. if cancel y/n would have no effect)
>> - If an action does not fetch this property, the transactional setup
>> flags an error and aborts the transaction with an error
>> - e.g. "Attempted to use property unsupported by action ..."
>>
>> With perhaps an allowance that, as long as the property is set to
>> default, actions aren't required to check it.
>
> Do we really need code to detect commands that don't know about the
> flag? As long as the number of transaction-capable commands is small,
> why not simple make them all aware of the flag? Make the ones that
> don't implement it fail, which nicely fails the whole transaction.
>
> [...]
>
Oh, I see: you're saying ...
each action fetches the property, and if it's true, fail (for 2.5, at
least) with e.g. a message saying "This property is not [yet?] supported
for this action"
That's simple. I always overcomplicate everything ...
--js
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH v7 11/14] block/backup: support block job transactions
2015-09-23 16:14 ` John Snow
@ 2015-09-24 6:34 ` Markus Armbruster
2015-09-25 16:05 ` John Snow
0 siblings, 1 reply; 30+ messages in thread
From: Markus Armbruster @ 2015-09-24 6:34 UTC (permalink / raw)
To: John Snow
Cc: Kevin Wolf, Fam Zheng, Jeff Cody, qemu-devel, Max Reitz,
vsementsov, stefanha
John Snow <jsnow@redhat.com> writes:
[...]
> Oh, I see: you're saying ...
>
> each action fetches the property, and if it's true, fail (for 2.5, at
> least) with e.g. a message saying "This property is not [yet?] supported
> for this action"
Yes.
> That's simple. I always overcomplicate everything ...
Simple solutions become simple only once you've thought of them :)
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH v7 09/14] block: Add block job transactions
2015-09-22 19:09 ` John Snow
@ 2015-09-24 8:29 ` Fam Zheng
0 siblings, 0 replies; 30+ messages in thread
From: Fam Zheng @ 2015-09-24 8:29 UTC (permalink / raw)
To: John Snow
Cc: Kevin Wolf, Jeff Cody, qemu-devel, Max Reitz, vsementsov,
stefanha
On Tue, 09/22 15:09, John Snow wrote:
>
>
> On 09/21/2015 10:46 PM, Fam Zheng wrote:
> > Sometimes block jobs must execute as a transaction group. Finishing
> > jobs wait until all other jobs are ready to complete successfully.
> > Failure or cancellation of one job cancels the other jobs in the group.
> >
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > [Rewrite the implementation which is now contained in block_job_completed.
> > --Fam]
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > Reviewed-by: Max Reitz <mreitz@redhat.com>
> > ---
> > blockjob.c | 135 ++++++++++++++++++++++++++++++++++++++++++++++-
> > include/block/block.h | 1 +
> > include/block/blockjob.h | 38 +++++++++++++
> > 3 files changed, 172 insertions(+), 2 deletions(-)
> >
> > diff --git a/blockjob.c b/blockjob.c
> > index 36c18e0..91e8d3c 100644
> > --- a/blockjob.c
> > +++ b/blockjob.c
> > @@ -36,6 +36,19 @@
> > #include "qemu/timer.h"
> > #include "qapi-event.h"
> >
> > +/* Transactional group of block jobs */
> > +struct BlockJobTxn {
> > +
> > + /* Is this txn being cancelled? */
> > + bool aborting;
> > +
> > + /* List of jobs */
> > + QLIST_HEAD(, BlockJob) jobs;
> > +
> > + /* Reference count */
> > + int refcnt;
> > +};
> > +
> > void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs,
> > int64_t speed, BlockCompletionFunc *cb,
> > void *opaque, Error **errp)
> > @@ -90,6 +103,86 @@ void block_job_unref(BlockJob *job)
> > }
> > }
> >
> > +static void block_job_completed_single(BlockJob *job)
> > +{
> > + if (!job->ret) {
> > + if (job->driver->commit) {
> > + job->driver->commit(job);
> > + }
> > + } else {
> > + if (job->driver->abort) {
> > + job->driver->abort(job);
> > + }
> > + }
> > + job->cb(job->opaque, job->ret);
> > + if (job->txn) {
> > + block_job_txn_unref(job->txn);
> > + }
> > + block_job_unref(job);
> > +}
> > +
> > +static void block_job_completed_txn_abort(BlockJob *job)
> > +{
> > + AioContext *ctx;
> > + BlockJobTxn *txn = job->txn;
> > + BlockJob *other_job, *next;
> > +
> > + if (txn->aborting) {
> > + /*
> > + * We are cancelled by another job, which will handle everything.
> > + */
> > + return;
> > + }
> > + txn->aborting = true;
> > + /* We are the first failed job. Cancel other jobs. */
> > + QLIST_FOREACH(other_job, &txn->jobs, txn_list) {
> > + ctx = bdrv_get_aio_context(other_job->bs);
> > + aio_context_acquire(ctx);
> > + }
> > + QLIST_FOREACH(other_job, &txn->jobs, txn_list) {
> > + if (other_job == job || other_job->completed) {
> > + /* Other jobs are "effectively" cancelled by us, set the status for
> > + * them; this job, however, may or may not be cancelled, depending
> > + * on the caller, so leave it. */
> > + if (other_job != job) {
> > + other_job->cancelled = true;
> > + }
> > + continue;
> > + }
> > + block_job_cancel_sync(other_job);
> > + assert(other_job->completed);
> > + }
> > + QLIST_FOREACH_SAFE(other_job, &txn->jobs, txn_list, next) {
> > + ctx = bdrv_get_aio_context(other_job->bs);
> > + block_job_completed_single(other_job);
> > + aio_context_release(ctx);
> > + }
> > +}
> > +
> > +static void block_job_completed_txn_success(BlockJob *job)
> > +{
> > + AioContext *ctx;
> > + BlockJobTxn *txn = job->txn;
> > + BlockJob *other_job, *next;
> > + /*
> > + * Successful completion, see if there are other running jobs in this
> > + * txn.
> > + */
> > + QLIST_FOREACH(other_job, &txn->jobs, txn_list) {
> > + if (!other_job->completed) {
> > + return;
> > + }
> > + }
> > + /* We are the last completed job, commit the transaction. */
> > + QLIST_FOREACH_SAFE(other_job, &txn->jobs, txn_list, next) {
> > + ctx = bdrv_get_aio_context(other_job->bs);
> > + aio_context_acquire(ctx);
> > + assert(other_job->ret == 0);
>
> Sorry for being a dense noggin about this, but is it documented anywhere
> (through an assertion or otherwise) that we will never return a
> positive, non-zero return code for a block job?
>
> I just don't want to get into a situation where, in the future, someone
> decides to do so and then mysteriously something breaks a version or two
> later.
No, I don't think it's documented. And yes we should document it.
Anyway if someone decides to do so he will hit the assertion here immediately,
instead of a version or two later.
Fam
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH v7 11/14] block/backup: support block job transactions
2015-09-24 6:34 ` Markus Armbruster
@ 2015-09-25 16:05 ` John Snow
0 siblings, 0 replies; 30+ messages in thread
From: John Snow @ 2015-09-25 16:05 UTC (permalink / raw)
To: Markus Armbruster
Cc: Kevin Wolf, Fam Zheng, Jeff Cody, qemu-devel, Max Reitz,
vsementsov, stefanha
On 09/24/2015 02:34 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
>
> [...]
>> Oh, I see: you're saying ...
>>
>> each action fetches the property, and if it's true, fail (for 2.5, at
>> least) with e.g. a message saying "This property is not [yet?] supported
>> for this action"
>
> Yes.
>
>> That's simple. I always overcomplicate everything ...
>
> Simple solutions become simple only once you've thought of them :)
>
I authored a quick RFC to illustrate what this solution might look like.
--js
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH v7 13/14] qmp-commands.hx: Update the supported 'transaction' operations
2015-09-22 2:46 ` [Qemu-devel] [PATCH v7 13/14] qmp-commands.hx: Update the supported 'transaction' operations Fam Zheng
@ 2015-09-30 18:56 ` John Snow
2015-10-01 10:57 ` Kashyap Chamarthy
0 siblings, 1 reply; 30+ messages in thread
From: John Snow @ 2015-09-30 18:56 UTC (permalink / raw)
To: Fam Zheng, qemu-devel
Cc: Kevin Wolf, Jeff Cody, Max Reitz, vsementsov, stefanha
On 09/21/2015 10:46 PM, Fam Zheng wrote:
> 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>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
> qmp-commands.hx | 21 ++++++++++++++++-----
> 1 file changed, 16 insertions(+), 5 deletions(-)
>
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 66f0300..365c874 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -1265,11 +1265,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
>
It recently occurred to me that this patch is incomplete, see just below:
'Arguments:
actions array:
- "type": the operation to perform. The only supported
value is "blockdev-snapshot-sync". (json-string)
'
That's not the only type, now -- all the others listed above are valid
too, so this section could use a brush-up as well.
--js
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH v7 13/14] qmp-commands.hx: Update the supported 'transaction' operations
2015-09-30 18:56 ` John Snow
@ 2015-10-01 10:57 ` Kashyap Chamarthy
0 siblings, 0 replies; 30+ messages in thread
From: Kashyap Chamarthy @ 2015-10-01 10:57 UTC (permalink / raw)
To: John Snow
Cc: Kevin Wolf, Fam Zheng, Jeff Cody, qemu-devel, Max Reitz,
vsementsov, stefanha
On Wed, Sep 30, 2015 at 02:56:14PM -0400, John Snow wrote:
>
>
> On 09/21/2015 10:46 PM, Fam Zheng wrote:
> > 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>
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > ---
> > qmp-commands.hx | 21 ++++++++++++++++-----
> > 1 file changed, 16 insertions(+), 5 deletions(-)
> >
> > diff --git a/qmp-commands.hx b/qmp-commands.hx
> > index 66f0300..365c874 100644
> > --- a/qmp-commands.hx
> > +++ b/qmp-commands.hx
> > @@ -1265,11 +1265,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
> >
>
> It recently occurred to me that this patch is incomplete, see just below:
>
> 'Arguments:
>
> actions array:
> - "type": the operation to perform. The only supported
> value is "blockdev-snapshot-sync". (json-string)
> '
>
>
> That's not the only type, now -- all the others listed above are valid
> too, so this section could use a brush-up as well.
Good find, John. I missed that detail.
Despite _testing_ multiple array types myself earlier :-)
-----
$ ./qmp-shell -v /tmp/qmp-sock
(QEMU) transaction(
TRANS> blockdev-snapshot-internal-sync device=drive-ide0-0-0 name=snapshot5
TRANS> block-dirty-bitmap-add node=drive-ide0-0-0 name=bitmap1
TRANS> block-dirty-bitmap-clear node=drive-ide0-0-0 name=bitmap0
TRANS> drive-backup device=drive-ide0-0-0 bitmap=bitmap1 sync=dirty-bitmap target=./incremental.0.img mode=existing format=qcow2
TRANS> )
{"execute": "transaction", "arguments": {"actions": [{"data": {"device": "drive-ide0-0-0", "name": "snapshot5"}, "type": "blockdev-snapshot-internal-sync"}, {"data": {"node": "drive-ide0-0-0", "name": "bitmap1"}, "type": "block-dirty-bitmap-add"}, {"data": {"node": "drive-ide0-0-0", "name": "bitmap0"}, "type": "block-dirty-bitmap-clear"}, {"data": {"target": "./incremental.0.img", "format": "qcow2", "sync": "dirty-bitmap", "bitmap": "bitmap1", "mode": "existing", "device": "drive-ide0-0-0"}, "type": "drive-backup"}]}}
{"return": {}}
(QEMU)
-----
Will send an updated patch.
--
/kashyap
^ permalink raw reply [flat|nested] 30+ messages in thread
end of thread, other threads:[~2015-10-01 10:57 UTC | newest]
Thread overview: 30+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-09-22 2:46 [Qemu-devel] [PATCH v7 00/14] block: incremental backup transactions using BlockJobTxn Fam Zheng
2015-09-22 2:46 ` [Qemu-devel] [PATCH v7 01/14] qapi: Add transaction support to block-dirty-bitmap operations Fam Zheng
2015-09-22 2:46 ` [Qemu-devel] [PATCH v7 02/14] iotests: add transactional incremental backup test Fam Zheng
2015-09-22 2:46 ` [Qemu-devel] [PATCH v7 03/14] block: rename BlkTransactionState and BdrvActionOps Fam Zheng
2015-09-22 2:46 ` [Qemu-devel] [PATCH v7 04/14] backup: Extract dirty bitmap handling as a separate function Fam Zheng
2015-09-22 2:46 ` [Qemu-devel] [PATCH v7 05/14] blockjob: Introduce reference count Fam Zheng
2015-09-22 2:46 ` [Qemu-devel] [PATCH v7 06/14] blockjob: Add .commit and .abort block job actions Fam Zheng
2015-09-22 2:46 ` [Qemu-devel] [PATCH v7 07/14] blockjob: Add "completed" and "ret" in BlockJob Fam Zheng
2015-09-22 2:46 ` [Qemu-devel] [PATCH v7 08/14] blockjob: Simplify block_job_finish_sync Fam Zheng
2015-09-22 2:46 ` [Qemu-devel] [PATCH v7 09/14] block: Add block job transactions Fam Zheng
2015-09-22 19:09 ` John Snow
2015-09-24 8:29 ` Fam Zheng
2015-09-22 2:46 ` [Qemu-devel] [PATCH v7 10/14] blockdev: make BlockJobTxn available to qmp 'transaction' Fam Zheng
2015-09-22 19:13 ` John Snow
2015-09-22 2:46 ` [Qemu-devel] [PATCH v7 11/14] block/backup: support block job transactions Fam Zheng
2015-09-22 16:03 ` Eric Blake
2015-09-22 21:08 ` John Snow
2015-09-22 22:34 ` Eric Blake
2015-09-22 23:27 ` John Snow
2015-09-23 11:09 ` Markus Armbruster
2015-09-23 16:14 ` John Snow
2015-09-24 6:34 ` Markus Armbruster
2015-09-25 16:05 ` John Snow
2015-09-22 2:46 ` [Qemu-devel] [PATCH v7 12/14] iotests: 124 - transactional failure test Fam Zheng
2015-09-22 2:46 ` [Qemu-devel] [PATCH v7 13/14] qmp-commands.hx: Update the supported 'transaction' operations Fam Zheng
2015-09-30 18:56 ` John Snow
2015-10-01 10:57 ` Kashyap Chamarthy
2015-09-22 2:46 ` [Qemu-devel] [PATCH v7 14/14] tests: add BlockJobTxn unit test Fam Zheng
2015-09-22 23:33 ` John Snow
2015-09-22 20:48 ` [Qemu-devel] [PATCH v7 00/14] block: incremental backup transactions using BlockJobTxn John Snow
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).