[Qemu-devel] [PATCH 0/4] blockdev: support dataplane in QMP 'transaction' command

[Qemu-devel] [PATCH 0/4] blockdev: support dataplane in QMP 'transaction' command

[Stefan Hajnoczi]

These patches make the QMP 'transaction' command work with virtio-blk
dataplane.  Each 'transaction' action must take care to acquire AioContext
around BlockDriverState accesses.  Once that protection is in place we can
unblock the op blockers for these commands.

The meat is in Patch 3.

Patches 1 & 2 are minor cleanups.
Patch 4 protects the external snapshot command (oops, we forgot!).

Stefan Hajnoczi (4):
  blockdev: update outdated qmp_transaction() comments
  blockdev: drop unnecessary DriveBackupState field assignment
  blockdev: acquire AioContext in QMP 'transaction' actions
  blockdev: check for BLOCK_OP_TYPE_EXTERNAL_SNAPSHOT

 blockdev.c                      | 74 +++++++++++++++++++++++++++++++++--------
 hw/block/dataplane/virtio-blk.c |  2 ++
 2 files changed, 63 insertions(+), 13 deletions(-)

-- 
2.1.0

[Qemu-devel] [PATCH 1/4] blockdev: update outdated qmp_transaction() comments

[Stefan Hajnoczi]

Originally the transaction QMP command was just for taking snapshots.
The command became more general when drive-backup and abort were added.

It is more accurate to say the command is about performing operations on
an atomic group than to say it is about snapshots.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 blockdev.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 57910b8..778509b 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1159,7 +1159,7 @@ SnapshotInfo *qmp_blockdev_snapshot_delete_internal_sync(const char *device,
     return info;
 }
 
-/* New and old BlockDriverState structs for group snapshots */
+/* New and old BlockDriverState structs for atomic group operations */
 
 typedef struct BlkTransactionState BlkTransactionState;
 
@@ -1530,9 +1530,8 @@ static const BdrvActionOps actions[] = {
 };
 
 /*
- * 'Atomic' group snapshots.  The snapshots are taken as a set, and if any fail
- *  then we do not pivot any of the devices in the group, and abandon the
- *  snapshots
+ * 'Atomic' group operations.  The operations are performed as a set, and if
+ * any fail then we roll back all operations in the group.
  */
 void qmp_transaction(TransactionActionList *dev_list, Error **errp)
 {
@@ -1543,10 +1542,10 @@ void qmp_transaction(TransactionActionList *dev_list, Error **errp)
     QSIMPLEQ_HEAD(snap_bdrv_states, BlkTransactionState) snap_bdrv_states;
     QSIMPLEQ_INIT(&snap_bdrv_states);
 
-    /* drain all i/o before any snapshots */
+    /* drain all i/o before any operations */
     bdrv_drain_all();
 
-    /* We don't do anything in this loop that commits us to the snapshot */
+    /* 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;
@@ -1581,10 +1580,7 @@ void qmp_transaction(TransactionActionList *dev_list, Error **errp)
     goto exit;
 
 delete_and_fail:
-    /*
-    * failure, and it is all-or-none; abandon each new bs, and keep using
-    * the original bs for all images
-    */
+    /* failure, and it is all-or-none; roll back all operations */
     QSIMPLEQ_FOREACH(state, &snap_bdrv_states, entry) {
         if (state->ops->abort) {
             state->ops->abort(state);
-- 
2.1.0

Re: [Qemu-devel] [PATCH 1/4] blockdev: update outdated qmp_transaction() comments

[Max Reitz]

On 2014-11-21 at 11:48, Stefan Hajnoczi wrote:
> Originally the transaction QMP command was just for taking snapshots.
> The command became more general when drive-backup and abort were added.
>
> It is more accurate to say the command is about performing operations on
> an atomic group than to say it is about snapshots.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>   blockdev.c | 16 ++++++----------
>   1 file changed, 6 insertions(+), 10 deletions(-)

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

Re: [Qemu-devel] [PATCH 1/4] blockdev: update outdated qmp_transaction() comments

[Max Reitz]

On 2014-11-21 at 14:22, Max Reitz wrote:
> On 2014-11-21 at 11:48, Stefan Hajnoczi wrote:
>> Originally the transaction QMP command was just for taking snapshots.
>> The command became more general when drive-backup and abort were added.
>>
>> It is more accurate to say the command is about performing operations on
>> an atomic group than to say it is about snapshots.
>>
>> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>> ---
>>   blockdev.c | 16 ++++++----------
>>   1 file changed, 6 insertions(+), 10 deletions(-)
>
> Reviewed-by: Max Reitz <mreitz@redhat.com>

Oh, I forgot to note: The patch is fine and it applies cleanly to 
master. But there is a contextual conflict when applying it on top of 
your previous series.

Max

[Qemu-devel] [PATCH 2/4] blockdev: drop unnecessary DriveBackupState field assignment

[Stefan Hajnoczi]

drive_backup_prepare() assigns DriveBackupState fields to NULL in the
error path.  This is unnecessary because the DriveBackupState is
allocated using g_malloc0() and other functions like
external_snapshot_prepare() already rely on this.

Do not explicitly assign fields to NULL so that the error path is
concise and does not require modification when fields are added to
DriveBackupState.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 blockdev.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 778509b..0d06983 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1475,8 +1475,6 @@ static void drive_backup_prepare(BlkTransactionState *common, Error **errp)
                      &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
-        state->bs = NULL;
-        state->job = NULL;
         return;
     }
 
-- 
2.1.0

Re: [Qemu-devel] [PATCH 2/4] blockdev: drop unnecessary DriveBackupState field assignment

[Max Reitz]

On 2014-11-21 at 11:48, Stefan Hajnoczi wrote:
> drive_backup_prepare() assigns DriveBackupState fields to NULL in the
> error path.  This is unnecessary because the DriveBackupState is
> allocated using g_malloc0() and other functions like
> external_snapshot_prepare() already rely on this.
>
> Do not explicitly assign fields to NULL so that the error path is
> concise and does not require modification when fields are added to
> DriveBackupState.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>   blockdev.c | 2 --
>   1 file changed, 2 deletions(-)

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

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

[Stefan Hajnoczi]

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 */
+    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);
+    }
 }
 
 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);
     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);
