From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42248) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xroch-00038l-3x for qemu-devel@nongnu.org; Fri, 21 Nov 2014 08:51:49 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XrocY-0003XZ-JZ for qemu-devel@nongnu.org; Fri, 21 Nov 2014 08:51:43 -0500 Received: from mx1.redhat.com ([209.132.183.28]:59918) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XrocY-0003XT-CF for qemu-devel@nongnu.org; Fri, 21 Nov 2014 08:51:34 -0500 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id sALDpXXm008635 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL) for ; Fri, 21 Nov 2014 08:51:33 -0500 Message-ID: <546F4362.3030608@redhat.com> Date: Fri, 21 Nov 2014 14:51:30 +0100 From: Max Reitz MIME-Version: 1.0 References: <1416566940-4430-1-git-send-email-stefanha@redhat.com> <1416566940-4430-4-git-send-email-stefanha@redhat.com> In-Reply-To: <1416566940-4430-4-git-send-email-stefanha@redhat.com> Content-Type: text/plain; charset=iso-8859-15; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 3/4] blockdev: acquire AioContext in QMP 'transaction' actions List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi , qemu-devel@nongnu.org Cc: Kevin Wolf , Paolo Bonzini 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 > --- > 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