Re: [PATCH] block/aio_task: allow start/wait task from any coroutine

[Vladimir Sementsov-Ogievskiy]

11.06.2020 15:31, Denis V. Lunev wrote:
> On 6/11/20 10:36 AM, Vladimir Sementsov-Ogievskiy wrote:
> > Currently, aio task pool assumes that there is a main coroutine, which
> > creates tasks and wait for them. Let's remove the restriction by using
> > CoQueue. Code becomes clearer, interface more obvious.
> >
> > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> > ---
> >
> > Hi! Here is my counter-propasal for
> > "[PATCH 1/2] aio: allow to wait for coroutine pool from different coroutine"
> > by Denis. I'm sure that if we are going to change something here, better
> > is make the interface work from any coroutine without the restriction of
> > only-on-waiter at the moment.
> >
> > (Note, that it is still not thread-safe)
> >
> >
> >   block/aio_task.c | 15 ++++-----------
> >   1 file changed, 4 insertions(+), 11 deletions(-)
> >
> > diff --git a/block/aio_task.c b/block/aio_task.c
> > index 88989fa248..d48b29ff83 100644
> > --- a/block/aio_task.c
> > +++ b/block/aio_task.c
> > @@ -27,11 +27,10 @@
> >   #include "block/aio_task.h"
> >   
> >   struct AioTaskPool {
> > -    Coroutine *main_co;
> >       int status;
> >       int max_busy_tasks;
> >       int busy_tasks;
> > -    bool waiting;
> > +    CoQueue waiters;
> >   };
> >   
> >   static void coroutine_fn aio_task_co(void *opaque)
> > @@ -52,21 +51,15 @@ static void coroutine_fn aio_task_co(void *opaque)
> >   
> >       g_free(task);
> >   
> > -    if (pool->waiting) {
> > -        pool->waiting = false;
> > -        aio_co_wake(pool->main_co);
> > -    }
> > +    qemu_co_queue_next(&pool->waiters);
> nope, this will wakeup only single waiter.
> the code will deadlock If there are 2 waiters for the last
> entry.

Hmm, right.
The problem is that original design combines into one thing two different:
 
  1. wait for all tasks to complete
  2. wait for a free slot, to start a new task

2. should work as is, with qemu_co_queue_next() and co-queue, and for 1. we
should have separate yield-point, to be waken up when busy_tasks becomes 0.

I'll resend.



--
Best regards,
Vladimir