Re: [Qemu-devel] [PATCH 02/20] blockjob: introduce .drain callback for jobs

[Paolo Bonzini]

----- Original Message -----
> From: "Vladimir Sementsov-Ogievskiy" <address@hidden>
> To: "Paolo Bonzini" <address@hidden>, address@hidden
> Cc: address@hidden, address@hidden, address@hidden
> Sent: Friday, December 2, 2016 3:01:30 PM
> Subject: Re: [Qemu-devel] [PATCH 02/20] blockjob: introduce .drain callback 
> for jobs
> 
> 27.10.2016 13:48, Paolo Bonzini wrote:
> > This is required to decouple block jobs from running in an
> > AioContext.  With multiqueue block devices, a BlockDriverState
> > does not really belong to a single AioContext.
> >
> > The solution is to first wait until all I/O operations are
> > complete; then loop in the main thread for the block job to
> > complete entirely.
>
> Looks like I have a problem with this. block_job_drain enters the job
> only if job.busy = false. But what if job yielded with busy = true?
> 
> My case is the following: in the job I call co_aio_sleep_ns() for some
> time without setting job.busy to false, and it looks like timer doesn't
> work while we are in "while() { block_job_drain() }" loop. If I just set
> "job.busy = false" and "job.busy = true" around co_aio_sleep_ns() all
> start to work.
> 
> I don't want set job.busy to false, because actually job is working -
> several additional coroutines do their work, only the main one (job.co)
> do nothing. I can remove timer, and make other coroutines wake up the
> main one when it needed, and, anyway it looks like better way..
> 
> But the question is: is it ok, that we can't use sleep timer in the job,
> without setting busy = true? Is it right that only io can wake up block
> job coroutine, if it yielded without setting busy=false?

That's more or less correct.  See for example mirror.c.  Whenever it yields
with busy=false, mirror_write_complete will wake the coroutine.

Note that it's also okay to use co_aio_sleep_ns if I/O and _also_ wake the
coroutine on I/O.  Reentering a coroutine automatically interrupts the sleep.

Paolo

> > +    while (!job->deferred_to_main_loop && !job->completed) {
> > +        block_job_drain(job);
> > +    }
> >       while (!job->completed) {
> > -        aio_poll(block_job_get_aio_context(job), true);
> > +        aio_poll(qemu_get_aio_context(), true);
> >       }
> >       ret = (job->cancelled && job->ret == 0) ? -ECANCELED : job->ret;
> >       block_job_unref(job);
> >
> 
> 
> --
> Best regards,
> Vladimir
> 
>