[Qemu-devel] [PATCH v2 0/5] block: keep AioContext pointer in BlockBackend

[Qemu-devel] [PATCH v2 0/5] block: keep AioContext pointer in BlockBackend

[Fam Zheng]

The first patches clean up usage of BlockBackend and changing of its (root's)
aio contexts; the last patch is an update of Stefan's previous version rebasing
on top of current master. The biggest change from the RFC is that blk_insert_bs
callers are responsible to put the BB and BDS on the same context before
calling it.

This fixes the crash triggered by "change" a scsi-cd on a virtio-scsi dataplane
device.

The new assertions in block-backend.c ensures we won't have a conflict pair of
BlockBackend users from different contextes.

Fam Zheng (4):
  blockdev-mirror: Sanity check before moving target_bs AioContext
  blockdev: Move BDS AioContext before inserting to BB
  block: Introduce and make use of blk_new_with_root
  migration: Set correct AioContext to BlockBackend

Stefan Hajnoczi (1):
  block: keep AioContext pointer in BlockBackend

 block/backup.c                   |  3 +--
 block/block-backend.c            | 48 +++++++++++++++++++++++++++++-----------
 block/commit.c                   | 12 ++++------
 block/mirror.c                   |  3 +--
 blockdev.c                       | 42 ++++++++++++++++++++++++++---------
 blockjob.c                       |  3 +--
 hmp.c                            |  3 +--
 hw/core/qdev-properties-system.c |  3 +--
 include/sysemu/block-backend.h   |  1 +
 migration/block.c                |  1 +
 nbd/server.c                     |  3 +--
 tests/test-blockjob.c            |  3 +--
 12 files changed, 79 insertions(+), 46 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH v2 1/5] blockdev-mirror: Sanity check before moving target_bs AioContext

[Fam Zheng]

Similar to blockdev-backup, if the target was already moved to a
different AioContext, bad things can happen. This happens when the
target belongs to a data plane device. It's a very unlikely case, but
let's check it anyway.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 blockdev.c | 35 ++++++++++++++++++++++++-----------
 1 file changed, 24 insertions(+), 11 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 29c6561..a4960b9 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3281,6 +3281,21 @@ BlockDeviceInfoList *qmp_query_named_block_nodes(Error **errp)
     return bdrv_named_nodes_list(errp);
 }
 
+static void blockdev_set_aio_context(BlockDriverState *bs, AioContext *ctx,
+                                     Error **errp)
+{
+    if (bdrv_get_aio_context(bs) != ctx) {
+        if (!bdrv_has_blk(bs)) {
+            /* The target BDS is not attached, we can safely move it to another
+             * AioContext. */
+            bdrv_set_aio_context(bs, ctx);
+        } else {
+            error_setg(errp, "Target is attached to a different thread from "
+                             "source.");
+        }
+    }
+}
+
 void do_blockdev_backup(BlockdevBackup *backup, BlockJobTxn *txn, Error **errp)
 {
     BlockDriverState *bs;
@@ -3317,16 +3332,10 @@ void do_blockdev_backup(BlockdevBackup *backup, BlockJobTxn *txn, Error **errp)
         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;
-        }
+    blockdev_set_aio_context(target_bs, aio_context, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        goto out;
     }
     backup_start(backup->job_id, bs, target_bs, backup->speed, backup->sync,
                  NULL, backup->compress, backup->on_source_error,
@@ -3538,7 +3547,11 @@ void qmp_drive_mirror(DriveMirror *arg, Error **errp)
         goto out;
     }
 
