Re: [Qemu-devel] [PATCH 2/3] io: make qio_channel_yield aware of AioContexts

[Paolo Bonzini <pbonzini@redhat.com>]

> > @@ -84,8 +83,8 @@ struct QIOChannel {
> >      unsigned int features; /* bitmask of QIOChannelFeatures */
> >      char *name;
> >      AioContext *ctx;
> > -    QIOChannelRestart *read_coroutine;
> > -    QIOChannelRestart *write_coroutine;
> > +    Coroutine *read_coroutine;
> > +    Coroutine *write_coroutine;
> 
> Seems this ought to have been squashed into the previous patch

Yeah, sent this last thing before Christmas and it shows. :)

> > +static void qio_channel_set_fd_handlers(QIOChannel *ioc)
> > +{
> > +    IOHandler *rd_handler = NULL, *wr_handler = NULL;
> > +
> > +    if (ioc->read_coroutine) {
> > +	rd_handler = qio_channel_restart_read;
> > +    }
> > +    if (ioc->write_coroutine) {
> > +	rd_handler = qio_channel_restart_write;
> > +    }
> > +
> > +    qio_channel_set_fd_handler(ioc,
> > +                               ioc->ctx ? ioc->ctx :
> > iohandler_get_aio_context(),
> > +                               rd_handler, wr_handler, ioc);
> > +}
> 
> ioc->read_coroutine & ioc->write_coroutine can only be non-NULL during
> a qio_channel_yield() caller. So it seems that calling
> qio_channel_set_fd_handlers() from the qio_channel_set_aio_context()
> method in the previous patch is not required, as those two callback
> pointers will always be NULL.

Not necessarily.  You can have one coroutine calling qio_channel_yield(),
and then the non-coroutine code can call qio_channel_set_aio_context()
before the coroutine reenters.

This actually happens in the next patch.  Where the NBD socket is quiescent
and no response is in flight, such as during a bdrv_drain_begin/end()
section, the "coroutine that receives NBD headers" has yielded.  This
is also the time when set_aio_context can be called.

> > +    if (condition == G_IO_IN) {
> > +        ioc->read_coroutine = qemu_coroutine_self();
> > +    } else if (condition == G_IO_OUT) {
> > +        ioc->write_coroutine = qemu_coroutine_self();
> > +    } else {
> > +        abort();
> > +    }
> 
> Do we really need this to be an either/or/abort ? It looks like
> the qio_channel_set_fd_handlers() method is happy top have both
> read_coroutine & write_coroutine set.

The idea is that this would be called by a coroutine after a
recv or send that returns EAGAIN (with G_IO_IN for recv and
G_IO_OUT for send).  If not exclusive, you'd have to check
for ioc->read_coroutine == ioc->write_coroutine in the handler.
Not a big deal, I can do it, but it adds an edge case and I
didn't see a use for it.

> If it does need to be exclusive though, can you update the API
> docs for this method to mention that.

Sure.

Thanks for the speedy review!

Paolo