Re: [PATCH v7 02/18] job.h: categorize fields in struct Job

[Vladimir Sementsov-Ogievskiy]

On 6/16/22 16:18, 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 | 61 +++++++++++++++++++++++++++-------------------
>   1 file changed, 36 insertions(+), 25 deletions(-)
>
> diff --git a/include/qemu/job.h b/include/qemu/job.h
> index d1192ffd61..876e13d549 100644
> --- a/include/qemu/job.h
> +++ b/include/qemu/job.h
> @@ -40,27 +40,52 @@ typedef struct JobTxn JobTxn;
>    * Long-running operation.
>    */
>   typedef struct Job {
> +
> +    /* Fields set at initialization (job_create), and never modified */
> +
>       /** The ID of the job. May be NULL for internal jobs. */
>       char *id;
>   
> -    /** The type of this job. */
> +    /**
> +     * The type of this job.
> +     * All callbacks are called with job_mutex *not* held.
> +     */
>       const JobDriver *driver;
>   
> -    /** Reference count of the block job */
> -    int refcnt;
> -
> -    /** Current state; See @JobStatus for details. */
> -    JobStatus status;
> -
> -    /** AioContext to run the job coroutine in */
> -    AioContext *aio_context;
> -
>       /**
>        * The coroutine that executes the job.  If not NULL, it is reentered when
>        * busy is false and the job is cancelled.
> +     * Initialized in job_start()
>        */
>       Coroutine *co;
>   
> +    /** True if this job should automatically finalize itself */
> +    bool auto_finalize;
> +
> +    /** True if this job should automatically dismiss itself */
> +    bool auto_dismiss;
> +
> +    /** The completion function that will be called when the job completes.  */
> +    BlockCompletionFunc *cb;
> +
> +    /** The opaque value that is passed to the completion function.  */
> +    void *opaque;
> +
> +    /* ProgressMeter API is thread-safe */
> +    ProgressMeter progress;
> +
> +
> +    /** Protected by AioContext lock */

Previous groups stats with '/*'. Should /** be substituted by /* ?

> +
> +    /** AioContext to run the job coroutine in */
> +    AioContext *aio_context;

Not sure how much is it protected. Probably we read it without locking. But 
that should go away anyway.

> +
> +    /** Reference count of the block job */
> +    int refcnt;
> +
> +    /** Current state; See @JobStatus for details. */
> +    JobStatus status;
> +
>       /**
>        * Timer that is used by @job_sleep_ns. Accessed under job_mutex (in
>        * job.c).
> @@ -112,14 +137,6 @@ typedef struct Job {
>       /** Set to true when the job has deferred work to the main loop. */
>       bool deferred_to_main_loop;
>   
> -    /** True if this job should automatically finalize itself */
> -    bool auto_finalize;
> -
> -    /** True if this job should automatically dismiss itself */
> -    bool auto_dismiss;
> -
> -    ProgressMeter progress;
> -
>       /**
>        * Return code from @run and/or @prepare callback(s).
>        * Not final until the job has reached the CONCLUDED status.
> @@ -134,12 +151,6 @@ typedef struct Job {
>        */
>       Error *err;
>   
> -    /** The completion function that will be called when the job completes.  */
> -    BlockCompletionFunc *cb;
> -
> -    /** The opaque value that is passed to the completion function.  */
> -    void *opaque;
> -
>       /** Notifiers called when a cancelled job is finalised */
>       NotifierList on_finalize_cancelled;
>   
> @@ -167,6 +178,7 @@ typedef struct Job {
>   
>   /**
>    * Callbacks and other information about a Job driver.
> + * All callbacks are invoked with job_mutex *not* held.

Should this be in this patch? Seems related. But will we have a lot more 
comments like this in later patches?

>    */
>   struct JobDriver {
>   
> @@ -472,7 +484,6 @@ void job_yield(Job *job);
>    */
>   void coroutine_fn job_sleep_ns(Job *job, int64_t ns);
>   
> -

I'd drop this, looks like accidental unrelated style fixing.

>   /** Returns the JobType of a given Job. */
>   JobType job_type(const Job *job);
>   

as is, or with dropped 1-2 last hunks:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

--
Best regards,
Vladimir