Re: [PATCH v10 18/21] job.c: enable job lock/unlock and remove Aiocontext locks

[Vladimir Sementsov-Ogievskiy]

On 8/18/22 10:46, Emanuele Giuseppe Esposito wrote:
> Am 17/08/2022 um 20:54 schrieb Vladimir Sementsov-Ogievskiy:
> > On 8/16/22 15:52, Emanuele Giuseppe Esposito wrote:
> > > > >     }
> > > > > @@ -501,8 +481,12 @@ void job_unref_locked(Job *job)>
> > > > > assert(!job->txn);
> > > > >               if (job->driver->free) {
> > > > > +            AioContext *aio_context = job->aio_context;
> > > > >                 job_unlock();
> > > > > +            /* FIXME: aiocontext lock is required because cb calls
> > > > > blk_unref */
> > > > > +            aio_context_acquire(aio_context);
> > > > >                 job->driver->free(job);
> > > > > +            aio_context_release(aio_context);
> > > > So, job_unref_locked() must be called with aio_context not locked,
> > > > otherwise we dead-lock here? That should be documented in function
> > > > declaration comment.
> > > >
> > > > For example in qemu-img.c in run_block_job() we do exactly that: call
> > > > job_unref_locked()  inside aio-context lock critical seaction..
> > > No, job_unref_locked has also status and refcnt and all the other fields
> > > that need to be protectd. Only free must be called without job lock held.
> >
> > I mean, don't we try here to acquire aio_context twice, when we call
> > job_unref_locked() with aio_context _already_ acquired?
> You're right, so why do we need the AioContext lock in qemu-img then?
> All jobs API don't need it, aio_poll seems not to need it either, and
> progress API has its own lock.
>
> I guess I could remove it?

Not sure, but hope that yes.

Anyway, trying to lock it twice will dead-lock, as I understand.


--
Best regards,
Vladimir