-    bdrv_set_aio_context(target_bs, aio_context);
+    blockdev_set_aio_context(target_bs, aio_context, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        goto out;
+    }
 
     blockdev_mirror_common(arg->has_job_id ? arg->job_id : NULL, bs, target_bs,
                            arg->has_replaces, arg->replaces, arg->sync,
-- 
2.7.4

Re: [Qemu-devel] [PATCH v2 1/5] blockdev-mirror: Sanity check before moving target_bs AioContext

[Max Reitz]

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

On 27.09.2016 08:37, Fam Zheng wrote:
> Similar to blockdev-backup, if the target was already moved to a
> different AioContext, bad things can happen. This happens when the
> target belongs to a data plane device. It's a very unlikely case, but
> let's check it anyway.

You didn't implement it for blockdev-mirror, though, but for
drive-mirror (which I don't think needs this check).

Max

> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  blockdev.c | 35 ++++++++++++++++++++++++-----------
>  1 file changed, 24 insertions(+), 11 deletions(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index 29c6561..a4960b9 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -3281,6 +3281,21 @@ BlockDeviceInfoList *qmp_query_named_block_nodes(Error **errp)
>      return bdrv_named_nodes_list(errp);
>  }
>  
> +static void blockdev_set_aio_context(BlockDriverState *bs, AioContext *ctx,
> +                                     Error **errp)
> +{
> +    if (bdrv_get_aio_context(bs) != ctx) {
> +        if (!bdrv_has_blk(bs)) {
> +            /* The target BDS is not attached, we can safely move it to another
> +             * AioContext. */
> +            bdrv_set_aio_context(bs, ctx);
> +        } else {
> +            error_setg(errp, "Target is attached to a different thread from "
> +                             "source.");
> +        }
> +    }
> +}
> +
>  void do_blockdev_backup(BlockdevBackup *backup, BlockJobTxn *txn, Error **errp)
>  {
>      BlockDriverState *bs;
> @@ -3317,16 +3332,10 @@ void do_blockdev_backup(BlockdevBackup *backup, BlockJobTxn *txn, Error **errp)
>          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;
> -        }
> +    blockdev_set_aio_context(target_bs, aio_context, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        goto out;
>      }
>      backup_start(backup->job_id, bs, target_bs, backup->speed, backup->sync,
>                   NULL, backup->compress, backup->on_source_error,
> @@ -3538,7 +3547,11 @@ void qmp_drive_mirror(DriveMirror *arg, Error **errp)
>          goto out;
>      }
>  
> -    bdrv_set_aio_context(target_bs, aio_context);
> +    blockdev_set_aio_context(target_bs, aio_context, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        goto out;
> +    }
>  
>      blockdev_mirror_common(arg->has_job_id ? arg->job_id : NULL, bs, target_bs,
>                             arg->has_replaces, arg->replaces, arg->sync,
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 480 bytes --]

Re: [Qemu-devel] [PATCH v2 1/5] blockdev-mirror: Sanity check before moving target_bs AioContext

[Fam Zheng]

On Wed, 09/28 18:37, Max Reitz wrote:
> On 27.09.2016 08:37, Fam Zheng wrote:
> > Similar to blockdev-backup, if the target was already moved to a
> > different AioContext, bad things can happen. This happens when the
> > target belongs to a data plane device. It's a very unlikely case, but
> > let's check it anyway.
> 
> You didn't implement it for blockdev-mirror, though, but for
> drive-mirror (which I don't think needs this check).

Ugh, will fix.

[Qemu-devel] [PATCH v2 2/5] blockdev: Move BDS AioContext before inserting to BB

[Fam Zheng]

The callers made sure the BDS has no BB attached before, so blk is
becoming the sole owner. It is necessary to move the BDS to the right
AioContext before inserting it, to keep them in sync.

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

diff --git a/blockdev.c b/blockdev.c
index a4960b9..9700dee 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2478,6 +2478,7 @@ out:
 static void qmp_blockdev_insert_anon_medium(BlockBackend *blk,
                                             BlockDriverState *bs, Error **errp)
 {
+    AioContext *ctx;
     bool has_device;
 
     /* For BBs without a device, we can exchange the BDS tree at will */
@@ -2498,6 +2499,12 @@ static void qmp_blockdev_insert_anon_medium(BlockBackend *blk,
         return;
     }
 
+    ctx = blk_get_aio_context(blk);
+    if (ctx != bdrv_get_aio_context(bs)) {
+        aio_context_acquire(ctx);
+        bdrv_set_aio_context(bs, ctx);
+        aio_context_release(ctx);
+    }
     blk_insert_bs(blk, bs);
 
     if (!blk_dev_has_tray(blk)) {
-- 
2.7.4

Re: [Qemu-devel] [PATCH v2 2/5] blockdev: Move BDS AioContext before inserting to BB

[Max Reitz]

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

On 27.09.2016 08:37, Fam Zheng wrote:
> The callers made sure the BDS has no BB attached before, so blk is
> becoming the sole owner. It is necessary to move the BDS to the right
> AioContext before inserting it, to keep them in sync.

Well, I'm not sure whether it's so simple, though, because you can
always have some BB attached down the chain. Imagine a qcow2 BDS named
"top" with a backing BDS named "base". Now let's say we already have a
BB at "base" (e.g. read-only for an NBD server). Just because "top" does
not have a BB does not mean that we can just move the whole tree into
another AioContext, because the BB at "base" may depend on the current one.

(Actually, NBD should be able to deal with that case, but other
frontends might not.)

Of course, it's possible the other way around, too. You can have a BB at
"top" and now want to attach a BB to "base". It's the same issue there,
basically. (Actually, it's worse here, because changes in the AioContext
only get propagated down the node tree, not up.)


The most important thing however is that unfortunately everything's
probably already broken today, so I won't be too pedantic about all of this.

> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  blockdev.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/blockdev.c b/blockdev.c
> index a4960b9..9700dee 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -2478,6 +2478,7 @@ out:
>  static void qmp_blockdev_insert_anon_medium(BlockBackend *blk,
>                                              BlockDriverState *bs, Error **errp)
>  {
> +    AioContext *ctx;
>      bool has_device;
>  
>      /* For BBs without a device, we can exchange the BDS tree at will */
> @@ -2498,6 +2499,12 @@ static void qmp_blockdev_insert_anon_medium(BlockBackend *blk,
>          return;
>      }
>  
> +    ctx = blk_get_aio_context(blk);
> +    if (ctx != bdrv_get_aio_context(bs)) {

A bit weird before patch 5 (I assume?) because ctx will always be the
main loop's context (and the BB actually doesn't have an inherent
AioContext yet).

While I see how this should be done before patch 5, it's actually not
quite correct before that patch. But I think it's fine to accept this
for the duration of three commits.

> +        aio_context_acquire(ctx);
> +        bdrv_set_aio_context(bs, ctx);
> +        aio_context_release(ctx);
> +    }
>      blk_insert_bs(blk, bs);
>  
>      if (!blk_dev_has_tray(blk)) {
> 

The reason I'd feel bad about giving an R-b is because of the lack of
propagation of the AioContext up the tree. We have to make sure that the
BDS really does not have any parents in another AioContext, and that not
only includes BBs but also other BDSs.

But the propagation down the tree is not quite flawless either: We do
have notifiers which can tell e.g. frontends when the AioContext is
changing, but only NBD and block jobs make use of this right now. Also,
these are only notifiers, so the frontend cannot prevent the BDS from
changing the AioContext, which is probably something that dataplane
would require.

So I think we first need some infrastructure which can test whether
moving a BDS tree to another AioContext is fine with all of the nodes in
the tree (and that includes BBs/frontends). For instance, NBD is always
fine with its BDS moving around, but dataplane probably is not.

Only if we know that moving the BDS tree to another context is fine, we
should actually do so, and in this case, we need to make sure not just
to propagate the new context down, but also up the graph (or we could
simply not allow changing the AioContext up the graph).

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 480 bytes --]

[Qemu-devel] [PATCH v2 3/5] block: Introduce and make use of blk_new_with_root

[Fam Zheng]

The idiom of "blk_new() + blk_insert_bs()" is common. Use a new
BB interface that does both things to make the coming transition that
adds AioContext to BB easier.

The added blk_new_with_ctx doesn't handle the passed ctx and is purely
here to avoid a small code churn.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/backup.c                   |  3 +--
 block/block-backend.c            | 24 ++++++++++++++++++------
 block/commit.c                   | 12 ++++--------
 block/mirror.c                   |  3 +--
 blockjob.c                       |  3 +--
 hmp.c                            |  3 +--
 hw/core/qdev-properties-system.c |  3 +--
 include/sysemu/block-backend.h   |  1 +
 nbd/server.c                     |  3 +--
 tests/test-blockjob.c            |  3 +--
 10 files changed, 30 insertions(+), 28 deletions(-)

diff --git a/block/backup.c b/block/backup.c
index 582bd0f..3eed9a1 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -601,8 +601,7 @@ void backup_start(const char *job_id, BlockDriverState *bs,
         goto error;
     }
 
-    job->target = blk_new();
-    blk_insert_bs(job->target, target);
+    job->target = blk_new_with_root(target);
 
     job->on_source_error = on_source_error;
     job->on_target_error = on_target_error;
diff --git a/block/block-backend.c b/block/block-backend.c
index 0bd19ab..b71babe 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -115,12 +115,7 @@ static const BdrvChildRole child_root = {
     .drained_end        = blk_root_drained_end,
 };
 
-/*
- * Create a new BlockBackend with a reference count of one.
- * Store an error through @errp on failure, unless it's null.
- * Return the new BlockBackend on success, null on failure.
- */
-BlockBackend *blk_new(void)
+static BlockBackend *blk_new_with_ctx(AioContext *ctx)
 {
     BlockBackend *blk;
 
@@ -139,6 +134,23 @@ BlockBackend *blk_new(void)
 }
 
 /*
+ * Create a new BlockBackend with a reference count of one.
+ * Store an error through @errp on failure, unless it's null.
+ * Return the new BlockBackend on success, null on failure.
+ */
+BlockBackend *blk_new(void)
+{
+    return blk_new_with_ctx(qemu_get_aio_context());
+}
+
+BlockBackend *blk_new_with_root(BlockDriverState *root)
+{
+    BlockBackend *blk = blk_new_with_ctx(bdrv_get_aio_context(root));
+    blk_insert_bs(blk, root);
+    return blk;
+}
+
+/*
  * Creates a new BlockBackend, opens a new BlockDriverState, and connects both.
  *
  * Just as with bdrv_open(), after having called this function the reference to
diff --git a/block/commit.c b/block/commit.c
index 9f67a8b..45d4f96 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -260,11 +260,9 @@ void commit_start(const char *job_id, BlockDriverState *bs,
     }
 
 
-    s->base = blk_new();
-    blk_insert_bs(s->base, base);
+    s->base = blk_new_with_root(base);
 
-    s->top = blk_new();
-    blk_insert_bs(s->top, top);
+    s->top = blk_new_with_root(top);
 
     s->active = bs;
 
@@ -314,11 +312,9 @@ int bdrv_commit(BlockDriverState *bs)
         }
     }
 
-    src = blk_new();
-    blk_insert_bs(src, bs);
+    src = blk_new_with_root(bs);
 
-    backing = blk_new();
-    blk_insert_bs(backing, bs->backing->bs);
+    backing = blk_new_with_root(bs->backing->bs);
 
     length = blk_getlength(src);
     if (length < 0) {
diff --git a/block/mirror.c b/block/mirror.c
index f9d1fec..8910d53 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -941,8 +941,7 @@ static void mirror_start_job(const char *job_id, BlockDriverState *bs,
         return;
     }
 
-    s->target = blk_new();
-    blk_insert_bs(s->target, target);
+    s->target = blk_new_with_root(target);
 
     s->replaces = g_strdup(replaces);
     s->on_source_error = on_source_error;
diff --git a/blockjob.c b/blockjob.c
index a167f96..8fe6d5d 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -148,8 +148,7 @@ void *block_job_create(const char *job_id, const BlockJobDriver *driver,
         return NULL;
     }
 
-    blk = blk_new();
-    blk_insert_bs(blk, bs);
+    blk = blk_new_with_root(bs);
 
     job = g_malloc0(driver->instance_size);
     error_setg(&job->blocker, "block device is in use by block job: %s",
diff --git a/hmp.c b/hmp.c
index 336e7bf..1ee83d2 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1934,8 +1934,7 @@ void hmp_qemu_io(Monitor *mon, const QDict *qdict)
     if (!blk) {
         BlockDriverState *bs = bdrv_lookup_bs(NULL, device, &err);
         if (bs) {
-            blk = local_blk = blk_new();
-            blk_insert_bs(blk, bs);
+            blk = local_blk = blk_new_with_root(bs);
         } else {
             goto fail;
         }
diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
index e55afe6..2b07beb 100644
--- a/hw/core/qdev-properties-system.c
+++ b/hw/core/qdev-properties-system.c
@@ -78,8 +78,7 @@ static void parse_drive(DeviceState *dev, const char *str, void **ptr,
     if (!blk) {
         BlockDriverState *bs = bdrv_lookup_bs(NULL, str, NULL);
         if (bs) {
-            blk = blk_new();
-            blk_insert_bs(blk, bs);
+            blk = blk_new_with_root(bs);
             blk_created = true;
         }
     }
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index 3b29317..a25ee69 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -79,6 +79,7 @@ typedef struct BlockBackendPublic {
 } BlockBackendPublic;
 
 BlockBackend *blk_new(void);
+BlockBackend *blk_new_with_root(BlockDriverState *root);
 BlockBackend *blk_new_open(const char *filename, const char *reference,
                            QDict *options, int flags, Error **errp);
 int blk_get_refcnt(BlockBackend *blk);
diff --git a/nbd/server.c b/nbd/server.c
index 472f584..c965cce 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -816,8 +816,7 @@ NBDExport *nbd_export_new(BlockDriverState *bs, off_t dev_offset, off_t size,
     BlockBackend *blk;
     NBDExport *exp = g_malloc0(sizeof(NBDExport));
 
-    blk = blk_new();
-    blk_insert_bs(blk, bs);
+    blk = blk_new_with_root(bs);
     blk_set_enable_write_cache(blk, !writethrough);
 
     exp->refcount = 1;
diff --git a/tests/test-blockjob.c b/tests/test-blockjob.c
index 5b0e934..74dc750 100644
--- a/tests/test-blockjob.c
+++ b/tests/test-blockjob.c
@@ -53,10 +53,9 @@ static BlockJob *do_test_id(BlockBackend *blk, const char *id,
  * BlockDriverState inserted. */
 static BlockBackend *create_blk(const char *name)
 {
-    BlockBackend *blk = blk_new();
     BlockDriverState *bs = bdrv_new();
+    BlockBackend *blk = blk_new_with_root(bs);
 
-    blk_insert_bs(blk, bs);
     bdrv_unref(bs);
 
     if (name) {
-- 
2.7.4

Re: [Qemu-devel] [PATCH v2 3/5] block: Introduce and make use of blk_new_with_root

[Max Reitz]

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

On 27.09.2016 08:37, Fam Zheng wrote:
> The idiom of "blk_new() + blk_insert_bs()" is common. Use a new
> BB interface that does both things to make the coming transition that
> adds AioContext to BB easier.

Indeed. It was called blk_new_with_bs() before and I dropped it. :-)

(The difference and the reason why I dropped it was that
blk_new_with_bs() created the BDS, too.)

((And as a side note, this pattern does not come from me dropping
blk_new_with_bs(). As far as I remember, I replaced most places where it
was used by blk_new_open().))

> The added blk_new_with_ctx doesn't handle the passed ctx and is purely
> here to avoid a small code churn.

Maybe add a "yet" in there, i.e. "does not handle the passed ctx yet"?

> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block/backup.c                   |  3 +--
>  block/block-backend.c            | 24 ++++++++++++++++++------
>  block/commit.c                   | 12 ++++--------
>  block/mirror.c                   |  3 +--
>  blockjob.c                       |  3 +--
>  hmp.c                            |  3 +--
>  hw/core/qdev-properties-system.c |  3 +--
>  include/sysemu/block-backend.h   |  1 +
>  nbd/server.c                     |  3 +--
>  tests/test-blockjob.c            |  3 +--
>  10 files changed, 30 insertions(+), 28 deletions(-)

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 480 bytes --]

[Qemu-devel] [PATCH v2 4/5] migration: Set correct AioContext to BlockBackend

[Fam Zheng]

The BB is newly created and it should follow the BDS's current context.

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

diff --git a/migration/block.c b/migration/block.c
index ebc10e6..b7365ee 100644
--- a/migration/block.c
+++ b/migration/block.c
@@ -445,6 +445,7 @@ static void init_blk_migration(QEMUFile *f)
         BlockDriverState *bs = bmds_bs[i].bs;
 
         if (bmds) {
+            blk_set_aio_context(bmds->blk, bdrv_get_aio_context(bs));
             blk_insert_bs(bmds->blk, bs);
 
             alloc_aio_bitmap(bmds);
-- 
2.7.4

Re: [Qemu-devel] [PATCH v2 4/5] migration: Set correct AioContext to BlockBackend

[Max Reitz]

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

On 27.09.2016 08:37, Fam Zheng wrote:
> The BB is newly created and it should follow the BDS's current context.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  migration/block.c | 1 +
>  1 file changed, 1 insertion(+)

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 480 bytes --]

[Qemu-devel] [PATCH v2 5/5] block: keep AioContext pointer in BlockBackend

[Fam Zheng]

From: Stefan Hajnoczi <stefanha@redhat.com>

blk_get/set_aio_context() delegate to BlockDriverState without storing
the AioContext pointer in BlockBackend.

There are two flaws:

1. BlockBackend falls back to the QEMU main loop AioContext when there
   is no root BlockDriverState.  This means the drive loses its
   AioContext during media change and would break dataplane.

2. BlockBackend state used from multiple threads has no lock.  Race
   conditions will creep in as functionality is moved from
   BlockDriverState to BlockBackend due to the absense of a lock.  The
   monitor cannot access BlockBackend state safely while an IOThread is
   also accessing the state.

Issue #1 can be triggered by "change" on virtio-scsi dataplane, causing
a assertion failure (virtio-blk is fine because medium change is not
possible). #2 may be possible with block accounting statistics in
BlockBackend but I'm not aware of a crash that can be triggered.

This patch stores the AioContext pointer in BlockBackend and puts newly
inserted BlockDriverStates into the AioContext.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/block-backend.c | 24 +++++++++++++++++-------
 1 file changed, 17 insertions(+), 7 deletions(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index b71babe..cda67cc 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -31,6 +31,7 @@ static AioContext *blk_aiocb_get_aio_context(BlockAIOCB *acb);
 struct BlockBackend {
     char *name;
     int refcnt;
+    AioContext *aio_context;
     BdrvChild *root;
     DriveInfo *legacy_dinfo;    /* null unless created by drive_new() */
     QTAILQ_ENTRY(BlockBackend) link;         /* for block_backends */
@@ -121,6 +122,7 @@ static BlockBackend *blk_new_with_ctx(AioContext *ctx)
 
     blk = g_new0(BlockBackend, 1);
     blk->refcnt = 1;
+    blk->aio_context = ctx;
     blk_set_enable_write_cache(blk, true);
 
     qemu_co_queue_init(&blk->public.throttled_reqs[0]);
@@ -510,6 +512,8 @@ void blk_remove_bs(BlockBackend *blk)
 void blk_insert_bs(BlockBackend *blk, BlockDriverState *bs)
 {
     bdrv_ref(bs);
+
+    assert(blk->aio_context == bdrv_get_aio_context(bs));
     blk->root = bdrv_root_attach_child(bs, "root", &child_root, blk);
 
     notifier_list_notify(&blk->insert_bs_notifiers, blk);
@@ -1413,13 +1417,7 @@ void blk_op_unblock_all(BlockBackend *blk, Error *reason)
 
 AioContext *blk_get_aio_context(BlockBackend *blk)
 {
-    BlockDriverState *bs = blk_bs(blk);
-
-    if (bs) {
-        return bdrv_get_aio_context(bs);
-    } else {
-        return qemu_get_aio_context();
-    }
+    return blk->aio_context;
 }
 
 static AioContext *blk_aiocb_get_aio_context(BlockAIOCB *acb)
@@ -1432,7 +1430,19 @@ void blk_set_aio_context(BlockBackend *blk, AioContext *new_context)
 {
     BlockDriverState *bs = blk_bs(blk);
 
+    blk->aio_context = new_context;
+
     if (bs) {
+        AioContext *ctx = bdrv_get_aio_context(bs);
+
+        if (ctx == new_context) {
+            return;
+        }
+        /* Moving context around happens when a block device is
+         * enabling/disabling data plane, in which case we own the root BDS and
+         * it cannot be associated with another AioContext. */
+        assert(ctx == qemu_get_aio_context() ||
+               new_context == qemu_get_aio_context());
         if (blk->public.throttle_state) {
             throttle_timers_detach_aio_context(&blk->public.throttle_timers);
         }
-- 
2.7.4

Re: [Qemu-devel] [PATCH v2 5/5] block: keep AioContext pointer in BlockBackend

[Max Reitz]

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

On 27.09.2016 08:37, Fam Zheng wrote:
> From: Stefan Hajnoczi <stefanha@redhat.com>
> 
> blk_get/set_aio_context() delegate to BlockDriverState without storing
> the AioContext pointer in BlockBackend.
> 
> There are two flaws:
> 
> 1. BlockBackend falls back to the QEMU main loop AioContext when there
>    is no root BlockDriverState.  This means the drive loses its
>    AioContext during media change and would break dataplane.
> 
> 2. BlockBackend state used from multiple threads has no lock.  Race
>    conditions will creep in as functionality is moved from
>    BlockDriverState to BlockBackend due to the absense of a lock.  The
>    monitor cannot access BlockBackend state safely while an IOThread is
>    also accessing the state.
> 
> Issue #1 can be triggered by "change" on virtio-scsi dataplane, causing
> a assertion failure (virtio-blk is fine because medium change is not
> possible). #2 may be possible with block accounting statistics in
> BlockBackend but I'm not aware of a crash that can be triggered.
> 
> This patch stores the AioContext pointer in BlockBackend and puts newly
> inserted BlockDriverStates into the AioContext.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block/block-backend.c | 24 +++++++++++++++++-------
>  1 file changed, 17 insertions(+), 7 deletions(-)
> 
> diff --git a/block/block-backend.c b/block/block-backend.c
> index b71babe..cda67cc 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -31,6 +31,7 @@ static AioContext *blk_aiocb_get_aio_context(BlockAIOCB *acb);
>  struct BlockBackend {
>      char *name;
>      int refcnt;
> +    AioContext *aio_context;
>      BdrvChild *root;
>      DriveInfo *legacy_dinfo;    /* null unless created by drive_new() */
>      QTAILQ_ENTRY(BlockBackend) link;         /* for block_backends */
> @@ -121,6 +122,7 @@ static BlockBackend *blk_new_with_ctx(AioContext *ctx)
>  
>      blk = g_new0(BlockBackend, 1);
>      blk->refcnt = 1;
> +    blk->aio_context = ctx;
>      blk_set_enable_write_cache(blk, true);
>  
>      qemu_co_queue_init(&blk->public.throttled_reqs[0]);
> @@ -510,6 +512,8 @@ void blk_remove_bs(BlockBackend *blk)
>  void blk_insert_bs(BlockBackend *blk, BlockDriverState *bs)
>  {
>      bdrv_ref(bs);
> +
> +    assert(blk->aio_context == bdrv_get_aio_context(bs));
>      blk->root = bdrv_root_attach_child(bs, "root", &child_root, blk);
>  
>      notifier_list_notify(&blk->insert_bs_notifiers, blk);
> @@ -1413,13 +1417,7 @@ void blk_op_unblock_all(BlockBackend *blk, Error *reason)
>  
>  AioContext *blk_get_aio_context(BlockBackend *blk)
>  {
> -    BlockDriverState *bs = blk_bs(blk);
> -
> -    if (bs) {
> -        return bdrv_get_aio_context(bs);
> -    } else {
> -        return qemu_get_aio_context();
> -    }
> +    return blk->aio_context;
>  }
>  
>  static AioContext *blk_aiocb_get_aio_context(BlockAIOCB *acb)
> @@ -1432,7 +1430,19 @@ void blk_set_aio_context(BlockBackend *blk, AioContext *new_context)
>  {
>      BlockDriverState *bs = blk_bs(blk);
>  
> +    blk->aio_context = new_context;
> +
>      if (bs) {
> +        AioContext *ctx = bdrv_get_aio_context(bs);
> +
> +        if (ctx == new_context) {
> +            return;
> +        }
> +        /* Moving context around happens when a block device is
> +         * enabling/disabling data plane, in which case we own the root BDS and
> +         * it cannot be associated with another AioContext. */
> +        assert(ctx == qemu_get_aio_context() ||
> +               new_context == qemu_get_aio_context());

I don't really see the point behind this assertion. I know it's not
currently possible, but you are basically asserting that we do not move
a BDS tree directly from some non-main-loop context to another
non-main-loop context, which in theory sounds completely fine to me.

Based on the "Write code for now and not for the future" rule, I'm fine
with this assertion if you can tell me what good it does us now.

The only thing I can personally imagine is that it's a safeguard that we
don't try to place a BDS tree into some other AioContext while having
ignored that there are still some other BBs attached to it which don't
want to agree on that new AioContext. But I think that should rather be
fixed before patch 2, i.e. as I said we need an infrastructure which can
tell us beforehand (and without failing assertions) whether we can move
a certain BDS tree to some other context.

So whether we can move a certain BB from some context to another depends
on what the frontend supports, I don't think there is a generic answer
we can implement here in the generic BB code. NBD for instance allows
any movement; but devices probably only allow movements they have
initiated themselves (e.g. dataplane will allow exactly what you
describe here with that assertion, and any other device will probably
not allow anything but the main loop).

Max

>          if (blk->public.throttle_state) {
>              throttle_timers_detach_aio_context(&blk->public.throttle_timers);
>          }
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 480 bytes --]

Re: [Qemu-devel] [PATCH v2 5/5] block: keep AioContext pointer in BlockBackend

[Fam Zheng]

On Wed, 09/28 19:47, Max Reitz wrote:
> On 27.09.2016 08:37, Fam Zheng wrote:
> > From: Stefan Hajnoczi <stefanha@redhat.com>
> > 
> > blk_get/set_aio_context() delegate to BlockDriverState without storing
> > the AioContext pointer in BlockBackend.
> > 
> > There are two flaws:
> > 
> > 1. BlockBackend falls back to the QEMU main loop AioContext when there
> >    is no root BlockDriverState.  This means the drive loses its
> >    AioContext during media change and would break dataplane.
> > 
> > 2. BlockBackend state used from multiple threads has no lock.  Race
> >    conditions will creep in as functionality is moved from
> >    BlockDriverState to BlockBackend due to the absense of a lock.  The
> >    monitor cannot access BlockBackend state safely while an IOThread is
> >    also accessing the state.
> > 
> > Issue #1 can be triggered by "change" on virtio-scsi dataplane, causing
> > a assertion failure (virtio-blk is fine because medium change is not
> > possible). #2 may be possible with block accounting statistics in
> > BlockBackend but I'm not aware of a crash that can be triggered.
> > 
> > This patch stores the AioContext pointer in BlockBackend and puts newly
> > inserted BlockDriverStates into the AioContext.
> > 
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > ---
> >  block/block-backend.c | 24 +++++++++++++++++-------
> >  1 file changed, 17 insertions(+), 7 deletions(-)
> > 
> > diff --git a/block/block-backend.c b/block/block-backend.c
> > index b71babe..cda67cc 100644
> > --- a/block/block-backend.c
> > +++ b/block/block-backend.c
> > @@ -31,6 +31,7 @@ static AioContext *blk_aiocb_get_aio_context(BlockAIOCB *acb);
> >  struct BlockBackend {
> >      char *name;
> >      int refcnt;
> > +    AioContext *aio_context;
> >      BdrvChild *root;
> >      DriveInfo *legacy_dinfo;    /* null unless created by drive_new() */
> >      QTAILQ_ENTRY(BlockBackend) link;         /* for block_backends */
> > @@ -121,6 +122,7 @@ static BlockBackend *blk_new_with_ctx(AioContext *ctx)
> >  
> >      blk = g_new0(BlockBackend, 1);
> >      blk->refcnt = 1;
> > +    blk->aio_context = ctx;
> >      blk_set_enable_write_cache(blk, true);
> >  
> >      qemu_co_queue_init(&blk->public.throttled_reqs[0]);
> > @@ -510,6 +512,8 @@ void blk_remove_bs(BlockBackend *blk)
> >  void blk_insert_bs(BlockBackend *blk, BlockDriverState *bs)
> >  {
> >      bdrv_ref(bs);
> > +
> > +    assert(blk->aio_context == bdrv_get_aio_context(bs));
> >      blk->root = bdrv_root_attach_child(bs, "root", &child_root, blk);
> >  
> >      notifier_list_notify(&blk->insert_bs_notifiers, blk);
> > @@ -1413,13 +1417,7 @@ void blk_op_unblock_all(BlockBackend *blk, Error *reason)
> >  
> >  AioContext *blk_get_aio_context(BlockBackend *blk)
> >  {
> > -    BlockDriverState *bs = blk_bs(blk);
> > -
> > -    if (bs) {
> > -        return bdrv_get_aio_context(bs);
> > -    } else {
> > -        return qemu_get_aio_context();
> > -    }
> > +    return blk->aio_context;
> >  }
> >  
> >  static AioContext *blk_aiocb_get_aio_context(BlockAIOCB *acb)
> > @@ -1432,7 +1430,19 @@ void blk_set_aio_context(BlockBackend *blk, AioContext *new_context)
> >  {
> >      BlockDriverState *bs = blk_bs(blk);
> >  
> > +    blk->aio_context = new_context;
> > +
> >      if (bs) {
> > +        AioContext *ctx = bdrv_get_aio_context(bs);
> > +
> > +        if (ctx == new_context) {
> > +            return;
> > +        }
> > +        /* Moving context around happens when a block device is
> > +         * enabling/disabling data plane, in which case we own the root BDS and
> > +         * it cannot be associated with another AioContext. */
> > +        assert(ctx == qemu_get_aio_context() ||
> > +               new_context == qemu_get_aio_context());
> 
> I don't really see the point behind this assertion. I know it's not
> currently possible, but you are basically asserting that we do not move
> a BDS tree directly from some non-main-loop context to another
> non-main-loop context, which in theory sounds completely fine to me.
> 
> Based on the "Write code for now and not for the future" rule, I'm fine
> with this assertion if you can tell me what good it does us now.
> 
> The only thing I can personally imagine is that it's a safeguard that we
> don't try to place a BDS tree into some other AioContext while having
> ignored that there are still some other BBs attached to it which don't
> want to agree on that new AioContext. But I think that should rather be
> fixed before patch 2, i.e. as I said we need an infrastructure which can
> tell us beforehand (and without failing assertions) whether we can move
> a certain BDS tree to some other context.
> 
> So whether we can move a certain BB from some context to another depends
> on what the frontend supports, I don't think there is a generic answer
> we can implement here in the generic BB code. NBD for instance allows
> any movement; but devices probably only allow movements they have
> initiated themselves (e.g. dataplane will allow exactly what you
> describe here with that assertion, and any other device will probably
> not allow anything but the main loop).

Indeed, you make me think this should be an op blocker (that applies on whole
graph).

> 
> Max
> 
> >          if (blk->public.throttle_state) {
> >              throttle_timers_detach_aio_context(&blk->public.throttle_timers);
> >          }
> > 
> 
> 

Re: [Qemu-devel] [PATCH v2 5/5] block: keep AioContext pointer in BlockBackend

[Paolo Bonzini]

On 29/09/2016 05:05, Fam Zheng wrote:
> > So whether we can move a certain BB from some context to another depends
> > on what the frontend supports, I don't think there is a generic answer
> > we can implement here in the generic BB code. NBD for instance allows
> > any movement; but devices probably only allow movements they have
> > initiated themselves (e.g. dataplane will allow exactly what you
> > describe here with that assertion, and any other device will probably
> > not allow anything but the main loop).
> 
> Indeed, you make me think this should be an op blocker (that applies on whole
> graph).

The concept of a BDS or BB's AioContext is going to disappear sooner or
later, take this into account to decide how much effort to put into
fixing this.

Paolo

Re: [Qemu-devel] [PATCH v2 5/5] block: keep AioContext pointer in BlockBackend

[Fam Zheng]

On Thu, 09/29 09:47, Paolo Bonzini wrote:
> 
> 
> On 29/09/2016 05:05, Fam Zheng wrote:
> > > So whether we can move a certain BB from some context to another depends
> > > on what the frontend supports, I don't think there is a generic answer
> > > we can implement here in the generic BB code. NBD for instance allows
> > > any movement; but devices probably only allow movements they have
> > > initiated themselves (e.g. dataplane will allow exactly what you
> > > describe here with that assertion, and any other device will probably
> > > not allow anything but the main loop).
> > 
> > Indeed, you make me think this should be an op blocker (that applies on whole
> > graph).
> 
> The concept of a BDS or BB's AioContext is going to disappear sooner or
> later, take this into account to decide how much effort to put into
> fixing this.

Taking one step back, the whole thing is so broken considering the node
reference. Currently, even this is possible:

    -drive file=null-co://,if=virtio,id=d0 \
    -drive file.file=d0,driver=raw,file.driver=raw,if=virtio

AioContext is actually a factor to make it worse. Maybe we should instead think
about some generic protection mechanism? Is this in the scope of new op
blocker?

Fam