Re: [Qemu-devel] [PATCH 3/4] blockdev: acquire AioContext in QMP 'transaction' actions

[Max Reitz <mreitz@redhat.com>]

On 2014-11-21 at 11:48, Stefan Hajnoczi wrote:
> The transaction QMP command performs operations atomically on a group of
> drives.  This command needs to acquire AioContext in order to work
> safely when virtio-blk dataplane IOThreads are accessing drives.
>
> The transactional nature of the command means that actions are split
> into prepare, commit, abort, and clean functions.  Acquire the
> AioContext in prepare and don't release it until one of the other
> functions is called.  This prevents the IOThread from running the
> AioContext before the transaction has completed.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>   blockdev.c                      | 52 ++++++++++++++++++++++++++++++++++++++++-
>   hw/block/dataplane/virtio-blk.c |  2 ++
>   2 files changed, 53 insertions(+), 1 deletion(-)
>
> diff --git a/blockdev.c b/blockdev.c
> index 0d06983..90cb33d 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1193,6 +1193,7 @@ struct BlkTransactionState {
>   typedef struct InternalSnapshotState {
>       BlkTransactionState common;
>       BlockDriverState *bs;
> +    AioContext *aio_context;
>       QEMUSnapshotInfo sn;
>   } InternalSnapshotState;
>   
> @@ -1226,6 +1227,10 @@ static void internal_snapshot_prepare(BlkTransactionState *common,
>           return;
>       }
>   
> +    /* AioContext is released in .clean() */
> +    state->aio_context = bdrv_get_aio_context(bs);
> +    aio_context_acquire(state->aio_context);
> +
>       if (!bdrv_is_inserted(bs)) {
>           error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, device);
>           return;
> @@ -1303,11 +1308,22 @@ static void internal_snapshot_abort(BlkTransactionState *common)
>       }
>   }
>   
> +static void internal_snapshot_clean(BlkTransactionState *common)
> +{
> +    InternalSnapshotState *state = DO_UPCAST(InternalSnapshotState,
> +                                             common, common);
> +
> +    if (state->aio_context) {
> +        aio_context_release(state->aio_context);
> +    }
> +}
> +
>   /* external snapshot private data */
>   typedef struct ExternalSnapshotState {
>       BlkTransactionState common;
>       BlockDriverState *old_bs;
>       BlockDriverState *new_bs;
> +    AioContext *aio_context;
>   } ExternalSnapshotState;
>   
>   static void external_snapshot_prepare(BlkTransactionState *common,
> @@ -1374,6 +1390,10 @@ static void external_snapshot_prepare(BlkTransactionState *common,
>           return;
>       }
>   
> +    /* Acquire AioContext now so any threads operating on old_bs stop */

Any reason why this comment differs so much from the one for internal 
snapshots?

> +    state->aio_context = bdrv_get_aio_context(state->old_bs);
> +    aio_context_acquire(state->aio_context);
> +
>       if (!bdrv_is_inserted(state->old_bs)) {
>           error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, device);
>           return;
> @@ -1432,6 +1452,8 @@ static void external_snapshot_commit(BlkTransactionState *common)
>       ExternalSnapshotState *state =
>                                DO_UPCAST(ExternalSnapshotState, common, common);
>   
> +    bdrv_set_aio_context(state->new_bs, state->aio_context);
> +
>       /* This removes our old bs and adds the new bs */
>       bdrv_append(state->new_bs, state->old_bs);
>       /* We don't need (or want) to use the transactional
> @@ -1439,6 +1461,8 @@ static void external_snapshot_commit(BlkTransactionState *common)
>        * don't want to abort all of them if one of them fails the reopen */
>       bdrv_reopen(state->new_bs, state->new_bs->open_flags & ~BDRV_O_RDWR,
>                   NULL);
> +
> +    aio_context_release(state->aio_context);
>   }
>   
>   static void external_snapshot_abort(BlkTransactionState *common)
> @@ -1448,23 +1472,38 @@ static void external_snapshot_abort(BlkTransactionState *common)
>       if (state->new_bs) {
>           bdrv_unref(state->new_bs);
>       }
> +    if (state->aio_context) {
> +        aio_context_release(state->aio_context);
> +    }
>   }

