Re: [PATCH v8 13/20] jobs: group together API calls under the same job lock

[Emanuele Giuseppe Esposito <eesposit@redhat.com>]

Am 05/07/2022 um 10:17 schrieb Emanuele Giuseppe Esposito:
> 
> 
> Am 05/07/2022 um 10:14 schrieb Stefan Hajnoczi:
>> On Wed, Jun 29, 2022 at 10:15:31AM -0400, Emanuele Giuseppe Esposito wrote:
>>> diff --git a/blockdev.c b/blockdev.c
>>> index 71f793c4ab..5b79093155 100644
>>> --- a/blockdev.c
>>> +++ b/blockdev.c
>>> @@ -150,12 +150,15 @@ void blockdev_mark_auto_del(BlockBackend *blk)
>>>          return;
>>>      }
>>>  
>>> -    for (job = block_job_next(NULL); job; job = block_job_next(job)) {
>>> +    JOB_LOCK_GUARD();
>>> +
>>> +    for (job = block_job_next_locked(NULL); job;
>>> +         job = block_job_next_locked(job)) {
>>>          if (block_job_has_bdrv(job, blk_bs(blk))) {
>>>              AioContext *aio_context = job->job.aio_context;
>>>              aio_context_acquire(aio_context);
>>
>> Is there a lock ordering rule for job_mutex and the AioContext lock? I
>> haven't audited the code, but there might be ABBA lock ordering issues.
> 
> Doesn't really matter here, as lock is nop. To be honest I forgot which
> one should go first, probably job_lock because the aiocontext lock can
> be taken and released in callbacks.
> 
> Should I resend with ordering fixed? Just to have a consistent logic

Well actually how do I fix that? I would just add useless additional
changes into the diff, because for example in the case below I am not
even sure what exactly is the aiocontext protecting.

So I guess I'll leave as it is. I will just update the commit message to
make sure it is clear that the lock is nop and ordering is mixed.

Thank you,
Emanuele
> 
>>
>>> diff --git a/qemu-img.c b/qemu-img.c
>>> index 4cf4d2423d..289d88a156 100644
>>> --- a/qemu-img.c
>>> +++ b/qemu-img.c
>>> @@ -912,25 +912,30 @@ static void run_block_job(BlockJob *job, Error **errp)
>>>      int ret = 0;
>>>  
>>>      aio_context_acquire(aio_context);
>>> -    job_ref(&job->job);
>>> -    do {
>>> -        float progress = 0.0f;
>>> -        aio_poll(aio_context, true);
>>> +    WITH_JOB_LOCK_GUARD() {
>>
>> Here the lock order is the opposite of above.
>>