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

[Kevin Wolf]

Am 17.08.2022 um 11:35 hat Emanuele Giuseppe Esposito geschrieben:
> 
> 
> Am 17/08/2022 um 10:46 schrieb Kevin Wolf:
> >>>> @@ -475,13 +477,15 @@ void *block_job_create(const char *job_id, const 
> >>>> BlockJobDriver *driver,
> >>>>      job->ready_notifier.notify = block_job_event_ready;
> >>>>      job->idle_notifier.notify = block_job_on_idle;
> >>>>  
> >>>> -    notifier_list_add(&job->job.on_finalize_cancelled,
> >>>> -                      &job->finalize_cancelled_notifier);
> >>>> -    notifier_list_add(&job->job.on_finalize_completed,
> >>>> -                      &job->finalize_completed_notifier);
> >>>> -    notifier_list_add(&job->job.on_pending, &job->pending_notifier);
> >>>> -    notifier_list_add(&job->job.on_ready, &job->ready_notifier);
> >>>> -    notifier_list_add(&job->job.on_idle, &job->idle_notifier);
> >>>> +    WITH_JOB_LOCK_GUARD() {
> >>>> +        notifier_list_add(&job->job.on_finalize_cancelled,
> >>>> +                          &job->finalize_cancelled_notifier);
> >>>> +        notifier_list_add(&job->job.on_finalize_completed,
> >>>> +                          &job->finalize_completed_notifier);
> >>>> +        notifier_list_add(&job->job.on_pending, &job->pending_notifier);
> >>>> +        notifier_list_add(&job->job.on_ready, &job->ready_notifier);
> >>>> +        notifier_list_add(&job->job.on_idle, &job->idle_notifier);
> >>>> +    }
> >>>>  
> >>>>      error_setg(&job->blocker, "block device is in use by block job: %s",
> >>>>                 job_type_str(&job->job));
> >>> Why is this the right scope for the lock? It looks very arbitrary to
> >>> lock only here, but not for the assignments above or the function calls
> >>> below.
> >>>
> >>> Given that job_create() already puts the job in the job_list so it
> >>> becomes visible for other code, should we not keep the job lock from the
> >>> moment that we create the job until it is fully initialised?
> >> I try to protect only what needs protection, nothing more. Otherwise
> >> then it is not clear what are we protecting and why. According to the
> >> split I made in job.h, things like job_type_str and whatever I did not
> >> protect are not protected because they don't need the lock.
> > I think the last paragraph above explains what it would protect?
> > 
> > Having a half-initialised job in the job list without holding the lock
> > sounds dangerous to me. Maybe it's actually okay in practice because
> > this is GLOBAL_STATE_CODE() and we can assume that code accessing
> > the job list outside of the main thread probably skips over the
> > half-initialised job, but it's another case where relying on the BQL is
> > confusing when there would be a more specific lock for it.
> > 
> 
> Ok, but this would imply:
> 1. create job_create_locked()
> 2. still drop the lock when calling block_job_add_bdrv(), since I am
> pretty sure it can drain. So we still split the function in two (or
> maybe three, if we need to reaqiure the lock after) parts.

Ah, crap. Temporarily dropping the lock makes it useless, the incomplete
state is then still visible to the outside. So the locked section would
have to end before it.

Maybe just leave it as it is then.

Kevin