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

[Stefan Hajnoczi]

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 <address@hidden>
> >---
> >  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.

pgpJxT9ee9IPR.pgp
Description: PGP signature