[Qemu-devel] [PATCH RFC 00/16] block: Protect AIO context change with perm API

[Qemu-devel] [PATCH RFC 00/16] block: Protect AIO context change with perm API

[Fam Zheng]

Eject / change of scsi-cd on a virtio-scsi dataplane bus causes abort() because
the new BDS doesn't get proper bdrv_set_aio_context().

Store the AioContext in BB and do it in blk_insert_bs. That is done by
Vladimir's patch.

Other patches are to make sure such a bdrv_set_aio_context() doesn't interfere
with other BBs using other nodes from this graph.

RFC note:

Unfortunately, a use-after-free crash in iotests 030 appears since patch 7,
which I believe is a latent bug that bdrv_reopen is "reentered" in existing
code, rather than from this series:

> #4  0x0000561ab90425a7 in bdrv_reopen
> #5  0x0000561ab8e1d28e in stream_complete
> #6  0x0000561ab9048543 in block_job_defer_to_main_loop_bh
> #7  0x0000561ab91305bc in aio_bh_call
> #8  0x0000561ab9130659 in aio_bh_poll
> #9  0x0000561ab9135656 in aio_poll
> #10 0x0000561ab90a6cf5 in bdrv_flush
> #11 0x0000561ab904285a in bdrv_reopen_prepare
> #12 0x0000561ab90423f0 in bdrv_reopen_multiple
> #13 0x0000561ab90425ef in bdrv_reopen
> #14 0x0000561ab909fa49 in commit_active_start
> #15 0x0000561ab8dd6ffb in qmp_block_commit
> #16 0x0000561ab8ded485 in qmp_marshal_block_commit
> #17 0x0000561ab9123e6c in do_qmp_dispatch
> #18 0x0000561ab9123fa4 in qmp_dispatch
> #19 0x0000561ab8ca26b7 in handle_qmp_command

I have a fix that I'll post separately.

The last patches are an alternative to patch 7, to "workaround" this in a
really non-obvious way.

Fam

Fam Zheng (15):
  block: Define BLK_PERM_AIO_CONTEXT_CHANGE
  block-backend: Add blk_request_perm
  blockjob: Add BLK_PERM_AIO_CONTEXT_CHANGE shared perm on bs
  block: Propagate BLK_PERM_AIO_CONTEXT_CHANGE down the graph
  backup: Request BLK_PERM_AIO_CONTEXT_CHANGE on target
  backup: Do initial aio context move of target via BB interface
  mirror: Request aio context change permission on target
  commit: Allow aio context change on s->base
  mirror: Do initial aio context move of target via BB interface
  virtio-scsi: Request BLK_PERM_AIO_CONTEXT_CHANGE for dataplane
  virtio-blk: Request BLK_PERM_AIO_CONTEXT_CHANGE for dataplane
  nbd: Allow BLK_PERM_AIO_CONTEXT_CHANGE on BB
  block: Add perm assertion on blk_set_aio_context
  mirror: Lazily request aio context change permission on target
  Revert "mirror: Request aio context change permission on target"

Vladimir Sementsov-Ogievskiy (1):
  blk: fix aio context loss on media change

 block.c                         | 20 ++++++++++++--------
 block/backup.c                  | 11 ++++++++++-
 block/block-backend.c           | 23 +++++++++++++++++++++++
 block/commit.c                  |  3 ++-
 block/mirror.c                  | 10 ++++++++++
 block/vvfat.c                   |  2 +-
 blockdev.c                      | 18 ------------------
 blockjob.c                      |  3 +++
 hw/block/dataplane/virtio-blk.c | 15 +++++++++++----
 hw/scsi/virtio-scsi.c           |  4 ++++
 include/block/block.h           |  7 ++++++-
 include/sysemu/block-backend.h  |  1 +
 nbd/server.c                    |  6 ++++--
 13 files changed, 87 insertions(+), 36 deletions(-)

-- 
2.9.3

[Qemu-devel] [PATCH RFC 01/16] block: Define BLK_PERM_AIO_CONTEXT_CHANGE

[Fam Zheng]

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block.c               | 2 ++
 include/block/block.h | 7 ++++++-
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/block.c b/block.c
index 6e906ec..ae9327b 100644
--- a/block.c
+++ b/block.c
@@ -1547,6 +1547,8 @@ static char *bdrv_perm_names(uint64_t perm)
         { BLK_PERM_WRITE_UNCHANGED, "write unchanged" },
         { BLK_PERM_RESIZE,          "resize" },
         { BLK_PERM_GRAPH_MOD,       "change children" },
+        { BLK_PERM_AIO_CONTEXT_CHANGE,
+                                    "aio context change" },
         { 0, NULL }
     };
 
