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

[Emanuele Giuseppe Esposito]

Am 11/07/2022 um 15:26 schrieb Vladimir Sementsov-Ogievskiy:
>>   }
>>     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]

I think you misunderstood: I aim to group API calls under the same lock.
One application of such grouping involves loops, but not only them.

Regarding the single-line WITH_JOB_LOCK_GUARD (job_next, job_pause,
job_resume and similar), I guess I will keep the not-locked function.

Emanuele