From patchwork Tue May 8 14:51:46 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Paolo Bonzini X-Patchwork-Id: 157742 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [208.118.235.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id 3F1E8B6FBC for ; Wed, 9 May 2012 02:35:48 +1000 (EST) Received: from localhost ([::1]:37173 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SRlnC-00028n-JP for incoming@patchwork.ozlabs.org; Tue, 08 May 2012 10:53:34 -0400 Received: from eggs.gnu.org ([208.118.235.92]:60011) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SRlmM-0008Sp-L1 for qemu-devel@nongnu.org; Tue, 08 May 2012 10:52:50 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SRlmH-0007XR-H7 for qemu-devel@nongnu.org; Tue, 08 May 2012 10:52:42 -0400 Received: from mail-pb0-f45.google.com ([209.85.160.45]:54112) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SRlmH-0007SM-8l for qemu-devel@nongnu.org; Tue, 08 May 2012 10:52:37 -0400 Received: by mail-pb0-f45.google.com with SMTP id ro12so10989636pbb.4 for ; Tue, 08 May 2012 07:52:36 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=sender:from:to:cc:subject:date:message-id:x-mailer:in-reply-to :references; bh=FnH9MApH/I8e8fEBgp/8kLnZiIiSWYsCXW1WjdR1AYU=; b=wJr/T5h58vKjcd4XdMEnuD3C78B7b6YPv5co9qeqCRduPBwlU4dpdLfG2loh02ANks bUl04iQupgZXtMlTeXjS7gkEj+NlZNyboIiz4+f1qffzzf5y3/EW8fZ+nzjFS5F390Rr tCoMROD0RoQMMMtIefg4CUk/zaOtfXRRmDACXeEDeBQFauQqOYGiDEVsoyVj9esrL7YC yPLjNa8KHOzG7dJNzSluIlHYmgSZQeswSSm25rPIzpzhkEEQyjVfrEnqkjcK1D8wHeKK wjBDSVDXndIglo8+y5GSBGLC/nVLLrTOHRQyLbNlvphWztZOuZuF9jY+jUgYjyerZ9hb I8zw== Received: by 10.68.216.199 with SMTP id os7mr1033590pbc.4.1336488756183; Tue, 08 May 2012 07:52:36 -0700 (PDT) Received: from yakj.usersys.redhat.com (93-34-182-16.ip50.fastwebnet.it. [93.34.182.16]) by mx.google.com with ESMTPS id i1sm2604863pbv.49.2012.05.08.07.52.33 (version=TLSv1/SSLv3 cipher=OTHER); Tue, 08 May 2012 07:52:35 -0700 (PDT) From: Paolo Bonzini To: qemu-devel@nongnu.org Date: Tue, 8 May 2012 16:51:46 +0200 Message-Id: <1336488722-13120-7-git-send-email-pbonzini@redhat.com> X-Mailer: git-send-email 1.7.10.1 In-Reply-To: <1336488722-13120-1-git-send-email-pbonzini@redhat.com> References: <1336488722-13120-1-git-send-email-pbonzini@redhat.com> X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. X-Received-From: 209.85.160.45 Cc: kwolf@redhat.com, stefanha@linux.vnet.ibm.com Subject: [Qemu-devel] [PATCH 1.1 06/22] block: wait for job callback in block_job_cancel_sync X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org The limitation on not having I/O after cancellation cannot really be kept. Even streaming has a very small race window where you could cancel a job and have it report completion. If this window is hit, bdrv_change_backing_file() will yield and possibly cause accesses to dangling pointers etc. So, let's just assume that we cannot know exactly what will happen after the coroutine has set busy to false. We can set a very lax condition: - if we cancel the job, the coroutine won't set it to false again (and hence will not call co_sleep_ns again). - block_job_cancel_sync will wait for the coroutine to exit, which pretty much ensures no race. Instead, we track the coroutine that executes the job and put very strict conditions on what to do while it is quiescent (busy = false). First of all, the coroutine must never set busy = false while the job has been cancelled. Second, the coroutine can be reentered arbitrarily while it is quiescent, so you cannot really do anything but co_sleep_ns at that time. This condition is obeyed by the block_job_sleep_ns function. Signed-off-by: Paolo Bonzini --- block.c | 36 ++++++++++++++++++++++++++++++++++-- block/stream.c | 7 +++---- block_int.h | 11 ++++++++++- 3 files changed, 47 insertions(+), 7 deletions(-) diff --git a/block.c b/block.c index f9a11aa..2978135 100644 --- a/block.c +++ b/block.c @@ -4238,6 +4238,9 @@ void block_job_set_speed(BlockJob *job, int64_t speed, Error **errp) void block_job_cancel(BlockJob *job) { job->cancelled = true; + if (job->co && !job->busy) { + qemu_coroutine_enter(job->co, NULL); + } } bool block_job_is_cancelled(BlockJob *job) @@ -4245,15 +4248,44 @@ bool block_job_is_cancelled(BlockJob *job) return job->cancelled; } -void block_job_cancel_sync(BlockJob *job) +struct BlockCancelData { + BlockJob *job; + BlockDriverCompletionFunc *cb; + void *opaque; + bool cancelled; + int ret; +}; + +static void block_job_cancel_cb(void *opaque, int ret) { + struct BlockCancelData *data = opaque; + + data->cancelled = block_job_is_cancelled(data->job); + data->ret = ret; + data->cb(data->opaque, ret); +} + +int block_job_cancel_sync(BlockJob *job) +{ + struct BlockCancelData data; BlockDriverState *bs = job->bs; assert(bs->job == job); + + /* Set up our own callback to store the result and chain to + * the original callback. + */ + data.job = job; + data.cb = job->cb; + data.opaque = job->opaque; + data.ret = -EINPROGRESS; + job->cb = block_job_cancel_cb; + job->opaque = &data; block_job_cancel(job); - while (bs->job != NULL && bs->job->busy) { + while (data.ret == -EINPROGRESS) { qemu_aio_wait(); } + return (data.cancelled && data.ret == 0) ? -ECANCELED : data.ret; } void block_job_sleep_ns(BlockJob *job, QEMUClock *clock, int64_t ns) diff --git a/block/stream.c b/block/stream.c index b279acd..930e3cf 100644 --- a/block/stream.c +++ b/block/stream.c @@ -270,7 +270,6 @@ void stream_start(BlockDriverState *bs, BlockDriverState *base, void *opaque, Error **errp) { StreamBlockJob *s; - Coroutine *co; s = block_job_create(&stream_job_type, bs, speed, cb, opaque, errp); if (!s) { @@ -282,7 +281,7 @@ void stream_start(BlockDriverState *bs, BlockDriverState *base, pstrcpy(s->backing_file_id, sizeof(s->backing_file_id), base_id); } - co = qemu_coroutine_create(stream_run); - trace_stream_start(bs, base, s, co, opaque); - qemu_coroutine_enter(co, s); + s->common.co = qemu_coroutine_create(stream_run); + trace_stream_start(bs, base, s, s->common.co, opaque); + qemu_coroutine_enter(s->common.co, s); } diff --git a/block_int.h b/block_int.h index 3bf2367..b80e66d 100644 --- a/block_int.h +++ b/block_int.h @@ -95,6 +95,12 @@ struct BlockJob { BlockDriverState *bs; /** + * The coroutine that executes the job. If not NULL, it is + * reentered when busy is false and the job is cancelled. + */ + Coroutine *co; + + /** * Set to true if the job should cancel itself. The flag must * always be tested just before toggling the busy flag from false * to true. After a job has been cancelled, it should only yield @@ -418,8 +424,11 @@ bool block_job_is_cancelled(BlockJob *job); * immediately after #block_job_cancel_sync. Users of block jobs * will usually protect the BlockDriverState objects with a reference * count, should this be a concern. + * + * Returns the return value from the job if the job actually completed + * during the call, or -ECANCELED if it was canceled. */ -void block_job_cancel_sync(BlockJob *job); +int block_job_cancel_sync(BlockJob *job); /** * stream_start: