[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 3/3] linux-aio: process completions from ioq_sub
From: |
Roman Penyaev |
Subject: |
Re: [Qemu-devel] [PATCH 3/3] linux-aio: process completions from ioq_submit() |
Date: |
Tue, 19 Jul 2016 13:44:38 +0200 |
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