Re: [PATCH v5 09/20] jobs: add job lock in find_* functions

[Emanuele Giuseppe Esposito]

On 17/02/2022 16:00, Stefan Hajnoczi wrote:
> On Tue, Feb 08, 2022 at 09:35:02AM -0500, Emanuele Giuseppe Esposito wrote:
>> diff --git a/blockdev.c b/blockdev.c
>> index c5fba4d157..08408cd44b 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -3311,7 +3311,10 @@ out:
>>      aio_context_release(aio_context);
>>  }
>>  
>> -/* Get a block job using its ID and acquire its AioContext */
>> +/*
>> + * Get a block job using its ID and acquire its AioContext.
>> + * Returns with job_lock held on success.
> 
> The caller needs to deal with unlocking anyway, so maybe ask the caller
> to acquire the lock too? That would make the function simpler to reason
> about.

Those aiocontext locks there are going to be removed when job
lock/unlock are enabled, so it is useless to modify the logic now.
It makes sense to apply this logic with job_lock/unlock though.

> 
>> @@ -60,6 +65,7 @@ void qmp_job_cancel(const char *id, Error **errp)
>>      trace_qmp_job_cancel(job);
>>      job_user_cancel(job, true, errp);
>>      aio_context_release(aio_context);
>> +    job_unlock();
>>  }
> 
> Is job_mutex -> AioContext lock ordering correct? I thought the
> AioContext must be held before taking the job lock according to "jobs:
> remove aiocontext locks since the functions are under BQL"?
> 

I think you got it at this point, but anyways: job_lock is nop and when
it will be enabled, aio_context_acquire will go away in the same patch.

Thank you,
Emanuele