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

[Vladimir Sementsov-Ogievskiy]

On 7/19/22 15:40, Emanuele Giuseppe Esposito wrote:
> 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.

I mean that pre-patch job->busy is accessed without any lock at all. So we not 
just group correctly locked calls into one critical section, we also fix direct 
field accesses to be under lock.


> 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


--
Best regards,
Vladimir