Re: [PATCH v8 08/20] blockjob.h: introduce block_job _locked() APIs

[Vladimir Sementsov-Ogievskiy]

On 7/6/22 15:59, Emanuele Giuseppe Esposito wrote:
> Am 06/07/2022 um 14:36 schrieb Emanuele Giuseppe Esposito:
> > Am 06/07/2022 um 14:23 schrieb Vladimir Sementsov-Ogievskiy:
> > > On 7/6/22 15:05, Emanuele Giuseppe Esposito wrote:
> > > > Am 05/07/2022 um 17:01 schrieb Vladimir Sementsov-Ogievskiy:
> > > > > On 6/29/22 17:15, Emanuele Giuseppe Esposito wrote:
> > > > > > Just as done with job.h, create _locked() functions in blockjob.h
> > > > >
> > > > > We modify not only blockjob.h, I'd s/blockjob.h/blockjob/ in subject.
> > > > >
> > > > > Also, we start to introduce _locked block_job_* APIs.
> > > > >
> > > > > Does it mean that BlockJob and Job share the global mutex to protect
> > > > > themselves? Than I think we should document in BlockJob struct what is
> > > > > protected by job_mutex.
> > > >
> > > > There is nothing in the struct (apart from Job) that is protected by the
> > > > job lock. I can add a comment "Protected by job mutex" on top of Job job
> > > > field?
> > >
> > > Yes, I think that's worth doing.
> > >
> > > Other fields doesn't need the lock?
> > Well I didn't plan to actually look at it but now that you ask:
> >
> > /** needs protection, so it can go under job lock */
> > BlockDeviceIoStatus iostatus;
> >
> > /** mostly under lock, not sure when it is called as notifier callback
> > though. I think they are GLOBAL_STATE, what do you think?  */
> > int64_t speed;

Hmm I doubt that notifier callbacks are always called from GS code.. But 
reading .speed to send an event doesn't seem to worth any locking.

> > /** thread safe API */
> > RateLimit limit;
> >
> > /** I think it's also thread safe */
> > Error *blocker;
> >
> > /* always under job lock */
> Actually that's wrong, they are just set once and never modified.
>
> And GSList *nodes; is also always called under GS.
>
> So there's only iostatus to protect and maybe speed.




--
Best regards,
Vladimir