From patchwork Tue Feb 28 12:54:10 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Kevin Wolf X-Patchwork-Id: 9595685 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 5EE3960453 for ; Tue, 28 Feb 2017 13:53:49 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 4D81C27FBC for ; Tue, 28 Feb 2017 13:53:49 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 424AB28518; Tue, 28 Feb 2017 13:53:49 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.9 required=2.0 tests=BAYES_00,RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 Received: from lists.gnu.org (lists.gnu.org [208.118.235.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id 95CCD27FBC for ; Tue, 28 Feb 2017 13:53:48 +0000 (UTC) Received: from localhost ([::1]:32942 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ciiDr-0007IY-O2 for patchwork-qemu-devel@patchwork.kernel.org; Tue, 28 Feb 2017 08:53:47 -0500 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43725) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1cihJQ-0004cT-8y for qemu-devel@nongnu.org; Tue, 28 Feb 2017 07:55:29 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1cihJO-0004MA-VI for qemu-devel@nongnu.org; Tue, 28 Feb 2017 07:55:28 -0500 Received: from mx1.redhat.com ([209.132.183.28]:53620) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1cihJK-0004Gy-Po; Tue, 28 Feb 2017 07:55:22 -0500 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 CF32A7E9D1; Tue, 28 Feb 2017 12:55:22 +0000 (UTC) Received: from noname.redhat.com (ovpn-116-177.ams2.redhat.com [10.36.116.177]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id v1SCsXQR029713; Tue, 28 Feb 2017 07:55:21 -0500 From: Kevin Wolf To: qemu-block@nongnu.org Date: Tue, 28 Feb 2017 13:54:10 +0100 Message-Id: <1488286469-9381-26-git-send-email-kwolf@redhat.com> In-Reply-To: <1488286469-9381-1-git-send-email-kwolf@redhat.com> References: <1488286469-9381-1-git-send-email-kwolf@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.26]); Tue, 28 Feb 2017 12:55:22 +0000 (UTC) X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] [fuzzy] X-Received-From: 209.132.183.28 Subject: [Qemu-devel] [PATCH v3 25/44] commit: Use real permissions in commit block job 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: kwolf@redhat.com, jcody@redhat.com, famz@redhat.com, qemu-devel@nongnu.org, mreitz@redhat.com Errors-To: qemu-devel-bounces+patchwork-qemu-devel=patchwork.kernel.org@nongnu.org Sender: "Qemu-devel" X-Virus-Scanned: ClamAV using ClamSMTP This is probably one of the most interesting conversions to the new op blocker system because a commit block job intentionally leaves some intermediate block nodes in the backing chain that aren't valid on their own any more; only the whole chain together results in a valid view. In order to provide the 'consistent read' permission to the parents of the 'top' node of the commit job, a new filter block driver is inserted above 'top' which doesn't require 'consistent read' on its backing chain. Subsequently, the commit job can block 'consistent read' on all intermediate nodes without causing a conflict. Signed-off-by: Kevin Wolf Reviewed-by: Max Reitz --- block/commit.c | 113 ++++++++++++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 95 insertions(+), 18 deletions(-) diff --git a/block/commit.c b/block/commit.c index b69586f..8de4473 100644 --- a/block/commit.c +++ b/block/commit.c @@ -36,6 +36,7 @@ typedef struct CommitBlockJob { BlockJob common; RateLimit limit; BlockDriverState *active; + BlockDriverState *commit_top_bs; BlockBackend *top; BlockBackend *base; BlockdevOnError on_error; @@ -83,12 +84,23 @@ static void commit_complete(BlockJob *job, void *opaque) BlockDriverState *active = s->active; BlockDriverState *top = blk_bs(s->top); BlockDriverState *base = blk_bs(s->base); - BlockDriverState *overlay_bs = bdrv_find_overlay(active, top); + BlockDriverState *overlay_bs = bdrv_find_overlay(active, s->commit_top_bs); int ret = data->ret; + bool remove_commit_top_bs = false; + + /* Remove base node parent that still uses BLK_PERM_WRITE/RESIZE before + * the normal backing chain can be restored. */ + blk_unref(s->base); if (!block_job_is_cancelled(&s->common) && ret == 0) { /* success */ - ret = bdrv_drop_intermediate(active, top, base, s->backing_file_str); + ret = bdrv_drop_intermediate(active, s->commit_top_bs, base, + s->backing_file_str); + } else if (overlay_bs) { + /* XXX Can (or should) we somehow keep 'consistent read' blocked even + * after the failed/cancelled commit job is gone? If we already wrote + * something to base, the intermediate images aren't valid any more. */ + remove_commit_top_bs = true; } /* restore base open flags here if appropriate (e.g., change the base back @@ -102,9 +114,15 @@ static void commit_complete(BlockJob *job, void *opaque) } g_free(s->backing_file_str); blk_unref(s->top); - blk_unref(s->base); block_job_completed(&s->common, ret); g_free(data); + + /* If bdrv_drop_intermediate() didn't already do that, remove the commit + * filter driver from the backing chain. Do this as the final step so that + * the 'consistent read' permission can be granted. */ + if (remove_commit_top_bs) { + bdrv_set_backing_hd(overlay_bs, top); + } } static void coroutine_fn commit_run(void *opaque) @@ -208,6 +226,34 @@ static const BlockJobDriver commit_job_driver = { .start = commit_run, }; +static int coroutine_fn bdrv_commit_top_preadv(BlockDriverState *bs, + uint64_t offset, uint64_t bytes, QEMUIOVector *qiov, int flags) +{ + return bdrv_co_preadv(bs->backing, offset, bytes, qiov, flags); +} + +static void bdrv_commit_top_close(BlockDriverState *bs) +{ +} + +static void bdrv_commit_top_child_perm(BlockDriverState *bs, BdrvChild *c, + const BdrvChildRole *role, + uint64_t perm, uint64_t shared, + uint64_t *nperm, uint64_t *nshared) +{ + *nperm = 0; + *nshared = BLK_PERM_ALL; +} + +/* Dummy node that provides consistent read to its users without requiring it + * from its backing file and that allows writes on the backing file chain. */ +static BlockDriver bdrv_commit_top = { + .format_name = "commit_top", + .bdrv_co_preadv = bdrv_commit_top_preadv, + .bdrv_close = bdrv_commit_top_close, + .bdrv_child_perm = bdrv_commit_top_child_perm, +}; + void commit_start(const char *job_id, BlockDriverState *bs, BlockDriverState *base, BlockDriverState *top, int64_t speed, BlockdevOnError on_error, const char *backing_file_str, @@ -219,6 +265,7 @@ void commit_start(const char *job_id, BlockDriverState *bs, int orig_base_flags; BlockDriverState *iter; BlockDriverState *overlay_bs; + BlockDriverState *commit_top_bs = NULL; Error *local_err = NULL; int ret; @@ -235,7 +282,6 @@ void commit_start(const char *job_id, BlockDriverState *bs, return; } - /* FIXME Use real permissions */ s = block_job_create(job_id, &commit_job_driver, bs, 0, BLK_PERM_ALL, speed, BLOCK_JOB_DEFAULT, NULL, NULL, errp); if (!s) { @@ -262,34 +308,62 @@ void commit_start(const char *job_id, BlockDriverState *bs, } } + /* Insert commit_top block node above top, so we can block consistent read + * on the backing chain below it */ + commit_top_bs = bdrv_new_open_driver(&bdrv_commit_top, NULL, 0, errp); + if (commit_top_bs == NULL) { + goto fail; + } + + bdrv_set_backing_hd(commit_top_bs, top); + bdrv_set_backing_hd(overlay_bs, commit_top_bs); + + s->commit_top_bs = commit_top_bs; + bdrv_unref(commit_top_bs); /* Block all nodes between top and base, because they will * disappear from the chain after this operation. */ assert(bdrv_chain_contains(top, base)); - for (iter = top; iter != backing_bs(base); iter = backing_bs(iter)) { - /* FIXME Use real permissions */ - block_job_add_bdrv(&s->common, "intermediate node", iter, 0, - BLK_PERM_ALL, &error_abort); + for (iter = top; iter != base; iter = backing_bs(iter)) { + /* XXX BLK_PERM_WRITE needs to be allowed so we don't block ourselves + * at s->base (if writes are blocked for a node, they are also blocked + * for its backing file). The other options would be a second filter + * driver above s->base. */ + ret = block_job_add_bdrv(&s->common, "intermediate node", iter, 0, + BLK_PERM_WRITE_UNCHANGED | BLK_PERM_WRITE, + errp); + if (ret < 0) { + goto fail; + } } + + ret = block_job_add_bdrv(&s->common, "base", base, 0, BLK_PERM_ALL, errp); + if (ret < 0) { + goto fail; + } + /* overlay_bs must be blocked because it needs to be modified to - * update the backing image string, but if it's the root node then - * don't block it again */ - if (bs != overlay_bs) { - /* FIXME Use real permissions */ - block_job_add_bdrv(&s->common, "overlay of top", overlay_bs, 0, - BLK_PERM_ALL, &error_abort); + * update the backing image string. */ + ret = block_job_add_bdrv(&s->common, "overlay of top", overlay_bs, + BLK_PERM_GRAPH_MOD, BLK_PERM_ALL, errp); + if (ret < 0) { + goto fail; } - /* FIXME Use real permissions */ - s->base = blk_new(0, BLK_PERM_ALL); + s->base = blk_new(BLK_PERM_CONSISTENT_READ + | BLK_PERM_WRITE + | BLK_PERM_RESIZE, + BLK_PERM_CONSISTENT_READ + | BLK_PERM_GRAPH_MOD + | BLK_PERM_WRITE_UNCHANGED); ret = blk_insert_bs(s->base, base, errp); if (ret < 0) { goto fail; } - /* FIXME Use real permissions */ + /* Required permissions are already taken with block_job_add_bdrv() */ s->top = blk_new(0, BLK_PERM_ALL); - ret = blk_insert_bs(s->top, top, errp); + blk_insert_bs(s->top, top, errp); if (ret < 0) { goto fail; } @@ -314,6 +388,9 @@ fail: if (s->top) { blk_unref(s->top); } + if (commit_top_bs) { + bdrv_set_backing_hd(overlay_bs, top); + } block_job_unref(&s->common); }