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

[Fam Zheng]

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?

> 
> 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 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.

Fam