[Qemu-devel] [PATCH for-2.3 0/4] blockdev: support dataplane in remaining QMP commands

[Qemu-devel] [PATCH for-2.3 0/4] blockdev: support dataplane in remaining QMP commands

[Stefan Hajnoczi]

This patch series adds virtio-blk dataplane support for the following QMP
commands:

 * eject
 * change
 * change-backing-file
 * block_passwd
 * blockdev-snapshot-delete-internal-sync

This requires acquiring and releasing the BlockDriverState's AioContext so that
the IOThread does not run while the monitor command is executing.

Monitor commands that use AioContext can be unblocked in the virtio-blk
dataplane op blockers list.

After this series only the transaction QMP command remains to be converted.

Stefan Hajnoczi (4):
  blockdev: acquire AioContext in blockdev-snapshot-delete-internal-sync
  blockdev: check for BLOCK_OP_TYPE_INTERNAL_SNAPSHOT_DELETE
  blockdev: acquire AioContext in eject, change, and block_passwd
  blockdev: acquire AioContext in change-backing-file

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

-- 
2.1.0

[Qemu-devel] [PATCH for-2.3 1/4] blockdev: acquire AioContext in blockdev-snapshot-delete-internal-sync

[Stefan Hajnoczi]

Add dataplane support to the blockdev-snapshot-delete-internal-sync QMP
command.  By acquiring the AioContext we avoid race conditions with the
dataplane thread which may also be accessing the BlockDriverState.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 blockdev.c                      | 16 +++++++++++++---
 hw/block/dataplane/virtio-blk.c |  2 ++
 2 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 57910b8..fb9a005 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1105,6 +1105,7 @@ SnapshotInfo *qmp_blockdev_snapshot_delete_internal_sync(const char *device,
                                                          Error **errp)
 {
     BlockDriverState *bs = bdrv_find(device);
+    AioContext *aio_context;
     QEMUSnapshotInfo sn;
     Error *local_err = NULL;
     SnapshotInfo *info = NULL;
@@ -1128,25 +1129,30 @@ SnapshotInfo *qmp_blockdev_snapshot_delete_internal_sync(const char *device,
         return NULL;
     }
 
+    aio_context = bdrv_get_aio_context(bs);
+    aio_context_acquire(aio_context);
+
     ret = bdrv_snapshot_find_by_id_and_name(bs, id, name, &sn, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
-        return NULL;
+        goto out_aio_context;
     }
     if (!ret) {
         error_setg(errp,
                    "Snapshot with id '%s' and name '%s' does not exist on "
                    "device '%s'",
                    STR_OR_NULL(id), STR_OR_NULL(name), device);
-        return NULL;
+        goto out_aio_context;
     }
 
     bdrv_snapshot_delete(bs, id, name, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
-        return NULL;
+        goto out_aio_context;
     }
 
+    aio_context_release(aio_context);
+
     info = g_new0(SnapshotInfo, 1);
     info->id = g_strdup(sn.id_str);
     info->name = g_strdup(sn.name);
@@ -1157,6 +1163,10 @@ SnapshotInfo *qmp_blockdev_snapshot_delete_internal_sync(const char *device,
     info->vm_clock_sec = sn.vm_clock_nsec / 1000000000;
 
     return info;
+
+out_aio_context:
+    aio_context_release(aio_context);
+    return NULL;
 }
 
 /* New and old BlockDriverState structs for group snapshots */
diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index 1222a37..5ab64c5 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_INTERNAL_SNAPSHOT_DELETE,
+                   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 for-2.3 1/4] blockdev: acquire AioContext in blockdev-snapshot-delete-internal-sync

[Max Reitz]

On 2014-11-19 at 15:19, Stefan Hajnoczi wrote:
> Add dataplane support to the blockdev-snapshot-delete-internal-sync QMP
> command.  By acquiring the AioContext we avoid race conditions with the
> dataplane thread which may also be accessing the BlockDriverState.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>   blockdev.c                      | 16 +++++++++++++---
>   hw/block/dataplane/virtio-blk.c |  2 ++
>   2 files changed, 15 insertions(+), 3 deletions(-)

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

