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

[Vladimir Sementsov-Ogievskiy]

On 7/25/22 10:38, 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*
>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

Honestly, you've changed the patch enough to drop my r-b.

> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>   include/qemu/job.h | 138 ++++++++++-

[..]

> +/* Called with job_mutex held, but releases it temporarily */
> +static int job_finalize_single_locked(Job *job)
>   {
> -    assert(job_is_completed(job));
> +    int job_ret;
> +
> +    assert(job_is_completed_locked(job));
>   
>       /* Ensure abort is called for late-transactional failures */
> -    job_update_rc(job);
> +    job_update_rc_locked(job);
> +
> +    job_unlock();

With this new variant of code you read job->ret not under mutex.. Is it correct?

Probably it's OK, as here we shouldn't race with something other.. But it's simple to just move 
job_unlock() to beginning of "if" body, and copy to the beginning of "else" 
body.

>   
>       if (!job->ret) {
>           job_commit(job);
> @@ -739,29 +895,37 @@ static int job_finalize_single(Job *job)
>       }
>       job_clean(job);
>   
> +    job_lock();
> +
>       if (job->cb) {
> -        job->cb(job->opaque, job->ret);
> +        job_ret = job->ret;
> +        job_unlock();
> +        job->cb(job->opaque, job_ret);
> +        job_lock();
>       }
>   

[..]


--
Best regards,
Vladimir