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

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

[Fam Zheng]

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                  | 13 ++++++++-
 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, 149 insertions(+), 35 deletions(-)
 create mode 100644 tests/test-blk-perm.c

-- 
2.9.3

[Qemu-devel] [PATCH v2 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 1fbbb8d..5a3affb 100644
--- a/block.c
+++ b/block.c
@@ -1562,6 +1562,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 5ddc0cf..3c409a3 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

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

[Stefan Hajnoczi]

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

On Wed, Apr 19, 2017 at 05:43:41PM +0800, Fam Zheng wrote:
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block.c               | 2 ++
>  include/block/block.h | 7 ++++++-
>  2 files changed, 8 insertions(+), 1 deletion(-)

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

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

[Qemu-devel] [PATCH v2 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 7405024..6bdd9ce 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -629,6 +629,18 @@ void blk_resume_after_migration(Error **errp)
     }
 }
 
+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 7462228..537f92f 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.3

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

[Stefan Hajnoczi]

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

On Wed, Apr 19, 2017 at 05:43:42PM +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(+)
> 
> diff --git a/block/block-backend.c b/block/block-backend.c
> index 7405024..6bdd9ce 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -629,6 +629,18 @@ void blk_resume_after_migration(Error **errp)
>      }
>  }
>  
> +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);
> +}

I'm slightly confused about why this function is needed.  blk_set_perm()
doesn't do quite the right thing?

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

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

[Fam Zheng]

On Thu, 05/11 15:32, Stefan Hajnoczi wrote:
> On Wed, Apr 19, 2017 at 05:43:42PM +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(+)
> > 
> > diff --git a/block/block-backend.c b/block/block-backend.c
> > index 7405024..6bdd9ce 100644
> > --- a/block/block-backend.c
> > +++ b/block/block-backend.c
> > @@ -629,6 +629,18 @@ void blk_resume_after_migration(Error **errp)
> >      }
> >  }
> >  
> > +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);
> > +}
> 
> I'm slightly confused about why this function is needed.  blk_set_perm()
> doesn't do quite the right thing?

Sorry for the late reply!

I think blk_set_perm() always calls bs->drv->bdrv_check_perm() even when the
required perm bits are already acquired.

Fam

