Re: [PATCH v3 14/16] job.c: use job_get_aio_context()

[Emanuele Giuseppe Esposito]

On 19/01/2022 11:31, Paolo Bonzini wrote:
> > diff --git a/blockjob.c b/blockjob.c
> > index cf1f49f6c2..468ba735c5 100644
> > --- a/blockjob.c
> > +++ b/blockjob.c
> > @@ -155,14 +155,16 @@ static void child_job_set_aio_ctx(BdrvChild *c, 
> > AioContext *ctx,
> >           bdrv_set_aio_context_ignore(sibling->bs, ctx, ignore);
> >       }
> > -    job->job.aio_context = ctx;
> > +    WITH_JOB_LOCK_GUARD() {
> > +        job->job.aio_context = ctx;
> > +    }
> >   }
> >   static AioContext *child_job_get_parent_aio_context(BdrvChild *c)
> >   {
> >       BlockJob *job = c->opaque;
> > -    return job->job.aio_context;
> > +    return job_get_aio_context(&job->job);
> >   }
> >   static const BdrvChildClass child_job = {
>
> Both called with BQL held, I think.

Yes, as their callbacks .get_parent_aio_context and .set_aio_context are 
defined as GS functions in block_int-common.h
> > @@ -218,19 +220,21 @@ int block_job_add_bdrv(BlockJob *job, const char 
> > *name, BlockDriverState *bs,
> >   {
> >       BdrvChild *c;
> >       bool need_context_ops;
> > +    AioContext *job_aiocontext;
> >       assert(qemu_in_main_thread());
> >       bdrv_ref(bs);
> > -    need_context_ops = bdrv_get_aio_context(bs) != job->job.aio_context;
> > +    job_aiocontext = job_get_aio_context(&job->job);
> > +    need_context_ops = bdrv_get_aio_context(bs) != job_aiocontext;
> > -    if (need_context_ops && job->job.aio_context != 
> > qemu_get_aio_context()) {
> > -        aio_context_release(job->job.aio_context);
> > +    if (need_context_ops && job_aiocontext != qemu_get_aio_context()) {
> > +        aio_context_release(job_aiocontext);
> >       }
> >       c = bdrv_root_attach_child(bs, name, &child_job, 0, perm, 
> > shared_perm, job,
> >                                  errp);
> > -    if (need_context_ops && job->job.aio_context != 
> > qemu_get_aio_context()) {
> > -        aio_context_acquire(job->job.aio_context);
> > +    if (need_context_ops && job_aiocontext != qemu_get_aio_context()) {
> > +        aio_context_acquire(job_aiocontext);
> >       }
> >       if (c == NULL) {
> >           return -EPERM;
>
> BQL held, too.

Wouldn't it be better to keep job_get_aio_context and implement like this:

AioContext *job_get_aio_context(Job *job)
{
    /*
     * Job AioContext can be written under BQL+job_mutex,
     * but can be read with just the BQL held.
     */
    assert(qemu_in_main_thread());
    return job->aio_context;
}

and instead job_set_aio_context:

void job_set_aio_context(Job *job, AioContext *ctx)
{
    JOB_LOCK_GUARD();
    assert(qemu_in_main_thread());
    job->aio_context = ctx;
}

(obviously implement also _locked version, if needed, and probably move 
the comment in get_aio_context in job.h).

Thank you,
Emanuele