Re: [PATCH v9 05/21] job.c: add job_lock/unlock while keeping job.h intact

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

On 7/6/22 23:15, Emanuele Giuseppe Esposito wrote:
> With "intact" we mean that all job.h functions implicitly
> take the lock. Therefore API callers are unmodified.
> 
> This means that:
> - many static functions that will be always called with job lock held
>    become _locked, and call _locked functions
> - all public functions take the lock internally if needed, and call _locked
>    functions
> - all public functions called internally by other functions in job.c will have a
>    _locked counterpart (sometimes public), to avoid deadlocks (job lock already taken).
>    These functions are not used for now.
> - some public functions called only from exernal files (not job.c) do not
>    have _locked() counterpart and take the lock inside. Others won't need
>    the lock at all because use fields only set at initialization and
>    never modified.
> 
> job_{lock/unlock} is independent from real_job_{lock/unlock}.
> 
> Note: at this stage, job_{lock/unlock} and job lock guard macros
> are *nop*
> 
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

I think, we still lack some comments on function lock-related interface, but it may be improved later.

[..]

>   
> -static int job_txn_apply(Job *job, int fn(Job *))
> +/* Called with job_mutex held, but releases it temporarily. */

Hmm. Yes, it may release it temprorarily when fn() release it.. Not very clear but OK..

> +static int job_txn_apply_locked(Job *job, int fn(Job *))
>   {
>       AioContext *inner_ctx;
>       Job *other_job, *next;
> @@ -170,7 +182,7 @@ static int job_txn_apply(Job *job, int fn(Job *))
>        * we need to release it here to avoid holding the lock twice - which would
>        * break AIO_WAIT_WHILE from within fn.
>        */
> -    job_ref(job);
> +    job_ref_locked(job);
>       aio_context_release(job->aio_context);
>   

[..]

> +
>   static bool job_started(Job *job)

So we can call it both with mutex locked and without. Hope it never race with job_start.

>   {
>       return job->co;
>   }
>   
> -static bool job_should_pause(Job *job)
> +/* Called with job_mutex held. */

[..]

>   
> -/** Useful only as a type shim for aio_bh_schedule_oneshot. */
> +/**
> + * Useful only as a type shim for aio_bh_schedule_oneshot.
> + * Called with job_mutex *not* held, but releases it temporarily.

", but releases it temprorarily" is misleading for me. If called with mutext not held, then "releases it temprorarily" is not part of function interface.. Many functions that take some mutex internally do release it temporarily and callers should not care of it.

So, better just "Called with job_mutex *not* held."

> + */
>   static void job_exit(void *opaque)
>   {
>       Job *job = (Job *)opaque;
>       AioContext *ctx;
> +    JOB_LOCK_GUARD();
>   



-- 
Best regards,
Vladimir