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

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

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..

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


-- 
Best regards,
Vladimir