From patchwork Wed Oct 1 17:01:58 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Stefan Hajnoczi X-Patchwork-Id: 395618 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 1E5181400DD for ; Thu, 2 Oct 2014 03:08:49 +1000 (EST) Received: from localhost ([::1]:56955 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XZNOQ-00059g-UH for incoming@patchwork.ozlabs.org; Wed, 01 Oct 2014 13:08:46 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34621) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XZNIQ-0002tq-DK for qemu-devel@nongnu.org; Wed, 01 Oct 2014 13:02:40 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XZNIK-0001jO-7l for qemu-devel@nongnu.org; Wed, 01 Oct 2014 13:02:34 -0400 Received: from mx1.redhat.com ([209.132.183.28]:60025) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XZNIK-0001jI-14 for qemu-devel@nongnu.org; Wed, 01 Oct 2014 13:02:28 -0400 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s91H2Rk2006158 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL) for ; Wed, 1 Oct 2014 13:02:27 -0400 Received: from localhost (ovpn-112-62.ams2.redhat.com [10.36.112.62]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id s91H2QdJ003661; Wed, 1 Oct 2014 13:02:26 -0400 From: Stefan Hajnoczi To: Date: Wed, 1 Oct 2014 18:01:58 +0100 Message-Id: <1412182919-9550-11-git-send-email-stefanha@redhat.com> In-Reply-To: <1412182919-9550-1-git-send-email-stefanha@redhat.com> References: <1412182919-9550-1-git-send-email-stefanha@redhat.com> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.23 X-detected-operating-system: by eggs.gnu.org: GNU/Linux 3.x X-Received-From: 209.132.183.28 Cc: Kevin Wolf , Paolo Bonzini , Fam Zheng , Stefan Hajnoczi , Max Reitz Subject: [Qemu-devel] [PATCH 10/11] block: let commit blockjob run in BDS AioContext 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 commit block job must run in the BlockDriverState AioContext so that it works with dataplane. Acquire the AioContext in blockdev.c so starting the block job is safe. One detail here is that the bdrv_drain_all() must be moved inside the aio_context_acquire() region so requests cannot sneak in between the drain and acquire. The completion code in block/commit.c must perform backing chain manipulation and bdrv_reopen() from the main loop. Use block_job_defer_to_main_loop() to achieve that. Signed-off-by: Stefan Hajnoczi Reviewed-by: Max Reitz --- block/commit.c | 72 +++++++++++++++++++++++++++++++++++++--------------------- blockdev.c | 29 +++++++++++++++-------- 2 files changed, 66 insertions(+), 35 deletions(-) diff --git a/block/commit.c b/block/commit.c index 91517d3..0fd05dc 100644 --- a/block/commit.c +++ b/block/commit.c @@ -60,17 +60,50 @@ static int coroutine_fn commit_populate(BlockDriverState *bs, return 0; } -static void coroutine_fn commit_run(void *opaque) +typedef struct { + int ret; +} CommitCompleteData; + +static void commit_complete(BlockJob *job, void *opaque) { - CommitBlockJob *s = opaque; + CommitBlockJob *s = container_of(job, CommitBlockJob, common); + CommitCompleteData *data = opaque; BlockDriverState *active = s->active; BlockDriverState *top = s->top; BlockDriverState *base = s->base; BlockDriverState *overlay_bs; + int ret = data->ret; + + if (!block_job_is_cancelled(&s->common) && ret == 0) { + /* success */ + ret = bdrv_drop_intermediate(active, top, base, s->backing_file_str); + } + + /* restore base open flags here if appropriate (e.g., change the base back + * to r/o). These reopens do not need to be atomic, since we won't abort + * even on failure here */ + if (s->base_flags != bdrv_get_flags(base)) { + bdrv_reopen(base, s->base_flags, NULL); + } + overlay_bs = bdrv_find_overlay(active, top); + if (overlay_bs && s->orig_overlay_flags != bdrv_get_flags(overlay_bs)) { + bdrv_reopen(overlay_bs, s->orig_overlay_flags, NULL); + } + g_free(s->backing_file_str); + block_job_completed(&s->common, ret); + g_free(data); +} + +static void coroutine_fn commit_run(void *opaque) +{ + CommitBlockJob *s = opaque; + CommitCompleteData *data; + BlockDriverState *top = s->top; + BlockDriverState *base = s->base; int64_t sector_num, end; int ret = 0; int n = 0; - void *buf; + void *buf = NULL; int bytes_written = 0; int64_t base_len; @@ -78,18 +111,18 @@ static void coroutine_fn commit_run(void *opaque) if (s->common.len < 0) { - goto exit_restore_reopen; + goto out; } ret = base_len = bdrv_getlength(base); if (base_len < 0) { - goto exit_restore_reopen; + goto out; } if (base_len < s->common.len) { ret = bdrv_truncate(base, s->common.len); if (ret) { - goto exit_restore_reopen; + goto out; } } @@ -128,7 +161,7 @@ wait: if (s->on_error == BLOCKDEV_ON_ERROR_STOP || s->on_error == BLOCKDEV_ON_ERROR_REPORT|| (s->on_error == BLOCKDEV_ON_ERROR_ENOSPC && ret == -ENOSPC)) { - goto exit_free_buf; + goto out; } else { n = 0; continue; @@ -140,27 +173,14 @@ wait: ret = 0; - if (!block_job_is_cancelled(&s->common) && sector_num == end) { - /* success */ - ret = bdrv_drop_intermediate(active, top, base, s->backing_file_str); +out: + if (buf) { + qemu_vfree(buf); } -exit_free_buf: - qemu_vfree(buf); - -exit_restore_reopen: - /* restore base open flags here if appropriate (e.g., change the base back - * to r/o). These reopens do not need to be atomic, since we won't abort - * even on failure here */ - if (s->base_flags != bdrv_get_flags(base)) { - bdrv_reopen(base, s->base_flags, NULL); - } - overlay_bs = bdrv_find_overlay(active, top); - if (overlay_bs && s->orig_overlay_flags != bdrv_get_flags(overlay_bs)) { - bdrv_reopen(overlay_bs, s->orig_overlay_flags, NULL); - } - g_free(s->backing_file_str); - block_job_completed(&s->common, ret); + data = g_malloc(sizeof(*data)); + data->ret = ret; + block_job_defer_to_main_loop(&s->common, commit_complete, data); } static void commit_set_speed(BlockJob *job, int64_t speed, Error **errp) diff --git a/blockdev.c b/blockdev.c index 5cf2058..d9333d7 100644 --- a/blockdev.c +++ b/blockdev.c @@ -1987,6 +1987,7 @@ void qmp_block_commit(const char *device, { BlockDriverState *bs; BlockDriverState *base_bs, *top_bs; + AioContext *aio_context; Error *local_err = NULL; /* This will be part of the QMP command, if/when the * BlockdevOnError change for blkmirror makes it in @@ -1997,9 +1998,6 @@ void qmp_block_commit(const char *device, speed = 0; } - /* drain all i/o before commits */ - bdrv_drain_all(); - /* Important Note: * libvirt relies on the DeviceNotFound error class in order to probe for * live commit feature versions; for this to work, we must make sure to @@ -2011,8 +2009,14 @@ void qmp_block_commit(const char *device, return; } + aio_context = bdrv_get_aio_context(bs); + aio_context_acquire(aio_context); + + /* drain all i/o before commits */ + bdrv_drain_all(); + if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_COMMIT, errp)) { - return; + goto out; } /* default top_bs is the active layer */ @@ -2026,9 +2030,11 @@ void qmp_block_commit(const char *device, if (top_bs == NULL) { error_setg(errp, "Top image file %s not found", top ? top : "NULL"); - return; + goto out; } + assert(bdrv_get_aio_context(top_bs) == aio_context); + if (has_base && base) { base_bs = bdrv_find_backing_image(top_bs, base); } else { @@ -2037,20 +2043,22 @@ void qmp_block_commit(const char *device, if (base_bs == NULL) { error_set(errp, QERR_BASE_NOT_FOUND, base ? base : "NULL"); - return; + goto out; } + assert(bdrv_get_aio_context(base_bs) == aio_context); + /* Do not allow attempts to commit an image into itself */ if (top_bs == base_bs) { error_setg(errp, "cannot commit an image into itself"); - return; + goto out; } if (top_bs == bs) { if (has_backing_file) { error_setg(errp, "'backing-file' specified," " but 'top' is the active layer"); - return; + goto out; } commit_active_start(bs, base_bs, speed, on_error, block_job_cb, bs, &local_err); @@ -2060,8 +2068,11 @@ void qmp_block_commit(const char *device, } if (local_err != NULL) { error_propagate(errp, local_err); - return; + goto out; } + +out: + aio_context_release(aio_context); } void qmp_drive_backup(const char *device, const char *target,