[Qemu-devel] [PATCH for-2.3 2/4] blockdev: check for BLOCK_OP_TYPE_INTERNAL_SNAPSHOT_DELETE

[Stefan Hajnoczi]

The BLOCK_OP_TYPE_INTERNAL_SNAPSHOT_DELETE op blocker exists but was
never used!  Let's fix that so snapshot delete 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 fb9a005..a7f1e09 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1132,6 +1132,10 @@ SnapshotInfo *qmp_blockdev_snapshot_delete_internal_sync(const char *device,
     aio_context = bdrv_get_aio_context(bs);
     aio_context_acquire(aio_context);
 
+    if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_INTERNAL_SNAPSHOT_DELETE, errp)) {
+        goto out_aio_context;
+    }
+
     ret = bdrv_snapshot_find_by_id_and_name(bs, id, name, &sn, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
-- 
2.1.0

Re: [Qemu-devel] [PATCH for-2.3 2/4] blockdev: check for BLOCK_OP_TYPE_INTERNAL_SNAPSHOT_DELETE

[Max Reitz]

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

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

[Qemu-devel] [PATCH for-2.3 3/4] blockdev: acquire AioContext in eject, change, and block_passwd

[Stefan Hajnoczi]

By acquiring the AioContext we avoid race conditions with the dataplane
thread which may also be accessing the BlockDriverState.

Fix up eject, change, and block_passwd in a single patch because
qmp_eject() and qmp_change_blockdev() both call eject_device().  Also
fix block_passwd while we're tackling a command that takes a block
encryption password.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 blockdev.c                      | 36 +++++++++++++++++++++++++++++-------
 hw/block/dataplane/virtio-blk.c |  1 +
 2 files changed, 30 insertions(+), 7 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index a7f1e09..7bf88d4 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1617,14 +1617,18 @@ exit:
 static void eject_device(BlockBackend *blk, int force, Error **errp)
 {
     BlockDriverState *bs = blk_bs(blk);
+    AioContext *aio_context;
+
+    aio_context = bdrv_get_aio_context(bs);
+    aio_context_acquire(aio_context);
 
     if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_EJECT, errp)) {
-        return;
+        goto out;
     }
     if (!blk_dev_has_removable_media(blk)) {
         error_setg(errp, "Device '%s' is not removable",
                    bdrv_get_device_name(bs));
-        return;
+        goto out;
     }
 
     if (blk_dev_is_medium_locked(blk) && !blk_dev_is_tray_open(blk)) {
@@ -1632,11 +1636,14 @@ static void eject_device(BlockBackend *blk, int force, Error **errp)
         if (!force) {
             error_setg(errp, "Device '%s' is locked",
                        bdrv_get_device_name(bs));
-            return;
+            goto out;
         }
     }
 
     bdrv_close(bs);
+
+out:
+    aio_context_release(aio_context);
 }
 
 void qmp_eject(const char *device, bool has_force, bool force, Error **errp)
