[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [RFC PATCH 2/6] job: _locked functions and public job_lock/unlock fo
From: |
Stefan Hajnoczi |
Subject: |
Re: [RFC PATCH 2/6] job: _locked functions and public job_lock/unlock for next patch |
Date: |
Tue, 13 Jul 2021 14:32:14 +0100 |
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
- [RFC PATCH 0/6] job: replace AioContext lock with job_mutex, Emanuele Giuseppe Esposito, 2021/07/07
- [RFC PATCH 1/6] job: use getter/setters instead of accessing the Job fields directly, Emanuele Giuseppe Esposito, 2021/07/07
- [RFC PATCH 4/6] job.h: categorize job fields, Emanuele Giuseppe Esposito, 2021/07/07
- [RFC PATCH 5/6] job: use global job_mutex to protect struct Job, Emanuele Giuseppe Esposito, 2021/07/07
- [RFC PATCH 6/6] jobs: remove unnecessary AioContext aquire/release pairs, Emanuele Giuseppe Esposito, 2021/07/07
- Re: [RFC PATCH 0/6] job: replace AioContext lock with job_mutex, Stefan Hajnoczi, 2021/07/08