Re: [Qemu-devel] [PATCH 09/15] linux-aio: fix submit aio as a batch

[Paolo Bonzini <pbonzini@redhat.com>]

Il 30/07/2014 19:32, Ming Lei ha scritto:
> On Wed, Jul 30, 2014 at 9:59 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> Il 30/07/2014 13:39, Ming Lei ha scritto:
>>> In the enqueue path, we can't complete request, otherwise
>>> "Co-routine re-entered recursively" may be caused, so this
>>> patch fixes the issue with below ideas:
>>>
>>>       - for -EAGAIN or partial completion, retry the submission by
>>>       an introduced event handler
>>>       - for part of completion, also update the io queue
>>>       - for other failure, return the failure if in enqueue path,
>>>       otherwise, abort all queued I/O
>>>
>>> Signed-off-by: Ming Lei <ming.lei@canonical.com>
>>> ---
>>>  block/linux-aio.c |   90 ++++++++++++++++++++++++++++++++++++++++-------------
>>>  1 file changed, 68 insertions(+), 22 deletions(-)
>>>
>>> diff --git a/block/linux-aio.c b/block/linux-aio.c
>>> index 7ac7e8c..5eb9c92 100644
>>> --- a/block/linux-aio.c
>>> +++ b/block/linux-aio.c
>>> @@ -51,6 +51,7 @@ struct qemu_laio_state {
>>>
>>>      /* io queue for submit at batch */
>>>      LaioQueue io_q;
>>> +    EventNotifier retry;      /* handle -EAGAIN and partial completion */
>>>  };
>>>
>>>  static inline ssize_t io_event_ret(struct io_event *ev)
>>> @@ -154,45 +155,80 @@ static void ioq_init(LaioQueue *io_q)
>>>      io_q->plugged = 0;
>>>  }
>>>
>>> -static int ioq_submit(struct qemu_laio_state *s)
>>> +static void abort_queue(struct qemu_laio_state *s)
>>> +{
>>> +    int i;
>>> +    for (i = 0; i < s->io_q.idx; i++) {
>>> +        struct qemu_laiocb *laiocb = container_of(s->io_q.iocbs[i],
>>> +                                                  struct qemu_laiocb,
>>> +                                                  iocb);
>>> +        laiocb->ret = -EIO;
>>> +        qemu_laio_process_completion(s, laiocb);
>>> +    }
>>> +}
>>> +
>>> +static int ioq_submit(struct qemu_laio_state *s, bool enqueue)
>>>  {
>>>      int ret, i = 0;
>>>      int len = s->io_q.idx;
>>> +    int j = 0;
>>>
>>> -    do {
>>> -        ret = io_submit(s->ctx, len, s->io_q.iocbs);
>>> -    } while (i++ < 3 && ret == -EAGAIN);
>>> +    if (!len) {
>>> +        return 0;
>>> +    }
>>>
>>> -    /* empty io queue */
>>> -    s->io_q.idx = 0;
>>> +    ret = io_submit(s->ctx, len, s->io_q.iocbs);
>>> +    if (ret == -EAGAIN) {
>>> +        event_notifier_set(&s->retry);
>>
>> Retrying immediately (and just doing a couple of system calls to waste
>> time) is not an improvement.  The right place to retry is in
>> qemu_laio_completion_cb, after io_getevents has been called and
>> presumably the queue depth has decreased.
> 
> Good point.
> 
>>
>> If !s->io_q.plugged but io_submit fails you can call ioq_enqueue and it
> 
> When will the queued I/O be submitted? That will introduce extra
> complexity definitely.

It will be submitted when qemu_laio_completion_cb is called.

> It is a change for !s->io_q.plugged case, and it isn't good to do that in
> this patch, IMO.

I agree with you that this series is doing too many things at a single
time.  You can submit separate series for 1) no-coroutine fast path, 2)
full queue, 3) multiqueue.  If you do things properly you won't have a
single conflict, since they affect respectively block.c,
block/linux-aio.c and hw/block/.

Paolo