[Qemu-devel] [PATCH v2 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.3

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

[Stefan Hajnoczi]

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

On Wed, Apr 19, 2017 at 05:43:43PM +0800, Fam Zheng wrote:
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  blockjob.c | 3 +++
>  1 file changed, 3 insertions(+)

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

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

[Qemu-devel] [PATCH v2 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 91d2c34..c07ee00 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 164438f..7ab3289 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1231,7 +1231,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.3

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

[Stefan Hajnoczi]

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

On Wed, Apr 19, 2017 at 05:43:44PM +0800, Fam Zheng wrote:
> 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(-)

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

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

[Qemu-devel] [PATCH v2 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 5a3affb..7e696e5 100644
--- a/block.c
+++ b/block.c
@@ -1685,7 +1685,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,
@@ -1728,9 +1729,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. */
@@ -1742,7 +1744,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;
     }
 
     *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

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

[Stefan Hajnoczi]

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

On Wed, Apr 19, 2017 at 05:43:45PM +0800, Fam Zheng wrote:
> 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(-)

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

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

[Qemu-devel] [PATCH v2 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.3

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

[Stefan Hajnoczi]

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

On Wed, Apr 19, 2017 at 05:43:46PM +0800, 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.

It's true that the backup block job must be able to set target's
AioContext, but does this change also allow other users to set target's
AioContext while the backup job is running?  If yes, then we need to
handle that.

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

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

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

[Fam Zheng]

On Thu, 05/11 15:41, Stefan Hajnoczi wrote:
> On Wed, Apr 19, 2017 at 05:43:46PM +0800, 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.
> 
> It's true that the backup block job must be able to set target's
> AioContext, but does this change also allow other users to set target's
> AioContext while the backup job is running?  If yes, then we need to
> handle that.

If through job->target, yes, but I don't think there is any user of job->target.
Otherwise, it's not allowed, because the second parameter of blk_new doesn't
have BLK_PERM_AIO_CONTEXT_CHANGE.

So it's okay.

Fam

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

[Stefan Hajnoczi]

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

On Wed, May 24, 2017 at 10:18:44AM +0800, Fam Zheng wrote:
> On Thu, 05/11 15:41, Stefan Hajnoczi wrote:
> > On Wed, Apr 19, 2017 at 05:43:46PM +0800, 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.
> > 
> > It's true that the backup block job must be able to set target's
> > AioContext, but does this change also allow other users to set target's
> > AioContext while the backup job is running?  If yes, then we need to
> > handle that.
> 
> If through job->target, yes, but I don't think there is any user of job->target.
> Otherwise, it's not allowed, because the second parameter of blk_new doesn't
> have BLK_PERM_AIO_CONTEXT_CHANGE.
> 
> So it's okay.

What about blockdev-backup?  It allows the user to specify 'target'.
Therefore the user can also run other monitor commands on target.  Some
of them could change the AioContext and the backup job wouldn't know!

Stefan

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

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

[Fam Zheng]

On Wed, 05/31 10:39, Stefan Hajnoczi wrote:
> On Wed, May 24, 2017 at 10:18:44AM +0800, Fam Zheng wrote:
> > On Thu, 05/11 15:41, Stefan Hajnoczi wrote:
> > > On Wed, Apr 19, 2017 at 05:43:46PM +0800, 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.
> > > 
> > > It's true that the backup block job must be able to set target's
> > > AioContext, but does this change also allow other users to set target's
> > > AioContext while the backup job is running?  If yes, then we need to
> > > handle that.
> > 
> > If through job->target, yes, but I don't think there is any user of job->target.
> > Otherwise, it's not allowed, because the second parameter of blk_new doesn't
> > have BLK_PERM_AIO_CONTEXT_CHANGE.
> > 
> > So it's okay.
> 
> What about blockdev-backup?  It allows the user to specify 'target'.
> Therefore the user can also run other monitor commands on target.  Some
> of them could change the AioContext and the backup job wouldn't know!

That will be rejected.

The contract is that any code that wants to change the AioContext of a BDS, in
this case the "target BDS", must do this:

1) create its own BB with perm.BLK_PERM_AIO_CONTEXT_CHANGE

2) attach BDS to this BB

3) call blk_set_aio_context and change the AioContext

This is basically how all users of a BDS coordinate through Kevin's new op
blocker API, and in your concerned case, when a user runs a second monitor
command that changes AioContext, step 2 will fail, because as in this patch, the
first job->target BB didn't set shared_perm.BLK_PERM_AIO_CONTEXT_CHANGE.

Fam

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

[Stefan Hajnoczi]

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

On Wed, May 31, 2017 at 05:57:46PM +0800, Fam Zheng wrote:
> On Wed, 05/31 10:39, Stefan Hajnoczi wrote:
> > On Wed, May 24, 2017 at 10:18:44AM +0800, Fam Zheng wrote:
> > > On Thu, 05/11 15:41, Stefan Hajnoczi wrote:
> > > > On Wed, Apr 19, 2017 at 05:43:46PM +0800, 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.
> > > > 
> > > > It's true that the backup block job must be able to set target's
> > > > AioContext, but does this change also allow other users to set target's
> > > > AioContext while the backup job is running?  If yes, then we need to
> > > > handle that.
> > > 
> > > If through job->target, yes, but I don't think there is any user of job->target.
> > > Otherwise, it's not allowed, because the second parameter of blk_new doesn't
> > > have BLK_PERM_AIO_CONTEXT_CHANGE.
> > > 
> > > So it's okay.
> > 
> > What about blockdev-backup?  It allows the user to specify 'target'.
> > Therefore the user can also run other monitor commands on target.  Some
> > of them could change the AioContext and the backup job wouldn't know!
> 
> That will be rejected.
> 
> The contract is that any code that wants to change the AioContext of a BDS, in
> this case the "target BDS", must do this:
> 
> 1) create its own BB with perm.BLK_PERM_AIO_CONTEXT_CHANGE
> 
> 2) attach BDS to this BB
> 
> 3) call blk_set_aio_context and change the AioContext
> 
> This is basically how all users of a BDS coordinate through Kevin's new op
> blocker API, and in your concerned case, when a user runs a second monitor
> command that changes AioContext, step 2 will fail, because as in this patch, the
> first job->target BB didn't set shared_perm.BLK_PERM_AIO_CONTEXT_CHANGE.

