Re: [RFC] block/nbd: Move s->ioc on AioContext change

[Eric Blake <eblake@redhat.com>]

On Fri, Jan 28, 2022 at 04:51:30PM +0100, Hanna Reitz wrote:
> s->ioc must always be attached to the NBD node's AioContext.  If that
> context changes, s->ioc must be attached to the new context.

Eww.  Good catch; and looks like it is not the first time where we've
run into issues (a quick grep for context through git history of nbd/
finds commit e6cada923).

> 
> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1990835
> Signed-off-by: Hanna Reitz <hreitz@redhat.com>
> ---
> This is an RFC because I believe there are some other things in the NBD
> block driver that need attention on an AioContext change, too.  Namely,
> there are two timers (reconnect_delay_timer and open_timer) that are

As Vladimir is more familiar with the timers needed for reconnecting
an NBD connection, I'm hoping he will chime in.  But yes, your worry
about context changes sounds reasonable.

> also attached to the node's AioContext, and I'm afraid they need to be
> handled, too.  Probably pause them on detach, and resume them on attach,
> but I'm not sure, which is why I'm posting this as an RFC to get some
> comments from that from someone who knows this code better than me. :)
> 
> (Also, in a real v1, of course I'd want to add a regression test.)
> ---
>  block/nbd.c | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/block/nbd.c b/block/nbd.c
> index 63dbfa807d..119a774c04 100644
> --- a/block/nbd.c
> +++ b/block/nbd.c
> @@ -2036,6 +2036,25 @@ static void nbd_cancel_in_flight(BlockDriverState *bs)
>      nbd_co_establish_connection_cancel(s->conn);
>  }
>  
> +static void nbd_attach_aio_context(BlockDriverState *bs,
> +                                   AioContext *new_context)
> +{
> +    BDRVNBDState *s = bs->opaque;
> +
> +    if (s->ioc) {
> +        qio_channel_attach_aio_context(s->ioc, new_context);
> +    }
> +}
> +
> +static void nbd_detach_aio_context(BlockDriverState *bs)
> +{
> +    BDRVNBDState *s = bs->opaque;
> +
> +    if (s->ioc) {
> +        qio_channel_detach_aio_context(s->ioc);
> +    }
> +}
> +
>  static BlockDriver bdrv_nbd = {
>      .format_name                = "nbd",
>      .protocol_name              = "nbd",
> @@ -2059,6 +2078,9 @@ static BlockDriver bdrv_nbd = {
>      .bdrv_dirname               = nbd_dirname,
>      .strong_runtime_opts        = nbd_strong_runtime_opts,
>      .bdrv_cancel_in_flight      = nbd_cancel_in_flight,
> +
> +    .bdrv_attach_aio_context    = nbd_attach_aio_context,
> +    .bdrv_detach_aio_context    = nbd_detach_aio_context,

Looks straightforward, but as you say, fleshing out what else may need
similar treatment could make the "real" v1 more interesting.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org