On 03/23/2017 01:39 PM, Paolo Bonzini wrote: > All block jobs are using block_job_defer_to_main_loop as the final > step just before the coroutine terminates. At this point, > block_job_enter should do nothing, but currently it restarts > the freed coroutine. > > Now, the job->co states should probably be changed to an enum > (e.g. BEFORE_START, STARTED, YIELDED, COMPLETED) subsuming
Yes, I'd love to formalize the FSM for jobs. > block_job_started, job->deferred_to_main_loop and job->busy. > For now, this patch eliminates the problematic reenter by > removing the reset of job->deferred_to_main_loop (which served > no purpose, as far as I could see) and checking the flag in > block_job_enter. Not sure -- the original commit 794f01414 makes it seem like it should stay so that the correct AIO context can be acquired. Probably a race as jobs don't often stay long once they've deferred to the main loop, but I think the reset is harmless as you say. > > Signed-off-by: Paolo Bonzini <pbonz...@redhat.com> > --- > blockjob.c | 10 ++++++++-- > include/block/blockjob_int.h | 3 ++- > 2 files changed, 10 insertions(+), 3 deletions(-) > > diff --git a/blockjob.c b/blockjob.c > index 3fa2885..2d80dae 100644 > --- a/blockjob.c > +++ b/blockjob.c > @@ -750,7 +750,14 @@ void block_job_resume_all(void) > > void block_job_enter(BlockJob *job) > { > - if (job->co && !job->busy) { > + if (!block_job_started(job)) { > + return; > + } > + if (job->deferred_to_main_loop) { > + return; > + } > + > + if (!job->busy) { > qemu_coroutine_enter(job->co); > } > } > @@ -874,7 +881,6 @@ static void block_job_defer_to_main_loop_bh(void *opaque) > aio_context_acquire(aio_context); > } > > - data->job->deferred_to_main_loop = false; > data->fn(data->job, data->opaque); > > if (aio_context != data->aio_context) { > diff --git a/include/block/blockjob_int.h b/include/block/blockjob_int.h > index 97ffc43..4d287ba 100644 > --- a/include/block/blockjob_int.h > +++ b/include/block/blockjob_int.h > @@ -227,7 +227,8 @@ typedef void BlockJobDeferToMainLoopFn(BlockJob *job, > void *opaque); > * @fn: The function to run in the main loop > * @opaque: The opaque value that is passed to @fn > * > - * Execute a given function in the main loop with the BlockDriverState > + * This function must be called by the main job coroutine just before it > + * returns. @fn is executed in the main loop with the BlockDriverState > * AioContext acquired. Block jobs must call bdrv_unref(), bdrv_close(), and > * anything that uses bdrv_drain_all() in the main loop. > * > Reviewed-by: John Snow <js...@redhat.com>