I was wondering how that works since do_blockdev_backup() does not use
BB to access target, but it does check whether a BB is already attached:

    target_bs = bdrv_lookup_bs(backup->target, backup->target, errp);
    if (!target_bs) {
        goto out;
    }

    if (bdrv_get_aio_context(target_bs) != aio_context) {
        if (!bdrv_has_blk(target_bs)) {   <----- fails when job is running
            /* 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;
        }
    }

Thanks!

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

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

[Fam Zheng]

On Thu, 06/01 14:26, Stefan Hajnoczi wrote:
> On Wed, May 31, 2017 at 05:57:46PM +0800, Fam Zheng wrote:
> > On Wed, 05/31 10:39, Stefan Hajnoczi wrote:
> > > On Wed, May 24, 2017 at 10:18:44AM +0800, Fam Zheng wrote:
> > > > On Thu, 05/11 15:41, Stefan Hajnoczi wrote:
> > > > > On Wed, Apr 19, 2017 at 05:43:46PM +0800, 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.
> > > > > 
> > > > > It's true that the backup block job must be able to set target's
> > > > > AioContext, but does this change also allow other users to set target's
> > > > > AioContext while the backup job is running?  If yes, then we need to
> > > > > handle that.
> > > > 
> > > > If through job->target, yes, but I don't think there is any user of job->target.
> > > > Otherwise, it's not allowed, because the second parameter of blk_new doesn't
> > > > have BLK_PERM_AIO_CONTEXT_CHANGE.
> > > > 
> > > > So it's okay.
> > > 
> > > What about blockdev-backup?  It allows the user to specify 'target'.
> > > Therefore the user can also run other monitor commands on target.  Some
> > > of them could change the AioContext and the backup job wouldn't know!
> > 
> > That will be rejected.
> > 
> > The contract is that any code that wants to change the AioContext of a BDS, in
> > this case the "target BDS", must do this:
> > 
> > 1) create its own BB with perm.BLK_PERM_AIO_CONTEXT_CHANGE
> > 
> > 2) attach BDS to this BB
> > 
> > 3) call blk_set_aio_context and change the AioContext
> > 
> > This is basically how all users of a BDS coordinate through Kevin's new op
> > blocker API, and in your concerned case, when a user runs a second monitor
> > command that changes AioContext, step 2 will fail, because as in this patch, the
> > first job->target BB didn't set shared_perm.BLK_PERM_AIO_CONTEXT_CHANGE.
> 
> I was wondering how that works since do_blockdev_backup() does not use
> BB to access target, but it does check whether a BB is already attached:
> 
>     target_bs = bdrv_lookup_bs(backup->target, backup->target, errp);
>     if (!target_bs) {
>         goto out;
>     }
> 
>     if (bdrv_get_aio_context(target_bs) != aio_context) {
>         if (!bdrv_has_blk(target_bs)) {   <----- fails when job is running
>             /* 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;
>         }
>     }

Yeah, this is the current way (before this series), and is incomplete in some
cases but too strict in others, for obvious reasons.  It is changed to always
create a BB in patch 7.

Fam

[Qemu-devel] [PATCH v2 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 4927914..dfd1385 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

Re: [Qemu-devel] [Qemu-block] [PATCH v2 07/16] backup: Do initial aio context move of target via BB interface

[Stefan Hajnoczi]

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

On Wed, Apr 19, 2017 at 05:43:47PM +0800, Fam Zheng wrote:
> 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(-)

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

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

[Qemu-devel] [PATCH v2 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 7ab3289..fe2fb7a 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1189,6 +1189,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

Re: [Qemu-devel] [Qemu-block] [PATCH v2 08/16] mirror: Request aio context change permission on target

[Stefan Hajnoczi]

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

On Wed, Apr 19, 2017 at 05:43:48PM +0800, 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/mirror.c | 1 +
>  1 file changed, 1 insertion(+)

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

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

[Qemu-devel] [PATCH v2 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 c07ee00..1241dd3 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.3

Re: [Qemu-devel] [PATCH v2 09/16] commit: Allow aio context change on s->base

[Stefan Hajnoczi]

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

On Wed, Apr 19, 2017 at 05:43:49PM +0800, Fam Zheng wrote:
> 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(-)

Aside from my question about another user calling bdrv_set_aio_context()
on base in a similar situation in an earlier patch:

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

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

[Qemu-devel] [PATCH v2 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 | 9 +++++++++
 blockdev.c     | 4 ----
 2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index fe2fb7a..2a287b0 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1117,6 +1117,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;
@@ -1200,6 +1201,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 dfd1385..53f9874 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

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

[Stefan Hajnoczi]

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

On Wed, Apr 19, 2017 at 05:43:50PM +0800, Fam Zheng wrote:
> diff --git a/blockdev.c b/blockdev.c
> index dfd1385..53f9874 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,

This patch post-pones bdrv_set_aio_context() until later on.  We now
call bdrv_get_info() without holding target_bs's AioContext in
mirror_start_job().

Please acquire the AioContext around target_bs accesses until we move it
to our AioContext.

Stefan

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

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

[Fam Zheng]

On Thu, 05/11 16:27, Stefan Hajnoczi wrote:
> On Wed, Apr 19, 2017 at 05:43:50PM +0800, Fam Zheng wrote:
> > diff --git a/blockdev.c b/blockdev.c
> > index dfd1385..53f9874 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,
> 
> This patch post-pones bdrv_set_aio_context() until later on.  We now
> call bdrv_get_info() without holding target_bs's AioContext in
> mirror_start_job().
> 
> Please acquire the AioContext around target_bs accesses until we move it
> to our AioContext.

Good catch! Will fix.

Fam

[Qemu-devel] [PATCH v2 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 bd62d08..d130ba3 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.3

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

[Stefan Hajnoczi]

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

On Wed, Apr 19, 2017 at 05:43:51PM +0800, 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(+)

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

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

[Qemu-devel] [PATCH v2 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.3

Re: [Qemu-devel] [Qemu-block] [PATCH v2 12/16] virtio-blk: Request BLK_PERM_AIO_CONTEXT_CHANGE for dataplane

[Stefan Hajnoczi]

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

On Wed, Apr 19, 2017 at 05:43:52PM +0800, Fam Zheng wrote:
> 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(-)

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

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

[Qemu-devel] [PATCH v2 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 6bdd9ce..6133917 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 {
@@ -565,6 +566,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(
@@ -1667,6 +1672,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

Re: [Qemu-devel] [Qemu-block] [PATCH v2 13/16] blk: fix aio context loss on media change

[Stefan Hajnoczi]

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

On Wed, Apr 19, 2017 at 05:43:53PM +0800, Fam Zheng wrote:
> 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(+)

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

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

[Qemu-devel] [PATCH v2 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.3

Re: [Qemu-devel] [Qemu-block] [PATCH v2 14/16] nbd: Allow BLK_PERM_AIO_CONTEXT_CHANGE on BB

[Stefan Hajnoczi]

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

On Wed, Apr 19, 2017 at 05:43:54PM +0800, Fam Zheng wrote:
> 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(-)

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

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

[Qemu-devel] [PATCH v2 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 6133917..166edac 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1670,8 +1670,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

Re: [Qemu-devel] [Qemu-block] [PATCH v2 15/16] block: Add perm assertion on blk_set_aio_context

[Stefan Hajnoczi]

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

On Wed, Apr 19, 2017 at 05:43:55PM +0800, Fam Zheng wrote:
> 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(+)

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

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

[Qemu-devel] [PATCH v2 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 f3de81f..b38e090 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)
@@ -543,6 +544,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..d6129ab
--- /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);
+
+    g_assert_nonnull(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.3

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

[Stefan Hajnoczi]

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

On Wed, Apr 19, 2017 at 05:43:56PM +0800, Fam Zheng wrote:
> +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);
> +
> +    g_assert_nonnull(local_err);

The following asserts and also frees the Error object to prevent leaking
memory:

error_free_or_abort(local_err);

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

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

[Stefan Hajnoczi]

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

On Wed, Apr 19, 2017 at 05:43:40PM +0800, Fam Zheng wrote:
> 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.

Looks pretty good.  I had two comments that apply across all patches:

First, it is not safe to enable the new permission without registering
an aio notifier.  Another user could look up the BDS and call
bdrv_set_aio_context() on it.  I believe this bug is present for block
jobs that have additional BDSes like base/target/etc.

Second, patches that post-pone bdrv_set_aio_context() must take care to
acquire the AioContext for BDS accesses that happen before the next
bdrv_set_aio_context() call.

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