Re: [Qemu-devel] [Qemu-block] [PATCH 2/2] blockjob: Fix hang in block_job_finish_sync

[Stefan Hajnoczi]

On Thu, Jan 28, 2016 at 10:10:06AM +0100, Paolo Bonzini wrote:
> 
> 
> On 28/01/2016 08:02, Fam Zheng wrote:
> > 
> > This is because the aio_poll() only processes the AIO context of bs
> > which has no more work to do, while the main loop BH that is scheduled
> > for setting the job->completed flag is never processed.
> > 
> > Fix this by adding a "ctx" pointer in BlockJob structure, to track which
> > context to poll for the block job to make progress. Its value is set to
> > the BDS context at block job creation, until
> > block_job_coroutine_complete() is called by the block job coroutine.
> > After that point, the block job's work is deferred to main loop BH.
> > 
> > Signed-off-by: Fam Zheng <address@hidden>
> > ---
> >  blockjob.c               | 4 +++-
> >  include/block/blockjob.h | 2 ++
> >  2 files changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/blockjob.c b/blockjob.c
> > index 4b16720..4ea1ce0 100644
> > --- a/blockjob.c
> > +++ b/blockjob.c
> > @@ -74,6 +74,7 @@ void *block_job_create(const BlockJobDriver *driver, 
> > BlockDriverState *bs,
> >      job->opaque        = opaque;
> >      job->busy          = true;
> >      job->refcnt        = 1;
> > +    job->ctx           = bdrv_get_aio_context(bs);
> 
> Can the context change if dataplane is started/stopped in the middle of
> a job?  (For example if you start migration).  Perhaps job->ctx == NULL
> could mean "use bdrv_get_aio_context(bs)".

Yes, it can change.

Normally a block job doesn't need to know its AioContext since the
coroutine only runs from inside the AioContext anyway.

Perhaps it's easier to make this special case explicit with a flag in
job->deferred_to_main_loop so the synchronous wait can do:

while (!job->completed) {
    AioContext *ctx = job->deferred_to_main_loop ?
                      qemu_get_aio_context() :
                      bdrv_get_aio_context(bs);

    aio_poll(ctx, true);
}

That way there's no need to store a possibly stale AioContext pointer.

Stefan

signature.asc
Description: PGP signature