[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v5 02/20] job.h: categorize fields in struct Job
From: |
Emanuele Giuseppe Esposito |
Subject: |
Re: [PATCH v5 02/20] job.h: categorize fields in struct Job |
Date: |
Fri, 11 Feb 2022 10:00:45 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.2.0 |
On 10/02/2022 18:35, Stefan Hajnoczi wrote:
> On Thu, Feb 10, 2022 at 05:26:52PM +0100, Emanuele Giuseppe Esposito wrote:
>>
>>
>> On 10/02/2022 16:40, Stefan Hajnoczi wrote:
>>> On Tue, Feb 08, 2022 at 09:34:55AM -0500, Emanuele Giuseppe Esposito wrote:
>>>> Categorize the fields in struct Job to understand which ones
>>>> need to be protected by the job mutex and which don't.
>>>>
>>>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
>>>> ---
>>>> include/qemu/job.h | 59 ++++++++++++++++++++++++++--------------------
>>>> 1 file changed, 34 insertions(+), 25 deletions(-)
>>>>
>>>> diff --git a/include/qemu/job.h b/include/qemu/job.h
>>>> index d1192ffd61..86ec46c09e 100644
>>>> --- a/include/qemu/job.h
>>>> +++ b/include/qemu/job.h
>>>> @@ -40,27 +40,50 @@ typedef struct JobTxn JobTxn;
>>>> * Long-running operation.
>>>> */
>>>> typedef struct Job {
>>>> +
>>>> + /* Fields set at initialization (job_create), and never modified */
>>>
>>> Is there a corresponding "Field protected by job_mutex" comment that
>>> separates fields that need locking?
>>>
>>
>> That would be the comment
>>
>> /** Protected by job_mutex */
>>
>> situated right after the field "ProgressMeter progress;".
>>
>> Do you want me to change it in "Fields protected by job_mutex"?
>
> I don't see it:
>
> + /** The opaque value that is passed to the completion function. */
> + void *opaque;
> +
> + /* ProgressMeter API is thread-safe */
> + ProgressMeter progress;
> +
> +
> + /** AioContext to run the job coroutine in */
> + AioContext *aio_context;
> +
> + /** Reference count of the block job */
> + int refcnt;
>
> Is it added by a later patch or did I miss it?
>
Yes sorry I forgot: it is added in patch 19, when the lock is not a nop
anymore.
I think this was suggested in one of the previous reviews, to avoid
having a misleading comment when the fields are not yet protected.
Emanuele
- [PATCH v5 03/20] job.c: API functions not used outside should be static, (continued)
[PATCH v5 05/20] aio-wait.h: introduce AIO_WAIT_WHILE_UNLOCKED, Emanuele Giuseppe Esposito, 2022/02/08
[PATCH v5 08/20] jobs: protect jobs with job_lock/unlock, Emanuele Giuseppe Esposito, 2022/02/08
[PATCH v5 10/20] jobs: use job locks also in the unit tests, Emanuele Giuseppe Esposito, 2022/02/08
[PATCH v5 11/20] block/mirror.c: use of job helpers in drivers to avoid TOC/TOU, Emanuele Giuseppe Esposito, 2022/02/08