Re: [RFC PATCH v2 02/14] job.h: categorize fields in struct Job

2021-12-21 Thread Emanuele Giuseppe Esposito




On 16/12/2021 17:21, Stefan Hajnoczi wrote:

On Thu, Nov 04, 2021 at 10:53:22AM -0400, 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 
---
  include/qemu/job.h | 57 +++---
  1 file changed, 34 insertions(+), 23 deletions(-)

diff --git a/include/qemu/job.h b/include/qemu/job.h
index ccf7826426..f7036ac6b3 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;


"Fields set at initialization (job_create), and never modified" does not
apply here. blockjob.c:child_job_set_aio_ctx() changes it at runtime.



Right. aio_context can theoretically avoid also the job_mutex, if we 
make sure that all klass->set_aio_ctx() are under BQL (they are) and 
under drains (work in progress). For now I will protect it with job_lock().


Thank you,
Emanuele




Re: [RFC PATCH v2 02/14] job.h: categorize fields in struct Job

2021-12-16 Thread Stefan Hajnoczi
On Thu, Nov 04, 2021 at 10:53:22AM -0400, 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 
> ---
>  include/qemu/job.h | 57 +++---
>  1 file changed, 34 insertions(+), 23 deletions(-)
> 
> diff --git a/include/qemu/job.h b/include/qemu/job.h
> index ccf7826426..f7036ac6b3 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;

"Fields set at initialization (job_create), and never modified" does not
apply here. blockjob.c:child_job_set_aio_ctx() changes it at runtime.


signature.asc
Description: PGP signature


[RFC PATCH v2 02/14] job.h: categorize fields in struct Job

2021-11-04 Thread Emanuele Giuseppe Esposito
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 
---
 include/qemu/job.h | 57 +++---
 1 file changed, 34 insertions(+), 23 deletions(-)

diff --git a/include/qemu/job.h b/include/qemu/job.h
index ccf7826426..f7036ac6b3 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 job_mutex */
+
+/** 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).
@@ -76,7 +101,7 @@ typedef struct Job {
 /**
  * Set to false by the job while the coroutine has yielded and may be
  * re-entered by job_enter(). There may still be I/O or event loop activity
- * pending. Accessed under block_job_mutex (in blockjob.c).
+ * pending. Accessed under job_mutex.
  *
  * When the job is deferred to the main loop, busy is true as long as the
  * bottom half is still pending.
@@ -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.
  */
 struct JobDriver {
 
@@ -460,7 +472,6 @@ void job_yield(Job *job);
  */
 void coroutine_fn job_sleep_ns(Job *job, int64_t ns);
 
-
 /** Returns the JobType of a given Job. */
 JobType job_type(const Job *job);
 
-- 
2.27.0