Re: [Qemu-devel] [PATCH v3] blockjob: Fix hang in block_job_finish_sync

[Stefan Hajnoczi]

On Mon, Feb 01, 2016 at 10:49:00AM +0800, Fam Zheng wrote:
> On Fri, 01/29 11:31, Stefan Hajnoczi wrote:
> > On Fri, Jan 29, 2016 at 10:19:49AM +0800, Fam Zheng wrote:
> > > @@ -402,6 +407,10 @@ typedef void BlockJobDeferToMainLoopFn(BlockJob 
> > > *job, void *opaque);
> > >   * AioContext acquired.  Block jobs must call bdrv_unref(), 
> > > bdrv_close(), and
> > >   * anything that uses bdrv_drain_all() in the main loop.
> > >   *
> > > + * The job->deferred_to_main_loop flag will be set. Caller must clear it 
> > > once
> > > + * the deferred work is done and the block job coroutine continues, 
> > > unless it's
> > > + * completing immediately.
> > > + *
> > 
> > It's not necessary to expose job->deferred_to_main_loop to the user.
> > Just clear it:
> > 
> > static void block_job_defer_to_main_loop_bh(void *opaque)
> > {
> >     BlockJobDeferToMainLoopData *data = opaque;
> >     AioContext *aio_context;
> > 
> >     qemu_bh_delete(data->bh);
> > 
> >     /* Prevent race with block_job_defer_to_main_loop() */
> >     aio_context_acquire(data->aio_context);
> > 
> >     /* Fetch BDS AioContext again, in case it has changed */
> >     aio_context = bdrv_get_aio_context(data->job->bs);
> >     aio_context_acquire(aio_context);
> > 
> >     data->fn(data->job, data->opaque);
> >     job->deferred_to_main_loop = false;  /* <----- HERE */
> 
> Maybe move one line above in case data->fn() does another
> block_job_defer_to_main_loop()?

Yes, good point.  Thanks for spotting the bug.

It's safe to clear the boolean as soon as we acquire aio_context.

Stefan

signature.asc
Description: PGP signature