Re: [PATCH v5 18/20] jobs: protect job.aio_context with BQL and job_mutex

[Stefan Hajnoczi]

On Tue, Feb 08, 2022 at 09:35:11AM -0500, Emanuele Giuseppe Esposito wrote:
>  static AioContext *child_job_get_parent_aio_context(BdrvChild *c)
>  {
>      BlockJob *job = c->opaque;
> +    assert(qemu_in_main_thread());
>  
>      return job->job.aio_context;
>  }

It's not clear to me that .get_parent_aio_context() should only be
called from the main thread. The API is read-only so someone might try
to call from I/O code in the future expecting it to work like other
read-only graph APIs that are available from I/O code.

Currently the assertion is true because the only user is
bdrv_attach_child_*() but please document this invariant for
bdrv_child_get_parent_aio_context() and the callback. Maybe move the
assertion into a higher-level function like
bdrv_child_get_parent_aio_context() (if that still covers all cases).

> diff --git a/include/qemu/job.h b/include/qemu/job.h
> index dfbf2ea501..ca46e46f5b 100644
> --- a/include/qemu/job.h
> +++ b/include/qemu/job.h
> @@ -75,7 +75,12 @@ typedef struct Job {
>      ProgressMeter progress;
>  
>  
> -    /** AioContext to run the job coroutine in */
> +    /**
> +     * AioContext to run the job coroutine in.
> +     * This field can be read when holding either the BQL (so we are in
> +     * the main loop) or the job_mutex.
> +     * Instead, it can be only written when we hold *both* BQL and job_mutex.

s/Instead,//

(It sounds weird because "instead" means "replacement" or "substitution"
We're comparing "read" and "write" here, not substituting them.
Something like "on the other hand" or "conversely" works.)

> +     */
>      AioContext *aio_context;
>  
>      /** Reference count of the block job */
> @@ -706,4 +711,16 @@ void job_dismiss_locked(Job **job, Error **errp);
>  int job_finish_sync_locked(Job *job, void (*finish)(Job *, Error **errp),
>                             Error **errp);
>  
> +/**
> + * Sets the @job->aio_context.
> + * Called with job_mutex *not* held.
> + *
> + * This function must run in the main thread to protect against
> + * concurrent read in job_finish_sync_locked(),
> + * takes the job_mutex lock to protect against the read in
> + * job_do_yield_locked(), and must be called when the coroutine
> + * is quiescent.
> + */
> +void job_set_aio_context(Job *job, AioContext *ctx);
> +
>  #endif
> diff --git a/job.c b/job.c
> index f05850a337..7a07d25ec3 100644
> --- a/job.c
> +++ b/job.c
> @@ -354,6 +354,17 @@ Job *job_get_locked(const char *id)
>      return NULL;
>  }
>  
> +void job_set_aio_context(Job *job, AioContext *ctx)
> +{
> +    /* protect against read in job_finish_sync_locked and job_start */
> +    assert(qemu_in_main_thread());
> +    /* protect against read in job_do_yield_locked */
> +    JOB_LOCK_GUARD();
> +    /* ensure the coroutine is quiescent while the AioContext is changed */
> +    assert(job->pause_count > 0);
> +    job->aio_context = ctx;
> +}
> +
>  /* Called with job_mutex *not* held. */
>  static void job_sleep_timer_cb(void *opaque)
>  {
> @@ -1256,6 +1267,7 @@ int job_finish_sync_locked(Job *job, void (*finish)(Job 
> *, Error **errp),
>  {
>      Error *local_err = NULL;
>      int ret;
> +    assert(qemu_in_main_thread());
>  
>      job_ref_locked(job);
>  
> -- 
> 2.31.1
>

signature.asc
Description: PGP signature