Re: [Qemu-devel] [PATCH v2 1/4] block: extract AIO_WAIT_WHILE() from BlockDriverState

[Eric Blake]

On 02/14/2018 08:06 AM, Stefan Hajnoczi wrote:
> On Tue, Feb 13, 2018 at 10:01:06AM -0600, Eric Blake wrote:
> > Trying to understand here:
> >
> >
> > > +#define AIO_WAIT_WHILE(wait, ctx, cond) ({                  \
> > > +    bool waited_ = false;                                   \
> > > +    bool busy_ = true;                                      \
> > > +    AioWait *wait_ = (wait);                                \
> > > +    AioContext *ctx_ = (ctx);                               \
> > > +    if (aio_context_in_iothread(ctx_)) {                    \
> > > +        while ((cond) || busy_) {                           \
> > > +            busy_ = aio_poll(ctx_, (cond));                 \
> > > +            waited_ |= !!(cond) | busy_;                    \
> > > +        }                                                   \
> >
> > If we are in an iothread already, we never dereference wait,
>
> No, the name and documentation for aio_context_in_iothread() is
> misleading.  It actually means "does this AioContext belong to the
> current thread?", which is more general than just the IOThread case.
>
> aio_context_in_iothread() returns true when:
> 1. We are the IOThread that owns ctx. <-- the case you thought of
> 2. We are the main loop and ctx == qemu_get_aio_context().
>     ^--- the sneaky case that BDRV_POLL_WHILE() has always relied on

Thanks, that helps.


> > > +    AIO_WAIT_WHILE(bdrv_get_aio_wait(bs_),                 \
> > > +                   bdrv_get_aio_context(bs_),              \
> > > +                   cond); })
> >
> > ...we can pass NULL as the wait parameter, which will crash.
>
> It won't crash since if (aio_context_in_iothread(ctx_)) will take the true
> case when bs_ == NULL.

Okay, you've solved that one.

> > > +++ b/block/io.c
> >
> > >    void bdrv_wakeup(BlockDriverState *bs)
> > >    {
> > > -    /* The barrier (or an atomic op) is in the caller.  */
> > > -    if (atomic_read(&bs->wakeup)) {
> > > -        aio_bh_schedule_oneshot(qemu_get_aio_context(), dummy_bh_cb, NULL);
> > > -    }
> > > +    aio_wait_kick(bdrv_get_aio_wait(bs));
> >
> > this is another case where passing NULL...
>
> bdrv_wakeup() is only called when bs != NULL.

And looks like we're safe, there, as well.

> I hope this explains things!  The main issue that raised these questions
> was that aio_context_in_iothread() has a misleading name.  Shall we
> rename it?

Maybe, but that's a separate patch.  What name would we bikeshed, maybe 
aio_context_correct_thread() (we are the correct thread if we are the 
iothread that owns ctx, or if we are the main thread and have properly 
acquired ctx) or aio_context_use_okay() (we can only use the ctx if we 
own it [native iothread] or have acquired it [main loop])

> I'm having a hard time picking a new name because it must not be
> confused with AioContext acquire/release, which doesn't influence the
> "native" AioContext that the current thread has an affinity with.

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