From f67432a2019caf05b57a146bf45c1024a5cb608e Mon Sep 17 00:00:00 2001 From: John Snow Date: Wed, 29 Aug 2018 21:57:26 -0400 Subject: jobs: change start callback to run callback Presently we codify the entry point for a job as the "start" callback, but a more apt name would be "run" to clarify the idea that when this function returns we consider the job to have "finished," except for any cleanup which occurs in separate callbacks later. As part of this clarification, change the signature to include an error object and a return code. The error ptr is not yet used, and the return code while captured, will be overwritten by actions in the job_completed function. Signed-off-by: John Snow Reviewed-by: Max Reitz Message-id: 20180830015734.19765-2-jsnow@redhat.com Reviewed-by: Jeff Cody Signed-off-by: Max Reitz --- include/qemu/job.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'include') diff --git a/include/qemu/job.h b/include/qemu/job.h index 18c9223e31..9cf463d228 100644 --- a/include/qemu/job.h +++ b/include/qemu/job.h @@ -169,7 +169,7 @@ struct JobDriver { JobType job_type; /** Mandatory: Entrypoint for the Coroutine. */ - CoroutineEntry *start; + int coroutine_fn (*run)(Job *job, Error **errp); /** * If the callback is not NULL, it will be invoked when the job transitions -- cgit v1.2.3 From 3d1f8b07a4c241f81949eff507d9f3a8fd73b87b Mon Sep 17 00:00:00 2001 From: John Snow Date: Wed, 29 Aug 2018 21:57:27 -0400 Subject: jobs: canonize Error object Jobs presently use both an Error object in the case of the create job, and char strings in the case of generic errors elsewhere. Unify the two paths as just j->err, and remove the extra argument from job_completed. The integer error code for job_completed is kept for now, to be removed shortly in a separate patch. Signed-off-by: John Snow Message-id: 20180830015734.19765-3-jsnow@redhat.com [mreitz: Dropped a superfluous g_strdup()] Reviewed-by: Eric Blake Signed-off-by: Max Reitz --- include/qemu/job.h | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) (limited to 'include') diff --git a/include/qemu/job.h b/include/qemu/job.h index 9cf463d228..e0e99870a1 100644 --- a/include/qemu/job.h +++ b/include/qemu/job.h @@ -124,12 +124,16 @@ typedef struct Job { /** Estimated progress_current value at the completion of the job */ int64_t progress_total; - /** Error string for a failed job (NULL if, and only if, job->ret == 0) */ - char *error; - /** ret code passed to job_completed. */ int ret; + /** + * Error object for a failed job. + * If job->ret is nonzero and an error object was not set, it will be set + * to strerror(-job->ret) during job_completed. + */ + Error *err; + /** The completion function that will be called when the job completes. */ BlockCompletionFunc *cb; @@ -484,15 +488,13 @@ void job_transition_to_ready(Job *job); /** * @job: The job being completed. * @ret: The status code. - * @error: The error message for a failing job (only with @ret < 0). If @ret is - * negative, but NULL is given for @error, strerror() is used. * * Marks @job as completed. If @ret is non-zero, the job transaction it is part * of is aborted. If @ret is zero, the job moves into the WAITING state. If it * is the last job to complete in its transaction, all jobs in the transaction * move from WAITING to PENDING. */ -void job_completed(Job *job, int ret, Error *error); +void job_completed(Job *job, int ret); /** Asynchronously complete the specified @job. */ void job_complete(Job *job, Error **errp); -- cgit v1.2.3 From 00359a71d45a414ee47d8e423104dc0afd24ec65 Mon Sep 17 00:00:00 2001 From: John Snow Date: Wed, 29 Aug 2018 21:57:28 -0400 Subject: jobs: add exit shim All jobs do the same thing when they leave their running loop: - Store the return code in a structure - wait to receive this structure in the main thread - signal job completion via job_completed Few jobs do anything beyond exactly this. Consolidate this exit logic for a net reduction in SLOC. More seriously, when we utilize job_defer_to_main_loop_bh to call a function that calls job_completed, job_finalize_single will run in a context where it has recursively taken the aio_context lock, which can cause hangs if it puts down a reference that causes a flush. You can observe this in practice by looking at mirror_exit's careful placement of job_completed and bdrv_unref calls. If we centralize job exiting, we can signal job completion from outside of the aio_context, which should allow for job cleanup code to run with only one lock, which makes cleanup callbacks less tricky to write. Signed-off-by: John Snow Reviewed-by: Max Reitz Message-id: 20180830015734.19765-4-jsnow@redhat.com Reviewed-by: Jeff Cody Signed-off-by: Max Reitz --- include/qemu/job.h | 11 +++++++++++ 1 file changed, 11 insertions(+) (limited to 'include') diff --git a/include/qemu/job.h b/include/qemu/job.h index e0e99870a1..1144d671a1 100644 --- a/include/qemu/job.h +++ b/include/qemu/job.h @@ -208,6 +208,17 @@ struct JobDriver { */ void (*drain)(Job *job); + /** + * If the callback is not NULL, exit will be invoked from the main thread + * when the job's coroutine has finished, but before transactional + * convergence; before @prepare or @abort. + * + * FIXME TODO: This callback is only temporary to transition remaining jobs + * to prepare/commit/abort/clean callbacks and will be removed before 3.1. + * is released. + */ + void (*exit)(Job *job); + /** * If the callback is not NULL, prepare will be invoked when all the jobs * belonging to the same transaction complete; or upon this job's completion -- cgit v1.2.3 From 404ff28d6ae59fc1c24d631710d4063fc68aed03 Mon Sep 17 00:00:00 2001 From: John Snow Date: Wed, 29 Aug 2018 21:57:33 -0400 Subject: jobs: remove ret argument to job_completed; privatize it Jobs are now expected to return their retcode on the stack, from the .run callback, so we can remove that argument. job_cancel does not need to set -ECANCELED because job_completed will update the return code itself if the job was canceled. While we're here, make job_completed static to job.c and remove it from job.h; move the documentation of return code to the .run() callback and to the job->ret property, accordingly. Signed-off-by: John Snow Message-id: 20180830015734.19765-9-jsnow@redhat.com Reviewed-by: Max Reitz Signed-off-by: Max Reitz --- include/qemu/job.h | 28 +++++++++++++++------------- 1 file changed, 15 insertions(+), 13 deletions(-) (limited to 'include') diff --git a/include/qemu/job.h b/include/qemu/job.h index 1144d671a1..23395c17fa 100644 --- a/include/qemu/job.h +++ b/include/qemu/job.h @@ -124,7 +124,11 @@ typedef struct Job { /** Estimated progress_current value at the completion of the job */ int64_t progress_total; - /** ret code passed to job_completed. */ + /** + * Return code from @run and/or @prepare callback(s). + * Not final until the job has reached the CONCLUDED status. + * 0 on success, -errno on failure. + */ int ret; /** @@ -172,7 +176,16 @@ struct JobDriver { /** Enum describing the operation */ JobType job_type; - /** Mandatory: Entrypoint for the Coroutine. */ + /** + * Mandatory: Entrypoint for the Coroutine. + * + * This callback will be invoked when moving from CREATED to RUNNING. + * + * If this callback returns nonzero, the job transaction it is part of is + * aborted. If it returns zero, the job moves into the WAITING state. If it + * is the last job to complete in its transaction, all jobs in the + * transaction move from WAITING to PENDING. + */ int coroutine_fn (*run)(Job *job, Error **errp); /** @@ -496,17 +509,6 @@ void job_early_fail(Job *job); /** Moves the @job from RUNNING to READY */ void job_transition_to_ready(Job *job); -/** - * @job: The job being completed. - * @ret: The status code. - * - * Marks @job as completed. If @ret is non-zero, the job transaction it is part - * of is aborted. If @ret is zero, the job moves into the WAITING state. If it - * is the last job to complete in its transaction, all jobs in the transaction - * move from WAITING to PENDING. - */ -void job_completed(Job *job, int ret); - /** Asynchronously complete the specified @job. */ void job_complete(Job *job, Error **errp); -- cgit v1.2.3 From e21a1c9831fc80ae3f3c1affdfa43350035d8588 Mon Sep 17 00:00:00 2001 From: John Snow Date: Wed, 29 Aug 2018 21:57:34 -0400 Subject: jobs: remove job_defer_to_main_loop Now that the job infrastructure is handling the job_completed call for all implemented jobs, we can remove the interface that allowed jobs to schedule their own completion. Signed-off-by: John Snow Reviewed-by: Max Reitz Message-id: 20180830015734.19765-10-jsnow@redhat.com Signed-off-by: Max Reitz --- include/qemu/job.h | 17 ----------------- 1 file changed, 17 deletions(-) (limited to 'include') diff --git a/include/qemu/job.h b/include/qemu/job.h index 23395c17fa..e0cff702b7 100644 --- a/include/qemu/job.h +++ b/include/qemu/job.h @@ -568,23 +568,6 @@ void job_finalize(Job *job, Error **errp); */ void job_dismiss(Job **job, Error **errp); -typedef void JobDeferToMainLoopFn(Job *job, void *opaque); - -/** - * @job: The job - * @fn: The function to run in the main loop - * @opaque: The opaque value that is passed to @fn - * - * This function must be called by the main job coroutine just before it - * returns. @fn is executed in the main loop with the job AioContext acquired. - * - * Block jobs must call bdrv_unref(), bdrv_close(), and anything that uses - * bdrv_drain_all() in the main loop. - * - * The @job AioContext is held while @fn executes. - */ -void job_defer_to_main_loop(Job *job, JobDeferToMainLoopFn *fn, void *opaque); - /** * Synchronously finishes the given @job. If @finish is given, it is called to * trigger completion or cancellation of the job. -- cgit v1.2.3