| Message ID | 20211104145334.1346363-3-eesposit@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | job: replace AioContext lock with job_mutex | expand |
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 <eesposit@redhat.com> > --- > 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.
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 <eesposit@redhat.com> >> --- >> 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
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);
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 | 57 +++++++++++++++++++++++++++------------------- 1 file changed, 34 insertions(+), 23 deletions(-)