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

[Emanuele Giuseppe Esposito]

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.

Is that what you had in mind?

Emanuele