@@ -1658,6 +1665,7 @@ void qmp_block_passwd(bool has_device, const char *device,
 {
     Error *local_err = NULL;
     BlockDriverState *bs;
+    AioContext *aio_context;
     int err;
 
     bs = bdrv_lookup_bs(has_device ? device : NULL,
@@ -1668,16 +1676,23 @@ void qmp_block_passwd(bool has_device, const char *device,
         return;
     }
 
+    aio_context = bdrv_get_aio_context(bs);
+    aio_context_acquire(aio_context);
+
     err = bdrv_set_key(bs, password);
     if (err == -EINVAL) {
         error_set(errp, QERR_DEVICE_NOT_ENCRYPTED, bdrv_get_device_name(bs));
-        return;
+        goto out;
     } else if (err < 0) {
         error_set(errp, QERR_INVALID_PASSWORD);
-        return;
+        goto out;
     }
+
+out:
+    aio_context_release(aio_context);
 }
 
+/* Assumes AioContext is held */
 static void qmp_bdrv_open_encrypted(BlockDriverState *bs, const char *filename,
                                     int bdrv_flags, BlockDriver *drv,
                                     const char *password, Error **errp)
@@ -1710,6 +1725,7 @@ void qmp_change_blockdev(const char *device, const char *filename,
 {
     BlockBackend *blk;
     BlockDriverState *bs;
+    AioContext *aio_context;
     BlockDriver *drv = NULL;
     int bdrv_flags;
     Error *err = NULL;
@@ -1721,24 +1737,30 @@ void qmp_change_blockdev(const char *device, const char *filename,
     }
     bs = blk_bs(blk);
 
+    aio_context = bdrv_get_aio_context(bs);
+    aio_context_acquire(aio_context);
+
     if (format) {
         drv = bdrv_find_whitelisted_format(format, bs->read_only);
         if (!drv) {
             error_set(errp, QERR_INVALID_BLOCK_FORMAT, format);
-            return;
+            goto out;
         }
     }
 
     eject_device(blk, 0, &err);
     if (err) {
         error_propagate(errp, err);
-        return;
+        goto out;
     }
 
     bdrv_flags = bdrv_is_read_only(bs) ? 0 : BDRV_O_RDWR;
     bdrv_flags |= bdrv_is_snapshot(bs) ? BDRV_O_SNAPSHOT : 0;
 
     qmp_bdrv_open_encrypted(bs, filename, bdrv_flags, drv, NULL, errp);
+
+out:
+    aio_context_release(aio_context);
 }
 
 /* throttling disk I/O limits */
diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index 5ab64c5..2548113 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -198,6 +198,7 @@ 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_EJECT, s->blocker);
     blk_op_unblock(conf->conf.blk, BLOCK_OP_TYPE_INTERNAL_SNAPSHOT_DELETE,
                    s->blocker);
     blk_op_unblock(conf->conf.blk, BLOCK_OP_TYPE_MIRROR, s->blocker);
-- 
2.1.0

Re: [Qemu-devel] [PATCH for-2.3 3/4] blockdev: acquire AioContext in eject, change, and block_passwd

[Max Reitz]

On 2014-11-19 at 15:19, Stefan Hajnoczi wrote:
> By acquiring the AioContext we avoid race conditions with the dataplane
> thread which may also be accessing the BlockDriverState.
>
> Fix up eject, change, and block_passwd in a single patch because
> qmp_eject() and qmp_change_blockdev() both call eject_device().  Also
> fix block_passwd while we're tackling a command that takes a block
> encryption password.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>   blockdev.c                      | 36 +++++++++++++++++++++++++++++-------
>   hw/block/dataplane/virtio-blk.c |  1 +
>   2 files changed, 30 insertions(+), 7 deletions(-)

You could've used blk_get_aio_context() for acquiring the AioContext in 
qmp_eject() instead of in eject_device() (and qmp_change_blockdev(), 
which is the other caller of eject_device(), holds the context anyway).

Anyway:

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

[Qemu-devel] [PATCH for-2.3 4/4] blockdev: acquire AioContext in change-backing-file

[Stefan Hajnoczi]

Add dataplane support to the change-backing-file QMP commands.  By
acquiring the AioContext we avoid race conditions with the dataplane
thread which may also be accessing the BlockDriverState.

Note that this command operates on both bs and a node in its chain
(image_bs).  The bdrv_chain_contains(bs, image_bs) check guarantees that
bs and image_bs are in the same AioContext.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 blockdev.c                      | 19 +++++++++++++------
 hw/block/dataplane/virtio-blk.c |  1 +
 2 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 7bf88d4..a52f205 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2584,6 +2584,7 @@ void qmp_change_backing_file(const char *device,
                              Error **errp)
 {
     BlockDriverState *bs = NULL;
+    AioContext *aio_context;
     BlockDriverState *image_bs = NULL;
     Error *local_err = NULL;
     bool ro;
@@ -2597,34 +2598,37 @@ void qmp_change_backing_file(const char *device,
         return;
     }
 
+    aio_context = bdrv_get_aio_context(bs);
+    aio_context_acquire(aio_context);
+
     image_bs = bdrv_lookup_bs(NULL, image_node_name, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
-        return;
+        goto out;
     }
 
     if (!image_bs) {
         error_setg(errp, "image file not found");
-        return;
+        goto out;
     }
 
     if (bdrv_find_base(image_bs) == image_bs) {
         error_setg(errp, "not allowing backing file change on an image "
                          "without a backing file");
-        return;
+        goto out;
     }
 
     /* even though we are not necessarily operating on bs, we need it to
      * determine if block ops are currently prohibited on the chain */
     if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_CHANGE, errp)) {
-        return;
+        goto out;
     }
 
     /* final sanity check */
     if (!bdrv_chain_contains(bs, image_bs)) {
         error_setg(errp, "'%s' and image file are not in the same chain",
                    device);
-        return;
+        goto out;
     }
 
     /* if not r/w, reopen to make r/w */
@@ -2635,7 +2639,7 @@ void qmp_change_backing_file(const char *device,
         bdrv_reopen(image_bs, open_flags | BDRV_O_RDWR, &local_err);
         if (local_err) {
             error_propagate(errp, local_err);
-            return;
+            goto out;
         }
     }
 
@@ -2655,6 +2659,9 @@ void qmp_change_backing_file(const char *device,
             error_propagate(errp, local_err); /* will preserve prior errp */
         }
     }
