On Tue, Jul 19, 2016 at 1:18 PM, Roman Penyaev <[email protected]> wrote: > On Tue, Jul 19, 2016 at 12:36 PM, Paolo Bonzini <[email protected]> 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
