On Mon, Jul 12, 2021 at 10:43:07AM +0200, Emanuele Giuseppe Esposito wrote: > > > On 08/07/2021 12:50, Stefan Hajnoczi wrote: > > On Wed, Jul 07, 2021 at 06:58:09PM +0200, Emanuele Giuseppe Esposito wrote: > > > diff --git a/job.c b/job.c > > > index 872bbebb01..96fb8e9730 100644 > > > --- a/job.c > > > +++ b/job.c > > > @@ -32,6 +32,10 @@ > > > #include "trace/trace-root.h" > > > #include "qapi/qapi-events-job.h" > > > +/* job_mutex protexts the jobs list, but also the job operations. */ > > > +static QemuMutex job_mutex; > > > > It's unclear what protecting "job operations" means. I would prefer a > > fine-grained per-job lock that protects the job's fields instead of a > > global lock with an unclear scope. > > As I wrote in the cover letter, I wanted to try to keep things as simple as > possible with a global lock. It is possible to try and have a per-job lock, > but I don't know how complex will that be then. > I will try and see what I can do. > > Maybe "job_mutex protexts the jobs list, but also makes the job API > thread-safe"?
That's clearer, thanks. I thought "job operations" meant the processing that the actual block jobs do (commit, mirror, stream, backup). > > > > > > + > > > +/* Protected by job_mutex */ > > > static QLIST_HEAD(, Job) jobs = QLIST_HEAD_INITIALIZER(jobs); > > > /* Job State Transition Table */ > > > @@ -64,27 +68,22 @@ bool JobVerbTable[JOB_VERB__MAX][JOB_STATUS__MAX] = { > > > /* Transactional group of jobs */ > > > struct JobTxn { > > > - /* Is this txn being cancelled? */ > > > + /* Is this txn being cancelled? Atomic.*/ > > > bool aborting; > > > > The comment says atomic but this field is not accessed using atomic > > operations (at least at this point in the patch series)? > > Yes sorry I messed up the hunks in one-two patches. These comments were > supposed to be on patch 4 "job.h: categorize job fields". Even though that > might also not be ideal, since that patch just introduces the comments, > without applying the locking/protection yet. > On the other side, if I merge everything together in patch 5, it will be > even harder to read. The commit description can describe changes that currently have no effect but are anticipating a later patch. That helps reviewers understand whether the change is intentional/correct. Stefan
signature.asc
Description: PGP signature