From a7f3b7ff03a4712b9fc1089cc568eea7296af069 Mon Sep 17 00:00:00 2001 From: Stefan Hajnoczi Date: Thu, 16 Jun 2016 17:56:23 +0100 Subject: blockjob: rename block_job_is_paused() The block_job_is_paused() function name is not great because callers only use it to determine whether pausing has been requested. Rename it to highlight those semantics and remove it from the public header file as there are no external callers. Signed-off-by: Stefan Hajnoczi Reviewed-by: Paolo Bonzini Reviewed-by: Fam Zheng Message-id: 1466096189-6477-3-git-send-email-stefanha@redhat.com --- include/block/blockjob.h | 9 --------- 1 file changed, 9 deletions(-) (limited to 'include') diff --git a/include/block/blockjob.h b/include/block/blockjob.h index 00ac4184cc..8fcecf9a79 100644 --- a/include/block/blockjob.h +++ b/include/block/blockjob.h @@ -347,15 +347,6 @@ void block_job_event_completed(BlockJob *job, const char *msg); */ void block_job_event_ready(BlockJob *job); -/** - * block_job_is_paused: - * @job: The job being queried. - * - * Returns whether the job is currently paused, or will pause - * as soon as it reaches a sleeping point. - */ -bool block_job_is_paused(BlockJob *job); - /** * block_job_cancel_sync: * @job: The job to be canceled. -- cgit v1.2.3-55-g7522 From fc9c0a9c4b2c07cf2b8683f2617af584f14c93e7 Mon Sep 17 00:00:00 2001 From: Stefan Hajnoczi Date: Thu, 16 Jun 2016 17:56:24 +0100 Subject: blockjob: add pause points Block jobs are coroutines that usually perform I/O but sometimes also sleep or yield. Currently only sleeping or yielded block jobs can be paused. This means jobs that do not sleep or yield (using block_job_yield()) are unaffected by block_job_pause(). Add block_job_pause_point() so that block jobs can mark quiescent points that are suitable for pausing. This solves the problem that it can take a block job a long time to pause if it is performing a long series of I/O operations. Transitioning to paused state involves a .pause()/.resume() callback. These callbacks are used to ensure that I/O and event loop activity has ceased while the job is at a pause point. Note that this patch introduces a stricter pause state than previously. The job->busy flag was incorrectly documented as a quiescent state without I/O pending. This is violated by any job that has I/O pending across sleep or block_job_yield(), like the mirror block job. [Add missing block_job_should_pause() check to avoid deadlock after job->driver->pause() in block_job_pause_point(). --Stefan] Signed-off-by: Stefan Hajnoczi Reviewed-by: Paolo Bonzini Reviewed-by: Fam Zheng Message-id: 1466096189-6477-4-git-send-email-stefanha@redhat.com --- blockjob.c | 38 +++++++++++++++++++++++++++++++++----- include/block/blockjob.h | 35 ++++++++++++++++++++++++++++++++--- 2 files changed, 65 insertions(+), 8 deletions(-) (limited to 'include') diff --git a/blockjob.c b/blockjob.c index 21cc3ee332..874b573bd5 100644 --- a/blockjob.c +++ b/blockjob.c @@ -257,6 +257,32 @@ static bool block_job_should_pause(BlockJob *job) return job->pause_count > 0; } +void coroutine_fn block_job_pause_point(BlockJob *job) +{ + if (!block_job_should_pause(job)) { + return; + } + if (block_job_is_cancelled(job)) { + return; + } + + if (job->driver->pause) { + job->driver->pause(job); + } + + if (block_job_should_pause(job) && !block_job_is_cancelled(job)) { + job->paused = true; + job->busy = false; + qemu_coroutine_yield(); /* wait for block_job_resume() */ + job->busy = true; + job->paused = false; + } + + if (job->driver->resume) { + job->driver->resume(job); + } +} + void block_job_resume(BlockJob *job) { assert(job->pause_count > 0); @@ -364,11 +390,9 @@ void block_job_sleep_ns(BlockJob *job, QEMUClockType type, int64_t ns) if (!block_job_should_pause(job)) { co_aio_sleep_ns(blk_get_aio_context(job->blk), type, ns); } - /* The job can be paused while sleeping, so check this again */ - if (block_job_should_pause(job)) { - qemu_coroutine_yield(); - } job->busy = true; + + block_job_pause_point(job); } void block_job_yield(BlockJob *job) @@ -381,8 +405,12 @@ void block_job_yield(BlockJob *job) } job->busy = false; - qemu_coroutine_yield(); + if (!block_job_should_pause(job)) { + qemu_coroutine_yield(); + } job->busy = true; + + block_job_pause_point(job); } BlockJobInfo *block_job_query(BlockJob *job) diff --git a/include/block/blockjob.h b/include/block/blockjob.h index 8fcecf9a79..7739f37c3a 100644 --- a/include/block/blockjob.h +++ b/include/block/blockjob.h @@ -70,6 +70,20 @@ typedef struct BlockJobDriver { * never both. */ void (*abort)(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. + */ + void coroutine_fn (*pause)(BlockJob *job); + + /** + * If the callback is not NULL, it will be invoked when the job transitions + * out of the paused state. Any asynchronous I/O or event loop activity + * should be restarted from this callback. + */ + void coroutine_fn (*resume)(BlockJob *job); } BlockJobDriver; /** @@ -119,12 +133,18 @@ struct BlockJob { bool user_paused; /** - * Set to false by the job while it is in a quiescent state, where - * no I/O is pending and the job has yielded on any condition - * that is not detected by #aio_poll, such as a timer. + * Set to false by the job while the coroutine has yielded and may be + * re-entered by block_job_enter(). There may still be I/O or event loop + * activity pending. */ bool busy; + /** + * Set to true by the job while it is in a quiescent state, where + * no I/O or event loop activity is pending. + */ + bool paused; + /** * Set to true when the job is ready to be completed. */ @@ -298,6 +318,15 @@ bool block_job_is_cancelled(BlockJob *job); */ BlockJobInfo *block_job_query(BlockJob *job); +/** + * block_job_pause_point: + * @job: The job that is ready to pause. + * + * Pause now if block_job_pause() has been called. Block jobs that perform + * lots of I/O must call this between requests so that the job can be paused. + */ +void coroutine_fn block_job_pause_point(BlockJob *job); + /** * block_job_pause: * @job: The job to be paused. -- cgit v1.2.3-55-g7522 From e8a095dadb70e2ea6d5169d261920db3747bfa45 Mon Sep 17 00:00:00 2001 From: Stefan Hajnoczi Date: Thu, 16 Jun 2016 17:56:26 +0100 Subject: block: use safe iteration over AioContext notifiers It's possible that an AioContext notifier user was close to finishing when .detach_aio_context() or .attached_aio_context() is called. In that case they may call bdrv_remove_aio_context_notifier() during the callback. Use safe iteration to avoid crashing when the notifier list is modified during iteration. We must not only handle the case where the current aio notifier is removed during a callback but also the one where any other aio notifier is removed. The next patch adds an AioContext notifier for block jobs and they really could be terminating just as .detach_aio_context() is invoked. Signed-off-by: Stefan Hajnoczi Reviewed-by: Paolo Bonzini Reviewed-by: Fam Zheng Message-id: 1466096189-6477-6-git-send-email-stefanha@redhat.com --- block.c | 46 ++++++++++++++++++++++++++++++++++++---------- include/block/block_int.h | 2 ++ 2 files changed, 38 insertions(+), 10 deletions(-) (limited to 'include') diff --git a/block.c b/block.c index b331eb9d38..c0ccc27c73 100644 --- a/block.c +++ b/block.c @@ -3609,18 +3609,34 @@ AioContext *bdrv_get_aio_context(BlockDriverState *bs) return bs->aio_context; } +static void bdrv_do_remove_aio_context_notifier(BdrvAioNotifier *ban) +{ + QLIST_REMOVE(ban, list); + g_free(ban); +} + void bdrv_detach_aio_context(BlockDriverState *bs) { - BdrvAioNotifier *baf; + BdrvAioNotifier *baf, *baf_tmp; BdrvChild *child; if (!bs->drv) { return; } - QLIST_FOREACH(baf, &bs->aio_notifiers, list) { - baf->detach_aio_context(baf->opaque); + assert(!bs->walking_aio_notifiers); + bs->walking_aio_notifiers = true; + QLIST_FOREACH_SAFE(baf, &bs->aio_notifiers, list, baf_tmp) { + if (baf->deleted) { + bdrv_do_remove_aio_context_notifier(baf); + } else { + baf->detach_aio_context(baf->opaque); + } } + /* Never mind iterating again to check for ->deleted. bdrv_close() will + * remove remaining aio notifiers if we aren't called again. + */ + bs->walking_aio_notifiers = false; if (bs->drv->bdrv_detach_aio_context) { bs->drv->bdrv_detach_aio_context(bs); @@ -3635,7 +3651,7 @@ void bdrv_detach_aio_context(BlockDriverState *bs) void bdrv_attach_aio_context(BlockDriverState *bs, AioContext *new_context) { - BdrvAioNotifier *ban; + BdrvAioNotifier *ban, *ban_tmp; BdrvChild *child; if (!bs->drv) { @@ -3651,9 +3667,16 @@ void bdrv_attach_aio_context(BlockDriverState *bs, bs->drv->bdrv_attach_aio_context(bs, new_context); } - QLIST_FOREACH(ban, &bs->aio_notifiers, list) { - ban->attached_aio_context(new_context, ban->opaque); + assert(!bs->walking_aio_notifiers); + bs->walking_aio_notifiers = true; + QLIST_FOREACH_SAFE(ban, &bs->aio_notifiers, list, ban_tmp) { + if (ban->deleted) { + bdrv_do_remove_aio_context_notifier(ban); + } else { + ban->attached_aio_context(new_context, ban->opaque); + } } + bs->walking_aio_notifiers = false; } void bdrv_set_aio_context(BlockDriverState *bs, AioContext *new_context) @@ -3695,11 +3718,14 @@ void bdrv_remove_aio_context_notifier(BlockDriverState *bs, QLIST_FOREACH_SAFE(ban, &bs->aio_notifiers, list, ban_next) { if (ban->attached_aio_context == attached_aio_context && ban->detach_aio_context == detach_aio_context && - ban->opaque == opaque) + ban->opaque == opaque && + ban->deleted == false) { - QLIST_REMOVE(ban, list); - g_free(ban); - + if (bs->walking_aio_notifiers) { + ban->deleted = true; + } else { + bdrv_do_remove_aio_context_notifier(ban); + } return; } } diff --git a/include/block/block_int.h b/include/block/block_int.h index 688c6be009..205715600b 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -361,6 +361,7 @@ typedef struct BdrvAioNotifier { void (*detach_aio_context)(void *opaque); void *opaque; + bool deleted; QLIST_ENTRY(BdrvAioNotifier) list; } BdrvAioNotifier; @@ -427,6 +428,7 @@ struct BlockDriverState { * BDS may register themselves in this list to be notified of changes * regarding this BDS's context */ QLIST_HEAD(, BdrvAioNotifier) aio_notifiers; + bool walking_aio_notifiers; /* to make removal during iteration safe */ char filename[PATH_MAX]; char backing_file[PATH_MAX]; /* if non zero, the image is a diff of -- cgit v1.2.3-55-g7522 From 463e0be101cb5a78ca6ee517d58604c3f3637bcd Mon Sep 17 00:00:00 2001 From: Stefan Hajnoczi Date: Thu, 16 Jun 2016 17:56:27 +0100 Subject: blockjob: add AioContext attached callback Block jobs that use additional BDSes or event loop resources need a callback to get their affairs in order when the AioContext is switched. Simple block jobs don't need an attach callback, they automatically work thanks to the generic attach/detach notifiers that this patch adds. Signed-off-by: Stefan Hajnoczi Reviewed-by: Paolo Bonzini Reviewed-by: Fam Zheng Message-id: 1466096189-6477-7-git-send-email-stefanha@redhat.com --- blockjob.c | 38 ++++++++++++++++++++++++++++++++++++++ include/block/blockjob.h | 7 +++++++ 2 files changed, 45 insertions(+) (limited to 'include') diff --git a/blockjob.c b/blockjob.c index 096a1b8715..90c4e262b0 100644 --- a/blockjob.c +++ b/blockjob.c @@ -71,6 +71,38 @@ static AioContext *block_job_get_aio_context(BlockJob *job) blk_get_aio_context(job->blk); } +static void block_job_attached_aio_context(AioContext *new_context, + void *opaque) +{ + BlockJob *job = opaque; + + if (job->driver->attached_aio_context) { + job->driver->attached_aio_context(job, new_context); + } + + block_job_resume(job); +} + +static void block_job_detach_aio_context(void *opaque) +{ + BlockJob *job = opaque; + + /* In case the job terminates during aio_poll()... */ + block_job_ref(job); + + block_job_pause(job); + + if (!job->paused) { + /* If job is !job->busy this kicks it into the next pause point. */ + block_job_enter(job); + } + while (!job->paused && !job->completed) { + aio_poll(block_job_get_aio_context(job), true); + } + + block_job_unref(job); +} + void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs, int64_t speed, BlockCompletionFunc *cb, void *opaque, Error **errp) @@ -103,6 +135,9 @@ void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs, QLIST_INSERT_HEAD(&block_jobs, job, job_list); + blk_add_aio_context_notifier(blk, block_job_attached_aio_context, + block_job_detach_aio_context, job); + /* Only set speed when necessary to avoid NotSupported error */ if (speed != 0) { Error *local_err = NULL; @@ -128,6 +163,9 @@ void block_job_unref(BlockJob *job) BlockDriverState *bs = blk_bs(job->blk); bs->job = NULL; bdrv_op_unblock_all(bs, job->blocker); + blk_remove_aio_context_notifier(job->blk, + block_job_attached_aio_context, + block_job_detach_aio_context, job); blk_unref(job->blk); error_free(job->blocker); g_free(job->id); diff --git a/include/block/blockjob.h b/include/block/blockjob.h index 7739f37c3a..7dc720c82b 100644 --- a/include/block/blockjob.h +++ b/include/block/blockjob.h @@ -84,6 +84,13 @@ typedef struct BlockJobDriver { * should be restarted from this callback. */ void coroutine_fn (*resume)(BlockJob *job); + + /* + * If the callback is not NULL, it will be invoked before the job is + * resumed in a new AioContext. This is the place to move any resources + * besides job->blk to the new AioContext. + */ + void (*attached_aio_context)(BlockJob *job, AioContext *new_context); } BlockJobDriver; /** -- cgit v1.2.3-55-g7522