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

[Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>]

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