Re: [PATCH v2 03/10] util/async: aio_co_enter(): do aio_co_schedule in general case

[Roman Kagan]

On Thu, Apr 08, 2021 at 06:54:30PM +0300, Roman Kagan wrote:
> On Thu, Apr 08, 2021 at 05:08:20PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> > With the following patch we want to call aio_co_wake() from thread.
> > And it works bad.
> > Assume we have no iothreads.
> > Assume we have a coroutine A, which waits in the yield point for external
> > aio_co_wake(), and no progress can be done until it happen.
> > Main thread is in blocking aio_poll() (for example, in blk_read()).
> > 
> > Now, in a separate thread we do aio_co_wake(). It calls  aio_co_enter(),
> > which goes through last "else" branch and do aio_context_acquire(ctx).
> > 
> > Now we have a deadlock, as aio_poll() will not release the context lock
> > until some progress is done, and progress can't be done until
> > aio_co_wake() wake the coroutine A. And it can't because it wait for
> > aio_context_acquire().
> > 
> > Still, aio_co_schedule() works well in parallel with blocking
> > aio_poll(). So let's use it in generic case and drop
> > aio_context_acquire/aio_context_release branch from aio_co_enter().
> > 
> > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> > ---
> >  util/async.c | 11 ++---------
> >  1 file changed, 2 insertions(+), 9 deletions(-)
> > 
> > diff --git a/util/async.c b/util/async.c
> > index 674dbefb7c..f05b883a39 100644
> > --- a/util/async.c
> > +++ b/util/async.c
> > @@ -614,19 +614,12 @@ void aio_co_wake(struct Coroutine *co)
> >  
> >  void aio_co_enter(AioContext *ctx, struct Coroutine *co)
> >  {
> > -    if (ctx != qemu_get_current_aio_context()) {
> > -        aio_co_schedule(ctx, co);
> > -        return;
> > -    }
> > -
> > -    if (qemu_in_coroutine()) {
> > +    if (ctx == qemu_get_current_aio_context() && qemu_in_coroutine()) {
> >          Coroutine *self = qemu_coroutine_self();
> >          assert(self != co);
> >          QSIMPLEQ_INSERT_TAIL(&self->co_queue_wakeup, co, co_queue_next);
> >      } else {
> > -        aio_context_acquire(ctx);
> > -        qemu_aio_coroutine_enter(ctx, co);
> > -        aio_context_release(ctx);
> > +        aio_co_schedule(ctx, co);
> >      }
> >  }
> 
> I'm fine with the change, but I find the log message to be a bit
> confusing (although correct).  AFAICS the problem is that calling
> aio_co_enter from a thread which has no associated aio_context works
> differently compared to calling it from a proper iothread: if the target
> context was qemu_aio_context, an iothread would just schedule the
> coroutine there, while a "dumb" thread would try lock the context
> potentially resulting in a deadlock.  This patch makes "dumb" threads
> and iothreads behave identically when entering a coroutine on a foreign
> context.
> 
> You may want to rephrase the log message to that end.
> 
> Anyway
> 
> Reviewed-by: Roman Kagan <rvkagan@yandex-team.ru>

I was too quick to reply.  Turns out this patch breaks a lot of stuff;
apparently the original behavior is relied upon somewhere.
In particular, in iotest 008 basic checks with '--aio threads' fail due
to the device being closed before the operation is performed, so the
latter returns with -ENOMEDIUM:

# .../qemu-io --cache writeback --aio threads -f qcow2 \
        -c 'aio_read -P 0xa 0 128M' .../scratch/t.qcow2 -T blk_\*
blk_root_attach child 0x560e9fb65a70 blk 0x560e9fb647b0 bs 0x560e9fb5f4b0
blk_root_detach child 0x560e9fb65a70 blk 0x560e9fb647b0 bs 0x560e9fb5f4b0
blk_root_attach child 0x560e9fb65a70 blk 0x560e9fb4c420 bs 0x560e9fb581a0
blk_root_detach child 0x560e9fb65a70 blk 0x560e9fb4c420 bs 0x560e9fb581a0
blk_co_preadv blk 0x560e9fb4c420 bs (nil) offset 0 bytes 134217728 flags 0x0
readv failed: No medium found

Let's drop this and get back to the original scheme with wakeup via BH.
It'll look just as nice, but won't touch the generic infrastructure with
unpredictable consequences.

Thanks,
Roman.