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

[Paolo Bonzini]

On Thu, Sep 22, 2022 at 4:42 PM Emanuele Giuseppe Esposito
<eesposit@redhat.com> wrote:
>
> Am 18/09/2022 um 19:12 schrieb Emanuele Giuseppe Esposito:
> >> In replication_stop, we call job_cancel_sync() inside
> >> aio_context_acquire - aio_context_release section. Should it be fixed?
>
> > I don't think it breaks anything. The question is: what is the
> > aiocontext there protecting? Do we need it? I have no idea.
>
> Ok Paolo reminded me that job_cancel_sync() calls
> AIO_WAIT_WHILE_UNLOCKED. This means that it indeed needs to be wrapped
> in an aiocontext release() + acquire() block.
>
> >> Another question, sometimes you move job_start out of
> >> aio-context-acquire critical section, sometimes not. As I understand,
> >> it's of for job_start to be called both with acquired aio-context or not
> >> acquired?
> >>
> > Same as above, job_start does not need the aiocontext to be taken
> > (otherwise job_lock would be useless).
>
> I still think here it does not matter if the aiocontext is taken or not.

What matters is that the AioContext lock is taken either inside or
outside the job_lock.

job_start() takes the job mutex, so you _always_ need to ensure that
the job mutex is taken inside AioContext lock and never the opposite.

>From quick grep of AIO_WAIT_WHILE_UNLOCKED(), job_finish_sync_locked()
is the only user and the only place where the two locks interact. It
is explicitly called with AioContext locks _not_ taken, and releases
job_lock when the AioContext lock might be taken inside
AIO_WAIT_WHILE_UNLOCKED(); so it should be fine. This also validates
the idea of AIO_WAIT_WHILE_UNLOCKED().

Paolo