It does work this way, but I would have gone for adding 
external_snapshot_clean() here, too.

>   typedef struct DriveBackupState {
>       BlkTransactionState common;
>       BlockDriverState *bs;
> +    AioContext *aio_context;
>       BlockJob *job;
>   } DriveBackupState;
>   
>   static void drive_backup_prepare(BlkTransactionState *common, Error **errp)
>   {
>       DriveBackupState *state = DO_UPCAST(DriveBackupState, common, common);
> +    BlockDriverState *bs;
>       DriveBackup *backup;
>       Error *local_err = NULL;
>   
>       assert(common->action->kind == TRANSACTION_ACTION_KIND_DRIVE_BACKUP);
>       backup = common->action->drive_backup;
>   
> +    bs = bdrv_find(backup->device);
> +    if (!bs) {
> +        error_set(errp, QERR_DEVICE_NOT_FOUND, backup->device);
> +        return;
> +    }
> +
> +    /* AioContext is released in .clean() */
> +    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,
> @@ -1478,7 +1517,7 @@ static void drive_backup_prepare(BlkTransactionState *common, Error **errp)
>           return;
>       }
>   
> -    state->bs = bdrv_find(backup->device);
> +    state->bs = bs;
>       state->job = state->bs->job;
>   }
>   
> @@ -1493,6 +1532,15 @@ static void drive_backup_abort(BlkTransactionState *common)
>       }
>   }
>   
> +static void drive_backup_clean(BlkTransactionState *common)
> +{
> +    DriveBackupState *state = DO_UPCAST(DriveBackupState, 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");
> @@ -1514,6 +1562,7 @@ static const BdrvActionOps actions[] = {
>           .instance_size = sizeof(DriveBackupState),
>           .prepare = drive_backup_prepare,
>           .abort = drive_backup_abort,
> +        .clean = drive_backup_clean,
>       },
>       [TRANSACTION_ACTION_KIND_ABORT] = {
>           .instance_size = sizeof(BlkTransactionState),
> @@ -1524,6 +1573,7 @@ static const BdrvActionOps actions[] = {
>           .instance_size = sizeof(InternalSnapshotState),
>           .prepare  = internal_snapshot_prepare,
>           .abort = internal_snapshot_abort,
> +        .clean = internal_snapshot_clean,
>       },
>   };
>   
> diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
> index 1222a37..25da32a 100644
> --- a/hw/block/dataplane/virtio-blk.c
> +++ b/hw/block/dataplane/virtio-blk.c
> @@ -198,6 +198,8 @@ void virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *conf,
>       blk_op_unblock(conf->conf.blk, BLOCK_OP_TYPE_DRIVE_DEL, s->blocker);
>       blk_op_unblock(conf->conf.blk, BLOCK_OP_TYPE_BACKUP_SOURCE, s->blocker);

BACKUP_SOURCE is already unblocked, good.

>       blk_op_unblock(conf->conf.blk, BLOCK_OP_TYPE_COMMIT, s->blocker);
> +    blk_op_unblock(conf->conf.blk, BLOCK_OP_TYPE_EXTERNAL_SNAPSHOT, s->blocker);
> +    blk_op_unblock(conf->conf.blk, BLOCK_OP_TYPE_INTERNAL_SNAPSHOT, s->blocker);
>       blk_op_unblock(conf->conf.blk, BLOCK_OP_TYPE_MIRROR, s->blocker);
>       blk_op_unblock(conf->conf.blk, BLOCK_OP_TYPE_STREAM, s->blocker);
>       blk_op_unblock(conf->conf.blk, BLOCK_OP_TYPE_REPLACE, s->blocker);

Reviewed-by: Max Reitz <mreitz@redhat.com>