On Tue, Jul 19, 2016 at 12:36 PM, Paolo Bonzini <pbonz...@redhat.com> 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.
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
