[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v8 13/20] jobs: group together API calls under the same job l
From: |
Emanuele Giuseppe Esposito |
Subject: |
Re: [PATCH v8 13/20] jobs: group together API calls under the same job lock |
Date: |
Tue, 5 Jul 2022 15:01:30 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.2.0 |
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.
>>