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
