Re: [Qemu-block] [PATCH 1/2] block: let blk_add/remove_aio_context_notifier() tolerate BDS changes

[Max Reitz]

On 2018-03-06 21:48, Stefan Hajnoczi wrote:
> Commit 2019ba0a0197 ("block: Add AioContextNotifier functions to BB")
> added blk_add/remove_aio_context_notifier() and implemented them by
> passing through the bdrv_*() equivalent.
> 
> This doesn't work across bdrv_append(), which detaches child->bs and
> re-attaches it to a new BlockDriverState.  When
> blk_remove_aio_context_notifier() is called we will access the new BDS
> instead of the one where the notifier was added!

And nice that we just did not do anything if there was no BDS (in
practice that can never happen, but still nice).

Also, I like your exclamation mark.  It makes this sound so excited! :D

> From the point of view of the blk_*() API user, changes to the root BDS
> should be transparent.
> 
> This patch maintains a list of AioContext notifiers in BlockBackend and
> adds/removes them from the BlockDriverState as needed.
> 
> Reported-by: Stefano Panella 
> Cc: Max Reitz 
> Signed-off-by: Stefan Hajnoczi 
> ---
>  block/block-backend.c | 63 
> +++
>  block/trace-events|  2 ++
>  2 files changed, 65 insertions(+)

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature

[Qemu-block] [PATCH 1/2] block: let blk_add/remove_aio_context_notifier() tolerate BDS changes

[Stefan Hajnoczi]

Commit 2019ba0a0197 ("block: Add AioContextNotifier functions to BB")
added blk_add/remove_aio_context_notifier() and implemented them by
passing through the bdrv_*() equivalent.

This doesn't work across bdrv_append(), which detaches child->bs and
re-attaches it to a new BlockDriverState.  When
blk_remove_aio_context_notifier() is called we will access the new BDS
instead of the one where the notifier was added!

>From the point of view of the blk_*() API user, changes to the root BDS
should be transparent.

This patch maintains a list of AioContext notifiers in BlockBackend and
adds/removes them from the BlockDriverState as needed.

Reported-by: Stefano Panella 
Cc: Max Reitz 
Signed-off-by: Stefan Hajnoczi 
---
 block/block-backend.c | 63 +++
 block/trace-events|  2 ++
 2 files changed, 65 insertions(+)

diff --git a/block/block-backend.c b/block/block-backend.c
index 94ffbb6a60..aa27698820 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -31,6 +31,13 @@
 
 static AioContext *blk_aiocb_get_aio_context(BlockAIOCB *acb);
 
+typedef struct BlockBackendAioNotifier {
+void (*attached_aio_context)(AioContext *new_context, void *opaque);
+void (*detach_aio_context)(void *opaque);
+void *opaque;
+QLIST_ENTRY(BlockBackendAioNotifier) list;
+} BlockBackendAioNotifier;
+
 struct BlockBackend {
 char *name;
 int refcnt;
@@ -69,6 +76,7 @@ struct BlockBackend {
 bool allow_write_beyond_eof;
 
 NotifierList remove_bs_notifiers, insert_bs_notifiers;
+QLIST_HEAD(, BlockBackendAioNotifier) aio_notifiers;
 
 int quiesce_counter;
 VMChangeStateEntry *vmsh;
@@ -239,6 +247,36 @@ static int blk_root_inactivate(BdrvChild *child)
 return 0;
 }
 
+static void blk_root_attach(BdrvChild *child)
+{
+BlockBackend *blk = child->opaque;
+BlockBackendAioNotifier *notifier;
+
+trace_blk_root_attach(child, blk, child->bs);
+
+QLIST_FOREACH(notifier, &blk->aio_notifiers, list) {
+bdrv_add_aio_context_notifier(child->bs,
+notifier->attached_aio_context,
+notifier->detach_aio_context,
+notifier->opaque);
+}
+}
+
+static void blk_root_detach(BdrvChild *child)
+{
+BlockBackend *blk = child->opaque;
+BlockBackendAioNotifier *notifier;
+
+trace_blk_root_detach(child, blk, child->bs);
+
+QLIST_FOREACH(notifier, &blk->aio_notifiers, list) {
+bdrv_remove_aio_context_notifier(child->bs,
+notifier->attached_aio_context,
+notifier->detach_aio_context,
+notifier->opaque);
+}
+}
+
 static const BdrvChildRole child_root = {
 .inherit_options= blk_root_inherit_options,
 
@@ -252,6 +290,9 @@ static const BdrvChildRole child_root = {
 
 .activate   = blk_root_activate,
 .inactivate = blk_root_inactivate,
+
+.attach = blk_root_attach,
+.detach = blk_root_detach,
 };
 
 /*
@@ -279,6 +320,7 @@ BlockBackend *blk_new(uint64_t perm, uint64_t shared_perm)
 
 notifier_list_init(&blk->remove_bs_notifiers);
 notifier_list_init(&blk->insert_bs_notifiers);
+QLIST_INIT(&blk->aio_notifiers);
 
 QTAILQ_INSERT_TAIL(&block_backends, blk, link);
 return blk;
@@ -356,6 +398,7 @@ static void blk_delete(BlockBackend *blk)
 }
 assert(QLIST_EMPTY(&blk->remove_bs_notifiers.notifiers));
 assert(QLIST_EMPTY(&blk->insert_bs_notifiers.notifiers));
+assert(QLIST_EMPTY(&blk->aio_notifiers));
 QTAILQ_REMOVE(&block_backends, blk, link);
 drive_info_del(blk->legacy_dinfo);
 block_acct_cleanup(&blk->stats);
@@ -1813,8 +1856,15 @@ void blk_add_aio_context_notifier(BlockBackend *blk,
 void (*attached_aio_context)(AioContext *new_context, void *opaque),
 void (*detach_aio_context)(void *opaque), void *opaque)
 {
+BlockBackendAioNotifier *notifier;
 BlockDriverState *bs = blk_bs(blk);
 
+notifier = g_new(BlockBackendAioNotifier, 1);
+notifier->attached_aio_context = attached_aio_context;
+notifier->detach_aio_context = detach_aio_context;
+notifier->opaque = opaque;
+QLIST_INSERT_HEAD(&blk->aio_notifiers, notifier, list);
+
 if (bs) {
 bdrv_add_aio_context_notifier(bs, attached_aio_context,
   detach_aio_context, opaque);
@@ -1827,12 +1877,25 @@ void blk_remove_aio_context_notifier(BlockBackend *blk,
  void (*detach_aio_context)(void *),
  void *opaque)
 {
+BlockBackendAioNotifier *notifier;
 BlockDriverState *bs = blk_bs(blk);
 
 if (bs) {
 bdrv_remove_aio_context_notifier(bs, attached_aio_context,
  detach_aio_context, opaque);
 }
+
+QLIST_FOREACH(notifier, &blk->aio_notifiers, list) {
+if (notifier->attached_aio_context == attached_aio_context &&
+notifier->detach_aio_c