[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 03/10] block: rename BlkTransactionState and
From: |
Fam Zheng |
Subject: |
Re: [Qemu-devel] [PATCH v2 03/10] block: rename BlkTransactionState and BdrvActionOps |
Date: |
Tue, 7 Jul 2015 14:50:58 +0800 |
User-agent: |
Mutt/1.5.23 (2014-03-12) |
On Mon, 07/06 15:24, Stefan Hajnoczi wrote:
> From: John Snow <address@hidden>
>
> 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 <address@hidden>
> Reviewed-by: Max Reitz <address@hidden>
> Reviewed-by: Stefan Hajnoczi <address@hidden>
> Signed-off-by: Stefan Hajnoczi <address@hidden>
Reviewed-by: Fam Zheng <address@hidden>
> ---
> blockdev.c | 116
> ++++++++++++++++++++++++++++++++++---------------------------
> 1 file changed, 65 insertions(+), 51 deletions(-)
>
> diff --git a/blockdev.c b/blockdev.c
> index a4d8f65..0ab8ad9 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1240,43 +1240,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;
> @@ -1372,7 +1386,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);
> @@ -1395,7 +1409,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);
> @@ -1407,13 +1421,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;
> @@ -1534,7 +1548,7 @@ static void
> external_snapshot_prepare(BlkTransactionState *common,
> }
> }
>
> -static void external_snapshot_commit(BlkTransactionState *common)
> +static void external_snapshot_commit(BlkActionState *common)
> {
> ExternalSnapshotState *state =
> DO_UPCAST(ExternalSnapshotState, common,
> common);
> @@ -1552,7 +1566,7 @@ static void
> external_snapshot_commit(BlkTransactionState *common)
> aio_context_release(state->aio_context);
> }
>
> -static void external_snapshot_abort(BlkTransactionState *common)
> +static void external_snapshot_abort(BlkActionState *common)
> {
> ExternalSnapshotState *state =
> DO_UPCAST(ExternalSnapshotState, common,
> common);
> @@ -1565,13 +1579,13 @@ static void
> external_snapshot_abort(BlkTransactionState *common)
> }
>
> typedef struct DriveBackupState {
> - BlkTransactionState common;
> + BlkActionState common;
> BlockDriverState *bs;
> AioContext *aio_context;
> BlockJob *job;
> } DriveBackupState;
>
> -static void drive_backup_prepare(BlkTransactionState *common, Error **errp)
> +static void drive_backup_prepare(BlkActionState *common, Error **errp)
> {
> DriveBackupState *state = DO_UPCAST(DriveBackupState, common, common);
> BlockDriverState *bs;
> @@ -1612,7 +1626,7 @@ static void drive_backup_prepare(BlkTransactionState
> *common, Error **errp)
> state->job = state->bs->job;
> }
>
> -static void drive_backup_abort(BlkTransactionState *common)
> +static void drive_backup_abort(BlkActionState *common)
> {
> DriveBackupState *state = DO_UPCAST(DriveBackupState, common, common);
> BlockDriverState *bs = state->bs;
> @@ -1623,7 +1637,7 @@ static void drive_backup_abort(BlkTransactionState
> *common)
> }
> }
>
> -static void drive_backup_clean(BlkTransactionState *common)
> +static void drive_backup_clean(BlkActionState *common)
> {
> DriveBackupState *state = DO_UPCAST(DriveBackupState, common, common);
>
> @@ -1633,13 +1647,13 @@ static void drive_backup_clean(BlkTransactionState
> *common)
> }
>
> typedef struct BlockdevBackupState {
> - BlkTransactionState common;
> + BlkActionState common;
> BlockDriverState *bs;
> BlockJob *job;
> AioContext *aio_context;
> } BlockdevBackupState;
>
> -static void blockdev_backup_prepare(BlkTransactionState *common, Error
> **errp)
> +static void blockdev_backup_prepare(BlkActionState *common, Error **errp)
> {
> BlockdevBackupState *state = DO_UPCAST(BlockdevBackupState, common,
> common);
> BlockdevBackup *backup;
> @@ -1688,7 +1702,7 @@ static void blockdev_backup_prepare(BlkTransactionState
> *common, Error **errp)
> state->job = state->bs->job;
> }
>
> -static void blockdev_backup_abort(BlkTransactionState *common)
> +static void blockdev_backup_abort(BlkActionState *common)
> {
> BlockdevBackupState *state = DO_UPCAST(BlockdevBackupState, common,
> common);
> BlockDriverState *bs = state->bs;
> @@ -1699,7 +1713,7 @@ static void blockdev_backup_abort(BlkTransactionState
> *common)
> }
> }
>
> -static void blockdev_backup_clean(BlkTransactionState *common)
> +static void blockdev_backup_clean(BlkActionState *common)
> {
> BlockdevBackupState *state = DO_UPCAST(BlockdevBackupState, common,
> common);
>
> @@ -1709,7 +1723,7 @@ static void blockdev_backup_clean(BlkTransactionState
> *common)
> }
>
> typedef struct BlockDirtyBitmapState {
> - BlkTransactionState common;
> + BlkActionState common;
> BdrvDirtyBitmap *bitmap;
> BlockDriverState *bs;
> AioContext *aio_context;
> @@ -1717,7 +1731,7 @@ typedef struct BlockDirtyBitmapState {
> bool prepared;
> } BlockDirtyBitmapState;
>
> -static void block_dirty_bitmap_add_prepare(BlkTransactionState *common,
> +static void block_dirty_bitmap_add_prepare(BlkActionState *common,
> Error **errp)
> {
> Error *local_err = NULL;
> @@ -1738,7 +1752,7 @@ static void
> block_dirty_bitmap_add_prepare(BlkTransactionState *common,
> }
> }
>
> -static void block_dirty_bitmap_add_abort(BlkTransactionState *common)
> +static void block_dirty_bitmap_add_abort(BlkActionState *common)
> {
> BlockDirtyBitmapAdd *action;
> BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
> @@ -1753,7 +1767,7 @@ static void
> block_dirty_bitmap_add_abort(BlkTransactionState *common)
> }
> }
>
> -static void block_dirty_bitmap_clear_prepare(BlkTransactionState *common,
> +static void block_dirty_bitmap_clear_prepare(BlkActionState *common,
> Error **errp)
> {
> BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
> @@ -1782,7 +1796,7 @@ static void
> block_dirty_bitmap_clear_prepare(BlkTransactionState *common,
> /* AioContext is released in .clean() */
> }
>
> -static void block_dirty_bitmap_clear_abort(BlkTransactionState *common)
> +static void block_dirty_bitmap_clear_abort(BlkActionState *common)
> {
> BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
> common, common);
> @@ -1790,7 +1804,7 @@ static void
> block_dirty_bitmap_clear_abort(BlkTransactionState *common)
> bdrv_undo_clear_dirty_bitmap(state->bitmap, state->backup);
> }
>
> -static void block_dirty_bitmap_clear_commit(BlkTransactionState *common)
> +static void block_dirty_bitmap_clear_commit(BlkActionState *common)
> {
> BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
> common, common);
> @@ -1798,7 +1812,7 @@ static void
> block_dirty_bitmap_clear_commit(BlkTransactionState *common)
> hbitmap_free(state->backup);
> }
>
> -static void block_dirty_bitmap_clear_clean(BlkTransactionState *common)
> +static void block_dirty_bitmap_clear_clean(BlkActionState *common)
> {
> BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
> common, common);
> @@ -1808,17 +1822,17 @@ static void
> block_dirty_bitmap_clear_clean(BlkTransactionState *common)
> }
> }
>
> -static void abort_prepare(BlkTransactionState *common, Error **errp)
> +static void abort_prepare(BlkActionState *common, Error **errp)
> {
> error_setg(errp, "Transaction aborted using Abort action");
> }
>
> -static void abort_commit(BlkTransactionState *common)
> +static void abort_commit(BlkActionState *common)
> {
> g_assert_not_reached(); /* this action never succeeds */
> }
>
> -static const BdrvActionOps actions[] = {
> +static const BlkActionOps actions[] = {
> [TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_SYNC] = {
> .instance_size = sizeof(ExternalSnapshotState),
> .prepare = external_snapshot_prepare,
> @@ -1838,7 +1852,7 @@ static const BdrvActionOps actions[] = {
> .clean = blockdev_backup_clean,
> },
> [TRANSACTION_ACTION_KIND_ABORT] = {
> - .instance_size = sizeof(BlkTransactionState),
> + .instance_size = sizeof(BlkActionState),
> .prepare = abort_prepare,
> .commit = abort_commit,
> },
> @@ -1869,10 +1883,10 @@ static const BdrvActionOps actions[] = {
> void qmp_transaction(TransactionActionList *dev_list, Error **errp)
> {
> TransactionActionList *dev_entry = dev_list;
> - BlkTransactionState *state, *next;
> + BlkActionState *state, *next;
> Error *local_err = NULL;
>
> - QSIMPLEQ_HEAD(snap_bdrv_states, BlkTransactionState) snap_bdrv_states;
> + QSIMPLEQ_HEAD(snap_bdrv_states, BlkActionState) snap_bdrv_states;
> QSIMPLEQ_INIT(&snap_bdrv_states);
>
> /* drain all i/o before any operations */
> @@ -1881,7 +1895,7 @@ void qmp_transaction(TransactionActionList *dev_list,
> Error **errp)
> /* We don't do anything in this loop that commits us to the operations */
> while (NULL != dev_entry) {
> TransactionAction *dev_info = NULL;
> - const BdrvActionOps *ops;
> + const BlkActionOps *ops;
>
> dev_info = dev_entry->value;
> dev_entry = dev_entry->next;
> --
> 2.4.3
>
- [Qemu-devel] [PATCH v2 00/10] block: incremental backup transactions using BlockJobTxn, Stefan Hajnoczi, 2015/07/06
- [Qemu-devel] [PATCH v2 02/10] iotests: add transactional incremental backup test, Stefan Hajnoczi, 2015/07/06
- [Qemu-devel] [PATCH v2 01/10] qapi: Add transaction support to block-dirty-bitmap operations, Stefan Hajnoczi, 2015/07/06
- [Qemu-devel] [PATCH v2 03/10] block: rename BlkTransactionState and BdrvActionOps, Stefan Hajnoczi, 2015/07/06
- Re: [Qemu-devel] [PATCH v2 03/10] block: rename BlkTransactionState and BdrvActionOps,
Fam Zheng <=
- [Qemu-devel] [PATCH v2 04/10] block: keep bitmap if incremental backup job is cancelled, Stefan Hajnoczi, 2015/07/06
- [Qemu-devel] [PATCH v2 06/10] blockdev: make BlockJobTxn available to qmp 'transaction', Stefan Hajnoczi, 2015/07/06
- [Qemu-devel] [PATCH v2 07/10] block/backup: support block job transactions, Stefan Hajnoczi, 2015/07/06
- [Qemu-devel] [PATCH v2 05/10] block: add block job transactions, Stefan Hajnoczi, 2015/07/06
[Qemu-devel] [PATCH v2 08/10] iotests: 124 - transactional failure test, Stefan Hajnoczi, 2015/07/06