Re: [PATCH v9 11/21] jobs: group together API calls under the same job lock

[Vladimir Sementsov-Ogievskiy]

On 7/6/22 23:15, Emanuele Giuseppe Esposito wrote:
> Now that the API offers also _locked() functions, take advantage
> of it and give also the caller control to take the lock and call
> _locked functions.
>
> This makes sense especially when we have for loops, because it
> makes no sense to have:
>
> for(job = job_next(); ...)
>
> where each job_next() takes the lock internally.
> Instead we want
>
> JOB_LOCK_GUARD();
> for(job = job_next_locked(); ...)
>
> Note: at this stage, job_{lock/unlock} and job lock guard macros
> are *nop*.
>
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---

[..]

>   
> diff --git a/blockjob.c b/blockjob.c
> index 0d59aba439..bce05a9096 100644
> --- a/blockjob.c
> +++ b/blockjob.c
> @@ -99,7 +99,9 @@ static char *child_job_get_parent_desc(BdrvChild *c)
>   static void child_job_drained_begin(BdrvChild *c)
>   {
>       BlockJob *job = c->opaque;
> -    job_pause(&job->job);
> +    WITH_JOB_LOCK_GUARD() {
> +        job_pause_locked(&job->job);
> +    }

What's the reason for it? I'd keep job_pause().

(and it doesn't correspond to what commit subject presents.)

>   }
>   
>   static bool child_job_drained_poll(BdrvChild *c)
> @@ -111,8 +113,10 @@ static bool child_job_drained_poll(BdrvChild *c)
>       /* An inactive or completed job doesn't have any pending requests. Jobs
>        * with !job->busy are either already paused or have a pause point after
>        * being reentered, so no job driver code will run before they pause. */
> -    if (!job->busy || job_is_completed(job)) {
> -        return false;
> +    WITH_JOB_LOCK_GUARD() {
> +        if (!job->busy || job_is_completed_locked(job)) {
> +            return false;
> +        }
>       }

This doesn't correspond to commit subject. I'd put such things to separate commit 
"correct use of job_mutex in blockjob.c".

>   
>       /* Otherwise, assume that it isn't fully stopped yet, but allow the job to
> @@ -127,7 +131,9 @@ static bool child_job_drained_poll(BdrvChild *c)
>   static void child_job_drained_end(BdrvChild *c, int *drained_end_counter)
>   {
>       BlockJob *job = c->opaque;
> -    job_resume(&job->job);
> +    WITH_JOB_LOCK_GUARD() {
> +        job_resume_locked(&job->job);
> +    }
>   }

Again, don't see a reason for such change.


[my comments relate to more similar cases in the patch]


--
Best regards,
Vladimir