On Fri, Nov 21, 2014 at 02:51:30PM +0100, Max Reitz wrote:
> 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 <stefa...@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.This is why the comment differs :). Since we already have these two functions I didn't add a separate .clean(). It could be done either way but unless anyone feels strongly about it, I'd like to do it like this.
pgpyyXWqRco_E.pgp
Description: PGP signature
