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

[Eric Blake <eblake@redhat.com>]

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