+
+out:
+    aio_context_release(aio_context);
 }
 
 void qmp_blockdev_add(BlockdevOptions *options, Error **errp)
diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index 2548113..90ab27e 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -197,6 +197,7 @@ void virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *conf,
     blk_op_unblock(conf->conf.blk, BLOCK_OP_TYPE_RESIZE, s->blocker);
     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_CHANGE, s->blocker);
     blk_op_unblock(conf->conf.blk, BLOCK_OP_TYPE_COMMIT, s->blocker);
     blk_op_unblock(conf->conf.blk, BLOCK_OP_TYPE_EJECT, s->blocker);
     blk_op_unblock(conf->conf.blk, BLOCK_OP_TYPE_INTERNAL_SNAPSHOT_DELETE,
-- 
2.1.0

Re: [Qemu-devel] [PATCH for-2.3 4/4] blockdev: acquire AioContext in change-backing-file

[Max Reitz]

On 2014-11-19 at 15:19, Stefan Hajnoczi wrote:
> Add dataplane support to the change-backing-file QMP commands.  By
> acquiring the AioContext we avoid race conditions with the dataplane
> thread which may also be accessing the BlockDriverState.
>
> Note that this command operates on both bs and a node in its chain
> (image_bs).  The bdrv_chain_contains(bs, image_bs) check guarantees that
> bs and image_bs are in the same AioContext.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>   blockdev.c                      | 19 +++++++++++++------
>   hw/block/dataplane/virtio-blk.c |  1 +
>   2 files changed, 14 insertions(+), 6 deletions(-)

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

Re: [Qemu-devel] [PATCH for-2.3 0/4] blockdev: support dataplane in remaining QMP commands

[Max Reitz]

On 2014-11-19 at 15:19, Stefan Hajnoczi wrote:
> This patch series adds virtio-blk dataplane support for the following QMP
> commands:
>
>   * eject
>   * change
>   * change-backing-file
>   * block_passwd
>   * blockdev-snapshot-delete-internal-sync
>
> This requires acquiring and releasing the BlockDriverState's AioContext so that
> the IOThread does not run while the monitor command is executing.
>
> Monitor commands that use AioContext can be unblocked in the virtio-blk
> dataplane op blockers list.
>
> After this series only the transaction QMP command remains to be converted.
>
> Stefan Hajnoczi (4):
>    blockdev: acquire AioContext in blockdev-snapshot-delete-internal-sync
>    blockdev: check for BLOCK_OP_TYPE_INTERNAL_SNAPSHOT_DELETE
>    blockdev: acquire AioContext in eject, change, and block_passwd
>    blockdev: acquire AioContext in change-backing-file
>
>   blockdev.c                      | 75 ++++++++++++++++++++++++++++++++---------
>   hw/block/dataplane/virtio-blk.c |  4 +++
>   2 files changed, 63 insertions(+), 16 deletions(-)

Thanks, applied to my block-next tree:

https://github.com/XanClic/qemu/commits/block-next