diff --git a/include/block/block.h b/include/block/block.h
index 5149260..989bdcc 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -221,7 +221,12 @@ enum {
      */
     BLK_PERM_GRAPH_MOD          = 0x10,
 
-    BLK_PERM_ALL                = 0x1f,
+    /**
+     * This permission is required to change the AioContext of this node.
+     */
+    BLK_PERM_AIO_CONTEXT_CHANGE = 0x20,
+
+    BLK_PERM_ALL                = 0x3f,
 };
 
 /* disk I/O throttling */
-- 
2.9.3

[Qemu-devel] [PATCH RFC 02/16] block-backend: Add blk_request_perm

[Fam Zheng]

This function tries to request, if not granted yet, for the given
permissions.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/block-backend.c          | 12 ++++++++++++
 include/sysemu/block-backend.h |  1 +
 2 files changed, 13 insertions(+)

diff --git a/block/block-backend.c b/block/block-backend.c
index 5742c09..5d17404 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -595,6 +595,18 @@ void blk_get_perm(BlockBackend *blk, uint64_t *perm, uint64_t *shared_perm)
     *shared_perm = blk->shared_perm;
 }
 
+int blk_request_perm(BlockBackend *blk, uint64_t perm, Error **errp)
+{
+    uint64_t blk_perm, shared_perm;
+
+    blk_get_perm(blk, &blk_perm, &shared_perm);
+    if ((blk_perm & perm) == perm) {
+        return 0;
+    }
+    blk_perm |= perm;
+    return blk_set_perm(blk, blk_perm, shared_perm, errp);
+}
+
 static int blk_do_attach_dev(BlockBackend *blk, void *dev)
 {
     if (blk->dev) {
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index 096c17f..016632e 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -108,6 +108,7 @@ bool bdrv_is_root_node(BlockDriverState *bs);
 int blk_set_perm(BlockBackend *blk, uint64_t perm, uint64_t shared_perm,
                  Error **errp);
 void blk_get_perm(BlockBackend *blk, uint64_t *perm, uint64_t *shared_perm);
+int blk_request_perm(BlockBackend *blk, uint64_t perm, Error **errp);
 
 void blk_set_allow_write_beyond_eof(BlockBackend *blk, bool allow);
 void blk_iostatus_enable(BlockBackend *blk);
-- 
2.9.3

[Qemu-devel] [PATCH RFC 03/16] blockjob: Add BLK_PERM_AIO_CONTEXT_CHANGE shared perm on bs

[Fam Zheng]

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 blockjob.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/blockjob.c b/blockjob.c
index 69126af..3fd84b7 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -197,6 +197,9 @@ void *block_job_create(const char *job_id, const BlockJobDriver *driver,
         }
     }
 
+    /* The notifier we'll register on @blk takes care of following context
+     * change, so permit it. */
+    shared_perm |= BLK_PERM_AIO_CONTEXT_CHANGE;
     blk = blk_new(perm, shared_perm);
     ret = blk_insert_bs(blk, bs, errp);
     if (ret < 0) {
-- 
2.9.3

[Qemu-devel] [PATCH RFC 04/16] block: Propagate BLK_PERM_AIO_CONTEXT_CHANGE down the graph

[Fam Zheng]

bdrv_set_aio_context can take care of children recursively, so it is
okay to pass down the perm.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block.c       | 18 ++++++++++--------
 block/vvfat.c |  2 +-
 2 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/block.c b/block.c
index ae9327b..0190087 100644
--- a/block.c
+++ b/block.c
@@ -1670,7 +1670,8 @@ int bdrv_child_try_set_perm(BdrvChild *c, uint64_t perm, uint64_t shared,
 #define DEFAULT_PERM_PASSTHROUGH (BLK_PERM_CONSISTENT_READ \
                                  | BLK_PERM_WRITE \
                                  | BLK_PERM_WRITE_UNCHANGED \
-                                 | BLK_PERM_RESIZE)
+                                 | BLK_PERM_RESIZE \
+                                 | BLK_PERM_AIO_CONTEXT_CHANGE)
 #define DEFAULT_PERM_UNCHANGED (BLK_PERM_ALL & ~DEFAULT_PERM_PASSTHROUGH)
 
 void bdrv_filter_default_perms(BlockDriverState *bs, BdrvChild *c,
@@ -1713,21 +1714,22 @@ void bdrv_format_default_perms(BlockDriverState *bs, BdrvChild *c,
         perm |= BLK_PERM_CONSISTENT_READ;
         shared &= ~(BLK_PERM_WRITE | BLK_PERM_RESIZE);
     } else {
-        /* We want consistent read from backing files if the parent needs it.
+        /* We want consistent read and aio context change from backing files if
+         * the parent needs it.
          * No other operations are performed on backing files. */
-        perm &= BLK_PERM_CONSISTENT_READ;
+        perm &= BLK_PERM_CONSISTENT_READ | BLK_PERM_AIO_CONTEXT_CHANGE;
 
-        /* If the parent can deal with changing data, we're okay with a
+        /* If the parent can deal with changing aio context, we're okay too;
+         * If the parent can deal with changing data, we're okay with a
          * writable and resizable backing file. */
         /* TODO Require !(perm & BLK_PERM_CONSISTENT_READ), too? */
+        shared &= BLK_PERM_AIO_CONTEXT_CHANGE | BLK_PERM_WRITE;
         if (shared & BLK_PERM_WRITE) {
-            shared = BLK_PERM_WRITE | BLK_PERM_RESIZE;
-        } else {
-            shared = 0;
+            shared |= BLK_PERM_WRITE | BLK_PERM_RESIZE;
         }
 
         shared |= BLK_PERM_CONSISTENT_READ | BLK_PERM_GRAPH_MOD |
-                  BLK_PERM_WRITE_UNCHANGED;
+                  BLK_PERM_WRITE_UNCHANGED | BLK_PERM_AIO_CONTEXT_CHANGE;
     }
 
     *nperm = perm;
diff --git a/block/vvfat.c b/block/vvfat.c
index af5153d..70ce452 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -3080,7 +3080,7 @@ static void vvfat_child_perm(BlockDriverState *bs, BdrvChild *c,
     if (c == s->qcow) {
         /* This is a private node, nobody should try to attach to it */
         *nperm = BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE;
-        *nshared = BLK_PERM_WRITE_UNCHANGED;
+        *nshared = BLK_PERM_WRITE_UNCHANGED | BLK_PERM_AIO_CONTEXT_CHANGE;
     } else {
         /* The backing file is there so 'commit' can use it. vvfat doesn't
          * access it in any way. */
-- 
2.9.3

[Qemu-devel] [PATCH RFC 05/16] backup: Request BLK_PERM_AIO_CONTEXT_CHANGE on target

[Fam Zheng]

What's done in the source's context change notifier is moving the
target's context to follow the new one, so we request this permission
here.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/backup.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/backup.c b/block/backup.c
index a4fb288..546c5c5 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -636,7 +636,7 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
     }
 
     /* The target must match the source in size, so no resize here either */
-    job->target = blk_new(BLK_PERM_WRITE,
+    job->target = blk_new(BLK_PERM_WRITE | BLK_PERM_AIO_CONTEXT_CHANGE,
                           BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE |
                           BLK_PERM_WRITE_UNCHANGED | BLK_PERM_GRAPH_MOD);
     ret = blk_insert_bs(job->target, target, errp);
-- 
2.9.3

[Qemu-devel] [PATCH RFC 06/16] backup: Do initial aio context move of target via BB interface

[Fam Zheng]

The old aio context check is hacky because when it was added we didn't
have the permission system to enforce a general rule. It only checks if
the target BDS has a BB, which is in fact insufficient because there may
be other BBs in the graph that cannot handle the aio context change.

Do this through blk_set_aio_context interface, in backup_job_create() as a
central place for both drive-backup and blockdev-backup, to take care of the
compatibility check.

Also the bdrv_set_aio_context in do_drive_backup could have been
conditional, to save a recursion when possible.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/backup.c |  9 +++++++++
 blockdev.c     | 14 --------------
 2 files changed, 9 insertions(+), 14 deletions(-)

diff --git a/block/backup.c b/block/backup.c
index 546c5c5..9136f91 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -564,6 +564,7 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
     BlockDriverInfo bdi;
     BackupBlockJob *job = NULL;
     int ret;
+    AioContext *aio_context, *target_context;
 
     assert(bs);
     assert(target);
@@ -644,6 +645,14 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
         goto error;
     }
 
+    aio_context = bdrv_get_aio_context(bs);
+    target_context = bdrv_get_aio_context(target);
+    if (target_context != aio_context) {
+        aio_context_acquire(target_context);
+        blk_set_aio_context(job->target, aio_context);
+        aio_context_release(target_context);
+    }
+
     job->on_source_error = on_source_error;
     job->on_target_error = on_target_error;
     job->sync_mode = sync_mode;
diff --git a/blockdev.c b/blockdev.c
index c5b2c2c..5d89a9a 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3259,8 +3259,6 @@ static BlockJob *do_drive_backup(DriveBackup *backup, BlockJobTxn *txn,
         goto out;
     }
 
-    bdrv_set_aio_context(target_bs, aio_context);
-
     if (backup->has_bitmap) {
         bmap = bdrv_find_dirty_bitmap(bs, backup->bitmap);
         if (!bmap) {
@@ -3337,18 +3335,6 @@ BlockJob *do_blockdev_backup(BlockdevBackup *backup, BlockJobTxn *txn,
     if (!target_bs) {
         goto out;
     }
-
-    if (bdrv_get_aio_context(target_bs) != aio_context) {
-        if (!bdrv_has_blk(target_bs)) {
-            /* The target BDS is not attached, we can safely move it to another
-             * AioContext. */
-            bdrv_set_aio_context(target_bs, aio_context);
-        } else {
-            error_setg(errp, "Target is attached to a different thread from "
-                             "source.");
-            goto out;
-        }
-    }
     job = backup_job_create(backup->job_id, bs, target_bs, backup->speed,
                             backup->sync, NULL, backup->compress,
                             backup->on_source_error, backup->on_target_error,
-- 
2.9.3

[Qemu-devel] [PATCH RFC 07/16] mirror: Request aio context change permission on target

[Fam Zheng]

What's done in the source's context change notifier is moving the
target's context to follow the new one, so we request this permission
here.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/mirror.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/block/mirror.c b/block/mirror.c
index ca4baa5..7101b11 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1185,6 +1185,7 @@ static void mirror_start_job(const char *job_id, BlockDriverState *bs,
     target_is_backing = bdrv_chain_contains(bs, target);
     target_graph_mod = (backing_mode != MIRROR_LEAVE_BACKING_CHAIN);
     s->target = blk_new(BLK_PERM_WRITE | BLK_PERM_RESIZE |
+                        BLK_PERM_AIO_CONTEXT_CHANGE |
                         (target_graph_mod ? BLK_PERM_GRAPH_MOD : 0),
                         BLK_PERM_WRITE_UNCHANGED |
                         (target_is_backing ? BLK_PERM_CONSISTENT_READ |
-- 
2.9.3

[Qemu-devel] [PATCH RFC 08/16] commit: Allow aio context change on s->base

[Fam Zheng]

The block job has the aio context change notifier, we should allow it
here as well.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/commit.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/block/commit.c b/block/commit.c
index 2832482..a1805c2 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -388,7 +388,8 @@ void commit_start(const char *job_id, BlockDriverState *bs,
                       | BLK_PERM_RESIZE,
                       BLK_PERM_CONSISTENT_READ
                       | BLK_PERM_GRAPH_MOD
-                      | BLK_PERM_WRITE_UNCHANGED);
+                      | BLK_PERM_WRITE_UNCHANGED
+                      | BLK_PERM_AIO_CONTEXT_CHANGE);
     ret = blk_insert_bs(s->base, base, errp);
     if (ret < 0) {
         goto fail;
-- 
2.9.3

[Qemu-devel] [PATCH RFC 09/16] mirror: Do initial aio context move of target via BB interface

[Fam Zheng]

While blockdev-backup tried to verify before moving target's aio context, the
same is missing for blockdev-mirror. Now that we have the right interface, fix
this as well.

As a bounus, the aio context move is now conditional, which avoids unnecessary
operations in bdrv_set_aio_context.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/mirror.c | 9 +++++++++
 blockdev.c     | 4 ----
 2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index 7101b11..ed26e8c 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1116,6 +1116,7 @@ static void mirror_start_job(const char *job_id, BlockDriverState *bs,
                              bool is_none_mode, BlockDriverState *base,
                              bool auto_complete, const char *filter_node_name)
 {
+    AioContext *aio_context, *target_context;
     MirrorBlockJob *s;
     BlockDriverState *mirror_top_bs;
     bool target_graph_mod;
@@ -1196,6 +1197,14 @@ static void mirror_start_job(const char *job_id, BlockDriverState *bs,
         goto fail;
     }
 
+    aio_context = bdrv_get_aio_context(bs);
+    target_context = bdrv_get_aio_context(target);
+    if (target_context != aio_context) {
+        aio_context_acquire(target_context);
+        blk_set_aio_context(s->target, aio_context);
+        aio_context_release(target_context);
+    }
+
     s->replaces = g_strdup(replaces);
     s->on_source_error = on_source_error;
     s->on_target_error = on_target_error;
diff --git a/blockdev.c b/blockdev.c
index 5d89a9a..5298a93 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3556,8 +3556,6 @@ void qmp_drive_mirror(DriveMirror *arg, Error **errp)
         goto out;
     }
 
-    bdrv_set_aio_context(target_bs, aio_context);
-
     blockdev_mirror_common(arg->has_job_id ? arg->job_id : NULL, bs, target_bs,
                            arg->has_replaces, arg->replaces, arg->sync,
                            backing_mode, arg->has_speed, arg->speed,
@@ -3608,8 +3606,6 @@ void qmp_blockdev_mirror(bool has_job_id, const char *job_id,
     aio_context = bdrv_get_aio_context(bs);
     aio_context_acquire(aio_context);
 
-    bdrv_set_aio_context(target_bs, aio_context);
-
     blockdev_mirror_common(has_job_id ? job_id : NULL, bs, target_bs,
                            has_replaces, replaces, sync, backing_mode,
                            has_speed, speed,
-- 
2.9.3

[Qemu-devel] [PATCH RFC 10/16] virtio-scsi: Request BLK_PERM_AIO_CONTEXT_CHANGE for dataplane

[Fam Zheng]

blk_set_aio_context is audited by perm API, so follow the protocol and
request for permission first.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 hw/scsi/virtio-scsi.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index 1dbc4bc..6a48356 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -811,6 +811,10 @@ static void virtio_scsi_hotplug(HotplugHandler *hotplug_dev, DeviceState *dev,
             return;
         }
         virtio_scsi_acquire(s);
+        if (!blk_request_perm(sd->conf.blk, BLK_PERM_AIO_CONTEXT_CHANGE, errp)) {
+            virtio_scsi_release(s);
+            return;
+        }
         blk_set_aio_context(sd->conf.blk, s->ctx);
         virtio_scsi_release(s);
 
-- 
2.9.3

[Qemu-devel] [PATCH RFC 11/16] virtio-blk: Request BLK_PERM_AIO_CONTEXT_CHANGE for dataplane

[Fam Zheng]

blk_set_aio_context is audited by perm API, so follow the protocol and
request for permission first.

Previously the return code in error path is hardcoded as -EPERM, but
returning 'r' is more meaningful here both for the new error and
existing errors.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 hw/block/dataplane/virtio-blk.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index 5556f0e..8f2ff2df 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -168,6 +168,7 @@ int virtio_blk_data_plane_start(VirtIODevice *vdev)
     unsigned i;
     unsigned nvqs = s->conf->num_queues;
     int r;
+    Error *local_err = NULL;
 
     if (vblk->dataplane_started || s->starting) {
         return 0;
@@ -175,12 +176,18 @@ int virtio_blk_data_plane_start(VirtIODevice *vdev)
 
     s->starting = true;
 
+    r = blk_request_perm(s->conf->conf.blk, BLK_PERM_AIO_CONTEXT_CHANGE,
+                         &local_err);
+    if (r) {
+        error_report_err(local_err);
+        goto fail;
+    }
     /* Set up guest notifier (irq) */
     r = k->set_guest_notifiers(qbus->parent, nvqs, true);
     if (r != 0) {
         fprintf(stderr, "virtio-blk failed to set guest notifier (%d), "
                 "ensure -enable-kvm is set\n", r);
-        goto fail_guest_notifiers;
+        goto fail;
     }
 
     /* Set up virtqueue notify */
@@ -191,7 +198,7 @@ int virtio_blk_data_plane_start(VirtIODevice *vdev)
             while (i--) {
                 virtio_bus_set_host_notifier(VIRTIO_BUS(qbus), i, false);
             }
-            goto fail_guest_notifiers;
+            goto fail;
         }
     }
 
@@ -219,11 +226,11 @@ int virtio_blk_data_plane_start(VirtIODevice *vdev)
     aio_context_release(s->ctx);
     return 0;
 
-  fail_guest_notifiers:
+fail:
     vblk->dataplane_disabled = true;
     s->starting = false;
     vblk->dataplane_started = true;
-    return -ENOSYS;
+    return r;
 }
 
 /* Context: QEMU global mutex held */
-- 
2.9.3

[Qemu-devel] [PATCH RFC 12/16] blk: fix aio context loss on media change

[Fam Zheng]

From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

If we have separate iothread for cdrom, we lose connection to it on
qmp_blockdev_change_medium, as aio_context is on bds which is dropped
and switched with new one.

As an example result, after such media change we have crash on
virtio_scsi_ctx_check: Assertion `blk_get_aio_context(d->conf.blk) == s->ctx' failed.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/block-backend.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/block/block-backend.c b/block/block-backend.c
index 5d17404..ec8747f 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -65,6 +65,8 @@ struct BlockBackend {
     bool allow_write_beyond_eof;
 
     NotifierList remove_bs_notifiers, insert_bs_notifiers;
+
+    AioContext *aio_context;
 };
 
 typedef struct BlockBackendAIOCB {
@@ -559,6 +561,10 @@ int blk_insert_bs(BlockBackend *blk, BlockDriverState *bs, Error **errp)
     }
     bdrv_ref(bs);
 
+    if (blk->aio_context != NULL) {
+        bdrv_set_aio_context(bs, blk->aio_context);
+    }
+
     notifier_list_notify(&blk->insert_bs_notifiers, blk);
     if (blk->public.throttle_state) {
         throttle_timers_attach_aio_context(
@@ -1619,6 +1625,7 @@ void blk_set_aio_context(BlockBackend *blk, AioContext *new_context)
 {
     BlockDriverState *bs = blk_bs(blk);
 
+    blk->aio_context = new_context;
     if (bs) {
         if (blk->public.throttle_state) {
             throttle_timers_detach_aio_context(&blk->public.throttle_timers);
-- 
2.9.3

[Qemu-devel] [PATCH RFC 13/16] nbd: Allow BLK_PERM_AIO_CONTEXT_CHANGE on BB

[Fam Zheng]

This is safe because of the aio context notifier we'll register on this
node. So allow it.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 nbd/server.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/nbd/server.c b/nbd/server.c
index 924a1fe..a8f58fb 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -900,8 +900,10 @@ NBDExport *nbd_export_new(BlockDriverState *bs, off_t dev_offset, off_t size,
     if ((nbdflags & NBD_FLAG_READ_ONLY) == 0) {
         perm |= BLK_PERM_WRITE;
     }
-    blk = blk_new(perm, BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE_UNCHANGED |
-                        BLK_PERM_WRITE | BLK_PERM_GRAPH_MOD);
+    blk = blk_new(perm,
+                  BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE_UNCHANGED |
+                  BLK_PERM_WRITE | BLK_PERM_GRAPH_MOD |
+                  BLK_PERM_AIO_CONTEXT_CHANGE);
     ret = blk_insert_bs(blk, bs, errp);
     if (ret < 0) {
         goto fail;
-- 
2.9.3

[Qemu-devel] [PATCH RFC 14/16] block: Add perm assertion on blk_set_aio_context

[Fam Zheng]

Now that all BB users comply with the BLK_PERM_AIO_CONTEXT_CHANGE
rule, we can assert it.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/block-backend.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/block/block-backend.c b/block/block-backend.c
index ec8747f..8284b83 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1623,8 +1623,12 @@ static AioContext *blk_aiocb_get_aio_context(BlockAIOCB *acb)
 
 void blk_set_aio_context(BlockBackend *blk, AioContext *new_context)
 {
+    uint64_t perm, shared_perm;
     BlockDriverState *bs = blk_bs(blk);
 
+    blk_get_perm(blk, &perm, &shared_perm);
+    assert(perm & BLK_PERM_AIO_CONTEXT_CHANGE);
+
     blk->aio_context = new_context;
     if (bs) {
         if (blk->public.throttle_state) {
-- 
2.9.3

[Qemu-devel] [PATCH RFC 15/16] mirror: Lazily request aio context change permission on target

[Fam Zheng]

What's done in the source's context change notifier is moving the
target's context to follow the new one, so we request this permission
here.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/mirror.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/block/mirror.c b/block/mirror.c
index ed26e8c..168cf60 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -983,6 +983,7 @@ static void mirror_attached_aio_context(BlockJob *job, AioContext *new_context)
 {
     MirrorBlockJob *s = container_of(job, MirrorBlockJob, common);
 
+    blk_request_perm(s->target, BLK_PERM_AIO_CONTEXT_CHANGE, &error_abort);
     blk_set_aio_context(s->target, new_context);
 }
 
-- 
2.9.3

[Qemu-devel] [PATCH RFC 16/16] Revert "mirror: Request aio context change permission on target"

[Fam Zheng]

This reverts commit bee8490438adfb30785cd5256019e8cba9fb3a07.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/mirror.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/block/mirror.c b/block/mirror.c
index 168cf60..240da19 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1187,7 +1187,6 @@ static void mirror_start_job(const char *job_id, BlockDriverState *bs,
     target_is_backing = bdrv_chain_contains(bs, target);
     target_graph_mod = (backing_mode != MIRROR_LEAVE_BACKING_CHAIN);
     s->target = blk_new(BLK_PERM_WRITE | BLK_PERM_RESIZE |
-                        BLK_PERM_AIO_CONTEXT_CHANGE |
                         (target_graph_mod ? BLK_PERM_GRAPH_MOD : 0),
                         BLK_PERM_WRITE_UNCHANGED |
                         (target_is_backing ? BLK_PERM_CONSISTENT_READ |
-- 
2.9.3

Re: [Qemu-devel] [Qemu-block] [PATCH RFC 04/16] block: Propagate BLK_PERM_AIO_CONTEXT_CHANGE down the graph

[Stefan Hajnoczi]

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

On Tue, Mar 21, 2017 at 11:16:23AM +0800, Fam Zheng wrote:
> @@ -1713,21 +1714,22 @@ void bdrv_format_default_perms(BlockDriverState *bs, BdrvChild *c,
>          perm |= BLK_PERM_CONSISTENT_READ;
>          shared &= ~(BLK_PERM_WRITE | BLK_PERM_RESIZE);
>      } else {
> -        /* We want consistent read from backing files if the parent needs it.
> +        /* We want consistent read and aio context change from backing files if
> +         * the parent needs it.
>           * No other operations are performed on backing files. */
> -        perm &= BLK_PERM_CONSISTENT_READ;
> +        perm &= BLK_PERM_CONSISTENT_READ | BLK_PERM_AIO_CONTEXT_CHANGE;
>  
> -        /* If the parent can deal with changing data, we're okay with a
> +        /* If the parent can deal with changing aio context, we're okay too;
> +         * If the parent can deal with changing data, we're okay with a
>           * writable and resizable backing file. */
>          /* TODO Require !(perm & BLK_PERM_CONSISTENT_READ), too? */
> +        shared &= BLK_PERM_AIO_CONTEXT_CHANGE | BLK_PERM_WRITE;
>          if (shared & BLK_PERM_WRITE) {
> -            shared = BLK_PERM_WRITE | BLK_PERM_RESIZE;
> -        } else {
> -            shared = 0;
> +            shared |= BLK_PERM_WRITE | BLK_PERM_RESIZE;

We already have BLK_PERM_WRITE so we're just adding BLK_PERM_RESIZE.
The following is clearer:

  shared |= BLK_PERM_RESIZE;

>          }
>  
>          shared |= BLK_PERM_CONSISTENT_READ | BLK_PERM_GRAPH_MOD |
> -                  BLK_PERM_WRITE_UNCHANGED;
> +                  BLK_PERM_WRITE_UNCHANGED | BLK_PERM_AIO_CONTEXT_CHANGE;

Why was shared &= BLK_PERM_AIO_CONTEXT_CHANGE necessary above if we
unconditionally OR it here?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH RFC 00/16] block: Protect AIO context change with perm API

[Stefan Hajnoczi]

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

On Tue, Mar 21, 2017 at 11:16:19AM +0800, Fam Zheng wrote:
> Eject / change of scsi-cd on a virtio-scsi dataplane bus causes abort() because
> the new BDS doesn't get proper bdrv_set_aio_context().
> 
> Store the AioContext in BB and do it in blk_insert_bs. That is done by
> Vladimir's patch.
> 
> Other patches are to make sure such a bdrv_set_aio_context() doesn't interfere
> with other BBs using other nodes from this graph.
> 
> RFC note:
> 
> Unfortunately, a use-after-free crash in iotests 030 appears since patch 7,
> which I believe is a latent bug that bdrv_reopen is "reentered" in existing
> code, rather than from this series:
> 
> > #4  0x0000561ab90425a7 in bdrv_reopen
> > #5  0x0000561ab8e1d28e in stream_complete
> > #6  0x0000561ab9048543 in block_job_defer_to_main_loop_bh
> > #7  0x0000561ab91305bc in aio_bh_call
> > #8  0x0000561ab9130659 in aio_bh_poll
> > #9  0x0000561ab9135656 in aio_poll
> > #10 0x0000561ab90a6cf5 in bdrv_flush
> > #11 0x0000561ab904285a in bdrv_reopen_prepare
> > #12 0x0000561ab90423f0 in bdrv_reopen_multiple
> > #13 0x0000561ab90425ef in bdrv_reopen
> > #14 0x0000561ab909fa49 in commit_active_start
> > #15 0x0000561ab8dd6ffb in qmp_block_commit
> > #16 0x0000561ab8ded485 in qmp_marshal_block_commit
> > #17 0x0000561ab9123e6c in do_qmp_dispatch
> > #18 0x0000561ab9123fa4 in qmp_dispatch
> > #19 0x0000561ab8ca26b7 in handle_qmp_command
> 
> I have a fix that I'll post separately.
> 
> The last patches are an alternative to patch 7, to "workaround" this in a
> really non-obvious way.

Are there qemu-iotests that cover both success and failure scenarios for
the new permission?

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH RFC 00/16] block: Protect AIO context change with perm API

[Fam Zheng]

On Mon, 04/10 10:04, Stefan Hajnoczi wrote:
> On Tue, Mar 21, 2017 at 11:16:19AM +0800, Fam Zheng wrote:
> > Eject / change of scsi-cd on a virtio-scsi dataplane bus causes abort() because
> > the new BDS doesn't get proper bdrv_set_aio_context().
> > 
> > Store the AioContext in BB and do it in blk_insert_bs. That is done by
> > Vladimir's patch.
> > 
> > Other patches are to make sure such a bdrv_set_aio_context() doesn't interfere
> > with other BBs using other nodes from this graph.
> > 
> > RFC note:
> > 
> > Unfortunately, a use-after-free crash in iotests 030 appears since patch 7,
> > which I believe is a latent bug that bdrv_reopen is "reentered" in existing
> > code, rather than from this series:
> > 
> > > #4  0x0000561ab90425a7 in bdrv_reopen
> > > #5  0x0000561ab8e1d28e in stream_complete
> > > #6  0x0000561ab9048543 in block_job_defer_to_main_loop_bh
> > > #7  0x0000561ab91305bc in aio_bh_call
> > > #8  0x0000561ab9130659 in aio_bh_poll
> > > #9  0x0000561ab9135656 in aio_poll
> > > #10 0x0000561ab90a6cf5 in bdrv_flush
> > > #11 0x0000561ab904285a in bdrv_reopen_prepare
> > > #12 0x0000561ab90423f0 in bdrv_reopen_multiple
> > > #13 0x0000561ab90425ef in bdrv_reopen
> > > #14 0x0000561ab909fa49 in commit_active_start
> > > #15 0x0000561ab8dd6ffb in qmp_block_commit
> > > #16 0x0000561ab8ded485 in qmp_marshal_block_commit
> > > #17 0x0000561ab9123e6c in do_qmp_dispatch
> > > #18 0x0000561ab9123fa4 in qmp_dispatch
> > > #19 0x0000561ab8ca26b7 in handle_qmp_command
> > 
> > I have a fix that I'll post separately.
> > 
> > The last patches are an alternative to patch 7, to "workaround" this in a
> > really non-obvious way.
> 
> Are there qemu-iotests that cover both success and failure scenarios for
> the new permission?

No, I'll add them.

Fam

Re: [Qemu-devel] [Qemu-block] [PATCH RFC 04/16] block: Propagate BLK_PERM_AIO_CONTEXT_CHANGE down the graph

[Fam Zheng]

On Mon, 04/10 09:57, Stefan Hajnoczi wrote:
> On Tue, Mar 21, 2017 at 11:16:23AM +0800, Fam Zheng wrote:
> > @@ -1713,21 +1714,22 @@ void bdrv_format_default_perms(BlockDriverState *bs, BdrvChild *c,
> >          perm |= BLK_PERM_CONSISTENT_READ;
> >          shared &= ~(BLK_PERM_WRITE | BLK_PERM_RESIZE);
> >      } else {
> > -        /* We want consistent read from backing files if the parent needs it.
> > +        /* We want consistent read and aio context change from backing files if
> > +         * the parent needs it.
> >           * No other operations are performed on backing files. */
> > -        perm &= BLK_PERM_CONSISTENT_READ;
> > +        perm &= BLK_PERM_CONSISTENT_READ | BLK_PERM_AIO_CONTEXT_CHANGE;
> >  
> > -        /* If the parent can deal with changing data, we're okay with a
> > +        /* If the parent can deal with changing aio context, we're okay too;
> > +         * If the parent can deal with changing data, we're okay with a
> >           * writable and resizable backing file. */
> >          /* TODO Require !(perm & BLK_PERM_CONSISTENT_READ), too? */
> > +        shared &= BLK_PERM_AIO_CONTEXT_CHANGE | BLK_PERM_WRITE;
> >          if (shared & BLK_PERM_WRITE) {
> > -            shared = BLK_PERM_WRITE | BLK_PERM_RESIZE;
> > -        } else {
> > -            shared = 0;
> > +            shared |= BLK_PERM_WRITE | BLK_PERM_RESIZE;
> 
> We already have BLK_PERM_WRITE so we're just adding BLK_PERM_RESIZE.
> The following is clearer:
> 
>   shared |= BLK_PERM_RESIZE;
> 
> >          }
> >  
> >          shared |= BLK_PERM_CONSISTENT_READ | BLK_PERM_GRAPH_MOD |
> > -                  BLK_PERM_WRITE_UNCHANGED;
> > +                  BLK_PERM_WRITE_UNCHANGED | BLK_PERM_AIO_CONTEXT_CHANGE;
> 
> Why was shared &= BLK_PERM_AIO_CONTEXT_CHANGE necessary above if we
> unconditionally OR it here?

It's redundant. Will fix both.

Fam