Re: [Qemu-devel] [PATCH 3/3] linux-aio: process completions from ioq_submit()

[Roman Penyaev]

On Tue, Jul 19, 2016 at 1:18 PM, Roman Penyaev
<address@hidden> wrote:
> On Tue, Jul 19, 2016 at 12:36 PM, Paolo Bonzini <address@hidden> wrote:
>>
>>
>> On 19/07/2016 12:25, Roman Pen wrote:
>>>      if (laiocb->co) {
>>> -        qemu_coroutine_enter(laiocb->co, NULL);
>>> +        if (laiocb->co == qemu_coroutine_self()) {
>>> +            laiocb->self_completed = true;
>>
>> No need for this new field.  You can just do nothing here and check
>> laiocb.ret == -EINPROGRESS here in laio_co_submit.
>
> I have thought but did not like it, because we depend on the value,
> which kernel writes there.  If kernel by chance writes -EINPROGRESS
> (whatever that means, bug in some ll driver?) we are screwed up.
> But probably that is my paranoia.
>
>>
>>> +        } else {
>>> +            qemu_coroutine_enter(laiocb->co, NULL);
>>> +        }
>>>      } else {
>>>          laiocb->common.cb(laiocb->common.opaque, ret);
>>>          qemu_aio_unref(laiocb);
>>> @@ -312,6 +317,12 @@ static void ioq_submit(LinuxAioState *s)
>>>          QSIMPLEQ_SPLIT_AFTER(&s->io_q.pending, aiocb, next, &completed);
>>>      } while (ret == len && !QSIMPLEQ_EMPTY(&s->io_q.pending));
>>>      s->io_q.blocked = (s->io_q.in_queue > 0);
>>> +
>>> +    if (s->io_q.in_flight) {
>>> +        /* We can try to complete something just right away if there are
>>> +         * still requests in-flight. */
>>> +        qemu_laio_process_completions(s);
>>> +    }
>>
>> Can this leave I/O stuck if in_queue > 0 && in_flight == 0 after the
>> return from qemu_laio_process_completions?  I think you need to goto the
>> beginning of the function to submit more I/O requests in that case.

Not quite.  I still leave '&s->e' as set, so we will return in a generic
qemu_laio_completion_cb(), will do 'event_notifier_test_and_clear(&s->e)'
and again will process events with normal submission.

IMHO this is better variant than spinning inside ioq_submit doing goto.
We do not occupy the whole events processing for our IO needs and give
a chance to complete other stuff.  But of course this is guts feeling
and I do not have any serious numbers.

--
Roman

>
> Indeed that can happen.  Will resend.
>
>>
>> In fact, perhaps it's useful to always do so if any I/Os were completed.
>>  Should qemu_laio_process_completions return the number of completed
>> requests?
>
> I would always check the in_flight counter, since we are interested in what
> is really left.  Also, returning the value of completed requests by _this_
> qemu_laio_process_completions() call does not mean anything, since we can
> be nested and bunch of completions can happen below us.  We can realy on true
> of false.  Not exact value.
>
> Also, I hope (I do not know how to reproduce this, virtio_blk does not nest),
> that we are allowed to nest (calling aio_poll() and all this machinery) from
> co-routine.  Are we?  We can lead to deep nesting inside the following 
> sequence:
> ioq_submit() -> complete() -> aio_poll() -> ioq_submit() -> complete() ...
> but that can happen of course even without this patch.  I would say this 
> nesting
> is a clumsy stuff.
>
> Locally I have another series, which allow completions from ioq_submit() only
> if upper block devices does not nest (like virtio_blk), but I decided to send
> the current changes and not to overcomplicate things.
>
> --
> Roman