* [PATCH v8 1/5] blockdev: refactor transaction to use Transaction API
2023-04-21 11:53 [PATCH v8 0/5] block: refactor blockdev transactions Vladimir Sementsov-Ogievskiy
@ 2023-04-21 11:53 ` Vladimir Sementsov-Ogievskiy
2023-05-10 11:10 ` Kevin Wolf
2023-04-21 11:53 ` [PATCH v8 2/5] blockdev: transactions: rename some things Vladimir Sementsov-Ogievskiy
` (3 subsequent siblings)
4 siblings, 1 reply; 12+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2023-04-21 11:53 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, hreitz, kwolf, den, alexander.ivanov,
Vladimir Sementsov-Ogievskiy
We are going to add more block-graph modifying transaction actions,
and block-graph modifying functions are already based on Transaction
API.
Next, we'll need to separately update permissions after several
graph-modifying actions, and this would be simple with help of
Transaction API.
So, now let's just transform what we have into new-style transaction
actions.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
blockdev.c | 317 +++++++++++++++++++++++++++++++----------------------
1 file changed, 186 insertions(+), 131 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index d7b5c18f0a..293f6a958e 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1200,10 +1200,7 @@ typedef struct BlkActionState BlkActionState;
*/
typedef struct BlkActionOps {
size_t instance_size;
- void (*prepare)(BlkActionState *common, Error **errp);
- void (*commit)(BlkActionState *common);
- void (*abort)(BlkActionState *common);
- void (*clean)(BlkActionState *common);
+ void (*action)(BlkActionState *common, Transaction *tran, Error **errp);
} BlkActionOps;
/**
@@ -1235,6 +1232,12 @@ typedef struct InternalSnapshotState {
bool created;
} InternalSnapshotState;
+static void internal_snapshot_abort(void *opaque);
+static void internal_snapshot_clean(void *opaque);
+TransactionActionDrv internal_snapshot_drv = {
+ .abort = internal_snapshot_abort,
+ .clean = internal_snapshot_clean,
+};
static int action_check_completion_mode(BlkActionState *s, Error **errp)
{
@@ -1249,8 +1252,8 @@ static int action_check_completion_mode(BlkActionState *s, Error **errp)
return 0;
}
-static void internal_snapshot_prepare(BlkActionState *common,
- Error **errp)
+static void internal_snapshot_action(BlkActionState *common,
+ Transaction *tran, Error **errp)
{
Error *local_err = NULL;
const char *device;
@@ -1269,6 +1272,8 @@ static void internal_snapshot_prepare(BlkActionState *common,
internal = common->action->u.blockdev_snapshot_internal_sync.data;
state = DO_UPCAST(InternalSnapshotState, common, common);
+ tran_add(tran, &internal_snapshot_drv, state);
+
/* 1. parse input */
device = internal->device;
name = internal->name;
@@ -1353,10 +1358,9 @@ out:
aio_context_release(aio_context);
}
-static void internal_snapshot_abort(BlkActionState *common)
+static void internal_snapshot_abort(void *opaque)
{
- InternalSnapshotState *state =
- DO_UPCAST(InternalSnapshotState, common, common);
+ InternalSnapshotState *state = opaque;
BlockDriverState *bs = state->bs;
QEMUSnapshotInfo *sn = &state->sn;
AioContext *aio_context;
@@ -1380,10 +1384,9 @@ static void internal_snapshot_abort(BlkActionState *common)
aio_context_release(aio_context);
}
-static void internal_snapshot_clean(BlkActionState *common)
+static void internal_snapshot_clean(void *opaque)
{
- InternalSnapshotState *state = DO_UPCAST(InternalSnapshotState,
- common, common);
+ InternalSnapshotState *state = opaque;
AioContext *aio_context;
if (!state->bs) {
@@ -1396,6 +1399,8 @@ static void internal_snapshot_clean(BlkActionState *common)
bdrv_drained_end(state->bs);
aio_context_release(aio_context);
+
+ g_free(state);
}
/* external snapshot private data */
@@ -1406,8 +1411,17 @@ typedef struct ExternalSnapshotState {
bool overlay_appended;
} ExternalSnapshotState;
-static void external_snapshot_prepare(BlkActionState *common,
- Error **errp)
+static void external_snapshot_commit(void *opaque);
+static void external_snapshot_abort(void *opaque);
+static void external_snapshot_clean(void *opaque);
+TransactionActionDrv external_snapshot_drv = {
+ .commit = external_snapshot_commit,
+ .abort = external_snapshot_abort,
+ .clean = external_snapshot_clean,
+};
+
+static void external_snapshot_action(BlkActionState *common, Transaction *tran,
+ Error **errp)
{
int ret;
int flags = 0;
@@ -1426,6 +1440,8 @@ static void external_snapshot_prepare(BlkActionState *common,
AioContext *aio_context;
uint64_t perm, shared;
+ tran_add(tran, &external_snapshot_drv, state);
+
/* 'blockdev-snapshot' and 'blockdev-snapshot-sync' have similar
* purpose but a different set of parameters */
switch (action->type) {
@@ -1578,10 +1594,9 @@ out:
aio_context_release(aio_context);
}
-static void external_snapshot_commit(BlkActionState *common)
+static void external_snapshot_commit(void *opaque)
{
- ExternalSnapshotState *state =
- DO_UPCAST(ExternalSnapshotState, common, common);
+ ExternalSnapshotState *state = opaque;
AioContext *aio_context;
aio_context = bdrv_get_aio_context(state->old_bs);
@@ -1597,10 +1612,9 @@ static void external_snapshot_commit(BlkActionState *common)
aio_context_release(aio_context);
}
-static void external_snapshot_abort(BlkActionState *common)
+static void external_snapshot_abort(void *opaque)
{
- ExternalSnapshotState *state =
- DO_UPCAST(ExternalSnapshotState, common, common);
+ ExternalSnapshotState *state = opaque;
if (state->new_bs) {
if (state->overlay_appended) {
AioContext *aio_context;
@@ -1640,10 +1654,9 @@ static void external_snapshot_abort(BlkActionState *common)
}
}
-static void external_snapshot_clean(BlkActionState *common)
+static void external_snapshot_clean(void *opaque)
{
- ExternalSnapshotState *state =
- DO_UPCAST(ExternalSnapshotState, common, common);
+ ExternalSnapshotState *state = opaque;
AioContext *aio_context;
if (!state->old_bs) {
@@ -1657,6 +1670,8 @@ static void external_snapshot_clean(BlkActionState *common)
bdrv_unref(state->new_bs);
aio_context_release(aio_context);
+
+ g_free(state);
}
typedef struct DriveBackupState {
@@ -1671,7 +1686,17 @@ static BlockJob *do_backup_common(BackupCommon *backup,
AioContext *aio_context,
JobTxn *txn, Error **errp);
-static void drive_backup_prepare(BlkActionState *common, Error **errp)
+static void drive_backup_commit(void *opaque);
+static void drive_backup_abort(void *opaque);
+static void drive_backup_clean(void *opaque);
+TransactionActionDrv drive_backup_drv = {
+ .commit = drive_backup_commit,
+ .abort = drive_backup_abort,
+ .clean = drive_backup_clean,
+};
+
+static void drive_backup_action(BlkActionState *common, Transaction *tran,
+ Error **errp)
{
DriveBackupState *state = DO_UPCAST(DriveBackupState, common, common);
DriveBackup *backup;
@@ -1688,6 +1713,8 @@ static void drive_backup_prepare(BlkActionState *common, Error **errp)
bool set_backing_hd = false;
int ret;
+ tran_add(tran, &drive_backup_drv, state);
+
assert(common->action->type == TRANSACTION_ACTION_KIND_DRIVE_BACKUP);
backup = common->action->u.drive_backup.data;
@@ -1818,9 +1845,9 @@ out:
aio_context_release(aio_context);
}
-static void drive_backup_commit(BlkActionState *common)
+static void drive_backup_commit(void *opaque)
{
- DriveBackupState *state = DO_UPCAST(DriveBackupState, common, common);
+ DriveBackupState *state = opaque;
AioContext *aio_context;
aio_context = bdrv_get_aio_context(state->bs);
@@ -1832,18 +1859,18 @@ static void drive_backup_commit(BlkActionState *common)
aio_context_release(aio_context);
}
-static void drive_backup_abort(BlkActionState *common)
+static void drive_backup_abort(void *opaque)
{
- DriveBackupState *state = DO_UPCAST(DriveBackupState, common, common);
+ DriveBackupState *state = opaque;
if (state->job) {
job_cancel_sync(&state->job->job, true);
}
}
-static void drive_backup_clean(BlkActionState *common)
+static void drive_backup_clean(void *opaque)
{
- DriveBackupState *state = DO_UPCAST(DriveBackupState, common, common);
+ DriveBackupState *state = opaque;
AioContext *aio_context;
if (!state->bs) {
@@ -1856,6 +1883,8 @@ static void drive_backup_clean(BlkActionState *common)
bdrv_drained_end(state->bs);
aio_context_release(aio_context);
+
+ g_free(state);
}
typedef struct BlockdevBackupState {
@@ -1864,7 +1893,17 @@ typedef struct BlockdevBackupState {
BlockJob *job;
} BlockdevBackupState;
-static void blockdev_backup_prepare(BlkActionState *common, Error **errp)
+static void blockdev_backup_commit(void *opaque);
+static void blockdev_backup_abort(void *opaque);
+static void blockdev_backup_clean(void *opaque);
+TransactionActionDrv blockdev_backup_drv = {
+ .commit = blockdev_backup_commit,
+ .abort = blockdev_backup_abort,
+ .clean = blockdev_backup_clean,
+};
+
+static void blockdev_backup_action(BlkActionState *common, Transaction *tran,
+ Error **errp)
{
BlockdevBackupState *state = DO_UPCAST(BlockdevBackupState, common, common);
BlockdevBackup *backup;
@@ -1874,6 +1913,8 @@ static void blockdev_backup_prepare(BlkActionState *common, Error **errp)
AioContext *old_context;
int ret;
+ tran_add(tran, &blockdev_backup_drv, state);
+
assert(common->action->type == TRANSACTION_ACTION_KIND_BLOCKDEV_BACKUP);
backup = common->action->u.blockdev_backup.data;
@@ -1912,9 +1953,9 @@ static void blockdev_backup_prepare(BlkActionState *common, Error **errp)
aio_context_release(aio_context);
}
-static void blockdev_backup_commit(BlkActionState *common)
+static void blockdev_backup_commit(void *opaque)
{
- BlockdevBackupState *state = DO_UPCAST(BlockdevBackupState, common, common);
+ BlockdevBackupState *state = opaque;
AioContext *aio_context;
aio_context = bdrv_get_aio_context(state->bs);
@@ -1926,18 +1967,18 @@ static void blockdev_backup_commit(BlkActionState *common)
aio_context_release(aio_context);
}
-static void blockdev_backup_abort(BlkActionState *common)
+static void blockdev_backup_abort(void *opaque)
{
- BlockdevBackupState *state = DO_UPCAST(BlockdevBackupState, common, common);
+ BlockdevBackupState *state = opaque;
if (state->job) {
job_cancel_sync(&state->job->job, true);
}
}
-static void blockdev_backup_clean(BlkActionState *common)
+static void blockdev_backup_clean(void *opaque)
{
- BlockdevBackupState *state = DO_UPCAST(BlockdevBackupState, common, common);
+ BlockdevBackupState *state = opaque;
AioContext *aio_context;
if (!state->bs) {
@@ -1950,6 +1991,8 @@ static void blockdev_backup_clean(BlkActionState *common)
bdrv_drained_end(state->bs);
aio_context_release(aio_context);
+
+ g_free(state);
}
typedef struct BlockDirtyBitmapState {
@@ -1961,14 +2004,22 @@ typedef struct BlockDirtyBitmapState {
bool was_enabled;
} BlockDirtyBitmapState;
-static void block_dirty_bitmap_add_prepare(BlkActionState *common,
- Error **errp)
+static void block_dirty_bitmap_add_abort(void *opaque);
+TransactionActionDrv block_dirty_bitmap_add_drv = {
+ .abort = block_dirty_bitmap_add_abort,
+ .clean = g_free,
+};
+
+static void block_dirty_bitmap_add_action(BlkActionState *common,
+ Transaction *tran, Error **errp)
{
Error *local_err = NULL;
BlockDirtyBitmapAdd *action;
BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
common, common);
+ tran_add(tran, &block_dirty_bitmap_add_drv, state);
+
if (action_check_completion_mode(common, errp) < 0) {
return;
}
@@ -1988,13 +2039,12 @@ static void block_dirty_bitmap_add_prepare(BlkActionState *common,
}
}
-static void block_dirty_bitmap_add_abort(BlkActionState *common)
+static void block_dirty_bitmap_add_abort(void *opaque)
{
BlockDirtyBitmapAdd *action;
- BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
- common, common);
+ BlockDirtyBitmapState *state = opaque;
- action = common->action->u.block_dirty_bitmap_add.data;
+ action = state->common.action->u.block_dirty_bitmap_add.data;
/* Should not be able to fail: IF the bitmap was added via .prepare(),
* then the node reference and bitmap name must have been valid.
*/
@@ -2003,13 +2053,23 @@ static void block_dirty_bitmap_add_abort(BlkActionState *common)
}
}
-static void block_dirty_bitmap_clear_prepare(BlkActionState *common,
- Error **errp)
+static void block_dirty_bitmap_restore(void *opaque);
+static void block_dirty_bitmap_free_backup(void *opaque);
+TransactionActionDrv block_dirty_bitmap_clear_drv = {
+ .abort = block_dirty_bitmap_restore,
+ .commit = block_dirty_bitmap_free_backup,
+ .clean = g_free,
+};
+
+static void block_dirty_bitmap_clear_action(BlkActionState *common,
+ Transaction *tran, Error **errp)
{
BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
common, common);
BlockDirtyBitmap *action;
+ tran_add(tran, &block_dirty_bitmap_clear_drv, state);
+
if (action_check_completion_mode(common, errp) < 0) {
return;
}
@@ -2030,31 +2090,37 @@ static void block_dirty_bitmap_clear_prepare(BlkActionState *common,
bdrv_clear_dirty_bitmap(state->bitmap, &state->backup);
}
-static void block_dirty_bitmap_restore(BlkActionState *common)
+static void block_dirty_bitmap_restore(void *opaque)
{
- BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
- common, common);
+ BlockDirtyBitmapState *state = opaque;
if (state->backup) {
bdrv_restore_dirty_bitmap(state->bitmap, state->backup);
}
}
-static void block_dirty_bitmap_free_backup(BlkActionState *common)
+static void block_dirty_bitmap_free_backup(void *opaque)
{
- BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
- common, common);
+ BlockDirtyBitmapState *state = opaque;
hbitmap_free(state->backup);
}
-static void block_dirty_bitmap_enable_prepare(BlkActionState *common,
- Error **errp)
+static void block_dirty_bitmap_enable_abort(void *opaque);
+TransactionActionDrv block_dirty_bitmap_enable_drv = {
+ .abort = block_dirty_bitmap_enable_abort,
+ .clean = g_free,
+};
+
+static void block_dirty_bitmap_enable_action(BlkActionState *common,
+ Transaction *tran, Error **errp)
{
BlockDirtyBitmap *action;
BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
common, common);
+ tran_add(tran, &block_dirty_bitmap_enable_drv, state);
+
if (action_check_completion_mode(common, errp) < 0) {
return;
}
@@ -2076,23 +2142,30 @@ static void block_dirty_bitmap_enable_prepare(BlkActionState *common,
bdrv_enable_dirty_bitmap(state->bitmap);
}
-static void block_dirty_bitmap_enable_abort(BlkActionState *common)
+static void block_dirty_bitmap_enable_abort(void *opaque)
{
- BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
- common, common);
+ BlockDirtyBitmapState *state = opaque;
if (!state->was_enabled) {
bdrv_disable_dirty_bitmap(state->bitmap);
}
}
-static void block_dirty_bitmap_disable_prepare(BlkActionState *common,
- Error **errp)
+static void block_dirty_bitmap_disable_abort(void *opaque);
+TransactionActionDrv block_dirty_bitmap_disable_drv = {
+ .abort = block_dirty_bitmap_disable_abort,
+ .clean = g_free,
+};
+
+static void block_dirty_bitmap_disable_action(BlkActionState *common,
+ Transaction *tran, Error **errp)
{
BlockDirtyBitmap *action;
BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
common, common);
+ tran_add(tran, &block_dirty_bitmap_disable_drv, state);
+
if (action_check_completion_mode(common, errp) < 0) {
return;
}
@@ -2114,23 +2187,30 @@ static void block_dirty_bitmap_disable_prepare(BlkActionState *common,
bdrv_disable_dirty_bitmap(state->bitmap);
}
-static void block_dirty_bitmap_disable_abort(BlkActionState *common)
+static void block_dirty_bitmap_disable_abort(void *opaque)
{
- BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
- common, common);
+ BlockDirtyBitmapState *state = opaque;
if (state->was_enabled) {
bdrv_enable_dirty_bitmap(state->bitmap);
}
}
-static void block_dirty_bitmap_merge_prepare(BlkActionState *common,
- Error **errp)
+TransactionActionDrv block_dirty_bitmap_merge_drv = {
+ .commit = block_dirty_bitmap_free_backup,
+ .abort = block_dirty_bitmap_restore,
+ .clean = g_free,
+};
+
+static void block_dirty_bitmap_merge_action(BlkActionState *common,
+ Transaction *tran, Error **errp)
{
BlockDirtyBitmapMerge *action;
BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
common, common);
+ tran_add(tran, &block_dirty_bitmap_merge_drv, state);
+
if (action_check_completion_mode(common, errp) < 0) {
return;
}
@@ -2142,13 +2222,23 @@ static void block_dirty_bitmap_merge_prepare(BlkActionState *common,
errp);
}
-static void block_dirty_bitmap_remove_prepare(BlkActionState *common,
- Error **errp)
+static void block_dirty_bitmap_remove_commit(void *opaque);
+static void block_dirty_bitmap_remove_abort(void *opaque);
+TransactionActionDrv block_dirty_bitmap_remove_drv = {
+ .commit = block_dirty_bitmap_remove_commit,
+ .abort = block_dirty_bitmap_remove_abort,
+ .clean = g_free,
+};
+
+static void block_dirty_bitmap_remove_action(BlkActionState *common,
+ Transaction *tran, Error **errp)
{
BlockDirtyBitmap *action;
BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
common, common);
+ tran_add(tran, &block_dirty_bitmap_remove_drv, state);
+
if (action_check_completion_mode(common, errp) < 0) {
return;
}
@@ -2163,10 +2253,9 @@ static void block_dirty_bitmap_remove_prepare(BlkActionState *common,
}
}
-static void block_dirty_bitmap_remove_abort(BlkActionState *common)
+static void block_dirty_bitmap_remove_abort(void *opaque)
{
- BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
- common, common);
+ BlockDirtyBitmapState *state = opaque;
if (state->bitmap) {
bdrv_dirty_bitmap_skip_store(state->bitmap, false);
@@ -2174,21 +2263,28 @@ static void block_dirty_bitmap_remove_abort(BlkActionState *common)
}
}
-static void block_dirty_bitmap_remove_commit(BlkActionState *common)
+static void block_dirty_bitmap_remove_commit(void *opaque)
{
- BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
- common, common);
+ BlockDirtyBitmapState *state = opaque;
bdrv_dirty_bitmap_set_busy(state->bitmap, false);
bdrv_release_dirty_bitmap(state->bitmap);
}
-static void abort_prepare(BlkActionState *common, Error **errp)
+static void abort_commit(void *opaque);
+TransactionActionDrv abort_drv = {
+ .commit = abort_commit,
+ .clean = g_free,
+};
+
+static void abort_action(BlkActionState *common, Transaction *tran,
+ Error **errp)
{
+ tran_add(tran, &abort_drv, common);
error_setg(errp, "Transaction aborted using Abort action");
}
-static void abort_commit(BlkActionState *common)
+static void abort_commit(void *opaque)
{
g_assert_not_reached(); /* this action never succeeds */
}
@@ -2196,75 +2292,51 @@ static void abort_commit(BlkActionState *common)
static const BlkActionOps actions[] = {
[TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT] = {
.instance_size = sizeof(ExternalSnapshotState),
- .prepare = external_snapshot_prepare,
- .commit = external_snapshot_commit,
- .abort = external_snapshot_abort,
- .clean = external_snapshot_clean,
+ .action = external_snapshot_action,
},
[TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_SYNC] = {
.instance_size = sizeof(ExternalSnapshotState),
- .prepare = external_snapshot_prepare,
- .commit = external_snapshot_commit,
- .abort = external_snapshot_abort,
- .clean = external_snapshot_clean,
+ .action = external_snapshot_action,
},
[TRANSACTION_ACTION_KIND_DRIVE_BACKUP] = {
.instance_size = sizeof(DriveBackupState),
- .prepare = drive_backup_prepare,
- .commit = drive_backup_commit,
- .abort = drive_backup_abort,
- .clean = drive_backup_clean,
+ .action = drive_backup_action,
},
[TRANSACTION_ACTION_KIND_BLOCKDEV_BACKUP] = {
.instance_size = sizeof(BlockdevBackupState),
- .prepare = blockdev_backup_prepare,
- .commit = blockdev_backup_commit,
- .abort = blockdev_backup_abort,
- .clean = blockdev_backup_clean,
+ .action = blockdev_backup_action,
},
[TRANSACTION_ACTION_KIND_ABORT] = {
.instance_size = sizeof(BlkActionState),
- .prepare = abort_prepare,
- .commit = abort_commit,
+ .action = abort_action,
},
[TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_INTERNAL_SYNC] = {
.instance_size = sizeof(InternalSnapshotState),
- .prepare = internal_snapshot_prepare,
- .abort = internal_snapshot_abort,
- .clean = internal_snapshot_clean,
+ .action = internal_snapshot_action,
},
[TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_ADD] = {
.instance_size = sizeof(BlockDirtyBitmapState),
- .prepare = block_dirty_bitmap_add_prepare,
- .abort = block_dirty_bitmap_add_abort,
+ .action = block_dirty_bitmap_add_action,
},
[TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_CLEAR] = {
.instance_size = sizeof(BlockDirtyBitmapState),
- .prepare = block_dirty_bitmap_clear_prepare,
- .commit = block_dirty_bitmap_free_backup,
- .abort = block_dirty_bitmap_restore,
+ .action = block_dirty_bitmap_clear_action,
},
[TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_ENABLE] = {
.instance_size = sizeof(BlockDirtyBitmapState),
- .prepare = block_dirty_bitmap_enable_prepare,
- .abort = block_dirty_bitmap_enable_abort,
+ .action = block_dirty_bitmap_enable_action,
},
[TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_DISABLE] = {
.instance_size = sizeof(BlockDirtyBitmapState),
- .prepare = block_dirty_bitmap_disable_prepare,
- .abort = block_dirty_bitmap_disable_abort,
+ .action = block_dirty_bitmap_disable_action,
},
[TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_MERGE] = {
.instance_size = sizeof(BlockDirtyBitmapState),
- .prepare = block_dirty_bitmap_merge_prepare,
- .commit = block_dirty_bitmap_free_backup,
- .abort = block_dirty_bitmap_restore,
+ .action = block_dirty_bitmap_merge_action,
},
[TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_REMOVE] = {
.instance_size = sizeof(BlockDirtyBitmapState),
- .prepare = block_dirty_bitmap_remove_prepare,
- .commit = block_dirty_bitmap_remove_commit,
- .abort = block_dirty_bitmap_remove_abort,
+ .action = block_dirty_bitmap_remove_action,
},
/* Where are transactions for MIRROR, COMMIT and STREAM?
* Although these blockjobs use transaction callbacks like the backup job,
@@ -2306,14 +2378,11 @@ void qmp_transaction(TransactionActionList *dev_list,
TransactionActionList *dev_entry = dev_list;
bool has_props = !!props;
JobTxn *block_job_txn = NULL;
- BlkActionState *state, *next;
Error *local_err = NULL;
+ Transaction *tran = tran_new();
GLOBAL_STATE_CODE();
- QTAILQ_HEAD(, BlkActionState) snap_bdrv_states;
- QTAILQ_INIT(&snap_bdrv_states);
-
/* Does this transaction get canceled as a group on failure?
* If not, we don't really need to make a JobTxn.
*/
@@ -2329,6 +2398,7 @@ void qmp_transaction(TransactionActionList *dev_list,
while (NULL != dev_entry) {
TransactionAction *dev_info = NULL;
const BlkActionOps *ops;
+ BlkActionState *state;
dev_info = dev_entry->value;
dev_entry = dev_entry->next;
@@ -2343,38 +2413,23 @@ void qmp_transaction(TransactionActionList *dev_list,
state->action = dev_info;
state->block_job_txn = block_job_txn;
state->txn_props = props;
- QTAILQ_INSERT_TAIL(&snap_bdrv_states, state, entry);
- state->ops->prepare(state, &local_err);
+ state->ops->action(state, tran, &local_err);
if (local_err) {
error_propagate(errp, local_err);
goto delete_and_fail;
}
}
- QTAILQ_FOREACH(state, &snap_bdrv_states, entry) {
- if (state->ops->commit) {
- state->ops->commit(state);
- }
- }
+ tran_commit(tran);
/* success */
goto exit;
delete_and_fail:
/* failure, and it is all-or-none; roll back all operations */
- QTAILQ_FOREACH_REVERSE(state, &snap_bdrv_states, entry) {
- if (state->ops->abort) {
- state->ops->abort(state);
- }
- }
+ tran_abort(tran);
exit:
- QTAILQ_FOREACH_SAFE(state, &snap_bdrv_states, entry, next) {
- if (state->ops->clean) {
- state->ops->clean(state);
- }
- g_free(state);
- }
if (!has_props) {
qapi_free_TransactionProperties(props);
}
--
2.34.1
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH v8 1/5] blockdev: refactor transaction to use Transaction API
2023-04-21 11:53 ` [PATCH v8 1/5] blockdev: refactor transaction to use Transaction API Vladimir Sementsov-Ogievskiy
@ 2023-05-10 11:10 ` Kevin Wolf
0 siblings, 0 replies; 12+ messages in thread
From: Kevin Wolf @ 2023-05-10 11:10 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy
Cc: qemu-block, qemu-devel, hreitz, den, alexander.ivanov
Am 21.04.2023 um 13:53 hat Vladimir Sementsov-Ogievskiy geschrieben:
> We are going to add more block-graph modifying transaction actions,
> and block-graph modifying functions are already based on Transaction
> API.
>
> Next, we'll need to separately update permissions after several
> graph-modifying actions, and this would be simple with help of
> Transaction API.
>
> So, now let's just transform what we have into new-style transaction
> actions.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
> blockdev.c | 317 +++++++++++++++++++++++++++++++----------------------
> 1 file changed, 186 insertions(+), 131 deletions(-)
> diff --git a/blockdev.c b/blockdev.c
> index d7b5c18f0a..293f6a958e 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1380,10 +1384,9 @@ static void internal_snapshot_abort(BlkActionState *common)
> aio_context_release(aio_context);
> }
>
> -static void internal_snapshot_clean(BlkActionState *common)
> +static void internal_snapshot_clean(void *opaque)
> {
> - InternalSnapshotState *state = DO_UPCAST(InternalSnapshotState,
> - common, common);
> + InternalSnapshotState *state = opaque;
> AioContext *aio_context;
>
> if (!state->bs) {
> @@ -1396,6 +1399,8 @@ static void internal_snapshot_clean(BlkActionState *common)
> bdrv_drained_end(state->bs);
>
> aio_context_release(aio_context);
> +
> + g_free(state);
> }
state is leaked if we take the early return a few lines above:
if (!state->bs) {
return;
}
> /* external snapshot private data */
> @@ -1657,6 +1670,8 @@ static void external_snapshot_clean(BlkActionState *common)
> bdrv_unref(state->new_bs);
>
> aio_context_release(aio_context);
> +
> + g_free(state);
> }
Same potential leak of state.
> typedef struct DriveBackupState {
> @@ -1856,6 +1883,8 @@ static void drive_backup_clean(BlkActionState *common)
> bdrv_drained_end(state->bs);
>
> aio_context_release(aio_context);
> +
> + g_free(state);
> }
Here as well.
> typedef struct BlockdevBackupState {
> @@ -1950,6 +1991,8 @@ static void blockdev_backup_clean(BlkActionState *common)
> bdrv_drained_end(state->bs);
>
> aio_context_release(aio_context);
> +
> + g_free(state);
> }
And here.
Other than that, the patch looks good to me.
Kevin
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v8 2/5] blockdev: transactions: rename some things
2023-04-21 11:53 [PATCH v8 0/5] block: refactor blockdev transactions Vladimir Sementsov-Ogievskiy
2023-04-21 11:53 ` [PATCH v8 1/5] blockdev: refactor transaction to use Transaction API Vladimir Sementsov-Ogievskiy
@ 2023-04-21 11:53 ` Vladimir Sementsov-Ogievskiy
2023-05-10 11:16 ` Kevin Wolf
2023-04-21 11:53 ` [PATCH v8 3/5] blockdev: qmp_transaction: refactor loop to classic for Vladimir Sementsov-Ogievskiy
` (2 subsequent siblings)
4 siblings, 1 reply; 12+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2023-04-21 11:53 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, hreitz, kwolf, den, alexander.ivanov,
Vladimir Sementsov-Ogievskiy
Look at qmp_transaction(): dev_list is not obvious name for list of
actions. Let's look at qapi spec, this argument is "actions". Let's
follow the common practice of using same argument names in qapi scheme
and code.
To be honest, rename props to properties for same reason.
Next, we have to rename global map of actions, to not conflict with new
name for function argument.
Rename also dev_entry loop variable accordingly to new name of the
list.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
blockdev.c | 30 +++++++++++++++---------------
1 file changed, 15 insertions(+), 15 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index 293f6a958e..2174ab2694 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2289,7 +2289,7 @@ static void abort_commit(void *opaque)
g_assert_not_reached(); /* this action never succeeds */
}
-static const BlkActionOps actions[] = {
+static const BlkActionOps actions_map[] = {
[TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT] = {
.instance_size = sizeof(ExternalSnapshotState),
.action = external_snapshot_action,
@@ -2371,12 +2371,12 @@ static TransactionProperties *get_transaction_properties(
*
* Always run under BQL.
*/
-void qmp_transaction(TransactionActionList *dev_list,
- struct TransactionProperties *props,
+void qmp_transaction(TransactionActionList *actions,
+ struct TransactionProperties *properties,
Error **errp)
{
- TransactionActionList *dev_entry = dev_list;
- bool has_props = !!props;
+ TransactionActionList *act = actions;
+ bool has_properties = !!properties;
JobTxn *block_job_txn = NULL;
Error *local_err = NULL;
Transaction *tran = tran_new();
@@ -2386,8 +2386,8 @@ void qmp_transaction(TransactionActionList *dev_list,
/* Does this transaction get canceled as a group on failure?
* If not, we don't really need to make a JobTxn.
*/
- props = get_transaction_properties(props);
- if (props->completion_mode != ACTION_COMPLETION_MODE_INDIVIDUAL) {
+ properties = get_transaction_properties(properties);
+ if (properties->completion_mode != ACTION_COMPLETION_MODE_INDIVIDUAL) {
block_job_txn = job_txn_new();
}
@@ -2395,24 +2395,24 @@ void qmp_transaction(TransactionActionList *dev_list,
bdrv_drain_all();
/* We don't do anything in this loop that commits us to the operations */
- while (NULL != dev_entry) {
+ while (NULL != act) {
TransactionAction *dev_info = NULL;
const BlkActionOps *ops;
BlkActionState *state;
- dev_info = dev_entry->value;
- dev_entry = dev_entry->next;
+ dev_info = act->value;
+ act = act->next;
- assert(dev_info->type < ARRAY_SIZE(actions));
+ assert(dev_info->type < ARRAY_SIZE(actions_map));
- ops = &actions[dev_info->type];
+ ops = &actions_map[dev_info->type];
assert(ops->instance_size > 0);
state = g_malloc0(ops->instance_size);
state->ops = ops;
state->action = dev_info;
state->block_job_txn = block_job_txn;
- state->txn_props = props;
+ state->txn_props = properties;
state->ops->action(state, tran, &local_err);
if (local_err) {
@@ -2430,8 +2430,8 @@ delete_and_fail:
/* failure, and it is all-or-none; roll back all operations */
tran_abort(tran);
exit:
- if (!has_props) {
- qapi_free_TransactionProperties(props);
+ if (!has_properties) {
+ qapi_free_TransactionProperties(properties);
}
job_txn_unref(block_job_txn);
}
--
2.34.1
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH v8 2/5] blockdev: transactions: rename some things
2023-04-21 11:53 ` [PATCH v8 2/5] blockdev: transactions: rename some things Vladimir Sementsov-Ogievskiy
@ 2023-05-10 11:16 ` Kevin Wolf
0 siblings, 0 replies; 12+ messages in thread
From: Kevin Wolf @ 2023-05-10 11:16 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy
Cc: qemu-block, qemu-devel, hreitz, den, alexander.ivanov
Am 21.04.2023 um 13:53 hat Vladimir Sementsov-Ogievskiy geschrieben:
> Look at qmp_transaction(): dev_list is not obvious name for list of
> actions. Let's look at qapi spec, this argument is "actions". Let's
> follow the common practice of using same argument names in qapi scheme
> and code.
>
> To be honest, rename props to properties for same reason.
>
> Next, we have to rename global map of actions, to not conflict with new
> name for function argument.
>
> Rename also dev_entry loop variable accordingly to new name of the
> list.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v8 3/5] blockdev: qmp_transaction: refactor loop to classic for
2023-04-21 11:53 [PATCH v8 0/5] block: refactor blockdev transactions Vladimir Sementsov-Ogievskiy
2023-04-21 11:53 ` [PATCH v8 1/5] blockdev: refactor transaction to use Transaction API Vladimir Sementsov-Ogievskiy
2023-04-21 11:53 ` [PATCH v8 2/5] blockdev: transactions: rename some things Vladimir Sementsov-Ogievskiy
@ 2023-04-21 11:53 ` Vladimir Sementsov-Ogievskiy
2023-05-10 11:17 ` Kevin Wolf
2023-04-21 11:53 ` [PATCH v8 4/5] blockdev: transaction: refactor handling transaction properties Vladimir Sementsov-Ogievskiy
2023-04-21 11:53 ` [PATCH v8 5/5] blockdev: qmp_transaction: drop extra generic layer Vladimir Sementsov-Ogievskiy
4 siblings, 1 reply; 12+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2023-04-21 11:53 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, hreitz, kwolf, den, alexander.ivanov,
Vladimir Sementsov-Ogievskiy
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
blockdev.c | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index 2174ab2694..89c573a094 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2375,7 +2375,7 @@ void qmp_transaction(TransactionActionList *actions,
struct TransactionProperties *properties,
Error **errp)
{
- TransactionActionList *act = actions;
+ TransactionActionList *act;
bool has_properties = !!properties;
JobTxn *block_job_txn = NULL;
Error *local_err = NULL;
@@ -2395,14 +2395,11 @@ void qmp_transaction(TransactionActionList *actions,
bdrv_drain_all();
/* We don't do anything in this loop that commits us to the operations */
- while (NULL != act) {
- TransactionAction *dev_info = NULL;
+ for (act = actions; act; act = act->next) {
+ TransactionAction *dev_info = act->value;
const BlkActionOps *ops;
BlkActionState *state;
- dev_info = act->value;
- act = act->next;
-
assert(dev_info->type < ARRAY_SIZE(actions_map));
ops = &actions_map[dev_info->type];
--
2.34.1
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH v8 4/5] blockdev: transaction: refactor handling transaction properties
2023-04-21 11:53 [PATCH v8 0/5] block: refactor blockdev transactions Vladimir Sementsov-Ogievskiy
` (2 preceding siblings ...)
2023-04-21 11:53 ` [PATCH v8 3/5] blockdev: qmp_transaction: refactor loop to classic for Vladimir Sementsov-Ogievskiy
@ 2023-04-21 11:53 ` Vladimir Sementsov-Ogievskiy
2023-05-10 11:21 ` Kevin Wolf
2023-04-21 11:53 ` [PATCH v8 5/5] blockdev: qmp_transaction: drop extra generic layer Vladimir Sementsov-Ogievskiy
4 siblings, 1 reply; 12+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2023-04-21 11:53 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, hreitz, kwolf, den, alexander.ivanov,
Vladimir Sementsov-Ogievskiy
Only backup supports GROUPED mode. Make this logic more clear. And
avoid passing extra thing to each action.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
blockdev.c | 92 +++++++++++-------------------------------------------
1 file changed, 19 insertions(+), 73 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index 89c573a094..edaf7c6601 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1220,7 +1220,6 @@ struct BlkActionState {
TransactionAction *action;
const BlkActionOps *ops;
JobTxn *block_job_txn;
- TransactionProperties *txn_props;
QTAILQ_ENTRY(BlkActionState) entry;
};
@@ -1239,19 +1238,6 @@ TransactionActionDrv internal_snapshot_drv = {
.clean = internal_snapshot_clean,
};
-static int action_check_completion_mode(BlkActionState *s, Error **errp)
-{
- if (s->txn_props->completion_mode != ACTION_COMPLETION_MODE_INDIVIDUAL) {
- error_setg(errp,
- "Action '%s' does not support Transaction property "
- "completion-mode = %s",
- TransactionActionKind_str(s->action->type),
- ActionCompletionMode_str(s->txn_props->completion_mode));
- return -1;
- }
- return 0;
-}
-
static void internal_snapshot_action(BlkActionState *common,
Transaction *tran, Error **errp)
{
@@ -1274,15 +1260,9 @@ static void internal_snapshot_action(BlkActionState *common,
tran_add(tran, &internal_snapshot_drv, state);
- /* 1. parse input */
device = internal->device;
name = internal->name;
- /* 2. check for validation */
- if (action_check_completion_mode(common, errp) < 0) {
- return;
- }
-
bs = qmp_get_root_bs(device, errp);
if (!bs) {
return;
@@ -1468,9 +1448,6 @@ static void external_snapshot_action(BlkActionState *common, Transaction *tran,
}
/* start processing */
- if (action_check_completion_mode(common, errp) < 0) {
- return;
- }
state->old_bs = bdrv_lookup_bs(device, node_name, errp);
if (!state->old_bs) {
@@ -2020,10 +1997,6 @@ static void block_dirty_bitmap_add_action(BlkActionState *common,
tran_add(tran, &block_dirty_bitmap_add_drv, state);
- if (action_check_completion_mode(common, errp) < 0) {
- return;
- }
-
action = common->action->u.block_dirty_bitmap_add.data;
/* AIO context taken and released within qmp_block_dirty_bitmap_add */
qmp_block_dirty_bitmap_add(action->node, action->name,
@@ -2070,10 +2043,6 @@ static void block_dirty_bitmap_clear_action(BlkActionState *common,
tran_add(tran, &block_dirty_bitmap_clear_drv, state);
- if (action_check_completion_mode(common, errp) < 0) {
- return;
- }
-
action = common->action->u.block_dirty_bitmap_clear.data;
state->bitmap = block_dirty_bitmap_lookup(action->node,
action->name,
@@ -2121,10 +2090,6 @@ static void block_dirty_bitmap_enable_action(BlkActionState *common,
tran_add(tran, &block_dirty_bitmap_enable_drv, state);
- if (action_check_completion_mode(common, errp) < 0) {
- return;
- }
-
action = common->action->u.block_dirty_bitmap_enable.data;
state->bitmap = block_dirty_bitmap_lookup(action->node,
action->name,
@@ -2166,10 +2131,6 @@ static void block_dirty_bitmap_disable_action(BlkActionState *common,
tran_add(tran, &block_dirty_bitmap_disable_drv, state);
- if (action_check_completion_mode(common, errp) < 0) {
- return;
- }
-
action = common->action->u.block_dirty_bitmap_disable.data;
state->bitmap = block_dirty_bitmap_lookup(action->node,
action->name,
@@ -2211,10 +2172,6 @@ static void block_dirty_bitmap_merge_action(BlkActionState *common,
tran_add(tran, &block_dirty_bitmap_merge_drv, state);
- if (action_check_completion_mode(common, errp) < 0) {
- return;
- }
-
action = common->action->u.block_dirty_bitmap_merge.data;
state->bitmap = block_dirty_bitmap_merge(action->node, action->target,
@@ -2239,10 +2196,6 @@ static void block_dirty_bitmap_remove_action(BlkActionState *common,
tran_add(tran, &block_dirty_bitmap_remove_drv, state);
- if (action_check_completion_mode(common, errp) < 0) {
- return;
- }
-
action = common->action->u.block_dirty_bitmap_remove.data;
state->bitmap = block_dirty_bitmap_remove(action->node, action->name,
@@ -2346,25 +2299,6 @@ static const BlkActionOps actions_map[] = {
*/
};
-/**
- * Allocate a TransactionProperties structure if necessary, and fill
- * that structure with desired defaults if they are unset.
- */
-static TransactionProperties *get_transaction_properties(
- TransactionProperties *props)
-{
- if (!props) {
- props = g_new0(TransactionProperties, 1);
- }
-
- if (!props->has_completion_mode) {
- props->has_completion_mode = true;
- props->completion_mode = ACTION_COMPLETION_MODE_INDIVIDUAL;
- }
-
- return props;
-}
-
/*
* 'Atomic' group operations. The operations are performed as a set, and if
* any fail then we roll back all operations in the group.
@@ -2376,18 +2310,34 @@ void qmp_transaction(TransactionActionList *actions,
Error **errp)
{
TransactionActionList *act;
- bool has_properties = !!properties;
JobTxn *block_job_txn = NULL;
Error *local_err = NULL;
Transaction *tran = tran_new();
+ ActionCompletionMode comp_mode =
+ properties ? properties->completion_mode :
+ ACTION_COMPLETION_MODE_INDIVIDUAL;
GLOBAL_STATE_CODE();
/* Does this transaction get canceled as a group on failure?
* If not, we don't really need to make a JobTxn.
*/
- properties = get_transaction_properties(properties);
- if (properties->completion_mode != ACTION_COMPLETION_MODE_INDIVIDUAL) {
+ if (comp_mode != ACTION_COMPLETION_MODE_INDIVIDUAL) {
+ for (act = actions; act; act = act->next) {
+ TransactionActionKind type = act->value->type;
+
+ if (type != TRANSACTION_ACTION_KIND_BLOCKDEV_BACKUP &&
+ type != TRANSACTION_ACTION_KIND_DRIVE_BACKUP)
+ {
+ error_setg(errp,
+ "Action '%s' does not support Transaction property "
+ "completion-mode = %s",
+ TransactionActionKind_str(type),
+ ActionCompletionMode_str(comp_mode));
+ return;
+ }
+ }
+
block_job_txn = job_txn_new();
}
@@ -2409,7 +2359,6 @@ void qmp_transaction(TransactionActionList *actions,
state->ops = ops;
state->action = dev_info;
state->block_job_txn = block_job_txn;
- state->txn_props = properties;
state->ops->action(state, tran, &local_err);
if (local_err) {
@@ -2427,9 +2376,6 @@ delete_and_fail:
/* failure, and it is all-or-none; roll back all operations */
tran_abort(tran);
exit:
- if (!has_properties) {
- qapi_free_TransactionProperties(properties);
- }
job_txn_unref(block_job_txn);
}
--
2.34.1
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH v8 4/5] blockdev: transaction: refactor handling transaction properties
2023-04-21 11:53 ` [PATCH v8 4/5] blockdev: transaction: refactor handling transaction properties Vladimir Sementsov-Ogievskiy
@ 2023-05-10 11:21 ` Kevin Wolf
0 siblings, 0 replies; 12+ messages in thread
From: Kevin Wolf @ 2023-05-10 11:21 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy
Cc: qemu-block, qemu-devel, hreitz, den, alexander.ivanov
Am 21.04.2023 um 13:53 hat Vladimir Sementsov-Ogievskiy geschrieben:
> Only backup supports GROUPED mode. Make this logic more clear. And
> avoid passing extra thing to each action.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
> blockdev.c | 92 +++++++++++-------------------------------------------
> 1 file changed, 19 insertions(+), 73 deletions(-)
> @@ -2376,18 +2310,34 @@ void qmp_transaction(TransactionActionList *actions,
> Error **errp)
> {
> TransactionActionList *act;
> - bool has_properties = !!properties;
> JobTxn *block_job_txn = NULL;
> Error *local_err = NULL;
> Transaction *tran = tran_new();
> + ActionCompletionMode comp_mode =
> + properties ? properties->completion_mode :
> + ACTION_COMPLETION_MODE_INDIVIDUAL;
>
> GLOBAL_STATE_CODE();
>
> /* Does this transaction get canceled as a group on failure?
> * If not, we don't really need to make a JobTxn.
> */
> - properties = get_transaction_properties(properties);
> - if (properties->completion_mode != ACTION_COMPLETION_MODE_INDIVIDUAL) {
> + if (comp_mode != ACTION_COMPLETION_MODE_INDIVIDUAL) {
> + for (act = actions; act; act = act->next) {
> + TransactionActionKind type = act->value->type;
> +
> + if (type != TRANSACTION_ACTION_KIND_BLOCKDEV_BACKUP &&
> + type != TRANSACTION_ACTION_KIND_DRIVE_BACKUP)
> + {
> + error_setg(errp,
> + "Action '%s' does not support Transaction property "
Should this be lower case "transaction"?
> + "completion-mode = %s",
> + TransactionActionKind_str(type),
> + ActionCompletionMode_str(comp_mode));
> + return;
This leaks tran.
> + }
> + }
> +
> block_job_txn = job_txn_new();
> }
Kevin
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v8 5/5] blockdev: qmp_transaction: drop extra generic layer
2023-04-21 11:53 [PATCH v8 0/5] block: refactor blockdev transactions Vladimir Sementsov-Ogievskiy
` (3 preceding siblings ...)
2023-04-21 11:53 ` [PATCH v8 4/5] blockdev: transaction: refactor handling transaction properties Vladimir Sementsov-Ogievskiy
@ 2023-04-21 11:53 ` Vladimir Sementsov-Ogievskiy
2023-05-10 11:47 ` Kevin Wolf
4 siblings, 1 reply; 12+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2023-04-21 11:53 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, hreitz, kwolf, den, alexander.ivanov,
Vladimir Sementsov-Ogievskiy
Let's simplify things:
First, actions generally don't need access to common BlkActionState
structure. The only exclusion are backup actions that need
block_job_txn.
Next, for transaction actions of Transaction API is more native to
allocated state structure in the action itself.
So, do the following transformation:
1. Let all actions be represented by a function with corresponding
structure as arguments.
2. Instead of array-map marshaller, let's make a function, that calls
corresponding action directly.
3. BlkActionOps and BlkActionState structures become unused
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
blockdev.c | 278 +++++++++++++++++------------------------------------
1 file changed, 89 insertions(+), 189 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index edaf7c6601..2bfa77e564 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1178,54 +1178,8 @@ out_aio_context:
return NULL;
}
-/* New and old BlockDriverState structs for atomic group operations */
-
-typedef struct BlkActionState BlkActionState;
-
-/**
- * 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.
- *
- * Always run under BQL.
- */
-typedef struct BlkActionOps {
- size_t instance_size;
- void (*action)(BlkActionState *common, Transaction *tran, Error **errp);
-} BlkActionOps;
-
-/**
- * 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.
- * @block_job_txn: Transaction which this action belongs to.
- * @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 BlkActionState {
- TransactionAction *action;
- const BlkActionOps *ops;
- JobTxn *block_job_txn;
- QTAILQ_ENTRY(BlkActionState) entry;
-};
-
/* internal snapshot private data */
typedef struct InternalSnapshotState {
- BlkActionState common;
BlockDriverState *bs;
QEMUSnapshotInfo sn;
bool created;
@@ -1238,7 +1192,7 @@ TransactionActionDrv internal_snapshot_drv = {
.clean = internal_snapshot_clean,
};
-static void internal_snapshot_action(BlkActionState *common,
+static void internal_snapshot_action(BlockdevSnapshotInternal *internal,
Transaction *tran, Error **errp)
{
Error *local_err = NULL;
@@ -1248,16 +1202,10 @@ static void internal_snapshot_action(BlkActionState *common,
QEMUSnapshotInfo old_sn, *sn;
bool ret;
int64_t rt;
- BlockdevSnapshotInternal *internal;
- InternalSnapshotState *state;
+ InternalSnapshotState *state = g_new0(InternalSnapshotState, 1);
AioContext *aio_context;
int ret1;
- g_assert(common->action->type ==
- TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_INTERNAL_SYNC);
- internal = common->action->u.blockdev_snapshot_internal_sync.data;
- state = DO_UPCAST(InternalSnapshotState, common, common);
-
tran_add(tran, &internal_snapshot_drv, state);
device = internal->device;
@@ -1385,7 +1333,6 @@ static void internal_snapshot_clean(void *opaque)
/* external snapshot private data */
typedef struct ExternalSnapshotState {
- BlkActionState common;
BlockDriverState *old_bs;
BlockDriverState *new_bs;
bool overlay_appended;
@@ -1400,8 +1347,8 @@ TransactionActionDrv external_snapshot_drv = {
.clean = external_snapshot_clean,
};
-static void external_snapshot_action(BlkActionState *common, Transaction *tran,
- Error **errp)
+static void external_snapshot_action(TransactionAction *action,
+ Transaction *tran, Error **errp)
{
int ret;
int flags = 0;
@@ -1414,9 +1361,7 @@ static void external_snapshot_action(BlkActionState *common, Transaction *tran,
const char *snapshot_ref;
/* File name of the new image (for 'blockdev-snapshot-sync') */
const char *new_image_file;
- ExternalSnapshotState *state =
- DO_UPCAST(ExternalSnapshotState, common, common);
- TransactionAction *action = common->action;
+ ExternalSnapshotState *state = g_new0(ExternalSnapshotState, 1);
AioContext *aio_context;
uint64_t perm, shared;
@@ -1652,7 +1597,6 @@ static void external_snapshot_clean(void *opaque)
}
typedef struct DriveBackupState {
- BlkActionState common;
BlockDriverState *bs;
BlockJob *job;
} DriveBackupState;
@@ -1672,11 +1616,11 @@ TransactionActionDrv drive_backup_drv = {
.clean = drive_backup_clean,
};
-static void drive_backup_action(BlkActionState *common, Transaction *tran,
- Error **errp)
+static void drive_backup_action(DriveBackup *backup,
+ JobTxn *block_job_txn,
+ Transaction *tran, Error **errp)
{
- DriveBackupState *state = DO_UPCAST(DriveBackupState, common, common);
- DriveBackup *backup;
+ DriveBackupState *state = g_new0(DriveBackupState, 1);
BlockDriverState *bs;
BlockDriverState *target_bs;
BlockDriverState *source = NULL;
@@ -1692,9 +1636,6 @@ static void drive_backup_action(BlkActionState *common, Transaction *tran,
tran_add(tran, &drive_backup_drv, state);
- assert(common->action->type == TRANSACTION_ACTION_KIND_DRIVE_BACKUP);
- backup = common->action->u.drive_backup.data;
-
if (!backup->has_mode) {
backup->mode = NEW_IMAGE_MODE_ABSOLUTE_PATHS;
}
@@ -1814,7 +1755,7 @@ static void drive_backup_action(BlkActionState *common, Transaction *tran,
state->job = do_backup_common(qapi_DriveBackup_base(backup),
bs, target_bs, aio_context,
- common->block_job_txn, errp);
+ block_job_txn, errp);
unref:
bdrv_unref(target_bs);
@@ -1865,7 +1806,6 @@ static void drive_backup_clean(void *opaque)
}
typedef struct BlockdevBackupState {
- BlkActionState common;
BlockDriverState *bs;
BlockJob *job;
} BlockdevBackupState;
@@ -1879,11 +1819,11 @@ TransactionActionDrv blockdev_backup_drv = {
.clean = blockdev_backup_clean,
};
-static void blockdev_backup_action(BlkActionState *common, Transaction *tran,
- Error **errp)
+static void blockdev_backup_action(BlockdevBackup *backup,
+ JobTxn *block_job_txn,
+ Transaction *tran, Error **errp)
{
- BlockdevBackupState *state = DO_UPCAST(BlockdevBackupState, common, common);
- BlockdevBackup *backup;
+ BlockdevBackupState *state = g_new0(BlockdevBackupState, 1);
BlockDriverState *bs;
BlockDriverState *target_bs;
AioContext *aio_context;
@@ -1892,9 +1832,6 @@ static void blockdev_backup_action(BlkActionState *common, Transaction *tran,
tran_add(tran, &blockdev_backup_drv, state);
- assert(common->action->type == TRANSACTION_ACTION_KIND_BLOCKDEV_BACKUP);
- backup = common->action->u.blockdev_backup.data;
-
bs = bdrv_lookup_bs(backup->device, backup->device, errp);
if (!bs) {
return;
@@ -1925,7 +1862,7 @@ static void blockdev_backup_action(BlkActionState *common, Transaction *tran,
state->job = do_backup_common(qapi_BlockdevBackup_base(backup),
bs, target_bs, aio_context,
- common->block_job_txn, errp);
+ block_job_txn, errp);
aio_context_release(aio_context);
}
@@ -1973,11 +1910,9 @@ static void blockdev_backup_clean(void *opaque)
}
typedef struct BlockDirtyBitmapState {
- BlkActionState common;
BdrvDirtyBitmap *bitmap;
BlockDriverState *bs;
HBitmap *backup;
- bool prepared;
bool was_enabled;
} BlockDirtyBitmapState;
@@ -1987,17 +1922,14 @@ TransactionActionDrv block_dirty_bitmap_add_drv = {
.clean = g_free,
};
-static void block_dirty_bitmap_add_action(BlkActionState *common,
+static void block_dirty_bitmap_add_action(BlockDirtyBitmapAdd *action,
Transaction *tran, Error **errp)
{
Error *local_err = NULL;
- BlockDirtyBitmapAdd *action;
- BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
- common, common);
+ BlockDirtyBitmapState *state = g_new0(BlockDirtyBitmapState, 1);
tran_add(tran, &block_dirty_bitmap_add_drv, state);
- action = common->action->u.block_dirty_bitmap_add.data;
/* 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,
@@ -2006,7 +1938,8 @@ static void block_dirty_bitmap_add_action(BlkActionState *common,
&local_err);
if (!local_err) {
- state->prepared = true;
+ state->bitmap = block_dirty_bitmap_lookup(action->node, action->name,
+ NULL, &error_abort);
} else {
error_propagate(errp, local_err);
}
@@ -2014,15 +1947,10 @@ static void block_dirty_bitmap_add_action(BlkActionState *common,
static void block_dirty_bitmap_add_abort(void *opaque)
{
- BlockDirtyBitmapAdd *action;
BlockDirtyBitmapState *state = opaque;
- action = state->common.action->u.block_dirty_bitmap_add.data;
- /* 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);
+ if (state->bitmap) {
+ bdrv_release_dirty_bitmap(state->bitmap);
}
}
@@ -2034,16 +1962,13 @@ TransactionActionDrv block_dirty_bitmap_clear_drv = {
.clean = g_free,
};
-static void block_dirty_bitmap_clear_action(BlkActionState *common,
+static void block_dirty_bitmap_clear_action(BlockDirtyBitmap *action,
Transaction *tran, Error **errp)
{
- BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
- common, common);
- BlockDirtyBitmap *action;
+ BlockDirtyBitmapState *state = g_new0(BlockDirtyBitmapState, 1);
tran_add(tran, &block_dirty_bitmap_clear_drv, state);
- action = common->action->u.block_dirty_bitmap_clear.data;
state->bitmap = block_dirty_bitmap_lookup(action->node,
action->name,
&state->bs,
@@ -2081,16 +2006,13 @@ TransactionActionDrv block_dirty_bitmap_enable_drv = {
.clean = g_free,
};
-static void block_dirty_bitmap_enable_action(BlkActionState *common,
+static void block_dirty_bitmap_enable_action(BlockDirtyBitmap *action,
Transaction *tran, Error **errp)
{
- BlockDirtyBitmap *action;
- BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
- common, common);
+ BlockDirtyBitmapState *state = g_new0(BlockDirtyBitmapState, 1);
tran_add(tran, &block_dirty_bitmap_enable_drv, state);
- action = common->action->u.block_dirty_bitmap_enable.data;
state->bitmap = block_dirty_bitmap_lookup(action->node,
action->name,
NULL,
@@ -2122,16 +2044,13 @@ TransactionActionDrv block_dirty_bitmap_disable_drv = {
.clean = g_free,
};
-static void block_dirty_bitmap_disable_action(BlkActionState *common,
+static void block_dirty_bitmap_disable_action(BlockDirtyBitmap *action,
Transaction *tran, Error **errp)
{
- BlockDirtyBitmap *action;
- BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
- common, common);
+ BlockDirtyBitmapState *state = g_new0(BlockDirtyBitmapState, 1);
tran_add(tran, &block_dirty_bitmap_disable_drv, state);
- action = common->action->u.block_dirty_bitmap_disable.data;
state->bitmap = block_dirty_bitmap_lookup(action->node,
action->name,
NULL,
@@ -2163,17 +2082,13 @@ TransactionActionDrv block_dirty_bitmap_merge_drv = {
.clean = g_free,
};
-static void block_dirty_bitmap_merge_action(BlkActionState *common,
+static void block_dirty_bitmap_merge_action(BlockDirtyBitmapMerge *action,
Transaction *tran, Error **errp)
{
- BlockDirtyBitmapMerge *action;
- BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
- common, common);
+ BlockDirtyBitmapState *state = g_new0(BlockDirtyBitmapState, 1);
tran_add(tran, &block_dirty_bitmap_merge_drv, state);
- action = common->action->u.block_dirty_bitmap_merge.data;
-
state->bitmap = block_dirty_bitmap_merge(action->node, action->target,
action->bitmaps, &state->backup,
errp);
@@ -2187,16 +2102,13 @@ TransactionActionDrv block_dirty_bitmap_remove_drv = {
.clean = g_free,
};
-static void block_dirty_bitmap_remove_action(BlkActionState *common,
+static void block_dirty_bitmap_remove_action(BlockDirtyBitmap *action,
Transaction *tran, Error **errp)
{
- BlockDirtyBitmap *action;
- BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
- common, common);
+ BlockDirtyBitmapState *state = g_new0(BlockDirtyBitmapState, 1);
tran_add(tran, &block_dirty_bitmap_remove_drv, state);
- action = common->action->u.block_dirty_bitmap_remove.data;
state->bitmap = block_dirty_bitmap_remove(action->node, action->name,
false, &state->bs, errp);
@@ -2227,13 +2139,11 @@ static void block_dirty_bitmap_remove_commit(void *opaque)
static void abort_commit(void *opaque);
TransactionActionDrv abort_drv = {
.commit = abort_commit,
- .clean = g_free,
};
-static void abort_action(BlkActionState *common, Transaction *tran,
- Error **errp)
+static void abort_action(Transaction *tran, Error **errp)
{
- tran_add(tran, &abort_drv, common);
+ tran_add(tran, &abort_drv, NULL);
error_setg(errp, "Transaction aborted using Abort action");
}
@@ -2242,62 +2152,66 @@ static void abort_commit(void *opaque)
g_assert_not_reached(); /* this action never succeeds */
}
-static const BlkActionOps actions_map[] = {
- [TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT] = {
- .instance_size = sizeof(ExternalSnapshotState),
- .action = external_snapshot_action,
- },
- [TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_SYNC] = {
- .instance_size = sizeof(ExternalSnapshotState),
- .action = external_snapshot_action,
- },
- [TRANSACTION_ACTION_KIND_DRIVE_BACKUP] = {
- .instance_size = sizeof(DriveBackupState),
- .action = drive_backup_action,
- },
- [TRANSACTION_ACTION_KIND_BLOCKDEV_BACKUP] = {
- .instance_size = sizeof(BlockdevBackupState),
- .action = blockdev_backup_action,
- },
- [TRANSACTION_ACTION_KIND_ABORT] = {
- .instance_size = sizeof(BlkActionState),
- .action = abort_action,
- },
- [TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_INTERNAL_SYNC] = {
- .instance_size = sizeof(InternalSnapshotState),
- .action = internal_snapshot_action,
- },
- [TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_ADD] = {
- .instance_size = sizeof(BlockDirtyBitmapState),
- .action = block_dirty_bitmap_add_action,
- },
- [TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_CLEAR] = {
- .instance_size = sizeof(BlockDirtyBitmapState),
- .action = block_dirty_bitmap_clear_action,
- },
- [TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_ENABLE] = {
- .instance_size = sizeof(BlockDirtyBitmapState),
- .action = block_dirty_bitmap_enable_action,
- },
- [TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_DISABLE] = {
- .instance_size = sizeof(BlockDirtyBitmapState),
- .action = block_dirty_bitmap_disable_action,
- },
- [TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_MERGE] = {
- .instance_size = sizeof(BlockDirtyBitmapState),
- .action = block_dirty_bitmap_merge_action,
- },
- [TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_REMOVE] = {
- .instance_size = sizeof(BlockDirtyBitmapState),
- .action = block_dirty_bitmap_remove_action,
- },
- /* Where are transactions for MIRROR, COMMIT and STREAM?
+static void transaction_action(TransactionAction *act, JobTxn *block_job_txn,
+ Transaction *tran, Error **errp)
+{
+ switch (act->type) {
+ case TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT:
+ case TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_SYNC:
+ external_snapshot_action(act, tran, errp);
+ return;
+ case TRANSACTION_ACTION_KIND_DRIVE_BACKUP:
+ drive_backup_action(act->u.drive_backup.data,
+ block_job_txn, tran, errp);
+ return;
+ case TRANSACTION_ACTION_KIND_BLOCKDEV_BACKUP:
+ blockdev_backup_action(act->u.blockdev_backup.data,
+ block_job_txn, tran, errp);
+ return;
+ case TRANSACTION_ACTION_KIND_ABORT:
+ abort_action(tran, errp);
+ return;
+ case TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_INTERNAL_SYNC:
+ internal_snapshot_action(act->u.blockdev_snapshot_internal_sync.data,
+ tran, errp);
+ return;
+ case TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_ADD:
+ block_dirty_bitmap_add_action(act->u.block_dirty_bitmap_add.data,
+ tran, errp);
+ return;
+ case TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_CLEAR:
+ block_dirty_bitmap_clear_action(act->u.block_dirty_bitmap_clear.data,
+ tran, errp);
+ return;
+ case TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_ENABLE:
+ block_dirty_bitmap_enable_action(act->u.block_dirty_bitmap_enable.data,
+ tran, errp);
+ return;
+ case TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_DISABLE:
+ block_dirty_bitmap_disable_action(
+ act->u.block_dirty_bitmap_disable.data, tran, errp);
+ return;
+ case TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_MERGE:
+ block_dirty_bitmap_merge_action(act->u.block_dirty_bitmap_merge.data,
+ tran, errp);
+ return;
+ case TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_REMOVE:
+ block_dirty_bitmap_remove_action(act->u.block_dirty_bitmap_remove.data,
+ tran, errp);
+ return;
+ /*
+ * Where are transactions for MIRROR, COMMIT and STREAM?
* Although these blockjobs use transaction callbacks like the backup job,
* these jobs do not necessarily adhere to transaction semantics.
* These jobs may not fully undo all of their actions on abort, nor do they
* necessarily work in transactions with more than one job in them.
*/
-};
+ case TRANSACTION_ACTION_KIND__MAX:
+ default:
+ g_assert_not_reached();
+ };
+}
+
/*
* 'Atomic' group operations. The operations are performed as a set, and if
@@ -2346,21 +2260,7 @@ void qmp_transaction(TransactionActionList *actions,
/* We don't do anything in this loop that commits us to the operations */
for (act = actions; act; act = act->next) {
- TransactionAction *dev_info = act->value;
- const BlkActionOps *ops;
- BlkActionState *state;
-
- assert(dev_info->type < ARRAY_SIZE(actions_map));
-
- ops = &actions_map[dev_info->type];
- assert(ops->instance_size > 0);
-
- state = g_malloc0(ops->instance_size);
- state->ops = ops;
- state->action = dev_info;
- state->block_job_txn = block_job_txn;
-
- state->ops->action(state, tran, &local_err);
+ transaction_action(act->value, block_job_txn, tran, &local_err);
if (local_err) {
error_propagate(errp, local_err);
goto delete_and_fail;
--
2.34.1
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH v8 5/5] blockdev: qmp_transaction: drop extra generic layer
2023-04-21 11:53 ` [PATCH v8 5/5] blockdev: qmp_transaction: drop extra generic layer Vladimir Sementsov-Ogievskiy
@ 2023-05-10 11:47 ` Kevin Wolf
2023-05-10 13:37 ` Vladimir Sementsov-Ogievskiy
0 siblings, 1 reply; 12+ messages in thread
From: Kevin Wolf @ 2023-05-10 11:47 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy
Cc: qemu-block, qemu-devel, hreitz, den, alexander.ivanov
Am 21.04.2023 um 13:53 hat Vladimir Sementsov-Ogievskiy geschrieben:
> Let's simplify things:
>
> First, actions generally don't need access to common BlkActionState
> structure. The only exclusion are backup actions that need
> block_job_txn.
>
> Next, for transaction actions of Transaction API is more native to
> allocated state structure in the action itself.
>
> So, do the following transformation:
>
> 1. Let all actions be represented by a function with corresponding
> structure as arguments.
>
> 2. Instead of array-map marshaller, let's make a function, that calls
> corresponding action directly.
>
> 3. BlkActionOps and BlkActionState structures become unused
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> @@ -1973,11 +1910,9 @@ static void blockdev_backup_clean(void *opaque)
> }
>
> typedef struct BlockDirtyBitmapState {
> - BlkActionState common;
> BdrvDirtyBitmap *bitmap;
> BlockDriverState *bs;
> HBitmap *backup;
> - bool prepared;
> bool was_enabled;
> } BlockDirtyBitmapState;
>
> @@ -1987,17 +1922,14 @@ TransactionActionDrv block_dirty_bitmap_add_drv = {
> .clean = g_free,
> };
>
> -static void block_dirty_bitmap_add_action(BlkActionState *common,
> +static void block_dirty_bitmap_add_action(BlockDirtyBitmapAdd *action,
> Transaction *tran, Error **errp)
> {
> Error *local_err = NULL;
> - BlockDirtyBitmapAdd *action;
> - BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
> - common, common);
> + BlockDirtyBitmapState *state = g_new0(BlockDirtyBitmapState, 1);
>
> tran_add(tran, &block_dirty_bitmap_add_drv, state);
>
> - action = common->action->u.block_dirty_bitmap_add.data;
> /* 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,
> @@ -2006,7 +1938,8 @@ static void block_dirty_bitmap_add_action(BlkActionState *common,
> &local_err);
>
> if (!local_err) {
> - state->prepared = true;
> + state->bitmap = block_dirty_bitmap_lookup(action->node, action->name,
> + NULL, &error_abort);
> } else {
> error_propagate(errp, local_err);
> }
> @@ -2014,15 +1947,10 @@ static void block_dirty_bitmap_add_action(BlkActionState *common,
>
> static void block_dirty_bitmap_add_abort(void *opaque)
> {
> - BlockDirtyBitmapAdd *action;
> BlockDirtyBitmapState *state = opaque;
>
> - action = state->common.action->u.block_dirty_bitmap_add.data;
> - /* 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);
> + if (state->bitmap) {
> + bdrv_release_dirty_bitmap(state->bitmap);
> }
> }
So you're moving the lookup of the bitmap from .abort to .prepare (or
*_action now). I'm not entirely sure how this is related to the goal of
this specific patch. So the first question is, should it be separate?
The second question is if it is right. Moving it like this means we must
be sure that the bitmap can't be deleted between the lookup and the
.abort call. How can we guarantee this?
On the other hand, we already used &error_abort before, so the
assumption isn't actually new. Just the failure mode changes from
abort() to accessing a dangling pointer, which could be a bit worse.
The rest of the patch looks good.
Kevin
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v8 5/5] blockdev: qmp_transaction: drop extra generic layer
2023-05-10 11:47 ` Kevin Wolf
@ 2023-05-10 13:37 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 12+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2023-05-10 13:37 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-block, qemu-devel, hreitz, den, alexander.ivanov
On 10.05.23 14:47, Kevin Wolf wrote:
> Am 21.04.2023 um 13:53 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> Let's simplify things:
>>
>> First, actions generally don't need access to common BlkActionState
>> structure. The only exclusion are backup actions that need
>> block_job_txn.
>>
>> Next, for transaction actions of Transaction API is more native to
>> allocated state structure in the action itself.
>>
>> So, do the following transformation:
>>
>> 1. Let all actions be represented by a function with corresponding
>> structure as arguments.
>>
>> 2. Instead of array-map marshaller, let's make a function, that calls
>> corresponding action directly.
>>
>> 3. BlkActionOps and BlkActionState structures become unused
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>
>> @@ -1973,11 +1910,9 @@ static void blockdev_backup_clean(void *opaque)
>> }
>>
>> typedef struct BlockDirtyBitmapState {
>> - BlkActionState common;
>> BdrvDirtyBitmap *bitmap;
>> BlockDriverState *bs;
>> HBitmap *backup;
>> - bool prepared;
>> bool was_enabled;
>> } BlockDirtyBitmapState;
>>
>> @@ -1987,17 +1922,14 @@ TransactionActionDrv block_dirty_bitmap_add_drv = {
>> .clean = g_free,
>> };
>>
>> -static void block_dirty_bitmap_add_action(BlkActionState *common,
>> +static void block_dirty_bitmap_add_action(BlockDirtyBitmapAdd *action,
>> Transaction *tran, Error **errp)
>> {
>> Error *local_err = NULL;
>> - BlockDirtyBitmapAdd *action;
>> - BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
>> - common, common);
>> + BlockDirtyBitmapState *state = g_new0(BlockDirtyBitmapState, 1);
>>
>> tran_add(tran, &block_dirty_bitmap_add_drv, state);
>>
>> - action = common->action->u.block_dirty_bitmap_add.data;
>> /* 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,
>> @@ -2006,7 +1938,8 @@ static void block_dirty_bitmap_add_action(BlkActionState *common,
>> &local_err);
>>
>> if (!local_err) {
>> - state->prepared = true;
>> + state->bitmap = block_dirty_bitmap_lookup(action->node, action->name,
>> + NULL, &error_abort);
>> } else {
>> error_propagate(errp, local_err);
>> }
>> @@ -2014,15 +1947,10 @@ static void block_dirty_bitmap_add_action(BlkActionState *common,
>>
>> static void block_dirty_bitmap_add_abort(void *opaque)
>> {
>> - BlockDirtyBitmapAdd *action;
>> BlockDirtyBitmapState *state = opaque;
>>
>> - action = state->common.action->u.block_dirty_bitmap_add.data;
>> - /* 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);
>> + if (state->bitmap) {
>> + bdrv_release_dirty_bitmap(state->bitmap);
>> }
>> }
>
> So you're moving the lookup of the bitmap from .abort to .prepare (or
> *_action now). I'm not entirely sure how this is related to the goal of
> this specific patch. So the first question is, should it be separate?
>
> The second question is if it is right. Moving it like this means we must
> be sure that the bitmap can't be deleted between the lookup and the
> .abort call. How can we guarantee this?
>
> On the other hand, we already used &error_abort before, so the
> assumption isn't actually new. Just the failure mode changes from
> abort() to accessing a dangling pointer, which could be a bit worse.
Agree, that's a tricky place and better be handled separately.
>
> The rest of the patch looks good.
>
> Kevin
>
Thanks for reviewing, I'll resend soon.
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 12+ messages in thread