* [Qemu-devel] [PATCH v4 00/11] block: incremental backup transactions
@ 2015-05-11 23:04 John Snow
2015-05-11 23:04 ` [Qemu-devel] [PATCH v4 01/11] qapi: Add transaction support to block-dirty-bitmap operations John Snow
` (10 more replies)
0 siblings, 11 replies; 29+ messages in thread
From: John Snow @ 2015-05-11 23:04 UTC (permalink / raw)
To: qemu-block
Cc: kwolf, famz, John Snow, qemu-devel, mreitz, vsementsov, stefanha
Patch 1 adds basic support for add and clear transactions.
Patch 2 tests this basic support.
Patches 3-4 refactor transactions a little bit, to add clarity.
Patch 5 adds the framework for error scenarios where only
some jobs that were launched by a transaction complete successfully,
and we need to perform context-sensitive cleanup after the transaction
itself has already completed.
Patches 6-7 add necessary bookkeeping information to backup job
data structures to take advantage of this new feature.
Patch 8 modifies qmp_drive_backup to support the new feature.
Patch 9 implements the new feature for drive_backup transaction actions.
Patch 10 tests the new feature.
Patch 11 updates documentation.
===
v4:
===
Key:
[----] : patches are identical
[####] : number of functional differences between upstream/downstream patch
[down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively
001/11:[0045] [FC] 'qapi: Add transaction support to block-dirty-bitmap operations'
002/11:[----] [--] 'iotests: add transactional incremental backup test'
003/11:[0002] [FC] 'block: rename BlkTransactionState and BdrvActionOps'
004/11:[----] [--] 'block: re-add BlkTransactionState'
005/11:[----] [--] 'block: add transactional callbacks feature'
006/11:[----] [--] 'block: add refcount to Job object'
007/11:[----] [--] 'block: add delayed bitmap successor cleanup'
008/11:[----] [-C] 'qmp: Add an implementation wrapper for qmp_drive_backup'
009/11:[----] [--] 'block: drive_backup transaction callback support'
010/11:[----] [--] 'iotests: 124 - transactional failure test'
011/11:[down] 'qmp-commands.hx: Update the supported 'transaction' operations'
01: New approach for clear transaction.
bdrv_clear_dirty_bitmap (and undo_clear) moved to block_int.h
Removed more outdated documentation from bitmaps.md
03: Fallout from adding a _clear_abort() method.
11: New documentation patch from Kashyap.
===
v3:
===
01: Removed "(Not yet implemented)" line from bitmaps.md.
Kept R-Bs, like a selfish person would.
02: Rebased on latest transactionless series.
Added some transaction helpers.
05: Removed superfluous deletion loop in put_blk_action_state.
08: Rebased without the preceding code motion patch.
Re-added forward declaration for do_drive_backup.
09: Fixed commit message whitespace.
Re-added block_job_cb forward declaration to avoid code motion patch.
10: Rebased on latest transactionless series;
Added "wait_qmp_backup" function to complement "do_qmp_backup."
General bike-shedding.
===
v2:
===
01: Fixed indentation.
Fixed QMP commands to behave with new bitmap_lookup from
transactionless-v4.
2.3 --> 2.4.
02: Folded in improvements to qemu-iotest 124 from transactional-v1.
03: NEW
04: NEW
05: A lot:
Don't delete the comma in the transaction actions config
use g_new0 instead of g_malloc0
Phrasing: "retcode" --> "Return code"
Use GCC attributes to mark functions as unused until future patches.
Added some data structure documentation.
Many structure and function renames, hopefully to improve readability.
Use just one list for all Actions instead of two separate lists.
Remove ActionState from the list upon deletion/decref
And many other small tweaks.
06: Comment phrasing.
07: Removed errp parameter from all functions introduced by this commit.
bdrv_dirty_bitmap_decref --> bdrv_frozen_bitmap_decref
08: NEW
09: _drive_backup --> do_drive_backup()
Forward declarations removed.
10: Rebased on top of drastically modified #05.
Phrasing: "BackupBlockJob" instead of "BackupJob" in comments.
11: Removed extra parameters to wait_incremental() in
test_transaction_failure()
==
For convenience, this branch is available at:
https://github.com/jnsnow/qemu.git branch incremental-transactions
https://github.com/jnsnow/qemu/tree/incremental-transactions
This version is tagged incremental-transactions-v4:
https://github.com/jnsnow/qemu/releases/tag/incremental-transactions-v4
==
John Snow (10):
qapi: Add transaction support to block-dirty-bitmap operations
iotests: add transactional incremental backup test
block: rename BlkTransactionState and BdrvActionOps
block: re-add BlkTransactionState
block: add transactional callbacks feature
block: add refcount to Job object
block: add delayed bitmap successor cleanup
qmp: Add an implementation wrapper for qmp_drive_backup
block: drive_backup transaction callback support
iotests: 124 - transactional failure test
Kashyap Chamarthy (1):
qmp-commands.hx: Update the supported 'transaction' operations
block.c | 84 ++++++-
block/backup.c | 29 ++-
blockdev.c | 580 +++++++++++++++++++++++++++++++++++++++------
blockjob.c | 18 +-
docs/bitmaps.md | 6 +-
include/block/block.h | 11 +-
include/block/block_int.h | 11 +
include/block/blockjob.h | 21 ++
qapi-schema.json | 6 +-
qmp-commands.hx | 13 +-
tests/qemu-iotests/124 | 174 +++++++++++++-
tests/qemu-iotests/124.out | 4 +-
12 files changed, 843 insertions(+), 114 deletions(-)
--
2.1.0
^ permalink raw reply [flat|nested] 29+ messages in thread
* [Qemu-devel] [PATCH v4 01/11] qapi: Add transaction support to block-dirty-bitmap operations
2015-05-11 23:04 [Qemu-devel] [PATCH v4 00/11] block: incremental backup transactions John Snow
@ 2015-05-11 23:04 ` John Snow
2015-05-18 16:14 ` Max Reitz
2015-05-11 23:04 ` [Qemu-devel] [PATCH v4 02/11] iotests: add transactional incremental backup test John Snow
` (9 subsequent siblings)
10 siblings, 1 reply; 29+ messages in thread
From: John Snow @ 2015-05-11 23:04 UTC (permalink / raw)
To: qemu-block
Cc: kwolf, famz, John Snow, qemu-devel, mreitz, vsementsov, stefanha
This adds two qmp commands to transactions.
block-dirty-bitmap-add allows you to create a bitmap simultaneously
alongside a new full backup to accomplish a clean synchronization
point.
block-dirty-bitmap-clear allows you to reset a bitmap back to as-if
it were new, which can also be used alongside a full backup to
accomplish a clean synchronization point.
Signed-off-by: Fam Zheng <famz@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
---
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 7904098..ca5b1e9 100644
--- a/block.c
+++ b/block.c
@@ -3306,10 +3306,25 @@ void bdrv_reset_dirty_bitmap(BdrvDirtyBitmap *bitmap,
hbitmap_reset(bitmap->bitmap, cur_sector, nr_sectors);
}
-void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap)
+void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap **out)
{
assert(bdrv_dirty_bitmap_enabled(bitmap));
- hbitmap_reset(bitmap->bitmap, 0, bitmap->size);
+ if (!out) {
+ hbitmap_reset(bitmap->bitmap, 0, bitmap->size);
+ } else {
+ HBitmap *backup = bitmap->bitmap;
+ bitmap->bitmap = hbitmap_alloc(bitmap->size,
+ hbitmap_granularity(backup));
+ *out = backup;
+ }
+}
+
+void bdrv_undo_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap *in)
+{
+ HBitmap *tmp = bitmap->bitmap;
+ assert(bdrv_dirty_bitmap_enabled(bitmap));
+ bitmap->bitmap = in;
+ hbitmap_free(tmp);
}
void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector,
diff --git a/blockdev.c b/blockdev.c
index 5eaf77e..a62cc4b 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1694,6 +1694,106 @@ static void blockdev_backup_clean(BlkTransactionState *common)
}
}
+typedef struct BlockDirtyBitmapState {
+ BlkTransactionState common;
+ BdrvDirtyBitmap *bitmap;
+ BlockDriverState *bs;
+ AioContext *aio_context;
+ HBitmap *backup;
+ bool prepared;
+} BlockDirtyBitmapState;
+
+static void block_dirty_bitmap_add_prepare(BlkTransactionState *common,
+ Error **errp)
+{
+ Error *local_err = NULL;
+ BlockDirtyBitmapAdd *action;
+ BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
+ common, common);
+
+ action = common->action->block_dirty_bitmap_add;
+ /* AIO context taken and released within qmp_block_dirty_bitmap_add */
+ qmp_block_dirty_bitmap_add(action->node, action->name,
+ action->has_granularity, action->granularity,
+ &local_err);
+
+ if (!local_err) {
+ state->prepared = true;
+ } else {
+ error_propagate(errp, local_err);
+ }
+}
+
+static void block_dirty_bitmap_add_abort(BlkTransactionState *common)
+{
+ BlockDirtyBitmapAdd *action;
+ BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
+ common, common);
+
+ action = common->action->block_dirty_bitmap_add;
+ /* Should not be able to fail: IF the bitmap was added via .prepare(),
+ * then the node reference and bitmap name must have been valid.
+ */
+ if (state->prepared) {
+ qmp_block_dirty_bitmap_remove(action->node, action->name, &error_abort);
+ }
+}
+
+static void block_dirty_bitmap_clear_prepare(BlkTransactionState *common,
+ Error **errp)
+{
+ BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
+ common, common);
+ BlockDirtyBitmap *action;
+
+ action = common->action->block_dirty_bitmap_clear;
+ state->bitmap = block_dirty_bitmap_lookup(action->node,
+ action->name,
+ &state->bs,
+ &state->aio_context,
+ errp);
+ if (!state->bitmap) {
+ return;
+ }
+
+ if (bdrv_dirty_bitmap_frozen(state->bitmap)) {
+ error_setg(errp, "Cannot modify a frozen bitmap");
+ return;
+ } else if (!bdrv_dirty_bitmap_enabled(state->bitmap)) {
+ error_setg(errp, "Cannot clear a disabled bitmap");
+ return;
+ }
+
+ bdrv_clear_dirty_bitmap(state->bitmap, &state->backup);
+ /* AioContext is released in .clean() */
+}
+
+static void block_dirty_bitmap_clear_abort(BlkTransactionState *common)
+{
+ BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
+ common, common);
+
+ bdrv_undo_clear_dirty_bitmap(state->bitmap, state->backup);
+}
+
+static void block_dirty_bitmap_clear_commit(BlkTransactionState *common)
+{
+ BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
+ common, common);
+
+ hbitmap_free(state->backup);
+}
+
+static void block_dirty_bitmap_clear_clean(BlkTransactionState *common)
+{
+ BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
+ common, common);
+
+ if (state->aio_context) {
+ aio_context_release(state->aio_context);
+ }
+}
+
static void abort_prepare(BlkTransactionState *common, Error **errp)
{
error_setg(errp, "Transaction aborted using Abort action");
@@ -1734,6 +1834,18 @@ static const BdrvActionOps actions[] = {
.abort = internal_snapshot_abort,
.clean = internal_snapshot_clean,
},
+ [TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_ADD] = {
+ .instance_size = sizeof(BlockDirtyBitmapState),
+ .prepare = block_dirty_bitmap_add_prepare,
+ .abort = block_dirty_bitmap_add_abort,
+ },
+ [TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_CLEAR] = {
+ .instance_size = sizeof(BlockDirtyBitmapState),
+ .prepare = block_dirty_bitmap_clear_prepare,
+ .commit = block_dirty_bitmap_clear_commit,
+ .abort = block_dirty_bitmap_clear_abort,
+ .clean = block_dirty_bitmap_clear_clean,
+ }
};
/*
@@ -2107,7 +2219,7 @@ void qmp_block_dirty_bitmap_clear(const char *node, const char *name,
goto out;
}
- bdrv_clear_dirty_bitmap(bitmap);
+ bdrv_clear_dirty_bitmap(bitmap, NULL);
out:
aio_context_release(aio_context);
diff --git a/docs/bitmaps.md b/docs/bitmaps.md
index f066b48..a60fee1 100644
--- a/docs/bitmaps.md
+++ b/docs/bitmaps.md
@@ -97,11 +97,7 @@ which is included at the end of this document.
}
```
-## Transactions (Not yet implemented)
-
-* Transactional commands are forthcoming in a future version,
- and are not yet available for use. This section serves as
- documentation of intent for their design and usage.
+## Transactions
### Justification
diff --git a/include/block/block.h b/include/block/block.h
index 7d1a717..9f0ff3d 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -479,7 +479,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 db29b74..50355df 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -638,4 +638,7 @@ void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector, int nr_sectors);
void bdrv_reset_dirty(BlockDriverState *bs, int64_t cur_sector,
int nr_sectors);
+void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap **out);
+void bdrv_undo_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap *in);
+
#endif /* BLOCK_INT_H */
diff --git a/qapi-schema.json b/qapi-schema.json
index 9c92482..7357289 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1467,6 +1467,8 @@
# abort since 1.6
# blockdev-snapshot-internal-sync since 1.7
# blockdev-backup since 2.3
+# block-dirty-bitmap-add since 2.4
+# block-dirty-bitmap-clear since 2.4
##
{ 'union': 'TransactionAction',
'data': {
@@ -1474,7 +1476,9 @@
'drive-backup': 'DriveBackup',
'blockdev-backup': 'BlockdevBackup',
'abort': 'Abort',
- 'blockdev-snapshot-internal-sync': 'BlockdevSnapshotInternal'
+ 'blockdev-snapshot-internal-sync': 'BlockdevSnapshotInternal',
+ 'block-dirty-bitmap-add': 'BlockDirtyBitmapAdd',
+ 'block-dirty-bitmap-clear': 'BlockDirtyBitmap'
} }
##
--
2.1.0
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [Qemu-devel] [PATCH v4 02/11] iotests: add transactional incremental backup test
2015-05-11 23:04 [Qemu-devel] [PATCH v4 00/11] block: incremental backup transactions John Snow
2015-05-11 23:04 ` [Qemu-devel] [PATCH v4 01/11] qapi: Add transaction support to block-dirty-bitmap operations John Snow
@ 2015-05-11 23:04 ` John Snow
2015-05-11 23:04 ` [Qemu-devel] [PATCH v4 03/11] block: rename BlkTransactionState and BdrvActionOps John Snow
` (8 subsequent siblings)
10 siblings, 0 replies; 29+ messages in thread
From: John Snow @ 2015-05-11 23:04 UTC (permalink / raw)
To: qemu-block
Cc: kwolf, famz, John Snow, qemu-devel, mreitz, vsementsov, stefanha
Test simple usage cases for using transactions to create
and synchronize incremental backups.
Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
---
tests/qemu-iotests/124 | 54 ++++++++++++++++++++++++++++++++++++++++++++++
tests/qemu-iotests/124.out | 4 ++--
2 files changed, 56 insertions(+), 2 deletions(-)
diff --git a/tests/qemu-iotests/124 b/tests/qemu-iotests/124
index 3ee78cd..2d50594 100644
--- a/tests/qemu-iotests/124
+++ b/tests/qemu-iotests/124
@@ -36,6 +36,23 @@ def try_remove(img):
pass
+def transaction_action(action, **kwargs):
+ return {
+ 'type': action,
+ 'data': kwargs
+ }
+
+
+def transaction_bitmap_clear(node, name, **kwargs):
+ return transaction_action('block-dirty-bitmap-clear',
+ node=node, name=name, **kwargs)
+
+
+def transaction_drive_backup(device, target, **kwargs):
+ return transaction_action('drive-backup', device=device, target=target,
+ **kwargs)
+
+
class Bitmap:
def __init__(self, name, drive):
self.name = name
@@ -264,6 +281,43 @@ class TestIncrementalBackup(iotests.QMPTestCase):
return self.do_incremental_simple(granularity=131072)
+ def test_incremental_transaction(self):
+ '''Test: Verify backups made from transactionally created bitmaps.
+
+ Create a bitmap "before" VM execution begins, then create a second
+ bitmap AFTER writes have already occurred. Use transactions to create
+ a full backup and synchronize both bitmaps to this backup.
+ Create an incremental backup through both bitmaps and verify that
+ both backups match the current drive0 image.
+ '''
+
+ drive0 = self.drives[0]
+ bitmap0 = self.add_bitmap('bitmap0', drive0)
+ self.hmp_io_writes(drive0['id'], (('0xab', 0, 512),
+ ('0xfe', '16M', '256k'),
+ ('0x64', '32736k', '64k')))
+ bitmap1 = self.add_bitmap('bitmap1', drive0)
+
+ result = self.vm.qmp('transaction', actions=[
+ transaction_bitmap_clear(bitmap0.drive['id'], bitmap0.name),
+ transaction_bitmap_clear(bitmap1.drive['id'], bitmap1.name),
+ transaction_drive_backup(drive0['id'], drive0['backup'],
+ sync='full', format=drive0['fmt'])
+ ])
+ self.assert_qmp(result, 'return', {})
+ self.wait_until_completed(drive0['id'])
+ self.files.append(drive0['backup'])
+
+ self.hmp_io_writes(drive0['id'], (('0x9a', 0, 512),
+ ('0x55', '8M', '352k'),
+ ('0x78', '15872k', '1M')))
+ # Both bitmaps should be correctly in sync.
+ self.create_incremental(bitmap0)
+ self.create_incremental(bitmap1)
+ self.vm.shutdown()
+ self.check_backups()
+
+
def test_incremental_failure(self):
'''Test: Verify backups made after a failure are correct.
diff --git a/tests/qemu-iotests/124.out b/tests/qemu-iotests/124.out
index 2f7d390..594c16f 100644
--- a/tests/qemu-iotests/124.out
+++ b/tests/qemu-iotests/124.out
@@ -1,5 +1,5 @@
-.......
+........
----------------------------------------------------------------------
-Ran 7 tests
+Ran 8 tests
OK
--
2.1.0
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [Qemu-devel] [PATCH v4 03/11] block: rename BlkTransactionState and BdrvActionOps
2015-05-11 23:04 [Qemu-devel] [PATCH v4 00/11] block: incremental backup transactions John Snow
2015-05-11 23:04 ` [Qemu-devel] [PATCH v4 01/11] qapi: Add transaction support to block-dirty-bitmap operations John Snow
2015-05-11 23:04 ` [Qemu-devel] [PATCH v4 02/11] iotests: add transactional incremental backup test John Snow
@ 2015-05-11 23:04 ` John Snow
2015-05-18 12:24 ` [Qemu-devel] [Qemu-block] " Stefan Hajnoczi
2015-05-11 23:04 ` [Qemu-devel] [PATCH v4 04/11] block: re-add BlkTransactionState John Snow
` (7 subsequent siblings)
10 siblings, 1 reply; 29+ messages in thread
From: John Snow @ 2015-05-11 23:04 UTC (permalink / raw)
To: qemu-block
Cc: kwolf, famz, John Snow, qemu-devel, mreitz, vsementsov, stefanha
These structures are misnomers, somewhat.
(1) BlockTransactionState is not state for a transaction,
but is rather state for a single transaction action.
Rename it "BlkActionState" to be more accurate.
(2) The BdrvActionOps describes operations for the BlkActionState,
above. This name might imply a 'BdrvAction' or a 'BdrvActionState',
which there isn't.
Rename this to 'BlkActionOps' to match 'BlkActionState'.
Lastly, update the surrounding in-line documentation and comments
to reflect the current nature of how Transactions operate.
This patch changes only comments and names, and should not affect
behavior in any way.
Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
blockdev.c | 116 ++++++++++++++++++++++++++++++++++---------------------------
1 file changed, 65 insertions(+), 51 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index a62cc4b..6df575d 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1228,43 +1228,57 @@ static BdrvDirtyBitmap *block_dirty_bitmap_lookup(const char *node,
/* New and old BlockDriverState structs for atomic group operations */
-typedef struct BlkTransactionState BlkTransactionState;
+typedef struct BlkActionState BlkActionState;
-/* Only prepare() may fail. In a single transaction, only one of commit() or
- abort() will be called, clean() will always be called if it present. */
-typedef struct BdrvActionOps {
- /* Size of state struct, in bytes. */
+/**
+ * BlkActionOps:
+ * Table of operations that define an Action.
+ *
+ * @instance_size: Size of state struct, in bytes.
+ * @prepare: Prepare the work, must NOT be NULL.
+ * @commit: Commit the changes, can be NULL.
+ * @abort: Abort the changes on fail, can be NULL.
+ * @clean: Clean up resources after all transaction actions have called
+ * commit() or abort(). Can be NULL.
+ *
+ * Only prepare() may fail. In a single transaction, only one of commit() or
+ * abort() will be called. clean() will always be called if it is present.
+ */
+typedef struct BlkActionOps {
size_t instance_size;
- /* Prepare the work, must NOT be NULL. */
- void (*prepare)(BlkTransactionState *common, Error **errp);
- /* Commit the changes, can be NULL. */
- void (*commit)(BlkTransactionState *common);
- /* Abort the changes on fail, can be NULL. */
- void (*abort)(BlkTransactionState *common);
- /* Clean up resource in the end, can be NULL. */
- void (*clean)(BlkTransactionState *common);
-} BdrvActionOps;
+ void (*prepare)(BlkActionState *common, Error **errp);
+ void (*commit)(BlkActionState *common);
+ void (*abort)(BlkActionState *common);
+ void (*clean)(BlkActionState *common);
+} BlkActionOps;
-/*
- * This structure must be arranged as first member in child type, assuming
- * that compiler will also arrange it to the same address with parent instance.
- * Later it will be used in free().
+/**
+ * BlkActionState:
+ * Describes one Action's state within a Transaction.
+ *
+ * @action: QAPI-defined enum identifying which Action to perform.
+ * @ops: Table of ActionOps this Action can perform.
+ * @entry: List membership for all Actions in this Transaction.
+ *
+ * This structure must be arranged as first member in a subclassed type,
+ * assuming that the compiler will also arrange it to the same offsets as the
+ * base class.
*/
-struct BlkTransactionState {
+struct BlkActionState {
TransactionAction *action;
- const BdrvActionOps *ops;
- QSIMPLEQ_ENTRY(BlkTransactionState) entry;
+ const BlkActionOps *ops;
+ QSIMPLEQ_ENTRY(BlkActionState) entry;
};
/* internal snapshot private data */
typedef struct InternalSnapshotState {
- BlkTransactionState common;
+ BlkActionState common;
BlockDriverState *bs;
AioContext *aio_context;
QEMUSnapshotInfo sn;
} InternalSnapshotState;
-static void internal_snapshot_prepare(BlkTransactionState *common,
+static void internal_snapshot_prepare(BlkActionState *common,
Error **errp)
{
Error *local_err = NULL;
@@ -1359,7 +1373,7 @@ static void internal_snapshot_prepare(BlkTransactionState *common,
state->bs = bs;
}
-static void internal_snapshot_abort(BlkTransactionState *common)
+static void internal_snapshot_abort(BlkActionState *common)
{
InternalSnapshotState *state =
DO_UPCAST(InternalSnapshotState, common, common);
@@ -1382,7 +1396,7 @@ static void internal_snapshot_abort(BlkTransactionState *common)
}
}
-static void internal_snapshot_clean(BlkTransactionState *common)
+static void internal_snapshot_clean(BlkActionState *common)
{
InternalSnapshotState *state = DO_UPCAST(InternalSnapshotState,
common, common);
@@ -1394,13 +1408,13 @@ static void internal_snapshot_clean(BlkTransactionState *common)
/* external snapshot private data */
typedef struct ExternalSnapshotState {
- BlkTransactionState common;
+ BlkActionState common;
BlockDriverState *old_bs;
BlockDriverState *new_bs;
AioContext *aio_context;
} ExternalSnapshotState;
-static void external_snapshot_prepare(BlkTransactionState *common,
+static void external_snapshot_prepare(BlkActionState *common,
Error **errp)
{
BlockDriver *drv;
@@ -1521,7 +1535,7 @@ static void external_snapshot_prepare(BlkTransactionState *common,
}
}
-static void external_snapshot_commit(BlkTransactionState *common)
+static void external_snapshot_commit(BlkActionState *common)
{
ExternalSnapshotState *state =
DO_UPCAST(ExternalSnapshotState, common, common);
@@ -1539,7 +1553,7 @@ static void external_snapshot_commit(BlkTransactionState *common)
aio_context_release(state->aio_context);
}
-static void external_snapshot_abort(BlkTransactionState *common)
+static void external_snapshot_abort(BlkActionState *common)
{
ExternalSnapshotState *state =
DO_UPCAST(ExternalSnapshotState, common, common);
@@ -1552,13 +1566,13 @@ static void external_snapshot_abort(BlkTransactionState *common)
}
typedef struct DriveBackupState {
- BlkTransactionState common;
+ BlkActionState common;
BlockDriverState *bs;
AioContext *aio_context;
BlockJob *job;
} DriveBackupState;
-static void drive_backup_prepare(BlkTransactionState *common, Error **errp)
+static void drive_backup_prepare(BlkActionState *common, Error **errp)
{
DriveBackupState *state = DO_UPCAST(DriveBackupState, common, common);
BlockDriverState *bs;
@@ -1598,7 +1612,7 @@ static void drive_backup_prepare(BlkTransactionState *common, Error **errp)
state->job = state->bs->job;
}
-static void drive_backup_abort(BlkTransactionState *common)
+static void drive_backup_abort(BlkActionState *common)
{
DriveBackupState *state = DO_UPCAST(DriveBackupState, common, common);
BlockDriverState *bs = state->bs;
@@ -1609,7 +1623,7 @@ static void drive_backup_abort(BlkTransactionState *common)
}
}
-static void drive_backup_clean(BlkTransactionState *common)
+static void drive_backup_clean(BlkActionState *common)
{
DriveBackupState *state = DO_UPCAST(DriveBackupState, common, common);
@@ -1619,13 +1633,13 @@ static void drive_backup_clean(BlkTransactionState *common)
}
typedef struct BlockdevBackupState {
- BlkTransactionState common;
+ BlkActionState common;
BlockDriverState *bs;
BlockJob *job;
AioContext *aio_context;
} BlockdevBackupState;
-static void blockdev_backup_prepare(BlkTransactionState *common, Error **errp)
+static void blockdev_backup_prepare(BlkActionState *common, Error **errp)
{
BlockdevBackupState *state = DO_UPCAST(BlockdevBackupState, common, common);
BlockdevBackup *backup;
@@ -1674,7 +1688,7 @@ static void blockdev_backup_prepare(BlkTransactionState *common, Error **errp)
state->job = state->bs->job;
}
-static void blockdev_backup_abort(BlkTransactionState *common)
+static void blockdev_backup_abort(BlkActionState *common)
{
BlockdevBackupState *state = DO_UPCAST(BlockdevBackupState, common, common);
BlockDriverState *bs = state->bs;
@@ -1685,7 +1699,7 @@ static void blockdev_backup_abort(BlkTransactionState *common)
}
}
-static void blockdev_backup_clean(BlkTransactionState *common)
+static void blockdev_backup_clean(BlkActionState *common)
{
BlockdevBackupState *state = DO_UPCAST(BlockdevBackupState, common, common);
@@ -1695,7 +1709,7 @@ static void blockdev_backup_clean(BlkTransactionState *common)
}
typedef struct BlockDirtyBitmapState {
- BlkTransactionState common;
+ BlkActionState common;
BdrvDirtyBitmap *bitmap;
BlockDriverState *bs;
AioContext *aio_context;
@@ -1703,7 +1717,7 @@ typedef struct BlockDirtyBitmapState {
bool prepared;
} BlockDirtyBitmapState;
-static void block_dirty_bitmap_add_prepare(BlkTransactionState *common,
+static void block_dirty_bitmap_add_prepare(BlkActionState *common,
Error **errp)
{
Error *local_err = NULL;
@@ -1724,7 +1738,7 @@ static void block_dirty_bitmap_add_prepare(BlkTransactionState *common,
}
}
-static void block_dirty_bitmap_add_abort(BlkTransactionState *common)
+static void block_dirty_bitmap_add_abort(BlkActionState *common)
{
BlockDirtyBitmapAdd *action;
BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
@@ -1739,7 +1753,7 @@ static void block_dirty_bitmap_add_abort(BlkTransactionState *common)
}
}
-static void block_dirty_bitmap_clear_prepare(BlkTransactionState *common,
+static void block_dirty_bitmap_clear_prepare(BlkActionState *common,
Error **errp)
{
BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
@@ -1768,7 +1782,7 @@ static void block_dirty_bitmap_clear_prepare(BlkTransactionState *common,
/* AioContext is released in .clean() */
}
-static void block_dirty_bitmap_clear_abort(BlkTransactionState *common)
+static void block_dirty_bitmap_clear_abort(BlkActionState *common)
{
BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
common, common);
@@ -1776,7 +1790,7 @@ static void block_dirty_bitmap_clear_abort(BlkTransactionState *common)
bdrv_undo_clear_dirty_bitmap(state->bitmap, state->backup);
}
-static void block_dirty_bitmap_clear_commit(BlkTransactionState *common)
+static void block_dirty_bitmap_clear_commit(BlkActionState *common)
{
BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
common, common);
@@ -1784,7 +1798,7 @@ static void block_dirty_bitmap_clear_commit(BlkTransactionState *common)
hbitmap_free(state->backup);
}
-static void block_dirty_bitmap_clear_clean(BlkTransactionState *common)
+static void block_dirty_bitmap_clear_clean(BlkActionState *common)
{
BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
common, common);
@@ -1794,17 +1808,17 @@ static void block_dirty_bitmap_clear_clean(BlkTransactionState *common)
}
}
-static void abort_prepare(BlkTransactionState *common, Error **errp)
+static void abort_prepare(BlkActionState *common, Error **errp)
{
error_setg(errp, "Transaction aborted using Abort action");
}
-static void abort_commit(BlkTransactionState *common)
+static void abort_commit(BlkActionState *common)
{
g_assert_not_reached(); /* this action never succeeds */
}
-static const BdrvActionOps actions[] = {
+static const BlkActionOps actions[] = {
[TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_SYNC] = {
.instance_size = sizeof(ExternalSnapshotState),
.prepare = external_snapshot_prepare,
@@ -1824,7 +1838,7 @@ static const BdrvActionOps actions[] = {
.clean = blockdev_backup_clean,
},
[TRANSACTION_ACTION_KIND_ABORT] = {
- .instance_size = sizeof(BlkTransactionState),
+ .instance_size = sizeof(BlkActionState),
.prepare = abort_prepare,
.commit = abort_commit,
},
@@ -1855,10 +1869,10 @@ static const BdrvActionOps actions[] = {
void qmp_transaction(TransactionActionList *dev_list, Error **errp)
{
TransactionActionList *dev_entry = dev_list;
- BlkTransactionState *state, *next;
+ BlkActionState *state, *next;
Error *local_err = NULL;
- QSIMPLEQ_HEAD(snap_bdrv_states, BlkTransactionState) snap_bdrv_states;
+ QSIMPLEQ_HEAD(snap_bdrv_states, BlkActionState) snap_bdrv_states;
QSIMPLEQ_INIT(&snap_bdrv_states);
/* drain all i/o before any operations */
@@ -1867,7 +1881,7 @@ void qmp_transaction(TransactionActionList *dev_list, Error **errp)
/* We don't do anything in this loop that commits us to the operations */
while (NULL != dev_entry) {
TransactionAction *dev_info = NULL;
- const BdrvActionOps *ops;
+ const BlkActionOps *ops;
dev_info = dev_entry->value;
dev_entry = dev_entry->next;
--
2.1.0
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [Qemu-devel] [PATCH v4 04/11] block: re-add BlkTransactionState
2015-05-11 23:04 [Qemu-devel] [PATCH v4 00/11] block: incremental backup transactions John Snow
` (2 preceding siblings ...)
2015-05-11 23:04 ` [Qemu-devel] [PATCH v4 03/11] block: rename BlkTransactionState and BdrvActionOps John Snow
@ 2015-05-11 23:04 ` John Snow
2015-05-18 12:33 ` [Qemu-devel] [Qemu-block] " Stefan Hajnoczi
2015-05-11 23:04 ` [Qemu-devel] [PATCH v4 05/11] block: add transactional callbacks feature John Snow
` (6 subsequent siblings)
10 siblings, 1 reply; 29+ messages in thread
From: John Snow @ 2015-05-11 23:04 UTC (permalink / raw)
To: qemu-block
Cc: kwolf, famz, John Snow, qemu-devel, mreitz, vsementsov, stefanha
Now that the structure formerly known as BlkTransactionState has been
renamed to something sensible (BlkActionState), re-introduce an actual
BlkTransactionState that actually manages state for the entire Transaction.
In the process, convert the old QSIMPLEQ list of actions into a QTAILQ,
to let us more efficiently delete items in arbitrary order, which will
be more important in the future when some actions will expire at the end
of the transaction, but others may persist until all callbacks triggered
by the transaction are recollected.
Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
blockdev.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-------
1 file changed, 59 insertions(+), 7 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index 6df575d..068eccb 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1253,6 +1253,19 @@ typedef struct BlkActionOps {
} BlkActionOps;
/**
+ * BlkTransactionState:
+ * Object to track the job completion info for jobs launched
+ * by a transaction group.
+ *
+ * @jobs: A reference count that tracks how many jobs still need to complete.
+ * @actions: A list of all Actions in the Transaction.
+ */
+typedef struct BlkTransactionState {
+ int jobs;
+ QTAILQ_HEAD(actions, BlkActionState) actions;
+} BlkTransactionState;
+
+/**
* BlkActionState:
* Describes one Action's state within a Transaction.
*
@@ -1267,9 +1280,45 @@ typedef struct BlkActionOps {
struct BlkActionState {
TransactionAction *action;
const BlkActionOps *ops;
- QSIMPLEQ_ENTRY(BlkActionState) entry;
+ QTAILQ_ENTRY(BlkActionState) entry;
};
+static BlkTransactionState *new_blk_transaction_state(void)
+{
+ BlkTransactionState *bts = g_new0(BlkTransactionState, 1);
+
+ /* The qmp_transaction function itself can be considered a pending job
+ * that should complete before pending action callbacks are executed,
+ * so increment the jobs remaining refcount to indicate this. */
+ bts->jobs = 1;
+ QTAILQ_INIT(&bts->actions);
+ return bts;
+}
+
+static void destroy_blk_transaction_state(BlkTransactionState *bts)
+{
+ BlkActionState *bas, *bas_next;
+
+ /* The list should in normal cases be empty,
+ * but in case someone really just wants to kibosh the whole deal: */
+ QTAILQ_FOREACH_SAFE(bas, &bts->actions, entry, bas_next) {
+ QTAILQ_REMOVE(&bts->actions, bas, entry);
+ g_free(bas);
+ }
+
+ g_free(bts);
+}
+
+static BlkTransactionState *transaction_job_complete(BlkTransactionState *bts)
+{
+ bts->jobs--;
+ if (bts->jobs == 0) {
+ destroy_blk_transaction_state(bts);
+ return NULL;
+ }
+ return bts;
+}
+
/* internal snapshot private data */
typedef struct InternalSnapshotState {
BlkActionState common;
@@ -1870,10 +1919,10 @@ void qmp_transaction(TransactionActionList *dev_list, Error **errp)
{
TransactionActionList *dev_entry = dev_list;
BlkActionState *state, *next;
+ BlkTransactionState *bts;
Error *local_err = NULL;
- QSIMPLEQ_HEAD(snap_bdrv_states, BlkActionState) snap_bdrv_states;
- QSIMPLEQ_INIT(&snap_bdrv_states);
+ bts = new_blk_transaction_state();
/* drain all i/o before any operations */
bdrv_drain_all();
@@ -1894,7 +1943,7 @@ void qmp_transaction(TransactionActionList *dev_list, Error **errp)
state = g_malloc0(ops->instance_size);
state->ops = ops;
state->action = dev_info;
- QSIMPLEQ_INSERT_TAIL(&snap_bdrv_states, state, entry);
+ QTAILQ_INSERT_TAIL(&bts->actions, state, entry);
state->ops->prepare(state, &local_err);
if (local_err) {
@@ -1903,7 +1952,7 @@ void qmp_transaction(TransactionActionList *dev_list, Error **errp)
}
}
- QSIMPLEQ_FOREACH(state, &snap_bdrv_states, entry) {
+ QTAILQ_FOREACH(state, &bts->actions, entry) {
if (state->ops->commit) {
state->ops->commit(state);
}
@@ -1914,18 +1963,21 @@ void qmp_transaction(TransactionActionList *dev_list, Error **errp)
delete_and_fail:
/* failure, and it is all-or-none; roll back all operations */
- QSIMPLEQ_FOREACH(state, &snap_bdrv_states, entry) {
+ QTAILQ_FOREACH(state, &bts->actions, entry) {
if (state->ops->abort) {
state->ops->abort(state);
}
}
exit:
- QSIMPLEQ_FOREACH_SAFE(state, &snap_bdrv_states, entry, next) {
+ QTAILQ_FOREACH_SAFE(state, &bts->actions, entry, next) {
if (state->ops->clean) {
state->ops->clean(state);
}
+ QTAILQ_REMOVE(&bts->actions, state, entry);
g_free(state);
}
+
+ transaction_job_complete(bts);
}
--
2.1.0
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [Qemu-devel] [PATCH v4 05/11] block: add transactional callbacks feature
2015-05-11 23:04 [Qemu-devel] [PATCH v4 00/11] block: incremental backup transactions John Snow
` (3 preceding siblings ...)
2015-05-11 23:04 ` [Qemu-devel] [PATCH v4 04/11] block: re-add BlkTransactionState John Snow
@ 2015-05-11 23:04 ` John Snow
2015-05-11 23:04 ` [Qemu-devel] [PATCH v4 06/11] block: add refcount to Job object John Snow
` (5 subsequent siblings)
10 siblings, 0 replies; 29+ messages in thread
From: John Snow @ 2015-05-11 23:04 UTC (permalink / raw)
To: qemu-block
Cc: kwolf, famz, John Snow, qemu-devel, mreitz, vsementsov, stefanha
The goal here is to add a new method to transactions that allows
developers to specify a callback that will get invoked only once
all jobs spawned by a transaction are completed, allowing developers
the chance to perform actions conditionally pending complete success,
partial failure, or complete failure.
In order to register the new callback to be invoked, a user must request
a callback pointer and closure by calling new_action_cb_wrapper, which
creates a wrapper around an opaque pointer and callback that would have
originally been passed to e.g. backup_start().
The function will return a function pointer and a new opaque pointer to
be passed instead. The transaction system will effectively intercept the
original callbacks and perform book-keeping on the transaction after it
has delivered the original enveloped callback.
This means that Transaction Action callback methods will be called after
all callbacks triggered by all Actions in the Transactional group have
been received.
This feature has no knowledge of any jobs spawned by Actions that do not
inform the system via new_action_cb_wrapper().
For an example of how to use the feature, please skip ahead to:
'block: drive_backup transaction callback support' which serves as an example
for how to hook up a post-transaction callback to the Drive Backup action.
Note 1: Defining a callback method alone is not sufficient to have the new
method invoked. You must call new_action_cb_wrapper() AND ensure the
callback it returns is the one used as the callback for the job
launched by the action.
Note 2: You can use this feature for any system that registers completions of
an asynchronous task via a callback of the form
(void *opaque, int ret), not just block job callbacks.
Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
blockdev.c | 183 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 179 insertions(+), 4 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index 068eccb..27db1b4 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1240,6 +1240,8 @@ typedef struct BlkActionState BlkActionState;
* @abort: Abort the changes on fail, can be NULL.
* @clean: Clean up resources after all transaction actions have called
* commit() or abort(). Can be NULL.
+ * @cb: Executed after all jobs launched by actions in the transaction finish,
+ * but only if requested by new_action_cb_wrapper() prior to clean().
*
* Only prepare() may fail. In a single transaction, only one of commit() or
* abort() will be called. clean() will always be called if it is present.
@@ -1250,6 +1252,7 @@ typedef struct BlkActionOps {
void (*commit)(BlkActionState *common);
void (*abort)(BlkActionState *common);
void (*clean)(BlkActionState *common);
+ void (*cb)(BlkActionState *common);
} BlkActionOps;
/**
@@ -1258,19 +1261,46 @@ typedef struct BlkActionOps {
* by a transaction group.
*
* @jobs: A reference count that tracks how many jobs still need to complete.
+ * @status: A cumulative return code for all actions that have reported
+ * a return code via callback in the transaction.
* @actions: A list of all Actions in the Transaction.
+ * However, once the transaction has completed, it will be only a list
+ * of transactions that have registered a post-transaction callback.
*/
typedef struct BlkTransactionState {
int jobs;
+ int status;
QTAILQ_HEAD(actions, BlkActionState) actions;
} BlkTransactionState;
+typedef void (CallbackFn)(void *opaque, int ret);
+
+/**
+ * BlkActionCallbackData:
+ * Necessary state for intercepting and
+ * re-delivering a callback triggered by an Action.
+ *
+ * @opaque: The data to be given to the encapsulated callback when
+ * a job launched by an Action completes.
+ * @ret: The status code that was delivered to the encapsulated callback.
+ * @callback: The encapsulated callback to invoke upon completion of
+ * the Job launched by the Action.
+ */
+typedef struct BlkActionCallbackData {
+ void *opaque;
+ int ret;
+ CallbackFn *callback;
+} BlkActionCallbackData;
+
/**
* BlkActionState:
* Describes one Action's state within a Transaction.
*
* @action: QAPI-defined enum identifying which Action to perform.
* @ops: Table of ActionOps this Action can perform.
+ * @transaction: A pointer back to the Transaction this Action belongs to.
+ * @cb_data: Information on this Action's encapsulated callback, if any.
+ * @refcount: reference count, allowing access to this state beyond clean().
* @entry: List membership for all Actions in this Transaction.
*
* This structure must be arranged as first member in a subclassed type,
@@ -1280,6 +1310,9 @@ typedef struct BlkTransactionState {
struct BlkActionState {
TransactionAction *action;
const BlkActionOps *ops;
+ BlkTransactionState *transaction;
+ BlkActionCallbackData *cb_data;
+ int refcount;
QTAILQ_ENTRY(BlkActionState) entry;
};
@@ -1295,6 +1328,18 @@ static BlkTransactionState *new_blk_transaction_state(void)
return bts;
}
+static BlkActionState *put_blk_action_state(BlkActionState *state)
+{
+ state->refcount--;
+ if (state->refcount == 0) {
+ QTAILQ_REMOVE(&state->transaction->actions, state, entry);
+ g_free(state->cb_data);
+ g_free(state);
+ return NULL;
+ }
+ return state;
+}
+
static void destroy_blk_transaction_state(BlkTransactionState *bts)
{
BlkActionState *bas, *bas_next;
@@ -1302,23 +1347,151 @@ static void destroy_blk_transaction_state(BlkTransactionState *bts)
/* The list should in normal cases be empty,
* but in case someone really just wants to kibosh the whole deal: */
QTAILQ_FOREACH_SAFE(bas, &bts->actions, entry, bas_next) {
- QTAILQ_REMOVE(&bts->actions, bas, entry);
- g_free(bas);
+ put_blk_action_state(bas);
}
g_free(bts);
}
+static void transaction_run_cb_action_ops(BlkTransactionState *bts)
+{
+ BlkActionState *bas, *bas_next;
+
+ QTAILQ_FOREACH_SAFE(bas, &bts->actions, entry, bas_next) {
+ if (bas->ops->cb) {
+ bas->ops->cb(bas);
+ }
+
+ g_free(bas->cb_data);
+ bas->cb_data = NULL;
+ put_blk_action_state(bas);
+ }
+}
+
static BlkTransactionState *transaction_job_complete(BlkTransactionState *bts)
{
bts->jobs--;
if (bts->jobs == 0) {
+ transaction_run_cb_action_ops(bts);
destroy_blk_transaction_state(bts);
return NULL;
}
return bts;
}
+static void transaction_job_cancel(BlkActionState *bas)
+{
+ assert(bas->transaction->jobs > 1);
+ transaction_job_complete(bas->transaction);
+}
+
+/**
+ * Intercept a callback that was issued due to a transactional action.
+ */
+static void transaction_action_callback(void *opaque, int ret)
+{
+ BlkActionState *common = opaque;
+ BlkActionCallbackData *cb_data = common->cb_data;
+ CallbackFn *callback = cb_data->callback;
+
+ /* Prepare data for ops->cb() */
+ cb_data->callback = NULL;
+ cb_data->ret = ret;
+
+ /* Cumulative return code will track the first error discovered */
+ if (!common->transaction->status) {
+ common->transaction->status = ret;
+ }
+
+ /* Deliver the intercepted callback prior to marking it as completed */
+ callback(cb_data->opaque, cb_data->ret);
+ transaction_job_complete(common->transaction);
+}
+
+/**
+ * Create a new transactional callback wrapper.
+ *
+ * Given a callback and a closure, generate a new
+ * callback and closure that will invoke the
+ * given callback with the given closure.
+ *
+ * After all wrappers in the transactional group have
+ * been processed, each action's .cb() method will be
+ * invoked.
+ *
+ * @common The transaction action state to set a callback for.
+ * @opaque A closure intended for the encapsulated callback.
+ * @callback The callback we are encapsulating.
+ * @new_opaque[out]: The closure to be used instead of @opaque.
+ *
+ * @return The callback to be used instead of @callback.
+ */
+__attribute__((__unused__))
+static CallbackFn *new_action_cb_wrapper(BlkActionState *common,
+ void *opaque,
+ CallbackFn *callback,
+ void **new_opaque)
+{
+ BlkActionCallbackData *cb_data = g_new0(BlkActionCallbackData, 1);
+ assert(new_opaque);
+
+ /* Stash the original callback information */
+ cb_data->opaque = opaque;
+ cb_data->callback = callback;
+ common->cb_data = cb_data;
+
+ /* The BAS itself will serve as our new closure */
+ *new_opaque = common;
+ common->refcount++;
+
+ /* Inform the transaction to expect one more return */
+ common->transaction->jobs++;
+
+ /* Lastly, the actual callback function to handle the interception. */
+ return transaction_action_callback;
+}
+
+/**
+ * Undo any actions performed by the above call.
+ */
+__attribute__((__unused__))
+static void cancel_action_cb_wrapper(BlkActionState *common)
+{
+ /* Stage 0: Wrapper was never created: */
+ if (common->cb_data == NULL) {
+ assert(common->refcount == 1);
+ return;
+ }
+
+ /* Stage 1: Callback was created, but the job never launched.
+ * (We assume that the job cannot possibly be running, because
+ * we assume it has been synchronously canceled if it was.)
+ *
+ * We also assume that any cleanup normally handled by cb()
+ * is not needed, because it should have been prepared after
+ * the job launched.
+ */
+ if (common->cb_data && common->cb_data->callback) {
+ transaction_job_cancel(common);
+ goto cleanup;
+ }
+
+ /* Stage 2: Job already completed, or was canceled.
+ *
+ * Force an error in the callback data and just invoke the completion
+ * handler to perform appropriate cleanup for us.
+ */
+ if (common->cb_data && !common->cb_data->callback) {
+ common->cb_data->ret = -ECANCELED;
+ common->ops->cb(common);
+ }
+
+ cleanup:
+ g_free(common->cb_data);
+ common->cb_data = NULL;
+ put_blk_action_state(common);
+}
+
/* internal snapshot private data */
typedef struct InternalSnapshotState {
BlkActionState common;
@@ -1941,8 +2114,10 @@ void qmp_transaction(TransactionActionList *dev_list, Error **errp)
assert(ops->instance_size > 0);
state = g_malloc0(ops->instance_size);
+ state->refcount = 1;
state->ops = ops;
state->action = dev_info;
+ state->transaction = bts;
QTAILQ_INSERT_TAIL(&bts->actions, state, entry);
state->ops->prepare(state, &local_err);
@@ -1973,10 +2148,10 @@ exit:
if (state->ops->clean) {
state->ops->clean(state);
}
- QTAILQ_REMOVE(&bts->actions, state, entry);
- g_free(state);
+ put_blk_action_state(state);
}
+ /* The implicit 'job' of the transaction itself is complete: */
transaction_job_complete(bts);
}
--
2.1.0
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [Qemu-devel] [PATCH v4 06/11] block: add refcount to Job object
2015-05-11 23:04 [Qemu-devel] [PATCH v4 00/11] block: incremental backup transactions John Snow
` (4 preceding siblings ...)
2015-05-11 23:04 ` [Qemu-devel] [PATCH v4 05/11] block: add transactional callbacks feature John Snow
@ 2015-05-11 23:04 ` John Snow
2015-05-18 15:45 ` Stefan Hajnoczi
2015-05-11 23:04 ` [Qemu-devel] [PATCH v4 07/11] block: add delayed bitmap successor cleanup John Snow
` (4 subsequent siblings)
10 siblings, 1 reply; 29+ messages in thread
From: John Snow @ 2015-05-11 23:04 UTC (permalink / raw)
To: qemu-block
Cc: kwolf, famz, John Snow, qemu-devel, mreitz, vsementsov, stefanha
If we want to get at the job after the life of the job,
we'll need a refcount for this object.
This may occur for example if we wish to inspect the actions
taken by a particular job after a transactional group of jobs
runs, and further actions are required.
Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
blockjob.c | 18 ++++++++++++++++--
include/block/blockjob.h | 21 +++++++++++++++++++++
2 files changed, 37 insertions(+), 2 deletions(-)
diff --git a/blockjob.c b/blockjob.c
index 2755465..9b3456f 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -35,6 +35,19 @@
#include "qemu/timer.h"
#include "qapi-event.h"
+void block_job_incref(BlockJob *job)
+{
+ job->refcount++;
+}
+
+void block_job_decref(BlockJob *job)
+{
+ job->refcount--;
+ if (job->refcount == 0) {
+ g_free(job);
+ }
+}
+
void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs,
int64_t speed, BlockCompletionFunc *cb,
void *opaque, Error **errp)
@@ -57,6 +70,7 @@ void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs,
job->cb = cb;
job->opaque = opaque;
job->busy = true;
+ job->refcount = 1;
bs->job = job;
/* Only set speed when necessary to avoid NotSupported error */
@@ -68,7 +82,7 @@ void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs,
bs->job = NULL;
bdrv_op_unblock_all(bs, job->blocker);
error_free(job->blocker);
- g_free(job);
+ block_job_decref(job);
error_propagate(errp, local_err);
return NULL;
}
@@ -85,7 +99,7 @@ void block_job_completed(BlockJob *job, int ret)
bs->job = NULL;
bdrv_op_unblock_all(bs, job->blocker);
error_free(job->blocker);
- g_free(job);
+ block_job_decref(job);
}
void block_job_set_speed(BlockJob *job, int64_t speed, Error **errp)
diff --git a/include/block/blockjob.h b/include/block/blockjob.h
index 57d8ef1..86d770a 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;
+
+ /** A reference count, allowing for post-job actions in e.g. transactions */
+ int refcount;
};
/**
@@ -147,6 +150,24 @@ void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs,
void *opaque, Error **errp);
/**
+ * block_job_incref:
+ * @job: The job to pick up a handle to
+ *
+ * Increment the refcount on @job, to be able to use it asynchronously
+ * from the job it is being used for. Put down the reference when done
+ * with @block_job_unref.
+ */
+void block_job_incref(BlockJob *job);
+
+/**
+ * block_job_decref:
+ * @job: The job to unreference and delete.
+ *
+ * Decrement the refcount on @job and delete it if there are no more references.
+ */
+void block_job_decref(BlockJob *job);
+
+/**
* block_job_sleep_ns:
* @job: The job that calls the function.
* @clock: The clock to sleep on.
--
2.1.0
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [Qemu-devel] [PATCH v4 07/11] block: add delayed bitmap successor cleanup
2015-05-11 23:04 [Qemu-devel] [PATCH v4 00/11] block: incremental backup transactions John Snow
` (5 preceding siblings ...)
2015-05-11 23:04 ` [Qemu-devel] [PATCH v4 06/11] block: add refcount to Job object John Snow
@ 2015-05-11 23:04 ` John Snow
2015-05-11 23:04 ` [Qemu-devel] [PATCH v4 08/11] qmp: Add an implementation wrapper for qmp_drive_backup John Snow
` (3 subsequent siblings)
10 siblings, 0 replies; 29+ messages in thread
From: John Snow @ 2015-05-11 23:04 UTC (permalink / raw)
To: qemu-block
Cc: kwolf, famz, John Snow, qemu-devel, mreitz, vsementsov, stefanha
Allow bitmap successors to carry reference counts.
We can in a later patch use this ability to clean up the dirty bitmap
according to both the individual job's success and the success of all
jobs in the transaction group.
The code for cleaning up a bitmap is also moved from backup_run to
backup_complete.
Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
block.c | 65 ++++++++++++++++++++++++++++++++++++++++++++++-----
block/backup.c | 20 ++++++----------
include/block/block.h | 10 ++++----
3 files changed, 70 insertions(+), 25 deletions(-)
diff --git a/block.c b/block.c
index ca5b1e9..d964564 100644
--- a/block.c
+++ b/block.c
@@ -51,6 +51,12 @@
#include <windows.h>
#endif
+typedef enum BitmapSuccessorAction {
+ SUCCESSOR_ACTION_UNDEFINED = 0,
+ SUCCESSOR_ACTION_ABDICATE,
+ SUCCESSOR_ACTION_RECLAIM
+} BitmapSuccessorAction;
+
/**
* A BdrvDirtyBitmap can be in three possible states:
* (1) successor is NULL and disabled is false: full r/w mode
@@ -65,6 +71,8 @@ struct BdrvDirtyBitmap {
char *name; /* Optional non-empty unique ID */
int64_t size; /* Size of the bitmap (Number of sectors) */
bool disabled; /* Bitmap is read-only */
+ int successor_refcount; /* Number of active handles to the successor */
+ BitmapSuccessorAction act; /* Action to take on successor upon release */
QLIST_ENTRY(BdrvDirtyBitmap) list;
};
@@ -3133,6 +3141,7 @@ int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
/* Install the successor and freeze the parent */
bitmap->successor = child;
+ bitmap->successor_refcount = 1;
return 0;
}
@@ -3140,9 +3149,9 @@ int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
* For a bitmap with a successor, yield our name to the successor,
* delete the old bitmap, and return a handle to the new bitmap.
*/
-BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs,
- BdrvDirtyBitmap *bitmap,
- Error **errp)
+static BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs,
+ BdrvDirtyBitmap *bitmap,
+ Error **errp)
{
char *name;
BdrvDirtyBitmap *successor = bitmap->successor;
@@ -3167,9 +3176,9 @@ BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs,
* we may wish to re-join the parent and child/successor.
* The merged parent will be un-frozen, but not explicitly re-enabled.
*/
-BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
- BdrvDirtyBitmap *parent,
- Error **errp)
+static BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
+ BdrvDirtyBitmap *parent,
+ Error **errp)
{
BdrvDirtyBitmap *successor = parent->successor;
@@ -3188,6 +3197,50 @@ BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
return parent;
}
+static BdrvDirtyBitmap *bdrv_free_bitmap_successor(BlockDriverState *bs,
+ BdrvDirtyBitmap *parent)
+{
+ assert(!parent->successor_refcount);
+
+ switch (parent->act) {
+ case SUCCESSOR_ACTION_RECLAIM:
+ return bdrv_reclaim_dirty_bitmap(bs, parent, NULL);
+ case SUCCESSOR_ACTION_ABDICATE:
+ return bdrv_dirty_bitmap_abdicate(bs, parent, NULL);
+ case SUCCESSOR_ACTION_UNDEFINED:
+ default:
+ g_assert_not_reached();
+ }
+}
+
+BdrvDirtyBitmap *bdrv_frozen_bitmap_decref(BlockDriverState *bs,
+ BdrvDirtyBitmap *parent,
+ int ret)
+{
+ assert(bdrv_dirty_bitmap_frozen(parent));
+ assert(parent->successor);
+
+ if (ret) {
+ parent->act = SUCCESSOR_ACTION_RECLAIM;
+ } else if (parent->act != SUCCESSOR_ACTION_RECLAIM) {
+ parent->act = SUCCESSOR_ACTION_ABDICATE;
+ }
+
+ parent->successor_refcount--;
+ if (parent->successor_refcount == 0) {
+ return bdrv_free_bitmap_successor(bs, parent);
+ }
+ return parent;
+}
+
+void bdrv_dirty_bitmap_incref(BdrvDirtyBitmap *parent)
+{
+ assert(bdrv_dirty_bitmap_frozen(parent));
+ assert(parent->successor);
+
+ parent->successor_refcount++;
+}
+
/**
* Truncates _all_ bitmaps attached to a BDS.
*/
diff --git a/block/backup.c b/block/backup.c
index d3f648d..4ac0be8 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -240,6 +240,12 @@ static void backup_complete(BlockJob *job, void *opaque)
bdrv_unref(s->target);
+ if (s->sync_bitmap) {
+ BdrvDirtyBitmap *bm;
+ bm = bdrv_frozen_bitmap_decref(job->bs, s->sync_bitmap, data->ret);
+ assert(bm);
+ }
+
block_job_completed(job, data->ret);
g_free(data);
}
@@ -428,18 +434,6 @@ static void coroutine_fn backup_run(void *opaque)
qemu_co_rwlock_wrlock(&job->flush_rwlock);
qemu_co_rwlock_unlock(&job->flush_rwlock);
- if (job->sync_bitmap) {
- BdrvDirtyBitmap *bm;
- if (ret < 0) {
- /* Merge the successor back into the parent, delete nothing. */
- bm = bdrv_reclaim_dirty_bitmap(bs, job->sync_bitmap, NULL);
- assert(bm);
- } else {
- /* Everything is fine, delete this bitmap and install the backup. */
- bm = bdrv_dirty_bitmap_abdicate(bs, job->sync_bitmap, NULL);
- assert(bm);
- }
- }
hbitmap_free(job->bitmap);
bdrv_iostatus_disable(target);
@@ -543,6 +537,6 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target,
error:
if (sync_bitmap) {
- bdrv_reclaim_dirty_bitmap(bs, sync_bitmap, NULL);
+ bdrv_frozen_bitmap_decref(bs, sync_bitmap, -ECANCELED);
}
}
diff --git a/include/block/block.h b/include/block/block.h
index 9f0ff3d..7c09903 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -457,12 +457,10 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
BdrvDirtyBitmap *bitmap,
Error **errp);
-BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs,
- BdrvDirtyBitmap *bitmap,
- Error **errp);
-BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
- BdrvDirtyBitmap *bitmap,
- Error **errp);
+BdrvDirtyBitmap *bdrv_frozen_bitmap_decref(BlockDriverState *bs,
+ BdrvDirtyBitmap *parent,
+ int ret);
+void bdrv_dirty_bitmap_incref(BdrvDirtyBitmap *parent);
BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState *bs,
const char *name);
void bdrv_dirty_bitmap_make_anon(BdrvDirtyBitmap *bitmap);
--
2.1.0
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [Qemu-devel] [PATCH v4 08/11] qmp: Add an implementation wrapper for qmp_drive_backup
2015-05-11 23:04 [Qemu-devel] [PATCH v4 00/11] block: incremental backup transactions John Snow
` (6 preceding siblings ...)
2015-05-11 23:04 ` [Qemu-devel] [PATCH v4 07/11] block: add delayed bitmap successor cleanup John Snow
@ 2015-05-11 23:04 ` John Snow
2015-05-18 14:42 ` [Qemu-devel] [Qemu-block] " Stefan Hajnoczi
2015-05-11 23:04 ` [Qemu-devel] [PATCH v4 09/11] block: drive_backup transaction callback support John Snow
` (2 subsequent siblings)
10 siblings, 1 reply; 29+ messages in thread
From: John Snow @ 2015-05-11 23:04 UTC (permalink / raw)
To: qemu-block
Cc: kwolf, famz, John Snow, qemu-devel, mreitz, vsementsov, stefanha
We'd like to be able to specify the callback given to backup_start
manually in the case of transactions, so split apart qmp_drive_backup
into an implementation and a wrapper.
Switch drive_backup_prepare to use the new wrapper, but don't overload
the callback and closure yet.
Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
blockdev.c | 78 +++++++++++++++++++++++++++++++++++++++++++++++---------------
1 file changed, 59 insertions(+), 19 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index 27db1b4..f391e18 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1794,6 +1794,19 @@ typedef struct DriveBackupState {
BlockJob *job;
} DriveBackupState;
+static void do_drive_backup(const char *device, const char *target,
+ bool has_format, const char *format,
+ enum MirrorSyncMode sync,
+ bool has_mode, enum NewImageMode mode,
+ bool has_speed, int64_t speed,
+ bool has_bitmap, const char *bitmap,
+ bool has_on_source_error,
+ BlockdevOnError on_source_error,
+ bool has_on_target_error,
+ BlockdevOnError on_target_error,
+ BlockCompletionFunc *cb, void *opaque,
+ Error **errp);
+
static void drive_backup_prepare(BlkActionState *common, Error **errp)
{
DriveBackupState *state = DO_UPCAST(DriveBackupState, common, common);
@@ -1816,15 +1829,16 @@ static void drive_backup_prepare(BlkActionState *common, Error **errp)
state->aio_context = bdrv_get_aio_context(bs);
aio_context_acquire(state->aio_context);
- qmp_drive_backup(backup->device, backup->target,
- backup->has_format, backup->format,
- backup->sync,
- backup->has_mode, backup->mode,
- backup->has_speed, backup->speed,
- backup->has_bitmap, backup->bitmap,
- backup->has_on_source_error, backup->on_source_error,
- backup->has_on_target_error, backup->on_target_error,
- &local_err);
+ do_drive_backup(backup->device, backup->target,
+ backup->has_format, backup->format,
+ backup->sync,
+ backup->has_mode, backup->mode,
+ backup->has_speed, backup->speed,
+ backup->has_bitmap, backup->bitmap,
+ backup->has_on_source_error, backup->on_source_error,
+ backup->has_on_target_error, backup->on_target_error,
+ NULL, NULL,
+ &local_err);
if (local_err) {
error_propagate(errp, local_err);
return;
@@ -2778,15 +2792,18 @@ out:
aio_context_release(aio_context);
}
-void qmp_drive_backup(const char *device, const char *target,
- bool has_format, const char *format,
- enum MirrorSyncMode sync,
- bool has_mode, enum NewImageMode mode,
- bool has_speed, int64_t speed,
- bool has_bitmap, const char *bitmap,
- bool has_on_source_error, BlockdevOnError on_source_error,
- bool has_on_target_error, BlockdevOnError on_target_error,
- Error **errp)
+static void do_drive_backup(const char *device, const char *target,
+ bool has_format, const char *format,
+ enum MirrorSyncMode sync,
+ bool has_mode, enum NewImageMode mode,
+ bool has_speed, int64_t speed,
+ bool has_bitmap, const char *bitmap,
+ bool has_on_source_error,
+ BlockdevOnError on_source_error,
+ bool has_on_target_error,
+ BlockdevOnError on_target_error,
+ BlockCompletionFunc *cb, void *opaque,
+ Error **errp)
{
BlockBackend *blk;
BlockDriverState *bs;
@@ -2900,9 +2917,16 @@ void qmp_drive_backup(const char *device, const char *target,
}
}
+ /* If we are not supplied with callback override info, use our defaults */
+ if (cb == NULL) {
+ cb = block_job_cb;
+ }
+ if (opaque == NULL) {
+ opaque = bs;
+ }
backup_start(bs, target_bs, speed, sync, bmap,
on_source_error, on_target_error,
- block_job_cb, bs, &local_err);
+ cb, opaque, &local_err);
if (local_err != NULL) {
bdrv_unref(target_bs);
error_propagate(errp, local_err);
@@ -2913,6 +2937,22 @@ out:
aio_context_release(aio_context);
}
+void qmp_drive_backup(const char *device, const char *target,
+ bool has_format, const char *format,
+ enum MirrorSyncMode sync,
+ bool has_mode, enum NewImageMode mode,
+ bool has_speed, int64_t speed,
+ bool has_bitmap, const char *bitmap,
+ bool has_on_source_error, BlockdevOnError on_source_error,
+ bool has_on_target_error, BlockdevOnError on_target_error,
+ Error **errp)
+{
+ do_drive_backup(device, target, has_format, format, sync, has_mode, mode,
+ has_speed, speed, has_bitmap, bitmap, has_on_source_error,
+ on_source_error, has_on_target_error, on_target_error,
+ NULL, NULL, errp);
+}
+
BlockDeviceInfoList *qmp_query_named_block_nodes(Error **errp)
{
return bdrv_named_nodes_list(errp);
--
2.1.0
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [Qemu-devel] [PATCH v4 09/11] block: drive_backup transaction callback support
2015-05-11 23:04 [Qemu-devel] [PATCH v4 00/11] block: incremental backup transactions John Snow
` (7 preceding siblings ...)
2015-05-11 23:04 ` [Qemu-devel] [PATCH v4 08/11] qmp: Add an implementation wrapper for qmp_drive_backup John Snow
@ 2015-05-11 23:04 ` John Snow
2015-05-18 15:35 ` Stefan Hajnoczi
2015-05-18 15:48 ` Stefan Hajnoczi
2015-05-11 23:04 ` [Qemu-devel] [PATCH v4 10/11] iotests: 124 - transactional failure test John Snow
2015-05-11 23:04 ` [Qemu-devel] [PATCH v4 11/11] qmp-commands.hx: Update the supported 'transaction' operations John Snow
10 siblings, 2 replies; 29+ messages in thread
From: John Snow @ 2015-05-11 23:04 UTC (permalink / raw)
To: qemu-block
Cc: kwolf, famz, John Snow, qemu-devel, mreitz, vsementsov, stefanha
This patch actually implements the transactional callback system
for the drive_backup action.
(1) We manually pick up a reference to the bitmap if present to allow
its cleanup to be delayed until after all drive_backup jobs launched
by the transaction have fully completed.
(2) We create a functional closure that envelops the original drive_backup
callback, to be able to intercept the completion status and return code
for the job.
(3) We add the drive_backup_cb method for the drive_backup action, which
unpacks the completion information and invokes the final cleanup.
(4) backup_transaction_complete will perform the final cleanup on the
backup job.
(5) In the case of transaction cancellation, drive_backup_cb is still
responsible for cleaning up the mess we may have already made.
Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
block/backup.c | 9 ++++++++
blockdev.c | 53 ++++++++++++++++++++++++++++++++++++++++++++---
include/block/block_int.h | 8 +++++++
3 files changed, 67 insertions(+), 3 deletions(-)
diff --git a/block/backup.c b/block/backup.c
index 4ac0be8..1634c88 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -233,6 +233,15 @@ typedef struct {
int ret;
} BackupCompleteData;
+void backup_transaction_complete(BlockJob *job, int ret)
+{
+ BackupBlockJob *s = container_of(job, BackupBlockJob, common);
+
+ if (s->sync_bitmap) {
+ bdrv_frozen_bitmap_decref(job->bs, s->sync_bitmap, ret);
+ }
+}
+
static void backup_complete(BlockJob *job, void *opaque)
{
BackupBlockJob *s = container_of(job, BackupBlockJob, common);
diff --git a/blockdev.c b/blockdev.c
index f391e18..c438949 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1426,7 +1426,6 @@ static void transaction_action_callback(void *opaque, int ret)
*
* @return The callback to be used instead of @callback.
*/
-__attribute__((__unused__))
static CallbackFn *new_action_cb_wrapper(BlkActionState *common,
void *opaque,
CallbackFn *callback,
@@ -1454,7 +1453,6 @@ static CallbackFn *new_action_cb_wrapper(BlkActionState *common,
/**
* Undo any actions performed by the above call.
*/
-__attribute__((__unused__))
static void cancel_action_cb_wrapper(BlkActionState *common)
{
/* Stage 0: Wrapper was never created: */
@@ -1806,6 +1804,7 @@ static void do_drive_backup(const char *device, const char *target,
BlockdevOnError on_target_error,
BlockCompletionFunc *cb, void *opaque,
Error **errp);
+static void block_job_cb(void *opaque, int ret);
static void drive_backup_prepare(BlkActionState *common, Error **errp)
{
@@ -1814,6 +1813,9 @@ static void drive_backup_prepare(BlkActionState *common, Error **errp)
BlockBackend *blk;
DriveBackup *backup;
Error *local_err = NULL;
+ CallbackFn *cb;
+ void *opaque;
+ BdrvDirtyBitmap *bmap = NULL;
assert(common->action->kind == TRANSACTION_ACTION_KIND_DRIVE_BACKUP);
backup = common->action->drive_backup;
@@ -1825,6 +1827,19 @@ static void drive_backup_prepare(BlkActionState *common, Error **errp)
}
bs = blk_bs(blk);
+ /* BackupBlockJob is opaque to us, so look up the bitmap ourselves */
+ if (backup->has_bitmap) {
+ bmap = bdrv_find_dirty_bitmap(bs, backup->bitmap);
+ if (!bmap) {
+ error_setg(errp, "Bitmap '%s' could not be found", backup->bitmap);
+ return;
+ }
+ }
+
+ /* Create our transactional callback wrapper,
+ and register that we'd like to call .cb() later. */
+ cb = new_action_cb_wrapper(common, bs, block_job_cb, &opaque);
+
/* AioContext is released in .clean() */
state->aio_context = bdrv_get_aio_context(bs);
aio_context_acquire(state->aio_context);
@@ -1837,7 +1852,7 @@ static void drive_backup_prepare(BlkActionState *common, Error **errp)
backup->has_bitmap, backup->bitmap,
backup->has_on_source_error, backup->on_source_error,
backup->has_on_target_error, backup->on_target_error,
- NULL, NULL,
+ cb, opaque,
&local_err);
if (local_err) {
error_propagate(errp, local_err);
@@ -1846,6 +1861,12 @@ static void drive_backup_prepare(BlkActionState *common, Error **errp)
state->bs = bs;
state->job = state->bs->job;
+ /* Keep the job alive until .cb(), too:
+ * References are only incremented after the job launches successfully. */
+ block_job_incref(state->job);
+ if (bmap) {
+ bdrv_dirty_bitmap_incref(bmap);
+ }
}
static void drive_backup_abort(BlkActionState *common)
@@ -1857,6 +1878,10 @@ static void drive_backup_abort(BlkActionState *common)
if (bs && bs->job && bs->job == state->job) {
block_job_cancel_sync(bs->job);
}
+
+ /* Undo any callback actions we may have done. Putting down references
+ * obtained during prepare() is handled by cb(). */
+ cancel_action_cb_wrapper(common);
}
static void drive_backup_clean(BlkActionState *common)
@@ -1868,6 +1893,27 @@ static void drive_backup_clean(BlkActionState *common)
}
}
+static void drive_backup_cb(BlkActionState *common)
+{
+ BlkActionCallbackData *cb_data = common->cb_data;
+ BlockDriverState *bs = cb_data->opaque;
+ DriveBackupState *state = DO_UPCAST(DriveBackupState, common, common);
+
+ assert(state->bs == bs);
+ if (bs->job) {
+ assert(state->job == bs->job);
+ }
+
+ state->aio_context = bdrv_get_aio_context(bs);
+ aio_context_acquire(state->aio_context);
+
+ /* Note: We also have the individual job's return code in cb_data->ret */
+ backup_transaction_complete(state->job, common->transaction->status);
+ block_job_decref(state->job);
+
+ aio_context_release(state->aio_context);
+}
+
typedef struct BlockdevBackupState {
BlkActionState common;
BlockDriverState *bs;
@@ -2066,6 +2112,7 @@ static const BlkActionOps actions[] = {
.prepare = drive_backup_prepare,
.abort = drive_backup_abort,
.clean = drive_backup_clean,
+ .cb = drive_backup_cb,
},
[TRANSACTION_ACTION_KIND_BLOCKDEV_BACKUP] = {
.instance_size = sizeof(BlockdevBackupState),
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 50355df..e345d62 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -627,6 +627,14 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target,
BlockCompletionFunc *cb, void *opaque,
Error **errp);
+/*
+ * backup_transaction_complete:
+ * @job: The BackupBlockJob that was associated with a transaction.
+ * @ret: The collective return code for the entire transaction.
+ * Expects zero for success, non-zero for an error otherwise.
+ */
+void backup_transaction_complete(BlockJob *job, int ret);
+
void blk_dev_change_media_cb(BlockBackend *blk, bool load);
bool blk_dev_has_removable_media(BlockBackend *blk);
void blk_dev_eject_request(BlockBackend *blk, bool force);
--
2.1.0
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [Qemu-devel] [PATCH v4 10/11] iotests: 124 - transactional failure test
2015-05-11 23:04 [Qemu-devel] [PATCH v4 00/11] block: incremental backup transactions John Snow
` (8 preceding siblings ...)
2015-05-11 23:04 ` [Qemu-devel] [PATCH v4 09/11] block: drive_backup transaction callback support John Snow
@ 2015-05-11 23:04 ` John Snow
2015-05-11 23:04 ` [Qemu-devel] [PATCH v4 11/11] qmp-commands.hx: Update the supported 'transaction' operations John Snow
10 siblings, 0 replies; 29+ messages in thread
From: John Snow @ 2015-05-11 23:04 UTC (permalink / raw)
To: qemu-block
Cc: kwolf, famz, John Snow, qemu-devel, mreitz, vsementsov, stefanha
Use a transaction to request an incremental backup across two drives.
Coerce one of the jobs to fail, and then re-run the transaction.
Verify that no bitmap data was lost due to the partial transaction
failure.
Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
tests/qemu-iotests/124 | 120 ++++++++++++++++++++++++++++++++++++++++++++-
tests/qemu-iotests/124.out | 4 +-
2 files changed, 121 insertions(+), 3 deletions(-)
diff --git a/tests/qemu-iotests/124 b/tests/qemu-iotests/124
index 2d50594..772edd4 100644
--- a/tests/qemu-iotests/124
+++ b/tests/qemu-iotests/124
@@ -139,9 +139,12 @@ class TestIncrementalBackup(iotests.QMPTestCase):
def do_qmp_backup(self, error='Input/output error', **kwargs):
res = self.vm.qmp('drive-backup', **kwargs)
self.assert_qmp(res, 'return', {})
+ return self.wait_qmp_backup(kwargs['device'], error)
+
+ def wait_qmp_backup(self, device, error='Input/output error'):
event = self.vm.event_wait(name="BLOCK_JOB_COMPLETED",
- match={'data': {'device': kwargs['device']}})
+ match={'data': {'device': device}})
self.assertIsNotNone(event)
try:
@@ -375,6 +378,121 @@ class TestIncrementalBackup(iotests.QMPTestCase):
self.check_backups()
+ def test_transaction_failure(self):
+ '''Test: Verify backups made from a transaction that partially fails.
+
+ Add a second drive with its own unique pattern, and add a bitmap to each
+ drive. Use blkdebug to interfere with the backup on just one drive and
+ attempt to create a coherent incremental backup across both drives.
+
+ verify a failure in one but not both, then delete the failed stubs and
+ re-run the same transaction.
+
+ verify that both incrementals are created successfully.
+ '''
+
+ # Create a second drive, with pattern:
+ drive1 = self.add_node('drive1')
+ self.img_create(drive1['file'], drive1['fmt'])
+ io_write_patterns(drive1['file'], (('0x14', 0, 512),
+ ('0x5d', '1M', '32k'),
+ ('0xcd', '32M', '124k')))
+
+ # Create a blkdebug interface to this img as 'drive1'
+ result = self.vm.qmp('blockdev-add', options={
+ 'id': drive1['id'],
+ 'driver': drive1['fmt'],
+ 'file': {
+ 'driver': 'blkdebug',
+ 'image': {
+ 'driver': 'file',
+ 'filename': drive1['file']
+ },
+ 'set-state': [{
+ 'event': 'flush_to_disk',
+ 'state': 1,
+ 'new_state': 2
+ }],
+ 'inject-error': [{
+ 'event': 'read_aio',
+ 'errno': 5,
+ 'state': 2,
+ 'immediately': False,
+ 'once': True
+ }],
+ }
+ })
+ self.assert_qmp(result, 'return', {})
+
+ # Create bitmaps and full backups for both drives
+ drive0 = self.drives[0]
+ dr0bm0 = self.add_bitmap('bitmap0', drive0)
+ dr1bm0 = self.add_bitmap('bitmap0', drive1)
+ self.create_anchor_backup(drive0)
+ self.create_anchor_backup(drive1)
+ self.assert_no_active_block_jobs()
+ self.assertFalse(self.vm.get_qmp_events(wait=False))
+
+ # Emulate some writes
+ self.hmp_io_writes(drive0['id'], (('0xab', 0, 512),
+ ('0xfe', '16M', '256k'),
+ ('0x64', '32736k', '64k')))
+ self.hmp_io_writes(drive1['id'], (('0xba', 0, 512),
+ ('0xef', '16M', '256k'),
+ ('0x46', '32736k', '64k')))
+
+ # Create incremental backup targets
+ target0 = self.prepare_backup(dr0bm0)
+ target1 = self.prepare_backup(dr1bm0)
+
+ # Ask for a new incremental backup per-each drive,
+ # expecting drive1's backup to fail:
+ transaction = [
+ transaction_drive_backup(drive0['id'], target0, sync='dirty-bitmap',
+ format=drive0['fmt'], mode='existing',
+ bitmap=dr0bm0.name),
+ transaction_drive_backup(drive1['id'], target1, sync='dirty-bitmap',
+ format=drive1['fmt'], mode='existing',
+ bitmap=dr1bm0.name),
+ ]
+ result = self.vm.qmp('transaction', actions=transaction)
+ self.assert_qmp(result, 'return', {})
+
+ # Observe that drive0's backup completes, but drive1's does not.
+ # Consume drive1's error and ensure all pending actions are completed.
+ self.assertTrue(self.wait_qmp_backup(drive0['id']))
+ self.assertFalse(self.wait_qmp_backup(drive1['id']))
+ error = self.vm.event_wait('BLOCK_JOB_ERROR')
+ self.assert_qmp(error, 'data', {'device': drive1['id'],
+ 'action': 'report',
+ 'operation': 'read'})
+ self.assertFalse(self.vm.get_qmp_events(wait=False))
+ self.assert_no_active_block_jobs()
+
+ # Delete drive0's successful target and eliminate our record of the
+ # unsuccessful drive1 target. Then re-run the same transaction.
+ dr0bm0.del_target()
+ dr1bm0.del_target()
+ target0 = self.prepare_backup(dr0bm0)
+ target1 = self.prepare_backup(dr1bm0)
+
+ # Re-run the exact same transaction.
+ result = self.vm.qmp('transaction', actions=transaction)
+ self.assert_qmp(result, 'return', {})
+
+ # Both should complete successfully this time.
+ self.assertTrue(self.wait_qmp_backup(drive0['id']))
+ self.assertTrue(self.wait_qmp_backup(drive1['id']))
+ self.make_reference_backup(dr0bm0)
+ self.make_reference_backup(dr1bm0)
+ self.assertFalse(self.vm.get_qmp_events(wait=False))
+ self.assert_no_active_block_jobs()
+
+ # And the images should of course validate.
+ self.vm.shutdown()
+ self.check_backups()
+
+
def test_sync_dirty_bitmap_missing(self):
self.assert_no_active_block_jobs()
self.files.append(self.err_img)
diff --git a/tests/qemu-iotests/124.out b/tests/qemu-iotests/124.out
index 594c16f..dae404e 100644
--- a/tests/qemu-iotests/124.out
+++ b/tests/qemu-iotests/124.out
@@ -1,5 +1,5 @@
-........
+.........
----------------------------------------------------------------------
-Ran 8 tests
+Ran 9 tests
OK
--
2.1.0
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [Qemu-devel] [PATCH v4 11/11] qmp-commands.hx: Update the supported 'transaction' operations
2015-05-11 23:04 [Qemu-devel] [PATCH v4 00/11] block: incremental backup transactions John Snow
` (9 preceding siblings ...)
2015-05-11 23:04 ` [Qemu-devel] [PATCH v4 10/11] iotests: 124 - transactional failure test John Snow
@ 2015-05-11 23:04 ` John Snow
2015-05-18 16:22 ` Max Reitz
10 siblings, 1 reply; 29+ messages in thread
From: John Snow @ 2015-05-11 23:04 UTC (permalink / raw)
To: qemu-block
Cc: kwolf, famz, Kashyap Chamarthy, John Snow, qemu-devel, mreitz,
vsementsov, stefanha
From: Kashyap Chamarthy <kchamart@redhat.com>
Although the canonical source of reference for QMP commands is
qapi-schema.json, for consistency's sake, update qmp-commands.hx to
state the list of supported transactionable operations, namely:
drive-backup
blockdev-backup
blockdev-snapshot-internal-sync
abort
block-dirty-bitmap-add
block-dirty-bitmap-clear
Signed-off-by: Kashyap Chamarthy <kchamart@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
---
qmp-commands.hx | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 7506774..363126a 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1238,11 +1238,14 @@ SQMP
transaction
-----------
-Atomically operate on one or more block devices. The only supported operations
-for now are drive-backup, internal and external snapshotting. A list of
-dictionaries is accepted, that contains the actions to be performed.
-If there is any failure performing any of the operations, all operations
-for the group are abandoned.
+Atomically operate on one or more block devices. Operations that are
+currently supported: drive-backup, blockdev-backup,
+blockdev-snapshot-sync, blockdev-snapshot-internal-sync, abort,
+block-dirty-bitmap-add, block-dirty-bitmap-clear (refer to the
+qemu/qapi-schema.json file for minimum required QEMU versions for these
+operations). A list of dictionaries is accepted, that contains the
+actions to be performed. If there is any failure performing any of the
+operations, all operations for the group are abandoned.
For external snapshots, the dictionary contains the device, the file to use for
the new snapshot, and the format. The default format, if not specified, is
--
2.1.0
^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH v4 03/11] block: rename BlkTransactionState and BdrvActionOps
2015-05-11 23:04 ` [Qemu-devel] [PATCH v4 03/11] block: rename BlkTransactionState and BdrvActionOps John Snow
@ 2015-05-18 12:24 ` Stefan Hajnoczi
0 siblings, 0 replies; 29+ messages in thread
From: Stefan Hajnoczi @ 2015-05-18 12:24 UTC (permalink / raw)
To: John Snow; +Cc: famz, qemu-block, qemu-devel, mreitz, vsementsov, stefanha
[-- Attachment #1: Type: text/plain, Size: 1051 bytes --]
On Mon, May 11, 2015 at 07:04:18PM -0400, John Snow wrote:
> These structures are misnomers, somewhat.
>
> (1) BlockTransactionState is not state for a transaction,
> but is rather state for a single transaction action.
> Rename it "BlkActionState" to be more accurate.
>
> (2) The BdrvActionOps describes operations for the BlkActionState,
> above. This name might imply a 'BdrvAction' or a 'BdrvActionState',
> which there isn't.
> Rename this to 'BlkActionOps' to match 'BlkActionState'.
>
> Lastly, update the surrounding in-line documentation and comments
> to reflect the current nature of how Transactions operate.
>
> This patch changes only comments and names, and should not affect
> behavior in any way.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> ---
> blockdev.c | 116 ++++++++++++++++++++++++++++++++++---------------------------
> 1 file changed, 65 insertions(+), 51 deletions(-)
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH v4 04/11] block: re-add BlkTransactionState
2015-05-11 23:04 ` [Qemu-devel] [PATCH v4 04/11] block: re-add BlkTransactionState John Snow
@ 2015-05-18 12:33 ` Stefan Hajnoczi
0 siblings, 0 replies; 29+ messages in thread
From: Stefan Hajnoczi @ 2015-05-18 12:33 UTC (permalink / raw)
To: John Snow; +Cc: famz, qemu-block, qemu-devel, mreitz, vsementsov, stefanha
[-- Attachment #1: Type: text/plain, Size: 861 bytes --]
On Mon, May 11, 2015 at 07:04:19PM -0400, John Snow wrote:
> + * @jobs: A reference count that tracks how many jobs still need to complete.
> + * @actions: A list of all Actions in the Transaction.
> + */
> +typedef struct BlkTransactionState {
> + int jobs;
...
> +static BlkTransactionState *new_blk_transaction_state(void)
> +{
> + BlkTransactionState *bts = g_new0(BlkTransactionState, 1);
> +
> + /* The qmp_transaction function itself can be considered a pending job
> + * that should complete before pending action callbacks are executed,
> + * so increment the jobs remaining refcount to indicate this. */
> + bts->jobs = 1;
'refcnt' would be a clearer name for this field. Then you can eliminate
the comment that explains why the field isn't really 'jobs'.
It's also the standard name for reference count fields in QEMU.
Stefan
[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH v4 08/11] qmp: Add an implementation wrapper for qmp_drive_backup
2015-05-11 23:04 ` [Qemu-devel] [PATCH v4 08/11] qmp: Add an implementation wrapper for qmp_drive_backup John Snow
@ 2015-05-18 14:42 ` Stefan Hajnoczi
2015-05-18 15:10 ` John Snow
0 siblings, 1 reply; 29+ messages in thread
From: Stefan Hajnoczi @ 2015-05-18 14:42 UTC (permalink / raw)
To: John Snow; +Cc: famz, qemu-block, qemu-devel, mreitz, vsementsov, stefanha
[-- Attachment #1: Type: text/plain, Size: 634 bytes --]
On Mon, May 11, 2015 at 07:04:23PM -0400, John Snow wrote:
> @@ -2900,9 +2917,16 @@ void qmp_drive_backup(const char *device, const char *target,
> }
> }
>
> + /* If we are not supplied with callback override info, use our defaults */
> + if (cb == NULL) {
> + cb = block_job_cb;
> + }
> + if (opaque == NULL) {
> + opaque = bs;
> + }
Why assign opaque separately, it raises the question what happens if a
custom cb is given but the caller really wants opaque to be NULL?
The following might be clearer:
if (cb == NULL) {
cb = block_job_cb;
opaque = bs;
}
[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH v4 08/11] qmp: Add an implementation wrapper for qmp_drive_backup
2015-05-18 14:42 ` [Qemu-devel] [Qemu-block] " Stefan Hajnoczi
@ 2015-05-18 15:10 ` John Snow
0 siblings, 0 replies; 29+ messages in thread
From: John Snow @ 2015-05-18 15:10 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: famz, qemu-block, qemu-devel, mreitz, vsementsov, stefanha
On 05/18/2015 10:42 AM, Stefan Hajnoczi wrote:
> On Mon, May 11, 2015 at 07:04:23PM -0400, John Snow wrote:
>> @@ -2900,9 +2917,16 @@ void qmp_drive_backup(const char *device,
>> const char *target, } }
>>
>> + /* If we are not supplied with callback override info, use
>> our defaults */ + if (cb == NULL) { + cb =
>> block_job_cb; + } + if (opaque == NULL) { + opaque =
>> bs; + }
>
> Why assign opaque separately, it raises the question what happens
> if a custom cb is given but the caller really wants opaque to be
> NULL?
>
> The following might be clearer:
>
> if (cb == NULL) { cb = block_job_cb; opaque = bs; }
>
It just wasn't a consideration when I was writing it, since the
transaction system won't ever want to pass NULL here.
It's easy enough to fix, though.
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH v4 09/11] block: drive_backup transaction callback support
2015-05-11 23:04 ` [Qemu-devel] [PATCH v4 09/11] block: drive_backup transaction callback support John Snow
@ 2015-05-18 15:35 ` Stefan Hajnoczi
2015-05-18 15:53 ` John Snow
2015-05-18 15:48 ` Stefan Hajnoczi
1 sibling, 1 reply; 29+ messages in thread
From: Stefan Hajnoczi @ 2015-05-18 15:35 UTC (permalink / raw)
To: John Snow; +Cc: kwolf, famz, qemu-block, qemu-devel, mreitz, vsementsov
[-- Attachment #1: Type: text/plain, Size: 682 bytes --]
On Mon, May 11, 2015 at 07:04:24PM -0400, John Snow wrote:
> +static void drive_backup_cb(BlkActionState *common)
> +{
> + BlkActionCallbackData *cb_data = common->cb_data;
> + BlockDriverState *bs = cb_data->opaque;
> + DriveBackupState *state = DO_UPCAST(DriveBackupState, common, common);
> +
> + assert(state->bs == bs);
> + if (bs->job) {
> + assert(state->job == bs->job);
> + }
What is the purpose of the if statement?
Why is it not okay for a new job to have started?
> +
> + state->aio_context = bdrv_get_aio_context(bs);
> + aio_context_acquire(state->aio_context);
The bs->job access above should be protected by aio_context_acquire().
[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH v4 06/11] block: add refcount to Job object
2015-05-11 23:04 ` [Qemu-devel] [PATCH v4 06/11] block: add refcount to Job object John Snow
@ 2015-05-18 15:45 ` Stefan Hajnoczi
2015-05-19 22:15 ` John Snow
0 siblings, 1 reply; 29+ messages in thread
From: Stefan Hajnoczi @ 2015-05-18 15:45 UTC (permalink / raw)
To: John Snow; +Cc: kwolf, famz, qemu-block, qemu-devel, mreitz, vsementsov
[-- Attachment #1: Type: text/plain, Size: 1084 bytes --]
On Mon, May 11, 2015 at 07:04:21PM -0400, John Snow wrote:
> If we want to get at the job after the life of the job,
> we'll need a refcount for this object.
>
> This may occur for example if we wish to inspect the actions
> taken by a particular job after a transactional group of jobs
> runs, and further actions are required.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> ---
> blockjob.c | 18 ++++++++++++++++--
> include/block/blockjob.h | 21 +++++++++++++++++++++
> 2 files changed, 37 insertions(+), 2 deletions(-)
I think the only reason for this refcount is so that
backup_transaction_complete() can be called. It accesses
BackupBlockJob->sync_bitmap so the BackupBlockJob instance needs to be
alive.
The bitmap refcount is incremented in blockdev.c, not block/backup.c, so
it is fine to drop backup_transaction_complete() and decrement the
bitmap refcount in blockdev.c instead.
If you do that then there is no need to add a refcount to block job.
This would simplify things.
[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH v4 09/11] block: drive_backup transaction callback support
2015-05-11 23:04 ` [Qemu-devel] [PATCH v4 09/11] block: drive_backup transaction callback support John Snow
2015-05-18 15:35 ` Stefan Hajnoczi
@ 2015-05-18 15:48 ` Stefan Hajnoczi
1 sibling, 0 replies; 29+ messages in thread
From: Stefan Hajnoczi @ 2015-05-18 15:48 UTC (permalink / raw)
To: John Snow; +Cc: kwolf, famz, qemu-block, qemu-devel, mreitz, vsementsov
[-- Attachment #1: Type: text/plain, Size: 435 bytes --]
On Mon, May 11, 2015 at 07:04:24PM -0400, John Snow wrote:
> @@ -1825,6 +1827,19 @@ static void drive_backup_prepare(BlkActionState *common, Error **errp)
> }
> bs = blk_bs(blk);
>
> + /* BackupBlockJob is opaque to us, so look up the bitmap ourselves */
> + if (backup->has_bitmap) {
> + bmap = bdrv_find_dirty_bitmap(bs, backup->bitmap);
This usage of bs needs to be inside aio_context_acquire().
[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH v4 09/11] block: drive_backup transaction callback support
2015-05-18 15:35 ` Stefan Hajnoczi
@ 2015-05-18 15:53 ` John Snow
0 siblings, 0 replies; 29+ messages in thread
From: John Snow @ 2015-05-18 15:53 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: kwolf, famz, qemu-block, qemu-devel, mreitz, vsementsov
On 05/18/2015 11:35 AM, Stefan Hajnoczi wrote:
> On Mon, May 11, 2015 at 07:04:24PM -0400, John Snow wrote:
>> +static void drive_backup_cb(BlkActionState *common) +{ +
>> BlkActionCallbackData *cb_data = common->cb_data; +
>> BlockDriverState *bs = cb_data->opaque; + DriveBackupState
>> *state = DO_UPCAST(DriveBackupState, common, common); + +
>> assert(state->bs == bs); + if (bs->job) { +
>> assert(state->job == bs->job); + }
>
> What is the purpose of the if statement?
>
> Why is it not okay for a new job to have started?
>
Hmm, maybe it's fine -- It was just my thought that it probably
/shouldn't/ occur under normal circumstances.
I think my assumption was that we want to impose an ordering that job
cleanup occurs before another job launches, in general.
I think, though, that you wanted to start allowing non-conflicting
jobs to run concurrently, though, so I'll just eye over this series
again to make sure it's okay for cleanup to happen after another job
starts ...
...Provided the second job does not fiddle with bitmaps, of course. We
should clean those up before another bitmap job starts, definitely.
>> + + state->aio_context = bdrv_get_aio_context(bs); +
>> aio_context_acquire(state->aio_context);
>
> The bs->job access above should be protected by
> aio_context_acquire().
>
Thanks,
--js
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH v4 01/11] qapi: Add transaction support to block-dirty-bitmap operations
2015-05-11 23:04 ` [Qemu-devel] [PATCH v4 01/11] qapi: Add transaction support to block-dirty-bitmap operations John Snow
@ 2015-05-18 16:14 ` Max Reitz
0 siblings, 0 replies; 29+ messages in thread
From: Max Reitz @ 2015-05-18 16:14 UTC (permalink / raw)
To: John Snow, qemu-block; +Cc: kwolf, famz, qemu-devel, vsementsov, stefanha
On 12.05.2015 01:04, John Snow wrote:
> This adds two qmp commands to transactions.
>
> block-dirty-bitmap-add allows you to create a bitmap simultaneously
> alongside a new full backup to accomplish a clean synchronization
> point.
>
> block-dirty-bitmap-clear allows you to reset a bitmap back to as-if
> it were new, which can also be used alongside a full backup to
> accomplish a clean synchronization point.
>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> block.c | 19 +++++++-
> blockdev.c | 114 +++++++++++++++++++++++++++++++++++++++++++++-
> docs/bitmaps.md | 6 +--
> include/block/block.h | 1 -
> include/block/block_int.h | 3 ++
> qapi-schema.json | 6 ++-
> 6 files changed, 139 insertions(+), 10 deletions(-)
Reviewed-by: Max Reitz <mreitz@redhat.com>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH v4 11/11] qmp-commands.hx: Update the supported 'transaction' operations
2015-05-11 23:04 ` [Qemu-devel] [PATCH v4 11/11] qmp-commands.hx: Update the supported 'transaction' operations John Snow
@ 2015-05-18 16:22 ` Max Reitz
2015-05-19 15:30 ` Kashyap Chamarthy
0 siblings, 1 reply; 29+ messages in thread
From: Max Reitz @ 2015-05-18 16:22 UTC (permalink / raw)
To: John Snow, qemu-block
Cc: kwolf, famz, Kashyap Chamarthy, qemu-devel, vsementsov, stefanha
On 12.05.2015 01:04, John Snow 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>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> qmp-commands.hx | 13 ++++++++-----
> 1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 7506774..363126a 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -1238,11 +1238,14 @@ 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
Hm, seven operations... Worth making it a real list?
Max
> (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
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH v4 11/11] qmp-commands.hx: Update the supported 'transaction' operations
2015-05-18 16:22 ` Max Reitz
@ 2015-05-19 15:30 ` Kashyap Chamarthy
2015-05-19 15:37 ` John Snow
0 siblings, 1 reply; 29+ messages in thread
From: Kashyap Chamarthy @ 2015-05-19 15:30 UTC (permalink / raw)
To: Max Reitz
Cc: kwolf, famz, qemu-block, qemu-devel, vsementsov, stefanha, John Snow
On Mon, May 18, 2015 at 06:22:22PM +0200, Max Reitz wrote:
> On 12.05.2015 01:04, John Snow 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>
> >Signed-off-by: John Snow <jsnow@redhat.com>
> >---
> > qmp-commands.hx | 13 ++++++++-----
> > 1 file changed, 8 insertions(+), 5 deletions(-)
> >
> >diff --git a/qmp-commands.hx b/qmp-commands.hx
> >index 7506774..363126a 100644
> >--- a/qmp-commands.hx
> >+++ b/qmp-commands.hx
> >@@ -1238,11 +1238,14 @@ 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
>
> Hm, seven operations... Worth making it a real list?
I don't have a preference. FWIW, I think it still retains the
readability. And, not sure if it's worth the churn.
> >(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
>
--
/kashyap
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH v4 11/11] qmp-commands.hx: Update the supported 'transaction' operations
2015-05-19 15:30 ` Kashyap Chamarthy
@ 2015-05-19 15:37 ` John Snow
2015-05-20 11:12 ` Kashyap Chamarthy
0 siblings, 1 reply; 29+ messages in thread
From: John Snow @ 2015-05-19 15:37 UTC (permalink / raw)
To: Kashyap Chamarthy, Max Reitz
Cc: kwolf, famz, qemu-block, qemu-devel, vsementsov, stefanha
On 05/19/2015 11:30 AM, Kashyap Chamarthy wrote:
> On Mon, May 18, 2015 at 06:22:22PM +0200, Max Reitz wrote:
>> On 12.05.2015 01:04, John Snow 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>
>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>> ---
>>> qmp-commands.hx | 13 ++++++++-----
>>> 1 file changed, 8 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/qmp-commands.hx b/qmp-commands.hx
>>> index 7506774..363126a 100644
>>> --- a/qmp-commands.hx
>>> +++ b/qmp-commands.hx
>>> @@ -1238,11 +1238,14 @@ 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
>>
>> Hm, seven operations... Worth making it a real list?
>
> I don't have a preference. FWIW, I think it still retains the
> readability. And, not sure if it's worth the churn.
>
>>> (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
>>
>
I have to respin the series anyway, so if you want Kashyap, you can
rewrite this and send it to me privately for inclusion, or I'll just
edit it myself.
--js
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH v4 06/11] block: add refcount to Job object
2015-05-18 15:45 ` Stefan Hajnoczi
@ 2015-05-19 22:15 ` John Snow
2015-05-20 9:27 ` Stefan Hajnoczi
0 siblings, 1 reply; 29+ messages in thread
From: John Snow @ 2015-05-19 22:15 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: kwolf, famz, qemu-block, qemu-devel, mreitz, vsementsov
On 05/18/2015 11:45 AM, Stefan Hajnoczi wrote:
> On Mon, May 11, 2015 at 07:04:21PM -0400, John Snow wrote:
>> If we want to get at the job after the life of the job,
>> we'll need a refcount for this object.
>>
>> This may occur for example if we wish to inspect the actions
>> taken by a particular job after a transactional group of jobs
>> runs, and further actions are required.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> Reviewed-by: Max Reitz <mreitz@redhat.com>
>> ---
>> blockjob.c | 18 ++++++++++++++++--
>> include/block/blockjob.h | 21 +++++++++++++++++++++
>> 2 files changed, 37 insertions(+), 2 deletions(-)
>
> I think the only reason for this refcount is so that
> backup_transaction_complete() can be called. It accesses
> BackupBlockJob->sync_bitmap so the BackupBlockJob instance needs to be
> alive.
>
> The bitmap refcount is incremented in blockdev.c, not block/backup.c, so
> it is fine to drop backup_transaction_complete() and decrement the
> bitmap refcount in blockdev.c instead.
>
> If you do that then there is no need to add a refcount to block job.
> This would simplify things.
>
So you are suggesting that I cache the bitmap reference (instead of the
job reference) and then just increment/decrement it directly in
.prepare, .abort and .cb as needed.
You did find the disparity with the reference count for the bitmap, at
least: that is kind of gross. I was coincidentally thinking of punting
it back into a backup_transaction_start to keep more code /out/ of
blockdev...
I'll sit on this one for a few more minutes. I'll try to get rid of the
job refcnt, but I also want to keep the transaction actions as tidy as I
can.
Maybe it's too much abstraction for a simple task, but I wanted to make
sure I wasn't hacking in transaction callbacks in a manner where they'd
only be useful to me, for only this one case. It's conceivable that if
anyone else attempts to use this callback hijacking mechanism that
they'll need to find a way to modify objects within the Job without
pulling everything up to the transaction actions, too.
Ho hum.
--js
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH v4 06/11] block: add refcount to Job object
2015-05-19 22:15 ` John Snow
@ 2015-05-20 9:27 ` Stefan Hajnoczi
2015-05-22 22:38 ` John Snow
0 siblings, 1 reply; 29+ messages in thread
From: Stefan Hajnoczi @ 2015-05-20 9:27 UTC (permalink / raw)
To: John Snow; +Cc: kwolf, famz, qemu-block, qemu-devel, mreitz, vsementsov
[-- Attachment #1: Type: text/plain, Size: 4267 bytes --]
On Tue, May 19, 2015 at 06:15:23PM -0400, John Snow wrote:
> On 05/18/2015 11:45 AM, Stefan Hajnoczi wrote:
> > On Mon, May 11, 2015 at 07:04:21PM -0400, John Snow wrote:
> >> If we want to get at the job after the life of the job,
> >> we'll need a refcount for this object.
> >>
> >> This may occur for example if we wish to inspect the actions
> >> taken by a particular job after a transactional group of jobs
> >> runs, and further actions are required.
> >>
> >> Signed-off-by: John Snow <jsnow@redhat.com>
> >> Reviewed-by: Max Reitz <mreitz@redhat.com>
> >> ---
> >> blockjob.c | 18 ++++++++++++++++--
> >> include/block/blockjob.h | 21 +++++++++++++++++++++
> >> 2 files changed, 37 insertions(+), 2 deletions(-)
> >
> > I think the only reason for this refcount is so that
> > backup_transaction_complete() can be called. It accesses
> > BackupBlockJob->sync_bitmap so the BackupBlockJob instance needs to be
> > alive.
> >
> > The bitmap refcount is incremented in blockdev.c, not block/backup.c, so
> > it is fine to drop backup_transaction_complete() and decrement the
> > bitmap refcount in blockdev.c instead.
> >
> > If you do that then there is no need to add a refcount to block job.
> > This would simplify things.
> >
>
> So you are suggesting that I cache the bitmap reference (instead of the
> job reference) and then just increment/decrement it directly in
> .prepare, .abort and .cb as needed.
>
> You did find the disparity with the reference count for the bitmap, at
> least: that is kind of gross. I was coincidentally thinking of punting
> it back into a backup_transaction_start to keep more code /out/ of
> blockdev...
>
> I'll sit on this one for a few more minutes. I'll try to get rid of the
> job refcnt, but I also want to keep the transaction actions as tidy as I
> can.
>
> Maybe it's too much abstraction for a simple task, but I wanted to make
> sure I wasn't hacking in transaction callbacks in a manner where they'd
> only be useful to me, for only this one case. It's conceivable that if
> anyone else attempts to use this callback hijacking mechanism that
> they'll need to find a way to modify objects within the Job without
> pulling everything up to the transaction actions, too.
Hmm...I think this could be achieved without refcounting in
transactions, actions, or block jobs.
Create a separate MultiCompletion struct with a counter and list of
callbacks:
typedef struct MultiCompletionCB {
BlockCompletionFunc cb;
void *opaque;
QSLIST_ENTRY(MultiCompletionCB) list;
} MultiCompletionCB;
typedef struct {
unsigned refcnt; /* callbacks invoked when this reaches zero */
QSLIST_HEAD(, MultiCompletionCB) callbacks;
int ret;
} MultiCompletion;
void multicomp_add_cb(MultiCompletion *mc, BlockCompletionFunc cb, void *opaque);
/* Note the first argument is MultiCompletion* but this prototype
* allows it to be used as a blockjob cb.
*/
void multicomp_complete(void *opaque, int ret)
{
MultiCompletion *mc = opaque;
if (ret < 0) {
mc->ret = ret;
}
if (--mc->refcnt == 0) {
MultiCompletionCB *cb_data;
while ((cb_data = QSLIST_FIRST(&mc->callbacks)) != NULL) {
cb_data->cb(cb_data->opaque, mc->ret);
QSLIST_REMOVE_HEAD(&mc->callbacks, list);
g_free(cb_data);
}
g_free(mc);
}
}
qmp_transaction() creates a MultiCompletion and passes it to action
.prepare(). If an action wishes to join the MultiCompletion, it calls
multicomp_add_cb() after creating a block job:
static void backup_completion(void *opaque, int ret)
{
HBitmap *bmap = opaque;
if (ret < 0) {
...merge bitmap back in...
} else {
...drop frozen bitmap...
}
}
...
backup_start(..., multicomp_completion, mc);
multicomp_add_cb(mc, backup_completion, bmap);
The multicomp_complete() function gets called by a blockjob cb function.
This approach is more reusable since MultiCompletion is independent of
transactions or block jobs. It also doesn't hold on to transactions,
actions, or block jobs after they have served their purpose.
Is this along the lines you were thinking about?
Stefan
[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH v4 11/11] qmp-commands.hx: Update the supported 'transaction' operations
2015-05-19 15:37 ` John Snow
@ 2015-05-20 11:12 ` Kashyap Chamarthy
2015-05-20 11:27 ` John Snow
0 siblings, 1 reply; 29+ messages in thread
From: Kashyap Chamarthy @ 2015-05-20 11:12 UTC (permalink / raw)
To: John Snow
Cc: kwolf, famz, qemu-block, qemu-devel, Max Reitz, vsementsov, stefanha
On Tue, May 19, 2015 at 11:37:32AM -0400, John Snow wrote:
> > On Mon, May 18, 2015 at 06:22:22PM +0200, Max Reitz wrote:
> >> On 12.05.2015 01:04, John Snow wrote:
[. . .]
> >>> diff --git a/qmp-commands.hx b/qmp-commands.hx
> >>> index 7506774..363126a 100644
> >>> --- a/qmp-commands.hx
> >>> +++ b/qmp-commands.hx
> >>> @@ -1238,11 +1238,14 @@ 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
> >>
> >> Hm, seven operations... Worth making it a real list?
> >
> > I don't have a preference. FWIW, I think it still retains the
> > readability. And, not sure if it's worth the churn.
> >
> >>> (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
> >>
> >
>
> I have to respin the series anyway, so if you want Kashyap, you can
> rewrite this and send it to me privately for inclusion
Done. Sent you (and Max) the revised patch. Max provided his R-b. Max
wondered whether you'd include this in your series on #qemu. I'll assume
you'll do.
Thanks.
--
/kashyap
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH v4 11/11] qmp-commands.hx: Update the supported 'transaction' operations
2015-05-20 11:12 ` Kashyap Chamarthy
@ 2015-05-20 11:27 ` John Snow
0 siblings, 0 replies; 29+ messages in thread
From: John Snow @ 2015-05-20 11:27 UTC (permalink / raw)
To: Kashyap Chamarthy
Cc: kwolf, famz, qemu-block, qemu-devel, Max Reitz, vsementsov, stefanha
On 05/20/2015 07:12 AM, Kashyap Chamarthy wrote:
> On Tue, May 19, 2015 at 11:37:32AM -0400, John Snow wrote:
>>> On Mon, May 18, 2015 at 06:22:22PM +0200, Max Reitz wrote:
>>>> On 12.05.2015 01:04, John Snow wrote:
>
> [. . .]
>
>>>>> diff --git a/qmp-commands.hx b/qmp-commands.hx
>>>>> index 7506774..363126a 100644
>>>>> --- a/qmp-commands.hx
>>>>> +++ b/qmp-commands.hx
>>>>> @@ -1238,11 +1238,14 @@ 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
>>>>
>>>> Hm, seven operations... Worth making it a real list?
>>>
>>> I don't have a preference. FWIW, I think it still retains the
>>> readability. And, not sure if it's worth the churn.
>>>
>>>>> (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
>>>>
>>>
>>
>> I have to respin the series anyway, so if you want Kashyap, you can
>> rewrite this and send it to me privately for inclusion
>
> Done. Sent you (and Max) the revised patch. Max provided his R-b. Max
> wondered whether you'd include this in your series on #qemu. I'll assume
> you'll do.
>
> Thanks.
>
Yes, I'll pull it in here again.
Thanks!
--js
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH v4 06/11] block: add refcount to Job object
2015-05-20 9:27 ` Stefan Hajnoczi
@ 2015-05-22 22:38 ` John Snow
0 siblings, 0 replies; 29+ messages in thread
From: John Snow @ 2015-05-22 22:38 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: kwolf, famz, qemu-block, qemu-devel, mreitz, vsementsov
On 05/20/2015 05:27 AM, Stefan Hajnoczi wrote:
> On Tue, May 19, 2015 at 06:15:23PM -0400, John Snow wrote:
>> On 05/18/2015 11:45 AM, Stefan Hajnoczi wrote:
>>> On Mon, May 11, 2015 at 07:04:21PM -0400, John Snow wrote:
>>>> If we want to get at the job after the life of the job, we'll
>>>> need a refcount for this object.
>>>>
>>>> This may occur for example if we wish to inspect the actions
>>>> taken by a particular job after a transactional group of
>>>> jobs runs, and further actions are required.
>>>>
>>>> Signed-off-by: John Snow <jsnow@redhat.com> Reviewed-by: Max
>>>> Reitz <mreitz@redhat.com> --- blockjob.c | 18
>>>> ++++++++++++++++-- include/block/blockjob.h | 21
>>>> +++++++++++++++++++++ 2 files changed, 37 insertions(+), 2
>>>> deletions(-)
>>>
>>> I think the only reason for this refcount is so that
>>> backup_transaction_complete() can be called. It accesses
>>> BackupBlockJob->sync_bitmap so the BackupBlockJob instance
>>> needs to be alive.
>>>
>>> The bitmap refcount is incremented in blockdev.c, not
>>> block/backup.c, so it is fine to drop
>>> backup_transaction_complete() and decrement the bitmap refcount
>>> in blockdev.c instead.
>>>
>>> If you do that then there is no need to add a refcount to block
>>> job. This would simplify things.
>>>
>>
>> So you are suggesting that I cache the bitmap reference (instead
>> of the job reference) and then just increment/decrement it
>> directly in .prepare, .abort and .cb as needed.
>>
>> You did find the disparity with the reference count for the
>> bitmap, at least: that is kind of gross. I was coincidentally
>> thinking of punting it back into a backup_transaction_start to
>> keep more code /out/ of blockdev...
>>
>> I'll sit on this one for a few more minutes. I'll try to get rid
>> of the job refcnt, but I also want to keep the transaction
>> actions as tidy as I can.
>>
>> Maybe it's too much abstraction for a simple task, but I wanted
>> to make sure I wasn't hacking in transaction callbacks in a
>> manner where they'd only be useful to me, for only this one case.
>> It's conceivable that if anyone else attempts to use this
>> callback hijacking mechanism that they'll need to find a way to
>> modify objects within the Job without pulling everything up to
>> the transaction actions, too.
>
> Hmm...I think this could be achieved without refcounting in
> transactions, actions, or block jobs.
>
> Create a separate MultiCompletion struct with a counter and list
> of callbacks:
>
> typedef struct MultiCompletionCB { BlockCompletionFunc cb; void
> *opaque; QSLIST_ENTRY(MultiCompletionCB) list; }
> MultiCompletionCB;
>
> typedef struct { unsigned refcnt; /* callbacks invoked when this
> reaches zero */ QSLIST_HEAD(, MultiCompletionCB) callbacks; int
> ret; } MultiCompletion;
>
> void multicomp_add_cb(MultiCompletion *mc, BlockCompletionFunc cb,
> void *opaque);
>
> /* Note the first argument is MultiCompletion* but this prototype *
> allows it to be used as a blockjob cb. */ void
> multicomp_complete(void *opaque, int ret) { MultiCompletion *mc =
> opaque;
>
> if (ret < 0) { mc->ret = ret; }
>
> if (--mc->refcnt == 0) { MultiCompletionCB *cb_data; while
> ((cb_data = QSLIST_FIRST(&mc->callbacks)) != NULL) {
> cb_data->cb(cb_data->opaque, mc->ret);
>
> QSLIST_REMOVE_HEAD(&mc->callbacks, list); g_free(cb_data); }
>
> g_free(mc); } }
>
> qmp_transaction() creates a MultiCompletion and passes it to
> action .prepare(). If an action wishes to join the
> MultiCompletion, it calls multicomp_add_cb() after creating a block
> job:
>
> static void backup_completion(void *opaque, int ret) { HBitmap
> *bmap = opaque; if (ret < 0) { ...merge bitmap back in... } else {
> ...drop frozen bitmap... } }
>
> ... backup_start(..., multicomp_completion, mc);
> multicomp_add_cb(mc, backup_completion, bmap);
>
> The multicomp_complete() function gets called by a blockjob cb
> function.
>
> This approach is more reusable since MultiCompletion is independent
> of transactions or block jobs. It also doesn't hold on to
> transactions, actions, or block jobs after they have served their
> purpose.
>
> Is this along the lines you were thinking about?
>
> Stefan
>
Yes, this is roughly what I was aiming for in the design of this
series as it stands now, except I did essentially bake the
MultiCompletion functionality into the BlkTransactionState structure
instead of leaving it separate.
I think it might be worth doing, though I could also just drop a lot
of the refcounts now and rework who picks up the bitmap reference
count and achieve something nearly similar.
I'll play around with the patches I have now and see if I can't tidy
it up to the point where I'm happy with the way it's managing
references, and if not I might scrap it and try the even more
abstracted version.
I'll probably just choose whatever looks cleanest :)
--js
^ permalink raw reply [flat|nested] 29+ messages in thread
end of thread, other threads:[~2015-05-22 22:38 UTC | newest]
Thread overview: 29+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-05-11 23:04 [Qemu-devel] [PATCH v4 00/11] block: incremental backup transactions John Snow
2015-05-11 23:04 ` [Qemu-devel] [PATCH v4 01/11] qapi: Add transaction support to block-dirty-bitmap operations John Snow
2015-05-18 16:14 ` Max Reitz
2015-05-11 23:04 ` [Qemu-devel] [PATCH v4 02/11] iotests: add transactional incremental backup test John Snow
2015-05-11 23:04 ` [Qemu-devel] [PATCH v4 03/11] block: rename BlkTransactionState and BdrvActionOps John Snow
2015-05-18 12:24 ` [Qemu-devel] [Qemu-block] " Stefan Hajnoczi
2015-05-11 23:04 ` [Qemu-devel] [PATCH v4 04/11] block: re-add BlkTransactionState John Snow
2015-05-18 12:33 ` [Qemu-devel] [Qemu-block] " Stefan Hajnoczi
2015-05-11 23:04 ` [Qemu-devel] [PATCH v4 05/11] block: add transactional callbacks feature John Snow
2015-05-11 23:04 ` [Qemu-devel] [PATCH v4 06/11] block: add refcount to Job object John Snow
2015-05-18 15:45 ` Stefan Hajnoczi
2015-05-19 22:15 ` John Snow
2015-05-20 9:27 ` Stefan Hajnoczi
2015-05-22 22:38 ` John Snow
2015-05-11 23:04 ` [Qemu-devel] [PATCH v4 07/11] block: add delayed bitmap successor cleanup John Snow
2015-05-11 23:04 ` [Qemu-devel] [PATCH v4 08/11] qmp: Add an implementation wrapper for qmp_drive_backup John Snow
2015-05-18 14:42 ` [Qemu-devel] [Qemu-block] " Stefan Hajnoczi
2015-05-18 15:10 ` John Snow
2015-05-11 23:04 ` [Qemu-devel] [PATCH v4 09/11] block: drive_backup transaction callback support John Snow
2015-05-18 15:35 ` Stefan Hajnoczi
2015-05-18 15:53 ` John Snow
2015-05-18 15:48 ` Stefan Hajnoczi
2015-05-11 23:04 ` [Qemu-devel] [PATCH v4 10/11] iotests: 124 - transactional failure test John Snow
2015-05-11 23:04 ` [Qemu-devel] [PATCH v4 11/11] qmp-commands.hx: Update the supported 'transaction' operations John Snow
2015-05-18 16:22 ` Max Reitz
2015-05-19 15:30 ` Kashyap Chamarthy
2015-05-19 15:37 ` John Snow
2015-05-20 11:12 ` Kashyap Chamarthy
2015-05-20 11:27 ` John Snow
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.