From ef770f8101f6a3f4fb38a4fa693dfe68a0ac6bf2 Mon Sep 17 00:00:00 2001 From: Alberto Faria Date: Wed, 5 Oct 2022 18:52:09 +0100 Subject: coroutine: Drop coroutine_fn annotation from qemu_coroutine_self() qemu_coroutine_self() can be called from outside coroutine context, returning the leader coroutine, and several such invocations currently exist (mostly in qcow2 tracing calls). Signed-off-by: Alberto Faria Message-Id: <20221005175209.975797-1-afaria@redhat.com> Reviewed-by: Kevin Wolf Reviewed-by: Paolo Bonzini Signed-off-by: Kevin Wolf --- include/qemu/coroutine.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'include/qemu') diff --git a/include/qemu/coroutine.h b/include/qemu/coroutine.h index 08c5bb3..414b677 100644 --- a/include/qemu/coroutine.h +++ b/include/qemu/coroutine.h @@ -97,7 +97,7 @@ AioContext *coroutine_fn qemu_coroutine_get_aio_context(Coroutine *co); /** * Get the currently executing coroutine */ -Coroutine *coroutine_fn qemu_coroutine_self(void); +Coroutine *qemu_coroutine_self(void); /** * Return whether or not currently inside a coroutine -- cgit v1.1 From a248b856a8a9683fa03bbb6e1d684a9c67d92791 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Thu, 22 Sep 2022 10:49:03 +0200 Subject: coroutine: remove incorrect coroutine_fn annotations qemu_coroutine_get_aio_context inspects a coroutine, but it does not have to be called from the coroutine itself (or from any coroutine). Reviewed-by: Alberto Faria Reviewed-by: Eric Blake Signed-off-by: Paolo Bonzini Message-Id: <20220922084924.201610-6-pbonzini@redhat.com> Reviewed-by: Kevin Wolf Signed-off-by: Kevin Wolf --- include/qemu/coroutine.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'include/qemu') diff --git a/include/qemu/coroutine.h b/include/qemu/coroutine.h index 414b677..aae33cc 100644 --- a/include/qemu/coroutine.h +++ b/include/qemu/coroutine.h @@ -92,7 +92,7 @@ void coroutine_fn qemu_coroutine_yield(void); /** * Get the AioContext of the given coroutine */ -AioContext *coroutine_fn qemu_coroutine_get_aio_context(Coroutine *co); +AioContext *qemu_coroutine_get_aio_context(Coroutine *co); /** * Get the currently executing coroutine -- cgit v1.1 From 06753a0750a1973983505c2b544445042f555ba6 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Thu, 22 Sep 2022 10:49:19 +0200 Subject: job: add missing coroutine_fn annotations Callers of coroutine_fn must be coroutine_fn themselves, or the call must be within "if (qemu_in_coroutine())". Apply coroutine_fn to functions where this holds. Reviewed-by: Alberto Faria Signed-off-by: Paolo Bonzini Message-Id: <20220922084924.201610-22-pbonzini@redhat.com> Reviewed-by: Kevin Wolf Signed-off-by: Kevin Wolf --- include/qemu/job.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'include/qemu') diff --git a/include/qemu/job.h b/include/qemu/job.h index c105b31..397ac39 100644 --- a/include/qemu/job.h +++ b/include/qemu/job.h @@ -436,7 +436,7 @@ void coroutine_fn job_pause_point(Job *job); * * Yield the job coroutine. */ -void job_yield(Job *job); +void coroutine_fn job_yield(Job *job); /** * @job: The job that calls the function. -- cgit v1.1 From 55c5a25a0363f153d8875a60001342eb6fe6e4f5 Mon Sep 17 00:00:00 2001 From: Emanuele Giuseppe Esposito Date: Mon, 26 Sep 2022 05:31:54 -0400 Subject: job.c: make job_mutex and job_lock/unlock() public 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. 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 Reviewed-by: Stefan Hajnoczi Reviewed-by: Vladimir Sementsov-Ogievskiy Message-Id: <20220926093214.506243-2-eesposit@redhat.com> Signed-off-by: Kevin Wolf --- include/qemu/job.h | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) (limited to 'include/qemu') diff --git a/include/qemu/job.h b/include/qemu/job.h index 397ac39..e625547 100644 --- a/include/qemu/job.h +++ b/include/qemu/job.h @@ -303,6 +303,30 @@ typedef enum JobCreateFlags { JOB_MANUAL_DISMISS = 0x04, } JobCreateFlags; +extern QemuMutex job_mutex; + +#define JOB_LOCK_GUARD() /* QEMU_LOCK_GUARD(&job_mutex) */ + +#define WITH_JOB_LOCK_GUARD() /* WITH_QEMU_LOCK_GUARD(&job_mutex) */ + +/** + * job_lock: + * + * Take the mutex protecting the list of jobs and their status. + * Most functions called by the monitor need to call job_lock + * and job_unlock manually. On the other hand, function called + * by the block jobs themselves and by the block layer will take the + * lock for you. + */ +void job_lock(void); + +/** + * job_unlock: + * + * Release the mutex protecting the list of jobs and their status. + */ +void job_unlock(void); + /** * Allocate and return a new job transaction. Jobs can be added to the * transaction using job_txn_add_job(). -- cgit v1.1 From d08f07541fa34eb83680837eb45a6e1f75d177bc Mon Sep 17 00:00:00 2001 From: Emanuele Giuseppe Esposito Date: Mon, 26 Sep 2022 05:31:55 -0400 Subject: job.h: categorize fields in struct 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 Reviewed-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Kevin Wolf Reviewed-by: Stefan Hajnoczi Message-Id: <20220926093214.506243-3-eesposit@redhat.com> Signed-off-by: Kevin Wolf --- include/qemu/job.h | 61 ++++++++++++++++++++++++++++++++---------------------- 1 file changed, 36 insertions(+), 25 deletions(-) (limited to 'include/qemu') diff --git a/include/qemu/job.h b/include/qemu/job.h index e625547..8530c6a 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 */ + + /** AioContext to run the job coroutine in */ + AioContext *aio_context; + + /** 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. */ struct JobDriver { @@ -472,7 +484,6 @@ void coroutine_fn 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); -- cgit v1.1 From 544f4d5258d4e4dd6652d28607b605e7801ed7dd Mon Sep 17 00:00:00 2001 From: Emanuele Giuseppe Esposito Date: Mon, 26 Sep 2022 05:31:56 -0400 Subject: job.c: API functions not used outside should be static job_event_* functions can all be static, as they are not used outside job.c. Same applies for job_txn_add_job(). Signed-off-by: Emanuele Giuseppe Esposito Reviewed-by: Stefan Hajnoczi Reviewed-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Kevin Wolf Message-Id: <20220926093214.506243-4-eesposit@redhat.com> Signed-off-by: Kevin Wolf --- include/qemu/job.h | 18 ------------------ 1 file changed, 18 deletions(-) (limited to 'include/qemu') diff --git a/include/qemu/job.h b/include/qemu/job.h index 8530c6a..e3e31e2 100644 --- a/include/qemu/job.h +++ b/include/qemu/job.h @@ -359,18 +359,6 @@ JobTxn *job_txn_new(void); void job_txn_unref(JobTxn *txn); /** - * @txn: The transaction (may be NULL) - * @job: Job to add to the transaction - * - * Add @job to the transaction. The @job must not already be in a transaction. - * The caller must call either job_txn_unref() or job_completed() to release - * the reference that is automatically grabbed here. - * - * If @txn is NULL, the function does nothing. - */ -void job_txn_add_job(JobTxn *txn, Job *job); - -/** * Create a new long-running job and return it. * * @job_id: The id of the newly-created job, or %NULL for internal jobs @@ -431,12 +419,6 @@ void job_progress_set_remaining(Job *job, uint64_t remaining); */ void job_progress_increase_remaining(Job *job, uint64_t delta); -/** To be called when a cancelled job is finalised. */ -void job_event_cancelled(Job *job); - -/** To be called when a successfully completed job is finalised. */ -void job_event_completed(Job *job); - /** * Conditionally enter the job coroutine if the job is ready to run, not * already busy and fn() returns true. fn() is called while under the job_lock -- cgit v1.1 From afe1e8a7b3e671993cf55e2321408650c7620999 Mon Sep 17 00:00:00 2001 From: Emanuele Giuseppe Esposito Date: Mon, 26 Sep 2022 05:31:58 -0400 Subject: job.c: add job_lock/unlock while keeping job.h intact With "intact" we mean that all job.h functions implicitly take the lock. Therefore API callers are unmodified. This means that: - many static functions that will be always called with job lock held become _locked, and call _locked functions - all public functions take the lock internally if needed, and call _locked functions - all public functions called internally by other functions in job.c will have a _locked counterpart (sometimes public), to avoid deadlocks (job lock already taken). These functions are not used for now. - some public functions called only from exernal files (not job.c) do not have _locked() counterpart and take the lock inside. Others won't need the lock at all because use fields only set at initialization and never modified. job_{lock/unlock} is independent from real_job_{lock/unlock}. Note: at this stage, job_{lock/unlock} and job lock guard macros are *nop* Signed-off-by: Emanuele Giuseppe Esposito Reviewed-by: Kevin Wolf Reviewed-by: Stefan Hajnoczi Reviewed-by: Vladimir Sementsov-Ogievskiy Message-Id: <20220926093214.506243-6-eesposit@redhat.com> Signed-off-by: Kevin Wolf --- include/qemu/job.h | 138 ++++++++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 131 insertions(+), 7 deletions(-) (limited to 'include/qemu') diff --git a/include/qemu/job.h b/include/qemu/job.h index e3e31e2..870dce1 100644 --- a/include/qemu/job.h +++ b/include/qemu/job.h @@ -358,8 +358,15 @@ JobTxn *job_txn_new(void); */ void job_txn_unref(JobTxn *txn); +/* + * Same as job_txn_unref(), but called with job lock held. + * Might release the lock temporarily. + */ +void job_txn_unref_locked(JobTxn *txn); + /** * Create a new long-running job and return it. + * Called with job_mutex *not* held. * * @job_id: The id of the newly-created job, or %NULL for internal jobs * @driver: The class object for the newly-created job. @@ -380,17 +387,25 @@ void *job_create(const char *job_id, const JobDriver *driver, JobTxn *txn, */ void job_ref(Job *job); +/* Same as job_ref(), but called with job lock held. */ +void job_ref_locked(Job *job); + /** * Release a reference that was previously acquired with job_ref() or * job_create(). If it's the last reference to the object, it will be freed. */ void job_unref(Job *job); +/* Same as job_unref(), but called with job lock held. */ +void job_unref_locked(Job *job); + /** * @job: The job that has made progress * @done: How much progress the job made since the last call * * Updates the progress counter of the job. + * + * May be called with mutex held or not held. */ void job_progress_update(Job *job, uint64_t done); @@ -401,6 +416,8 @@ void job_progress_update(Job *job, uint64_t done); * * Sets the expected end value of the progress counter of a job so that a * completion percentage can be calculated when the progress is updated. + * + * May be called with mutex held or not held. */ void job_progress_set_remaining(Job *job, uint64_t remaining); @@ -416,6 +433,8 @@ void job_progress_set_remaining(Job *job, uint64_t remaining); * length before, and job_progress_update() afterwards. * (So the operation acts as a parenthesis in regards to the main job * operation running in background.) + * + * May be called with mutex held or not held. */ void job_progress_increase_remaining(Job *job, uint64_t delta); @@ -426,11 +445,19 @@ void job_progress_increase_remaining(Job *job, uint64_t delta); */ void job_enter_cond(Job *job, bool(*fn)(Job *job)); +/* + * Same as job_enter_cond(), but called with job lock held. + * Might release the lock temporarily. + */ +void job_enter_cond_locked(Job *job, bool(*fn)(Job *job)); + /** * @job: A job that has not yet been started. * * Begins execution of a job. * Takes ownership of one reference to the job object. + * + * Called with job_mutex *not* held. */ void job_start(Job *job); @@ -438,6 +465,7 @@ void job_start(Job *job); * @job: The job to enter. * * Continue the specified job by entering the coroutine. + * Called with job_mutex *not* held. */ void job_enter(Job *job); @@ -446,6 +474,8 @@ void job_enter(Job *job); * * Pause now if job_pause() has been called. Jobs that perform lots of I/O * must call this between requests so that the job can be paused. + * + * Called with job_mutex *not* held. */ void coroutine_fn job_pause_point(Job *job); @@ -453,6 +483,7 @@ void coroutine_fn job_pause_point(Job *job); * @job: The job that calls the function. * * Yield the job coroutine. + * Called with job_mutex *not* held. */ void coroutine_fn job_yield(Job *job); @@ -463,6 +494,8 @@ void coroutine_fn job_yield(Job *job); * Put the job to sleep (assuming that it wasn't canceled) for @ns * %QEMU_CLOCK_REALTIME nanoseconds. Canceling the job will immediately * interrupt the wait. + * + * Called with job_mutex *not* held. */ void coroutine_fn job_sleep_ns(Job *job, int64_t ns); @@ -475,21 +508,40 @@ const char *job_type_str(const Job *job); /** Returns true if the job should not be visible to the management layer. */ bool job_is_internal(Job *job); -/** Returns whether the job is being cancelled. */ +/** + * Returns whether the job is being cancelled. + * Called with job_mutex *not* held. + */ bool job_is_cancelled(Job *job); +/* Same as job_is_cancelled(), but called with job lock held. */ +bool job_is_cancelled_locked(Job *job); + /** * Returns whether the job is scheduled for cancellation (at an * indefinite point). + * Called with job_mutex *not* held. */ bool job_cancel_requested(Job *job); -/** Returns whether the job is in a completed state. */ +/** + * Returns whether the job is in a completed state. + * Called with job_mutex *not* held. + */ bool job_is_completed(Job *job); -/** Returns whether the job is ready to be completed. */ +/* Same as job_is_completed(), but called with job lock held. */ +bool job_is_completed_locked(Job *job); + +/** + * Returns whether the job is ready to be completed. + * Called with job_mutex *not* held. + */ bool job_is_ready(Job *job); +/* Same as job_is_ready(), but called with job lock held. */ +bool job_is_ready_locked(Job *job); + /** * Request @job to pause at the next pause point. Must be paired with * job_resume(). If the job is supposed to be resumed by user action, call @@ -497,24 +549,45 @@ bool job_is_ready(Job *job); */ void job_pause(Job *job); +/* Same as job_pause(), but called with job lock held. */ +void job_pause_locked(Job *job); + /** Resumes a @job paused with job_pause. */ void job_resume(Job *job); +/* + * Same as job_resume(), but called with job lock held. + * Might release the lock temporarily. + */ +void job_resume_locked(Job *job); + /** * Asynchronously pause the specified @job. * Do not allow a resume until a matching call to job_user_resume. */ void job_user_pause(Job *job, Error **errp); +/* Same as job_user_pause(), but called with job lock held. */ +void job_user_pause_locked(Job *job, Error **errp); + /** Returns true if the job is user-paused. */ bool job_user_paused(Job *job); +/* Same as job_user_paused(), but called with job lock held. */ +bool job_user_paused_locked(Job *job); + /** * Resume the specified @job. * Must be paired with a preceding job_user_pause. */ void job_user_resume(Job *job, Error **errp); +/* + * Same as job_user_resume(), but called with job lock held. + * Might release the lock temporarily. + */ +void job_user_resume_locked(Job *job, Error **errp); + /** * Get the next element from the list of block jobs after @job, or the * first one if @job is %NULL. @@ -523,6 +596,9 @@ void job_user_resume(Job *job, Error **errp); */ Job *job_next(Job *job); +/* Same as job_next(), but called with job lock held. */ +Job *job_next_locked(Job *job); + /** * Get the job identified by @id (which must not be %NULL). * @@ -530,6 +606,9 @@ Job *job_next(Job *job); */ Job *job_get(const char *id); +/* Same as job_get(), but called with job lock held. */ +Job *job_get_locked(const char *id); + /** * Check whether the verb @verb can be applied to @job in its current state. * Returns 0 if the verb can be applied; otherwise errp is set and -EPERM @@ -537,27 +616,48 @@ Job *job_get(const char *id); */ int job_apply_verb(Job *job, JobVerb verb, Error **errp); -/** The @job could not be started, free it. */ +/* Same as job_apply_verb, but called with job lock held. */ +int job_apply_verb_locked(Job *job, JobVerb verb, Error **errp); + +/** + * The @job could not be started, free it. + * Called with job_mutex *not* held. + */ void job_early_fail(Job *job); -/** Moves the @job from RUNNING to READY */ +/** + * Moves the @job from RUNNING to READY. + * Called with job_mutex *not* held. + */ void job_transition_to_ready(Job *job); /** Asynchronously complete the specified @job. */ void job_complete(Job *job, Error **errp); +/* + * Same as job_complete(), but called with job lock held. + * Might release the lock temporarily. + */ +void job_complete_locked(Job *job, Error **errp); + /** * Asynchronously cancel the specified @job. If @force is true, the job should * be cancelled immediately without waiting for a consistent state. */ void job_cancel(Job *job, bool force); +/* Same as job_cancel(), but called with job lock held. */ +void job_cancel_locked(Job *job, bool force); + /** * Cancels the specified job like job_cancel(), but may refuse to do so if the * operation isn't meaningful in the current state of the job. */ void job_user_cancel(Job *job, bool force, Error **errp); +/* Same as job_user_cancel(), but called with job lock held. */ +void job_user_cancel_locked(Job *job, bool force, Error **errp); + /** * Synchronously cancel the @job. The completion callback is called * before the function returns. If @force is false, the job may @@ -571,7 +671,14 @@ void job_user_cancel(Job *job, bool force, Error **errp); */ int job_cancel_sync(Job *job, bool force); -/** Synchronously force-cancels all jobs using job_cancel_sync(). */ +/* Same as job_cancel_sync, but called with job lock held. */ +int job_cancel_sync_locked(Job *job, bool force); + +/** + * Synchronously force-cancels all jobs using job_cancel_sync_locked(). + * + * Called with job_lock *not* held. + */ void job_cancel_sync_all(void); /** @@ -590,6 +697,9 @@ void job_cancel_sync_all(void); */ int job_complete_sync(Job *job, Error **errp); +/* Same as job_complete_sync, but called with job lock held. */ +int job_complete_sync_locked(Job *job, Error **errp); + /** * For a @job that has finished its work and is pending awaiting explicit * acknowledgement to commit its work, this will commit that work. @@ -600,12 +710,18 @@ int job_complete_sync(Job *job, Error **errp); */ void job_finalize(Job *job, Error **errp); +/* Same as job_finalize(), but called with job lock held. */ +void job_finalize_locked(Job *job, Error **errp); + /** * Remove the concluded @job from the query list and resets the passed pointer * to %NULL. Returns an error if the job is not actually concluded. */ void job_dismiss(Job **job, Error **errp); +/* Same as job_dismiss(), but called with job lock held. */ +void job_dismiss_locked(Job **job, Error **errp); + /** * Synchronously finishes the given @job. If @finish is given, it is called to * trigger completion or cancellation of the job. @@ -615,6 +731,14 @@ void job_dismiss(Job **job, Error **errp); * * Callers must hold the AioContext lock of job->aio_context. */ -int job_finish_sync(Job *job, void (*finish)(Job *, Error **errp), Error **errp); +int job_finish_sync(Job *job, void (*finish)(Job *, Error **errp), + Error **errp); + +/* + * Same as job_finish_sync(), but called with job lock held. + * Might release the lock temporarily. + */ +int job_finish_sync_locked(Job *job, void (*finish)(Job *, Error **errp), + Error **errp); #endif -- cgit v1.1 From 3ed4f708fe12537066d21f3dd111af013f7a6b8c Mon Sep 17 00:00:00 2001 From: Emanuele Giuseppe Esposito Date: Mon, 26 Sep 2022 05:32:06 -0400 Subject: jobs: protect job.aio_context with BQL and job_mutex In order to make it thread safe, implement a "fake rwlock", where we allow reads under BQL *or* job_mutex held, but writes only under BQL *and* job_mutex. The only write we have is in child_job_set_aio_ctx, which always happens under drain (so the job is paused). For this reason, introduce job_set_aio_context and make sure that the context is set under BQL, job_mutex and drain. Also make sure all other places where the aiocontext is read are protected. The reads in commit.c and mirror.c are actually safe, because always done under BQL. Note: at this stage, job_{lock/unlock} and job lock guard macros are *nop*. Suggested-by: Paolo Bonzini Signed-off-by: Emanuele Giuseppe Esposito Message-Id: <20220926093214.506243-14-eesposit@redhat.com> Reviewed-by: Kevin Wolf Reviewed-by: Vladimir Sementsov-Ogievskiy Signed-off-by: Kevin Wolf --- include/qemu/job.h | 23 ++++++++++++++++++++--- 1 file changed, 20 insertions(+), 3 deletions(-) (limited to 'include/qemu') diff --git a/include/qemu/job.h b/include/qemu/job.h index 870dce1..c963870 100644 --- a/include/qemu/job.h +++ b/include/qemu/job.h @@ -74,11 +74,17 @@ typedef struct Job { /* ProgressMeter API is thread-safe */ ProgressMeter progress; + /** + * AioContext to run the job coroutine in. + * The job Aiocontext can be read when holding *either* + * the BQL (so we are in the main loop) or the job_mutex. + * It can only be written when we hold *both* BQL + * and the job_mutex. + */ + AioContext *aio_context; - /** Protected by AioContext lock */ - /** AioContext to run the job coroutine in */ - AioContext *aio_context; + /** Protected by AioContext lock */ /** Reference count of the block job */ int refcnt; @@ -741,4 +747,15 @@ int job_finish_sync(Job *job, void (*finish)(Job *, Error **errp), int job_finish_sync_locked(Job *job, void (*finish)(Job *, Error **errp), Error **errp); +/** + * Sets the @job->aio_context. + * Called with job_mutex *not* held. + * + * This function must run in the main thread to protect against + * concurrent read in job_finish_sync_locked(), takes the job_mutex + * lock to protect against the read in job_do_yield_locked(), and must + * be called when the job is quiescent. + */ +void job_set_aio_context(Job *job, AioContext *ctx); + #endif -- cgit v1.1 From 2fc3bdc3843d2d8bde54c2be4d4f4cc8a9ffcf50 Mon Sep 17 00:00:00 2001 From: Emanuele Giuseppe Esposito Date: Mon, 26 Sep 2022 05:32:10 -0400 Subject: job.h: categorize JobDriver callbacks that need the AioContext lock Some callbacks implementation use bdrv_* APIs that assume the AioContext lock is held. Make sure this invariant is documented. Signed-off-by: Emanuele Giuseppe Esposito Reviewed-by: Vladimir Sementsov-Ogievskiy Message-Id: <20220926093214.506243-18-eesposit@redhat.com> Reviewed-by: Kevin Wolf Signed-off-by: Kevin Wolf --- include/qemu/job.h | 27 +++++++++++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-) (limited to 'include/qemu') diff --git a/include/qemu/job.h b/include/qemu/job.h index c963870..b943d90 100644 --- a/include/qemu/job.h +++ b/include/qemu/job.h @@ -65,7 +65,11 @@ typedef struct Job { /** True if this job should automatically dismiss itself */ bool auto_dismiss; - /** The completion function that will be called when the job completes. */ + /** + * The completion function that will be called when the job completes. + * Called with AioContext lock held, since many callback implementations + * use bdrv_* functions that require to hold the lock. + */ BlockCompletionFunc *cb; /** The opaque value that is passed to the completion function. */ @@ -260,6 +264,9 @@ struct JobDriver { * * This callback will not be invoked if the job has already failed. * If it fails, abort and then clean will be called. + * + * Called with AioContext lock held, since many callbacs implementations + * use bdrv_* functions that require to hold the lock. */ int (*prepare)(Job *job); @@ -270,6 +277,9 @@ struct JobDriver { * * All jobs will complete with a call to either .commit() or .abort() but * never both. + * + * Called with AioContext lock held, since many callback implementations + * use bdrv_* functions that require to hold the lock. */ void (*commit)(Job *job); @@ -280,6 +290,9 @@ struct JobDriver { * * All jobs will complete with a call to either .commit() or .abort() but * never both. + * + * Called with AioContext lock held, since many callback implementations + * use bdrv_* functions that require to hold the lock. */ void (*abort)(Job *job); @@ -288,6 +301,9 @@ struct JobDriver { * .commit() or .abort(). Regardless of which callback is invoked after * completion, .clean() will always be called, even if the job does not * belong to a transaction group. + * + * Called with AioContext lock held, since many callbacs implementations + * use bdrv_* functions that require to hold the lock. */ void (*clean)(Job *job); @@ -302,11 +318,18 @@ struct JobDriver { * READY). * (If the callback is NULL, the job is assumed to terminate * without I/O.) + * + * Called with AioContext lock held, since many callback implementations + * use bdrv_* functions that require to hold the lock. */ bool (*cancel)(Job *job, bool force); - /** Called when the job is freed */ + /** + * Called when the job is freed. + * Called with AioContext lock held, since many callback implementations + * use bdrv_* functions that require to hold the lock. + */ void (*free)(Job *job); }; -- cgit v1.1 From 6f592e5aca1a27fe1c1f661cfe68b35b90850acf Mon Sep 17 00:00:00 2001 From: Emanuele Giuseppe Esposito Date: Mon, 26 Sep 2022 05:32:11 -0400 Subject: job.c: enable job lock/unlock and remove Aiocontext locks Change the job_{lock/unlock} and macros to use job_mutex. Now that they are not nop anymore, remove the aiocontext to avoid deadlocks. Therefore: - when possible, remove completely the aiocontext lock/unlock pair - if it is used by some other function too, reduce the locking section as much as possible, leaving the job API outside. - change AIO_WAIT_WHILE in AIO_WAIT_WHILE_UNLOCKED, since we are not using the aiocontext lock anymore The only functions that still need the aiocontext lock are: - the JobDriver callbacks, already documented in job.h - job_cancel_sync() in replication.c is called with aio_context_lock taken, but now job is using AIO_WAIT_WHILE_UNLOCKED so we need to release the lock. Reduce the locking section to only cover the callback invocation and document the functions that take the AioContext lock, to avoid taking it twice. Also remove real_job_{lock/unlock}, as they are replaced by the public functions. Signed-off-by: Emanuele Giuseppe Esposito Message-Id: <20220926093214.506243-19-eesposit@redhat.com> Reviewed-by: Kevin Wolf Reviewed-by: Vladimir Sementsov-Ogievskiy Signed-off-by: Kevin Wolf --- include/qemu/job.h | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) (limited to 'include/qemu') diff --git a/include/qemu/job.h b/include/qemu/job.h index b943d90..a54fb83 100644 --- a/include/qemu/job.h +++ b/include/qemu/job.h @@ -88,7 +88,7 @@ typedef struct Job { AioContext *aio_context; - /** Protected by AioContext lock */ + /** Protected by job_mutex */ /** Reference count of the block job */ int refcnt; @@ -111,7 +111,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. @@ -346,9 +346,9 @@ typedef enum JobCreateFlags { extern QemuMutex job_mutex; -#define JOB_LOCK_GUARD() /* QEMU_LOCK_GUARD(&job_mutex) */ +#define JOB_LOCK_GUARD() QEMU_LOCK_GUARD(&job_mutex) -#define WITH_JOB_LOCK_GUARD() /* WITH_QEMU_LOCK_GUARD(&job_mutex) */ +#define WITH_JOB_LOCK_GUARD() WITH_QEMU_LOCK_GUARD(&job_mutex) /** * job_lock: @@ -422,6 +422,8 @@ void job_ref_locked(Job *job); /** * Release a reference that was previously acquired with job_ref() or * job_create(). If it's the last reference to the object, it will be freed. + * + * Takes AioContext lock internally to invoke a job->driver callback. */ void job_unref(Job *job); @@ -696,7 +698,7 @@ void job_user_cancel_locked(Job *job, bool force, Error **errp); * Returns the return value from the job if the job actually completed * during the call, or -ECANCELED if it was canceled. * - * Callers must hold the AioContext lock of job->aio_context. + * Called with job_lock *not* held. */ int job_cancel_sync(Job *job, bool force); @@ -721,8 +723,7 @@ void job_cancel_sync_all(void); * function). * * Returns the return value from the job. - * - * Callers must hold the AioContext lock of job->aio_context. + * Called with job_lock *not* held. */ int job_complete_sync(Job *job, Error **errp); @@ -758,7 +759,7 @@ void job_dismiss_locked(Job **job, Error **errp); * Returns 0 if the job is successfully completed, -ECANCELED if the job was * cancelled before completing, and -errno in other error cases. * - * Callers must hold the AioContext lock of job->aio_context. + * Called with job_lock *not* held. */ int job_finish_sync(Job *job, void (*finish)(Job *, Error **errp), Error **errp); -- cgit v1.1 From 9bd4d3c2e3d2e1df979e818ff0a5c05ca455721a Mon Sep 17 00:00:00 2001 From: Emanuele Giuseppe Esposito Date: Mon, 26 Sep 2022 05:32:14 -0400 Subject: job: remove unused functions These public functions are not used anywhere, thus can be dropped. Also, since this is the final job API that doesn't use AioContext lock and replaces it with job_lock, adjust all remaining function documentation to clearly specify if the job lock is taken or not. Also document the locking requirements for a few functions where the second version is not removed. Signed-off-by: Emanuele Giuseppe Esposito Reviewed-by: Stefan Hajnoczi Reviewed-by: Kevin Wolf Message-Id: <20220926093214.506243-22-eesposit@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy Signed-off-by: Kevin Wolf --- include/qemu/job.h | 110 ++++++++++++++++++----------------------------------- 1 file changed, 38 insertions(+), 72 deletions(-) (limited to 'include/qemu') diff --git a/include/qemu/job.h b/include/qemu/job.h index a54fb83..e502787 100644 --- a/include/qemu/job.h +++ b/include/qemu/job.h @@ -384,6 +384,8 @@ JobTxn *job_txn_new(void); /** * Release a reference that was previously acquired with job_txn_add_job or * job_txn_new. If it's the last reference to the object, it will be freed. + * + * Called with job lock *not* held. */ void job_txn_unref(JobTxn *txn); @@ -413,21 +415,18 @@ void *job_create(const char *job_id, const JobDriver *driver, JobTxn *txn, /** * Add a reference to Job refcnt, it will be decreased with job_unref, and then * be freed if it comes to be the last reference. + * + * Called with job lock held. */ -void job_ref(Job *job); - -/* Same as job_ref(), but called with job lock held. */ void job_ref_locked(Job *job); /** - * Release a reference that was previously acquired with job_ref() or + * Release a reference that was previously acquired with job_ref_locked() or * job_create(). If it's the last reference to the object, it will be freed. * * Takes AioContext lock internally to invoke a job->driver callback. + * Called with job lock held. */ -void job_unref(Job *job); - -/* Same as job_unref(), but called with job lock held. */ void job_unref_locked(Job *job); /** @@ -473,12 +472,8 @@ void job_progress_increase_remaining(Job *job, uint64_t delta); * Conditionally enter the job coroutine if the job is ready to run, not * already busy and fn() returns true. fn() is called while under the job_lock * critical section. - */ -void job_enter_cond(Job *job, bool(*fn)(Job *job)); - -/* - * Same as job_enter_cond(), but called with job lock held. - * Might release the lock temporarily. + * + * Called with job lock held, but might release it temporarily. */ void job_enter_cond_locked(Job *job, bool(*fn)(Job *job)); @@ -557,11 +552,8 @@ bool job_cancel_requested(Job *job); /** * Returns whether the job is in a completed state. - * Called with job_mutex *not* held. + * Called with job lock held. */ -bool job_is_completed(Job *job); - -/* Same as job_is_completed(), but called with job lock held. */ bool job_is_completed_locked(Job *job); /** @@ -576,14 +568,16 @@ bool job_is_ready_locked(Job *job); /** * Request @job to pause at the next pause point. Must be paired with * job_resume(). If the job is supposed to be resumed by user action, call - * job_user_pause() instead. + * job_user_pause_locked() instead. + * + * Called with job lock *not* held. */ void job_pause(Job *job); /* Same as job_pause(), but called with job lock held. */ void job_pause_locked(Job *job); -/** Resumes a @job paused with job_pause. */ +/** Resumes a @job paused with job_pause. Called with job lock *not* held. */ void job_resume(Job *job); /* @@ -595,27 +589,20 @@ void job_resume_locked(Job *job); /** * Asynchronously pause the specified @job. * Do not allow a resume until a matching call to job_user_resume. + * Called with job lock held. */ -void job_user_pause(Job *job, Error **errp); - -/* Same as job_user_pause(), but called with job lock held. */ void job_user_pause_locked(Job *job, Error **errp); -/** Returns true if the job is user-paused. */ -bool job_user_paused(Job *job); - -/* Same as job_user_paused(), but called with job lock held. */ +/** + * Returns true if the job is user-paused. + * Called with job lock held. + */ bool job_user_paused_locked(Job *job); /** * Resume the specified @job. - * Must be paired with a preceding job_user_pause. - */ -void job_user_resume(Job *job, Error **errp); - -/* - * Same as job_user_resume(), but called with job lock held. - * Might release the lock temporarily. + * Must be paired with a preceding job_user_pause_locked. + * Called with job lock held, but might release it temporarily. */ void job_user_resume_locked(Job *job, Error **errp); @@ -624,6 +611,7 @@ void job_user_resume_locked(Job *job, Error **errp); * first one if @job is %NULL. * * Returns the requested job, or %NULL if there are no more jobs left. + * Called with job lock *not* held. */ Job *job_next(Job *job); @@ -634,20 +622,17 @@ Job *job_next_locked(Job *job); * Get the job identified by @id (which must not be %NULL). * * Returns the requested job, or %NULL if it doesn't exist. + * Called with job lock held. */ -Job *job_get(const char *id); - -/* Same as job_get(), but called with job lock held. */ Job *job_get_locked(const char *id); /** * Check whether the verb @verb can be applied to @job in its current state. * Returns 0 if the verb can be applied; otherwise errp is set and -EPERM * returned. + * + * Called with job lock held. */ -int job_apply_verb(Job *job, JobVerb verb, Error **errp); - -/* Same as job_apply_verb, but called with job lock held. */ int job_apply_verb_locked(Job *job, JobVerb verb, Error **errp); /** @@ -662,31 +647,24 @@ void job_early_fail(Job *job); */ void job_transition_to_ready(Job *job); -/** Asynchronously complete the specified @job. */ -void job_complete(Job *job, Error **errp); - -/* - * Same as job_complete(), but called with job lock held. - * Might release the lock temporarily. +/** + * Asynchronously complete the specified @job. + * Called with job lock held, but might release it temporarily. */ void job_complete_locked(Job *job, Error **errp); /** * Asynchronously cancel the specified @job. If @force is true, the job should * be cancelled immediately without waiting for a consistent state. + * Called with job lock held. */ -void job_cancel(Job *job, bool force); - -/* Same as job_cancel(), but called with job lock held. */ void job_cancel_locked(Job *job, bool force); /** - * Cancels the specified job like job_cancel(), but may refuse to do so if the - * operation isn't meaningful in the current state of the job. + * Cancels the specified job like job_cancel_locked(), but may refuse + * to do so if the operation isn't meaningful in the current state of the job. + * Called with job lock held. */ -void job_user_cancel(Job *job, bool force, Error **errp); - -/* Same as job_user_cancel(), but called with job lock held. */ void job_user_cancel_locked(Job *job, bool force, Error **errp); /** @@ -714,7 +692,7 @@ void job_cancel_sync_all(void); /** * @job: The job to be completed. - * @errp: Error object which may be set by job_complete(); this is not + * @errp: Error object which may be set by job_complete_locked(); this is not * necessarily set on every error, the job return value has to be * checked as well. * @@ -723,11 +701,8 @@ void job_cancel_sync_all(void); * function). * * Returns the return value from the job. - * Called with job_lock *not* held. + * Called with job_lock held. */ -int job_complete_sync(Job *job, Error **errp); - -/* Same as job_complete_sync, but called with job lock held. */ int job_complete_sync_locked(Job *job, Error **errp); /** @@ -737,19 +712,17 @@ int job_complete_sync_locked(Job *job, Error **errp); * FIXME: Make the below statement universally true: * For jobs that support the manual workflow mode, all graph changes that occur * as a result will occur after this command and before a successful reply. + * + * Called with job lock held. */ -void job_finalize(Job *job, Error **errp); - -/* Same as job_finalize(), but called with job lock held. */ void job_finalize_locked(Job *job, Error **errp); /** * Remove the concluded @job from the query list and resets the passed pointer * to %NULL. Returns an error if the job is not actually concluded. + * + * Called with job lock held. */ -void job_dismiss(Job **job, Error **errp); - -/* Same as job_dismiss(), but called with job lock held. */ void job_dismiss_locked(Job **job, Error **errp); /** @@ -759,14 +732,7 @@ void job_dismiss_locked(Job **job, Error **errp); * Returns 0 if the job is successfully completed, -ECANCELED if the job was * cancelled before completing, and -errno in other error cases. * - * Called with job_lock *not* held. - */ -int job_finish_sync(Job *job, void (*finish)(Job *, Error **errp), - Error **errp); - -/* - * Same as job_finish_sync(), but called with job lock held. - * Might release the lock temporarily. + * Called with job_lock held, but might release it temporarily. */ int job_finish_sync_locked(Job *job, void (*finish)(Job *, Error **errp), Error **errp); -- cgit v1.1