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