Re: [Qemu-block] [RFC PATCH 00/11] aio: Introduce handler type to fix nested aio_poll for dataplane

[Paolo Bonzini]

On 23/07/2015 13:43, Fam Zheng wrote:
> On Thu, 07/23 10:15, Paolo Bonzini wrote:
>>
>>
>> On 23/07/2015 08:32, Fam Zheng wrote:
>>>
>>> What is realized in this series is similar except the "disable, poll, 
>>> enable"
>>> method, instead the bit mask of interesting client types is passed to
>>> aio_poll() (to be exact, passed to aio_poll_clients()). That is because,
>>> aio_poll may release the AioContext lock around ppoll, avoiding state will 
>>> make
>>> the interface clearer.
>>
>> However, this means that you must keep the AioContext lock during all
>> the long-running operations.  Is this actually necessary?  The original
>> purpose of keeping the lock (which didn't quite work) was to block out
>> dataplane operations.
> 
> I don't really see how we can avoid taking the lock around nested aio_poll
> either way.  Could you elaborate how it works in your original proposal?

There are two parts to the answer.  First, aio_poll is taking locks
already, so the data structures are protected.  The question then is
whether recursive locking is necessary in general or just in the current
implementation.

To answer this, let's look at how aio_poll is used, for example in
bdrv_prwv_co:

        co = qemu_coroutine_create(bdrv_rw_co_entry);
        qemu_coroutine_enter(co, &rwco);
        while (rwco.ret == NOT_DONE) {
            aio_poll(aio_context, true);
        }

There are three cases, with the critical sections becoming smaller and
smaller:

1) the caller takes the lock for the whole duration of the operation.
In this case, the caller works just as in the non-dataplane case.  It
can use bdrv_aio_poll as in your patches.  This is the way
aio_context_acquire/release are currently used.

2) the caller takes the lock just around bdrv_read.  In this case,
bdrv_prwv_co will never be interrupted but I/O from the guest might
sneak in between one bdrv_* call and the next.  If I/O from the guest is
blocked with aio_disable_clients + bdrv_drain, on the other hand, this
can work.

3) the caller doesn't have to take the lock; bdrv_rw_co_entry takes care
of locking and also releases the lock around yields.  Note that in this
case the locking can be finer-grained.  Ultimately you could have a
different lock per BDS.  The AioContext lock would only protect the
AioContext's own data structures (bottom halves, file descriptors, etc.)
and it probably would not need to be recursive.

Case 3 is the interesting one, because of the two aio_poll invocations
(from bdrv_read and from the dataplane thread) you don't know which
aio_poll completes the coroutine and sets rwco.ret.  However, *it does
not matter*.  Whenever I/O finishes, both invocations will be woken up;
one of them will acquire the lock and set rwco.ret, the other will wait
on the lock, find it has nothing to do and, release the lock, and when
it returns the while loop will exit.

So it is not important to lock out other executions of aio_poll, as long
as all executions of aio_poll do exactly the same thing.
aio_disable/enable_clients lets all executions of aio_poll do exactly
the same thing.

>> Also, this requirements means you really need the AioContext lock to be
>> recursive.  This is not a huge deal, but I'd prefer that requirement not
>> to be too ingrained.
> 
> Regarding the AioContext lock, I see the point that it's an obstacle to
> enabling virtio-scsi-dataplane MMIO.
> 
> Thinking out loud, maybe the recursive lock is too general: as you said, it 
> has
> always been used under BQL to block dataplane operations, instead of being
> arbitrarily used by all kinds of threads.  I'm wondering if we could just
> replace it with some pause / resume mechanism of the dataplane iothreads (e.g.
> iothread_pause / iothread_resume, a bit similar to block_job_pause)?

That's also an idea, and it lets you use bdrv_aio_poll as in this series.

> That way, aio_context_acquire can be dropped by:
> 
>     /* @pause_owner_thread: a callback which will be called when _main thread_
>      * wants exclusively operate on the AioContext, for example with a nested
>      * aio_poll().
>      * @resume_owner_thread: a callback which will be called when _main 
> thread_
>      * has done the exclusive operation.
>      */
>     AioContext *aio_context_new(AioContextPauseResumeFn *pause_owner_thread,
>                                 AioContextPauseResumeFn *resume_owner_thread,
>                                 void *opaque,
>                                 Error **errp):
> 
>     /* Try to pause the owning thread */
>     void aio_context_pause(AioContext *ctx, Error **errp);
> 
>     /* Try to resume the paused owning thread, cannot fail */
>     void aio_context_resume(AioContext *ctx);
> 
> Then, in iothread.c:
> 
>     iothread->ctx = aio_context_new(iothread_pause, iothread_resume,
>                                     iothread, &local_err);
> 
> Where iothread_pause can block the thread with a QemuCond.

Condition variables don't mix well with recursive mutexes...  Once we
have bottom halves outside the AioContext lock, however, we could use a
separate lock for this condition variable.  That would work.

I like it, but I still ask myself... what's the difference between
aio_context_pause/resume and aio_disable/enable_clients? :)  There is
still state, just in the iothread rather than in the AioContext.

Paolo