Re: [RFC PATCH 0/6] job: replace AioContext lock with job_mutex

[Emanuele Giuseppe Esposito]

On 08/07/2021 15:09, Stefan Hajnoczi wrote:
> On Wed, Jul 07, 2021 at 06:58:07PM +0200, Emanuele Giuseppe Esposito wrote:
> > This is a continuation on the work to reduce (and possibly get rid of) the 
> > usage of AioContext lock, by introducing smaller granularity locks to keep the 
> > thread safety.
> >
> > This series aims to:
> > 1) remove the aiocontext lock and substitute it with the already existing
> >     global job_mutex
> > 2) fix what it looks like to be an oversight when moving the blockjob.c logic
> >     into the more generic job.c: job_mutex was introduced especially to
> >     protect job->busy flag, but it seems that it was not used in successive
> >     patches, because there are multiple code sections that directly
> >     access the field without any locking.
> > 3) use job_mutex instead of the aiocontext_lock
> > 4) extend the reach of the job_mutex to protect all shared fields
> >     that the job structure has.
>
> Can you explain the big picture:
>
> 1. What are the rules for JobDrivers? Imagine you are implementing a new
>     JobDriver. What do you need to know in order to write correct code?

I think that in general, the rules for JobDrivers remain the same. The 
job_mutex lock should be invisible (or almost) from the point of view of 
a JobDriver, because the job API available for it should take care of 
the necessary locking/unlocking.

> 2. What are the rules for monitor? The main pattern is looking up a job,
>     invoking a job API on it, and then calling job_unlock().

The monitor instead is aware of this lock: the reason for that is 
exactly what you have described here.
Looking up + invoking a job API operation (for example calling 
find_job() and then job_pause() ) must be performed with the same lock 
hold all the time, otherwise other threads could modify the job while 
the monitor runs its command.

Please let me know if something is not clear and/or if you have 
additional comments on this!

Emanuele

> Stefan