From patchwork Wed Oct 5 16:47:39 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Paolo Bonzini X-Patchwork-Id: 678532 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 3sq1z4710xz9sCZ for ; Thu, 6 Oct 2016 03:52:00 +1100 (AEDT) Received: from localhost ([::1]:50329 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1brpQD-0005K7-Sa for incoming@patchwork.ozlabs.org; Wed, 05 Oct 2016 12:51:57 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36648) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1brpMH-0002l8-Vb for qemu-devel@nongnu.org; Wed, 05 Oct 2016 12:47:57 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1brpMF-000287-7N for qemu-devel@nongnu.org; Wed, 05 Oct 2016 12:47:52 -0400 Received: from mx1.redhat.com ([209.132.183.28]:50442) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1brpM8-00025F-1p; Wed, 05 Oct 2016 12:47:44 -0400 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 63B515411B; Wed, 5 Oct 2016 16:47:43 +0000 (UTC) Received: from donizetti.redhat.com (ovpn-112-40.ams2.redhat.com [10.36.112.40]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u95GlfI2023869; Wed, 5 Oct 2016 12:47:41 -0400 From: Paolo Bonzini To: qemu-devel@nongnu.org Date: Wed, 5 Oct 2016 18:47:39 +0200 Message-Id: <1475686060-28707-1-git-send-email-pbonzini@redhat.com> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.23 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.27]); Wed, 05 Oct 2016 16:47:43 +0000 (UTC) X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 209.132.183.28 Subject: [Qemu-devel] [PATCH] blockjob: introduce .drain callback for jobs X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: jsnow@redhat.com, qemu-block@nongnu.org Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: "Qemu-devel" This is required to decouple block jobs from running in an AioContext. With multiqueue block devices, a BlockDriverState does not really belong to a single AioContext. The solution is to first wait until all I/O operations are complete; then loop in the main thread for the block job to complete entirely. Signed-off-by: Paolo Bonzini --- This patch comes from the multiqueue series. It is self-contained so I'm sending it now; it achieves the purpose of getting rid of block_job_get_aio_context, so that any other use of it would be more visible. But if you all think it's premature, I'm okay with keeping it out some more. block/backup.c | 16 ++++++++++++++++ block/mirror.c | 34 ++++++++++++++++++++++++++-------- blockjob.c | 37 ++++++++++++++++++++----------------- include/block/blockjob.h | 7 +++++++ 4 files changed, 69 insertions(+), 25 deletions(-) diff --git a/block/backup.c b/block/backup.c index 582bd0f..0350cfc 100644 --- a/block/backup.c +++ b/block/backup.c @@ -300,6 +300,20 @@ void backup_cow_request_end(CowRequest *req) cow_request_end(req); } +static void backup_drain(BlockJob *job) +{ + BackupBlockJob *s = container_of(job, BackupBlockJob, common); + + /* Need to keep a reference in case blk_drain triggers execution + * of backup_complete... + */ + if (s->target) { + blk_ref(s->target); + blk_drain(s->target); + blk_unref(s->target); + } +} + static const BlockJobDriver backup_job_driver = { .instance_size = sizeof(BackupBlockJob), .job_type = BLOCK_JOB_TYPE_BACKUP, @@ -307,6 +321,7 @@ static const BlockJobDriver backup_job_driver = { .commit = backup_commit, .abort = backup_abort, .attached_aio_context = backup_attached_aio_context, + .drain = backup_drain, }; static BlockErrorAction backup_error_action(BackupBlockJob *job, @@ -331,6 +346,7 @@ static void backup_complete(BlockJob *job, void *opaque) BackupCompleteData *data = opaque; blk_unref(s->target); + s->target = NULL; block_job_completed(job, data->ret); g_free(data); diff --git a/block/mirror.c b/block/mirror.c index a0ac81a..4cd6bab2 100644 --- a/block/mirror.c +++ b/block/mirror.c @@ -471,7 +471,11 @@ static void mirror_free_init(MirrorBlockJob *s) } } -static void mirror_drain(MirrorBlockJob *s) +/* This is also used for the .pause callback. There is no matching + * mirror_resume() because mirror_run() will begin iterating again + * when the job is resumed. + */ +static void mirror_wait_for_all_io(MirrorBlockJob *s) { while (s->in_flight > 0) { mirror_wait_for_io(s); @@ -530,6 +534,7 @@ static void mirror_exit(BlockJob *job, void *opaque) g_free(s->replaces); bdrv_op_unblock_all(target_bs, s->common.blocker); blk_unref(s->target); + s->target = NULL; block_job_completed(&s->common, data->ret); g_free(data); bdrv_drained_end(src); @@ -584,7 +589,7 @@ static int coroutine_fn mirror_dirty_init(MirrorBlockJob *s) sector_num += nb_sectors; } - mirror_drain(s); + mirror_wait_for_all_io(s); } /* First part, loop on the sectors and initialize the dirty bitmap. */ @@ -802,7 +807,7 @@ immediate_exit: */ assert(ret < 0 || (!s->synced && block_job_is_cancelled(&s->common))); assert(need_drain); - mirror_drain(s); + mirror_wait_for_all_io(s); } assert(s->in_flight == 0); @@ -887,14 +892,11 @@ static void mirror_complete(BlockJob *job, Error **errp) block_job_enter(&s->common); } -/* There is no matching mirror_resume() because mirror_run() will begin - * iterating again when the job is resumed. - */ -static void coroutine_fn mirror_pause(BlockJob *job) +static void mirror_pause(BlockJob *job) { MirrorBlockJob *s = container_of(job, MirrorBlockJob, common); - mirror_drain(s); + mirror_wait_for_all_io(s); } static void mirror_attached_aio_context(BlockJob *job, AioContext *new_context) @@ -904,6 +906,20 @@ static void mirror_attached_aio_context(BlockJob *job, AioContext *new_context) blk_set_aio_context(s->target, new_context); } +static void mirror_drain(BlockJob *job) +{ + MirrorBlockJob *s = container_of(job, MirrorBlockJob, common); + + /* Need to keep a reference in case blk_drain triggers execution + * of mirror_complete... + */ + if (s->target) { + blk_ref(s->target); + blk_drain(s->target); + blk_unref(s->target); + } +} + static const BlockJobDriver mirror_job_driver = { .instance_size = sizeof(MirrorBlockJob), .job_type = BLOCK_JOB_TYPE_MIRROR, @@ -911,6 +927,7 @@ static const BlockJobDriver mirror_job_driver = { .complete = mirror_complete, .pause = mirror_pause, .attached_aio_context = mirror_attached_aio_context, + .drain = mirror_drain, }; static const BlockJobDriver commit_active_job_driver = { @@ -920,6 +937,7 @@ static const BlockJobDriver commit_active_job_driver = { .complete = mirror_complete, .pause = mirror_pause, .attached_aio_context = mirror_attached_aio_context, + .drain = mirror_drain, }; static void mirror_start_job(const char *job_id, BlockDriverState *bs, diff --git a/blockjob.c b/blockjob.c index 43fecbe..7c88b30 100644 --- a/blockjob.c +++ b/blockjob.c @@ -74,17 +74,6 @@ BlockJob *block_job_get(const char *id) return NULL; } -/* Normally the job runs in its BlockBackend's AioContext. The exception is - * block_job_defer_to_main_loop() where it runs in the QEMU main loop. Code - * that supports both cases uses this helper function. - */ -static AioContext *block_job_get_aio_context(BlockJob *job) -{ - return job->deferred_to_main_loop ? - qemu_get_aio_context() : - blk_get_aio_context(job->blk); -} - static void block_job_attached_aio_context(AioContext *new_context, void *opaque) { @@ -97,6 +86,17 @@ static void block_job_attached_aio_context(AioContext *new_context, block_job_resume(job); } +static void block_job_drain(BlockJob *job) +{ + /* If job is !job->busy this kicks it into the next pause point. */ + block_job_enter(job); + + blk_drain(job->blk); + if (job->driver->drain) { + job->driver->drain(job); + } +} + static void block_job_detach_aio_context(void *opaque) { BlockJob *job = opaque; @@ -106,12 +106,8 @@ static void block_job_detach_aio_context(void *opaque) 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_drain(job); } block_job_unref(job); @@ -413,14 +409,21 @@ static int block_job_finish_sync(BlockJob *job, assert(blk_bs(job->blk)->job == job); block_job_ref(job); + finish(job, &local_err); if (local_err) { error_propagate(errp, local_err); block_job_unref(job); return -EBUSY; } + /* block_job_drain calls block_job_enter, and it should be enough to + * induce progress until the job completes or moves to the main thread. + */ + while (!job->deferred_to_main_loop && !job->completed) { + block_job_drain(job); + } while (!job->completed) { - aio_poll(block_job_get_aio_context(job), true); + aio_poll(qemu_get_aio_context(), true); } ret = (job->cancelled && job->ret == 0) ? -ECANCELED : job->ret; block_job_unref(job); diff --git a/include/block/blockjob.h b/include/block/blockjob.h index 4ddb4ae..2bb39f4 100644 --- a/include/block/blockjob.h +++ b/include/block/blockjob.h @@ -92,6 +92,13 @@ typedef struct BlockJobDriver { * besides job->blk to the new AioContext. */ void (*attached_aio_context)(BlockJob *job, AioContext *new_context); + + /* + * If the callback is not NULL, it will be invoked when the job has to be + * synchronously cancelled or completed; it should drain BlockDriverStates + * as required to ensure progress. + */ + void (*drain)(BlockJob *job); } BlockJobDriver; /**