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

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

[Fam Zheng]

v3: Move blk_set_aio_context to the front of mirror_start_job to avoid
    accessing target without acquiring its aio context. [Stefan]
    Use error_free_or_abort in test code. [Stefan]

v2: Address Stefan's comments:

    - Clean up redundancy in bdrv_format_default_perms change.
    - Add a test case to check both success/failure cases.
      A failure case is not possible at user interface level because of other
      checks we have, so write a unit test in tests/test-blk-perm.c.

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.

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
  blockjob: Allow aio context change on intermediate nodes
  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
  tests: Add test case for BLK_PERM_AIO_CONTEXT_CHANGE

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

 block.c                         | 12 ++++++---
 block/backup.c                  | 11 +++++++-
 block/block-backend.c           | 22 +++++++++++++++
 block/commit.c                  |  6 +++--
 block/mirror.c                  | 56 +++++++++++++++++++++++---------------
 block/stream.c                  |  3 ++-
 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 +++--
 tests/Makefile.include          |  2 ++
 tests/test-blk-perm.c           | 59 +++++++++++++++++++++++++++++++++++++++++
 16 files changed, 171 insertions(+), 56 deletions(-)
 create mode 100644 tests/test-blk-perm.c

-- 
2.9.4

[Qemu-devel] [PATCH v3 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 50ba264..d98662f 100644
--- a/block.c
+++ b/block.c
@@ -1649,6 +1649,8 @@ 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 9b355e9..017d6c8 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -225,7 +225,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,
 };
 
 char *bdrv_perm_names(uint64_t perm);
-- 
2.9.4

