Re: [PATCH v9 07/21] blockjob: introduce block_job _locked() APIs

[Emanuele Giuseppe Esposito]

Am 11/07/2022 um 14:04 schrieb Vladimir Sementsov-Ogievskiy:
> On 7/6/22 23:15, Emanuele Giuseppe Esposito wrote:
>> Just as done with job.h, create _locked() functions in blockjob.h
>>
>> These functions will be later useful when caller has already taken
>> the lock. All blockjob _locked functions call job _locked functions.
>>
>> Note: at this stage, job_{lock/unlock} and job lock guard macros
>> are *nop*.
>>
>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> 
> We not only create _locked() interfaces, but also make some functions
> correct relatively to job_mutex that was not correct pre-patch, for
> example:
> 
> block_job_iostatus_reset(): the function doesn't call any job_* APIs,
> but it access Job fields. After patch fields are accessed under mutex
> which is correct, and that's the significant change worth mentioning in
> commit message.
> 
> So, more correct is to say, that we make some functions of blockjob API
> correct relatively to job_mutex and Job fields.
> 
> With at least this clarified, I'm OK with the patch as is:
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> 
> 
> What kept in mind after the patch:
> 
> 1. Some functions still need an update, for example
> block_job_error_action, that access Job.user_pause without locking the
> job_mutex. Or, block_job_event_completed that accesses Job.ret..

This comes afterward, I didn't check the patches but the end result
covers what you mention above.

> 
> 2. What about BlockJob.nodes field? Shouldn't we protect it by the mutex?
> 

As I wrote in the comment in patch 17, seems to be always modified under
GLOBAL_STATE_CODE.

Thank you,
Emanuele