[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v7 01/18] job.c: make job_mutex and job_lock/unlock() public
From: |
Emanuele Giuseppe Esposito |
Subject: |
Re: [PATCH v7 01/18] job.c: make job_mutex and job_lock/unlock() public |
Date: |
Tue, 28 Jun 2022 15:08:53 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.2.0 |
Am 24/06/2022 um 20:22 schrieb Vladimir Sementsov-Ogievskiy:
> I've already acked this (honestly, because Stefan do), but still, want
> to clarify:
>
> On 6/16/22 16:18, Emanuele Giuseppe Esposito wrote:
>> job mutex will be used to protect the job struct elements and list,
>> replacing AioContext locks.
>>
>> Right now use a shared lock for all jobs, in order to keep things
>> simple. Once the AioContext lock is gone, we can introduce per-job
>> locks.
>>
>> To simplify the switch from aiocontext to job lock, introduce
>> *nop* lock/unlock functions and macros.
>> We want to always call job_lock/unlock outside the AioContext locks,
>> and not vice-versa, otherwise we might get a deadlock.
>
> Could you describe here, why we get a deadlock?
>
> As I understand, we'll deadlock if two code paths exist simultaneously:
>
> 1. we take job mutex under aiocontext lock
> 2. we take aiocontex lock under job mutex
>
> If these paths exists, it's possible that one thread goes through [1]
> and another through [2]. If thread [1] holds job-mutex and want to take
> aiocontext-lock, and in the same time thread [2] holds aiocontext-lock
> and want to take job-mutext, that's a dead-lock.
>
> If you say, that we must avoid [1], do you have in mind that we have [2]
> somewhere? If so, this should be mentioned here
>
> If not, could we just make a normal mutex, not a noop?
Of course we have [2] somewhere, otherwise I wouldn't even think about
creating a noop function. This idea came up in v1-v2.
Regarding the specific case, I don't remember. But there are tons of
functions that are acquiring the AioContext lock and then calling job_*
API, such as job_cancel_sync in blockdev.c.
I might use job_cancel_sync as example and write it in the commit
message though.
Thank you,
Emanuele
>> This is not
>> straightforward to do, and that's why we start with nop functions.
>> Once everything is protected by job_lock/unlock, we can change the nop
>> into
>> an actual mutex and remove the aiocontext lock.
>>
>> Since job_mutex is already being used, add static
>> real_job_{lock/unlock} for the existing usage.
>>
>> Signed-off-by: Emanuele Giuseppe Esposito<eesposit@redhat.com>
>> Reviewed-by: Stefan Hajnoczi<stefanha@redhat.com>
>
>
[PATCH v7 16/18] jobs: protect job.aio_context with BQL and job_mutex, Emanuele Giuseppe Esposito, 2022/06/16
[PATCH v7 06/18] jobs: protect jobs with job_lock/unlock, Emanuele Giuseppe Esposito, 2022/06/16
[PATCH v7 13/18] job.h: define unlocked functions, Emanuele Giuseppe Esposito, 2022/06/16
[PATCH v7 07/18] jobs: add job lock in find_* functions, Emanuele Giuseppe Esposito, 2022/06/16
[PATCH v7 11/18] job.h: rename job API functions called with job_mutex held, Emanuele Giuseppe Esposito, 2022/06/16
[PATCH v7 02/18] job.h: categorize fields in struct Job, Emanuele Giuseppe Esposito, 2022/06/16