[Qemu-devel] [PATCH v3 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 f3a6008..5492f64 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -654,6 +654,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 840ad61..fc23a9e 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -116,6 +116,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.4

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

[Stefan Hajnoczi]

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

On Wed, May 24, 2017 at 10:52:21AM +0800, Fam Zheng wrote:
> 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(+)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

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

[Qemu-devel] [PATCH v3 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 6e48932..b20fb86 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -214,6 +214,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.4

[Qemu-devel] [PATCH v3 04/16] blockjob: Allow aio context change on intermediate nodes

[Fam Zheng]

The intermediate nodes do work with aio context change, so allow that
operations.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/commit.c | 3 ++-
 block/mirror.c | 3 ++-
 block/stream.c | 3 ++-
 3 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/block/commit.c b/block/commit.c
index 76a0d98..e2ee0ff 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -365,7 +365,8 @@ void commit_start(const char *job_id, BlockDriverState *bs,
          * for its backing file). The other options would be a second filter
          * driver above s->base. */
         ret = block_job_add_bdrv(&s->common, "intermediate node", iter, 0,
-                                 BLK_PERM_WRITE_UNCHANGED | BLK_PERM_WRITE,
+                                 BLK_PERM_WRITE_UNCHANGED | BLK_PERM_WRITE |
+                                 BLK_PERM_AIO_CONTEXT_CHANGE,
                                  errp);
         if (ret < 0) {
             goto fail;
diff --git a/block/mirror.c b/block/mirror.c
index e86f8f8..03e82eb 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1232,7 +1232,8 @@ static void mirror_start_job(const char *job_id, BlockDriverState *bs,
              * also blocked for its backing file). The other options would be a
              * second filter driver above s->base (== target). */
             ret = block_job_add_bdrv(&s->common, "intermediate node", iter, 0,
-                                     BLK_PERM_WRITE_UNCHANGED | BLK_PERM_WRITE,
+                                     BLK_PERM_WRITE_UNCHANGED | BLK_PERM_WRITE |
+                                     BLK_PERM_AIO_CONTEXT_CHANGE,
                                      errp);
             if (ret < 0) {
                 goto fail;
diff --git a/block/stream.c b/block/stream.c
index 0113710..2fab1f4 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -265,7 +265,8 @@ void stream_start(const char *job_id, BlockDriverState *bs,
      * and resizes. */
     for (iter = backing_bs(bs); iter && iter != base; iter = backing_bs(iter)) {
         block_job_add_bdrv(&s->common, "intermediate node", iter, 0,
-                           BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE_UNCHANGED,
+                           BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE_UNCHANGED |
+                           BLK_PERM_AIO_CONTEXT_CHANGE,
                            &error_abort);
     }
 
-- 
2.9.4

[Qemu-devel] [PATCH v3 05/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       | 10 ++++++----
 block/vvfat.c |  2 +-
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/block.c b/block.c
index d98662f..1e5eae7 100644
--- a/block.c
+++ b/block.c
@@ -1772,7 +1772,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,
@@ -1815,9 +1816,10 @@ 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
          * writable and resizable backing file. */
@@ -1829,7 +1831,7 @@ void bdrv_format_default_perms(BlockDriverState *bs, BdrvChild *c,
         }
 
         shared |= BLK_PERM_CONSISTENT_READ | BLK_PERM_GRAPH_MOD |
-                  BLK_PERM_WRITE_UNCHANGED;
+                  BLK_PERM_WRITE_UNCHANGED | BLK_PERM_AIO_CONTEXT_CHANGE;
     }
 
     if (bs->open_flags & BDRV_O_INACTIVE) {
diff --git a/block/vvfat.c b/block/vvfat.c
index 426ca70..599a370 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -3091,7 +3091,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.4

[Qemu-devel] [PATCH v3 06/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.4

Re: [Qemu-devel] [PATCH v3 06/16] backup: Request BLK_PERM_AIO_CONTEXT_CHANGE on target

[Fam Zheng]

On Wed, 05/24 10:52, Fam Zheng wrote:
> 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);

Since fc0932fdc we reuse @bs as the backing of @target if sync=none, and this
hunk causes problem: the device's BB probably doesn't permit
BLK_PERM_AIO_CONTEXT_CHANGE.

Still scratching my head on this.

Fam

[Qemu-devel] [PATCH v3 07/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()
which is 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 c63f4e8..e8f72a1 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3240,8 +3240,6 @@ static BlockJob *do_drive_backup(DriveBackup *backup, BlockJobTxn *txn,
         goto out;
     }
 
-    bdrv_set_aio_context(target_bs, aio_context);
-
     if (set_backing_hd) {
         bdrv_set_backing_hd(target_bs, source, &local_err);
         if (local_err) {
@@ -3326,18 +3324,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.4

[Qemu-devel] [PATCH v3 08/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 03e82eb..a3337ee 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1190,6 +1190,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.4

[Qemu-devel] [PATCH v3 09/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 e2ee0ff..bbf7768 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -391,7 +391,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.4

[Qemu-devel] [PATCH v3 10/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 bonus, the aio context move is now conditional, which avoids some
unnecessary operations in bdrv_set_aio_context.

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

diff --git a/block/mirror.c b/block/mirror.c
index a3337ee..4eccb8d 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1118,13 +1118,44 @@ static void mirror_start_job(const char *job_id, BlockDriverState *bs,
                              bool auto_complete, const char *filter_node_name,
                              Error **errp)
 {
+    AioContext *aio_context, *target_context;
     MirrorBlockJob *s;
     BlockDriverState *mirror_top_bs;
+    BlockBackend *target_blk;
     bool target_graph_mod;
     bool target_is_backing;
     Error *local_err = NULL;
     int ret;
 
+    /* No resize for the target either; while the mirror is still running, a
+     * consistent read isn't necessarily possible. We could possibly allow
+     * writes and graph modifications, though it would likely defeat the
+     * purpose of a mirror, so leave them blocked for now.
+     *
+     * In the case of active commit, things look a bit different, though,
+     * because the target is an already populated backing file in active use.
+     * We can allow anything except resize there.*/
+    target_is_backing = bdrv_chain_contains(bs, target);
+    target_graph_mod = (backing_mode != MIRROR_LEAVE_BACKING_CHAIN);
+    target_blk = 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 |
+                                              BLK_PERM_WRITE |
+                                              BLK_PERM_GRAPH_MOD : 0));
+    ret = blk_insert_bs(target_blk, target, errp);
+    if (ret < 0) {
+        blk_unref(target_blk);
+        return;
+    }
+    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(target_blk, aio_context);
+        aio_context_release(target_context);
+    }
     if (granularity == 0) {
         granularity = bdrv_get_default_bitmap_granularity(target);
     }
@@ -1179,28 +1210,7 @@ static void mirror_start_job(const char *job_id, BlockDriverState *bs,
     s->source = bs;
     s->mirror_top_bs = mirror_top_bs;
 
-    /* No resize for the target either; while the mirror is still running, a
-     * consistent read isn't necessarily possible. We could possibly allow
-     * writes and graph modifications, though it would likely defeat the
-     * purpose of a mirror, so leave them blocked for now.
-     *
-     * In the case of active commit, things look a bit different, though,
-     * because the target is an already populated backing file in active use.
-     * We can allow anything except resize there.*/
-    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 |
-                                             BLK_PERM_WRITE |
-                                             BLK_PERM_GRAPH_MOD : 0));
-    ret = blk_insert_bs(s->target, target, errp);
-    if (ret < 0) {
-        goto fail;
-    }
-
+    s->target = target_blk;
     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 e8f72a1..52d81c4 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3545,8 +3545,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,
@@ -3597,8 +3595,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.4

Re: [Qemu-devel] [Qemu-block] [PATCH v3 10/16] mirror: Do initial aio context move of target via BB interface

[Stefan Hajnoczi]

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

On Wed, May 24, 2017 at 10:52:29AM +0800, Fam Zheng wrote:
> 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 bonus, the aio context move is now conditional, which avoids some
> unnecessary operations in bdrv_set_aio_context.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block/mirror.c | 54 ++++++++++++++++++++++++++++++++----------------------
>  blockdev.c     |  4 ----
>  2 files changed, 32 insertions(+), 26 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

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

[Qemu-devel] [PATCH v3 11/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 46a3e3f..074e235 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -794,6 +794,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.4

Re: [Qemu-devel] [PATCH v3 11/16] virtio-scsi: Request BLK_PERM_AIO_CONTEXT_CHANGE for dataplane

[Fam Zheng]

On Wed, 05/24 10:52, Fam Zheng wrote:
> 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 46a3e3f..074e235 100644
> --- a/hw/scsi/virtio-scsi.c
> +++ b/hw/scsi/virtio-scsi.c
> @@ -794,6 +794,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)) {

Inversed condition, should be s/!//.

Fam

> +            virtio_scsi_release(s);
> +            return;
> +        }
>          blk_set_aio_context(sd->conf.blk, s->ctx);
>          virtio_scsi_release(s);
>  
> -- 
> 2.9.4
> 
> 

[Qemu-devel] [PATCH v3 12/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 was hardcoded as -EPERM, but
returning 'r' is more meaningful here both for the new error and
existing ones.

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

[Qemu-devel] [PATCH v3 13/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 | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/block/block-backend.c b/block/block-backend.c
index 5492f64..dfe577d 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -68,6 +68,7 @@ struct BlockBackend {
     NotifierList remove_bs_notifiers, insert_bs_notifiers;
 
     int quiesce_counter;
+    AioContext *aio_context;
 };
 
 typedef struct BlockBackendAIOCB {
@@ -618,6 +619,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(
@@ -1692,6 +1697,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.4

[Qemu-devel] [PATCH v3 14/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.4

[Qemu-devel] [PATCH v3 15/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 dfe577d..51c62ea 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1695,8 +1695,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.4

[Qemu-devel] [PATCH v3 16/16] tests: Add test case for BLK_PERM_AIO_CONTEXT_CHANGE

[Fam Zheng]

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 tests/Makefile.include |  2 ++
 tests/test-blk-perm.c  | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 61 insertions(+)
 create mode 100644 tests/test-blk-perm.c

diff --git a/tests/Makefile.include b/tests/Makefile.include
index 7589383..5811296 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -56,6 +56,7 @@ gcov-files-test-thread-pool-y = thread-pool.c
 gcov-files-test-hbitmap-y = util/hbitmap.c
 check-unit-y += tests/test-hbitmap$(EXESUF)
 gcov-files-test-hbitmap-y = blockjob.c
+check-unit-y += tests/test-blk-perm$(EXESUF)
 check-unit-y += tests/test-blockjob$(EXESUF)
 check-unit-y += tests/test-blockjob-txn$(EXESUF)
 check-unit-y += tests/test-x86-cpuid$(EXESUF)
@@ -548,6 +549,7 @@ tests/test-coroutine$(EXESUF): tests/test-coroutine.o $(test-block-obj-y)
 tests/test-aio$(EXESUF): tests/test-aio.o $(test-block-obj-y)
 tests/test-aio-multithread$(EXESUF): tests/test-aio-multithread.o $(test-block-obj-y)
 tests/test-throttle$(EXESUF): tests/test-throttle.o $(test-block-obj-y)
+tests/test-blk-perm$(EXESUF): tests/test-blk-perm.o $(test-block-obj-y)
 tests/test-blockjob$(EXESUF): tests/test-blockjob.o $(test-block-obj-y) $(test-util-obj-y)
 tests/test-blockjob-txn$(EXESUF): tests/test-blockjob-txn.o $(test-block-obj-y) $(test-util-obj-y)
 tests/test-thread-pool$(EXESUF): tests/test-thread-pool.o $(test-block-obj-y)
diff --git a/tests/test-blk-perm.c b/tests/test-blk-perm.c
new file mode 100644
index 0000000..1c7b5d2
--- /dev/null
+++ b/tests/test-blk-perm.c
@@ -0,0 +1,59 @@
+/*
+ * Block permission tests
+ *
+ * Copyright Red Hat, Inc. 2017
+ *
+ * Authors:
+ *  Fam Zheng <famz@redhat.com>
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2 or later.
+ * See the COPYING.LIB file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "sysemu/block-backend.h"
+
+static void test_aio_context_success(void)
+{
+    BlockBackend *blk1 = blk_new(BLK_PERM_AIO_CONTEXT_CHANGE, BLK_PERM_ALL);
+    BlockBackend *blk2 = blk_new(BLK_PERM_AIO_CONTEXT_CHANGE, BLK_PERM_ALL);
+    BlockDriverState *bs = bdrv_open("null-co://", NULL, NULL, 0, &error_abort);
+
+    blk_insert_bs(blk1, bs, &error_abort);
+    blk_insert_bs(blk2, bs, &error_abort);
+
+    blk_unref(blk1);
+    blk_unref(blk2);
+    bdrv_unref(bs);
+}
+
+static void test_aio_context_failure(void)
+{
+    Error *local_err = NULL;
+    BlockBackend *blk1 = blk_new(BLK_PERM_AIO_CONTEXT_CHANGE,
+                                 BLK_PERM_ALL & ~BLK_PERM_AIO_CONTEXT_CHANGE);
+    BlockBackend *blk2 = blk_new(BLK_PERM_AIO_CONTEXT_CHANGE, BLK_PERM_ALL);
+    BlockDriverState *bs = bdrv_open("null-co://", NULL, NULL, 0, &error_abort);
+
+    blk_insert_bs(blk1, bs, &error_abort);
+    blk_insert_bs(blk2, bs, &local_err);
+
+    error_free_or_abort(&local_err);
+
+    blk_unref(blk1);
+    blk_unref(blk2);
+    bdrv_unref(bs);
+}
+
+int main(int argc, char **argv)
+{
+    bdrv_init();
+    qemu_init_main_loop(&error_abort);
+    g_test_init(&argc, &argv, NULL);
+    g_test_add_func("/block/perm/aio-context/success",
+                    test_aio_context_success);
+    g_test_add_func("/block/perm/aio-context/failure",
+                    test_aio_context_failure);
+    return g_test_run();
+}
-- 
2.9.4

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

[Stefan Hajnoczi]

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

On Wed, May 24, 2017 at 10:52:19AM +0800, Fam Zheng wrote:
> v3: Move blk_set_aio_context to the front of mirror_start_job to avoid
>     accessing target without acquiring its aio context. [Stefan]
>     Use error_free_or_abort in test code. [Stefan]

Please include my Reviewed-bys from the previous revision.  That way
they will be included in the commit log and I don't need to look back at
previous revisions when you send the next revision.

I've also left a comment on one of your replies to the previous
revision because I think blockdev-backup's 'target' parameter isn't
protected against AioContext changes.

Thanks!

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

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

[Fam Zheng]

On Wed, 05/31 10:45, Stefan Hajnoczi wrote:
> On Wed, May 24, 2017 at 10:52:19AM +0800, Fam Zheng wrote:
> > v3: Move blk_set_aio_context to the front of mirror_start_job to avoid
> >     accessing target without acquiring its aio context. [Stefan]
> >     Use error_free_or_abort in test code. [Stefan]
> 
> Please include my Reviewed-bys from the previous revision.  That way
> they will be included in the commit log and I don't need to look back at
> previous revisions when you send the next revision.

Yes, sorry for forgetting that.

Fam