From e8a40bf71d606f9f87866fb2461eaed52814b38e Mon Sep 17 00:00:00 2001 From: John Snow Date: Tue, 8 Nov 2016 01:50:35 -0500 Subject: blockjob: add .clean property Cleaning up after we have deferred to the main thread but before the transaction has converged can be dangerous and result in deadlocks if the job cleanup invokes any BH polling loops. A job may attempt to begin cleaning up, but may induce another job to enter its cleanup routine. The second job, part of our same transaction, will block waiting for the first job to finish, so neither job may now make progress. To rectify this, allow jobs to register a cleanup operation that will always run regardless of if the job was in a transaction or not, and if the transaction job group completed successfully or not. Move sensitive cleanup to this callback instead which is guaranteed to be run only after the transaction has converged, which removes sensitive timing constraints from said cleanup. Furthermore, in future patches these cleanup operations will be performed regardless of whether or not we actually started the job. Therefore, cleanup callbacks should essentially confine themselves to undoing create operations, e.g. setup actions taken in what is now backup_start. Reported-by: Vladimir Sementsov-Ogievskiy Signed-off-by: John Snow Reviewed-by: Kevin Wolf Message-id: 1478587839-9834-3-git-send-email-jsnow@redhat.com Signed-off-by: Jeff Cody --- include/block/blockjob_int.h | 8 ++++++++ 1 file changed, 8 insertions(+) (limited to 'include') diff --git a/include/block/blockjob_int.h b/include/block/blockjob_int.h index 40275e4..60d91a0 100644 --- a/include/block/blockjob_int.h +++ b/include/block/blockjob_int.h @@ -74,6 +74,14 @@ struct BlockJobDriver { void (*abort)(BlockJob *job); /** + * If the callback is not NULL, it will be invoked after a call to either + * .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. + */ + void (*clean)(BlockJob *job); + + /** * If the callback is not NULL, it will be invoked when the job transitions * into the paused state. Paused jobs must not perform any asynchronous * I/O or event loop activity. This callback is used to quiesce jobs. -- cgit v1.1 From a7815a764c40c9dcf204f666c2d90248095376a8 Mon Sep 17 00:00:00 2001 From: John Snow Date: Tue, 8 Nov 2016 01:50:36 -0500 Subject: blockjob: add .start field Add an explicit start field to specify the entrypoint. We already have ownership of the coroutine itself AND managing the lifetime of the coroutine, let's take control of creation of the coroutine, too. This will allow us to delay creation of the actual coroutine until we know we'll actually start a BlockJob in block_job_start. This avoids the sticky question of how to "un-create" a Coroutine that hasn't been started yet. Signed-off-by: John Snow Reviewed-by: Kevin Wolf Message-id: 1478587839-9834-4-git-send-email-jsnow@redhat.com Signed-off-by: Jeff Cody --- include/block/blockjob_int.h | 3 +++ 1 file changed, 3 insertions(+) (limited to 'include') diff --git a/include/block/blockjob_int.h b/include/block/blockjob_int.h index 60d91a0..8223822 100644 --- a/include/block/blockjob_int.h +++ b/include/block/blockjob_int.h @@ -47,6 +47,9 @@ struct BlockJobDriver { /** Optional callback for job types that need to forward I/O status reset */ void (*iostatus_reset)(BlockJob *job); + /** Mandatory: Entrypoint for the Coroutine. */ + CoroutineEntry *start; + /** * Optional callback for job types whose completion must be triggered * manually. -- cgit v1.1 From 5ccac6f186e4a480074880d958760c7105cf9ba8 Mon Sep 17 00:00:00 2001 From: John Snow Date: Tue, 8 Nov 2016 01:50:37 -0500 Subject: blockjob: add block_job_start Instead of automatically starting jobs at creation time via backup_start et al, we'd like to return a job object pointer that can be started manually at later point in time. For now, add the block_job_start mechanism and start the jobs automatically as we have been doing, with conversions job-by-job coming in later patches. Of note: cancellation of unstarted jobs will perform all the normal cleanup as if the job had started, particularly abort and clean. The only difference is that we will not emit any events, because the job never actually started. Signed-off-by: John Snow Message-id: 1478587839-9834-5-git-send-email-jsnow@redhat.com Signed-off-by: Jeff Cody --- include/block/blockjob.h | 9 +++++++++ 1 file changed, 9 insertions(+) (limited to 'include') diff --git a/include/block/blockjob.h b/include/block/blockjob.h index 356cacf..1acb256 100644 --- a/include/block/blockjob.h +++ b/include/block/blockjob.h @@ -189,6 +189,15 @@ void block_job_add_bdrv(BlockJob *job, BlockDriverState *bs); void block_job_set_speed(BlockJob *job, int64_t speed, Error **errp); /** + * block_job_start: + * @job: A job that has not yet been started. + * + * Begins execution of a block job. + * Takes ownership of one reference to the job object. + */ +void block_job_start(BlockJob *job); + +/** * block_job_cancel: * @job: The job to be canceled. * -- cgit v1.1 From 111049a4ecefc9cf1ac75c773f4c5c165f27fe63 Mon Sep 17 00:00:00 2001 From: John Snow Date: Tue, 8 Nov 2016 01:50:38 -0500 Subject: blockjob: refactor backup_start as backup_job_create Refactor backup_start as backup_job_create, which only creates the job, but does not automatically start it. The old interface, 'backup_start', is not kept in favor of limiting the number of nearly-identical interfaces that would have to be edited to keep up with QAPI changes in the future. Callers that wish to synchronously start the backup_block_job can instead just call block_job_start immediately after calling backup_job_create. Transactions are updated to use the new interface, calling block_job_start only during the .commit phase, which helps prevent race conditions where jobs may finish before we even finish building the transaction. This may happen, for instance, during empty block backup jobs. Reported-by: Vladimir Sementsov-Ogievskiy Signed-off-by: John Snow Message-id: 1478587839-9834-6-git-send-email-jsnow@redhat.com Signed-off-by: Jeff Cody --- include/block/block_int.h | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) (limited to 'include') diff --git a/include/block/block_int.h b/include/block/block_int.h index b02abbd..83a423c 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -748,7 +748,7 @@ void mirror_start(const char *job_id, BlockDriverState *bs, bool unmap, Error **errp); /* - * backup_start: + * backup_job_create: * @job_id: The id of the newly-created job, or %NULL to use the * device name of @bs. * @bs: Block device to operate on. @@ -764,18 +764,19 @@ void mirror_start(const char *job_id, BlockDriverState *bs, * @opaque: Opaque pointer value passed to @cb. * @txn: Transaction that this job is part of (may be NULL). * - * Start a backup operation on @bs. Clusters in @bs are written to @target + * Create a backup operation on @bs. Clusters in @bs are written to @target * until the job is cancelled or manually completed. */ -void backup_start(const char *job_id, BlockDriverState *bs, - BlockDriverState *target, int64_t speed, - MirrorSyncMode sync_mode, BdrvDirtyBitmap *sync_bitmap, - bool compress, - BlockdevOnError on_source_error, - BlockdevOnError on_target_error, - int creation_flags, - BlockCompletionFunc *cb, void *opaque, - BlockJobTxn *txn, Error **errp); +BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs, + BlockDriverState *target, int64_t speed, + MirrorSyncMode sync_mode, + BdrvDirtyBitmap *sync_bitmap, + bool compress, + BlockdevOnError on_source_error, + BlockdevOnError on_target_error, + int creation_flags, + BlockCompletionFunc *cb, void *opaque, + BlockJobTxn *txn, Error **errp); void hmp_drive_add_node(Monitor *mon, const char *optstr); -- cgit v1.1