-- 
2.1.0

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

[Max Reitz]

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>

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

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 4560 bytes --]

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 <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.

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.

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

[Qemu-devel] [PATCH 4/4] blockdev: check for BLOCK_OP_TYPE_EXTERNAL_SNAPSHOT

[Stefan Hajnoczi]

The BLOCK_OP_TYPE_EXTERNAL_SNAPSHOT op blocker exists but was never
used!  Let's fix that so external snapshot can be blocked.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 blockdev.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/blockdev.c b/blockdev.c
index 90cb33d..d63bb7f 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1236,6 +1236,10 @@ static void internal_snapshot_prepare(BlkTransactionState *common,
         return;
     }
 
+    if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_INTERNAL_SNAPSHOT, errp)) {
+        return;
+    }
+
     if (bdrv_is_read_only(bs)) {
         error_set(errp, QERR_DEVICE_IS_READ_ONLY, device);
         return;
-- 
2.1.0

Re: [Qemu-devel] [PATCH 4/4] blockdev: check for BLOCK_OP_TYPE_EXTERNAL_SNAPSHOT

[Paolo Bonzini]

On 21/11/2014 11:49, Stefan Hajnoczi wrote:
> The BLOCK_OP_TYPE_EXTERNAL_SNAPSHOT op blocker exists but was never
> used!  Let's fix that so external snapshot can be blocked.

The patch is about internal snapshots tho. :)

Paolo

> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  blockdev.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/blockdev.c b/blockdev.c
> index 90cb33d..d63bb7f 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1236,6 +1236,10 @@ static void internal_snapshot_prepare(BlkTransactionState *common,
>          return;
>      }
>  
> +    if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_INTERNAL_SNAPSHOT, errp)) {
> +        return;
> +    }
> +
>      if (bdrv_is_read_only(bs)) {
>          error_set(errp, QERR_DEVICE_IS_READ_ONLY, device);
>          return;
> 

Re: [Qemu-devel] [PATCH 4/4] blockdev: check for BLOCK_OP_TYPE_EXTERNAL_SNAPSHOT

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 375 bytes --]

On Fri, Nov 21, 2014 at 12:58:56PM +0100, Paolo Bonzini wrote:
> 
> 
> On 21/11/2014 11:49, Stefan Hajnoczi wrote:
> > The BLOCK_OP_TYPE_EXTERNAL_SNAPSHOT op blocker exists but was never
> > used!  Let's fix that so external snapshot can be blocked.
> 
> The patch is about internal snapshots tho. :)

Oops, will fix the typo when merging or if a respin is needed.

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 4/4] blockdev: check for BLOCK_OP_TYPE_EXTERNAL_SNAPSHOT

[Max Reitz]

On 2014-11-21 at 11:49, Stefan Hajnoczi wrote:
> The BLOCK_OP_TYPE_EXTERNAL_SNAPSHOT op blocker exists but was never
> used!  Let's fix that so external snapshot can be blocked.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>   blockdev.c | 4 ++++
>   1 file changed, 4 insertions(+)

With commit message and title fixed (there are three instances of 
/external/i):

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

Re: [Qemu-devel] [PATCH 0/4] blockdev: support dataplane in QMP 'transaction' command

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1067 bytes --]

On Fri, Nov 21, 2014 at 10:48:56AM +0000, Stefan Hajnoczi wrote:
> These patches make the QMP 'transaction' command work with virtio-blk
> dataplane.  Each 'transaction' action must take care to acquire AioContext
> around BlockDriverState accesses.  Once that protection is in place we can
> unblock the op blockers for these commands.
> 
> The meat is in Patch 3.
> 
> Patches 1 & 2 are minor cleanups.
> Patch 4 protects the external snapshot command (oops, we forgot!).
> 
> Stefan Hajnoczi (4):
>   blockdev: update outdated qmp_transaction() comments
>   blockdev: drop unnecessary DriveBackupState field assignment
>   blockdev: acquire AioContext in QMP 'transaction' actions
>   blockdev: check for BLOCK_OP_TYPE_EXTERNAL_SNAPSHOT
> 
>  blockdev.c                      | 74 +++++++++++++++++++++++++++++++++--------
>  hw/block/dataplane/virtio-blk.c |  2 ++
>  2 files changed, 63 insertions(+), 13 deletions(-)
> 
> -- 
> 2.1.0
> 
> 

Applied to my block-next tree:
https://github.com/stefanha/qemu/commits/block-next

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]