{"id":818226,"url":"http://patchwork.ozlabs.org/api/patches/818226/?format=json","web_url":"http://patchwork.ozlabs.org/project/qemu-devel/patch/20170925122808.14561-6-kwolf@redhat.com/","project":{"id":14,"url":"http://patchwork.ozlabs.org/api/projects/14/?format=json","name":"QEMU Development","link_name":"qemu-devel","list_id":"qemu-devel.nongnu.org","list_email":"qemu-devel@nongnu.org","web_url":"","scm_url":"","webscm_url":"","list_archive_url":"","list_archive_url_format":"","commit_url_format":""},"msgid":"<20170925122808.14561-6-kwolf@redhat.com>","list_archive_url":null,"date":"2017-09-25T12:28:08","name":"[5/5] commit: Remove overlay_bs","commit_ref":null,"pull_url":null,"state":"new","archived":false,"hash":"45eaae3f9bd59d232dca0fac168f129e05930da3","submitter":{"id":2714,"url":"http://patchwork.ozlabs.org/api/people/2714/?format=json","name":"Kevin Wolf","email":"kwolf@redhat.com"},"delegate":null,"mbox":"http://patchwork.ozlabs.org/project/qemu-devel/patch/20170925122808.14561-6-kwolf@redhat.com/mbox/","series":[{"id":4949,"url":"http://patchwork.ozlabs.org/api/series/4949/?format=json","web_url":"http://patchwork.ozlabs.org/project/qemu-devel/list/?series=4949","date":"2017-09-25T12:28:05","name":"commit: Support multiple roots above top node","version":1,"mbox":"http://patchwork.ozlabs.org/series/4949/mbox/"}],"comments":"http://patchwork.ozlabs.org/api/patches/818226/comments/","check":"pending","checks":"http://patchwork.ozlabs.org/api/patches/818226/checks/","tags":{},"related":[],"headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=nongnu.org\n\t(client-ip=2001:4830:134:3::11; helo=lists.gnu.org;\n\tenvelope-from=qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org;\n\treceiver=)","ext-mx08.extmail.prod.ext.phx2.redhat.com;\n\tdmarc=none (p=none dis=none) header.from=redhat.com","ext-mx08.extmail.prod.ext.phx2.redhat.com;\n\tspf=fail smtp.mailfrom=kwolf@redhat.com"],"Received":["from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11])\n\t(using TLSv1 with cipher AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3y13PL1nzkz9tX3\n\tfor ;\n\tMon, 25 Sep 2017 22:32:06 +1000 (AEST)","from localhost ([::1]:42275 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1dwSYO-00070m-Bd\n\tfor incoming@patchwork.ozlabs.org; Mon, 25 Sep 2017 08:32:04 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:47317)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1dwSUw-0003kM-Uv\n\tfor qemu-devel@nongnu.org; Mon, 25 Sep 2017 08:28:32 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1dwSUt-00068b-9Z\n\tfor qemu-devel@nongnu.org; Mon, 25 Sep 2017 08:28:30 -0400","from mx1.redhat.com ([209.132.183.28]:45472)\n\tby eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32)\n\t(Exim 4.71) (envelope-from )\n\tid 1dwSUn-00065D-6E; Mon, 25 Sep 2017 08:28:21 -0400","from smtp.corp.redhat.com\n\t(int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12])\n\t(using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby mx1.redhat.com (Postfix) with ESMTPS id 51D7BC0587EF;\n\tMon, 25 Sep 2017 12:28:20 +0000 (UTC)","from localhost.localdomain.com (ovpn-116-119.ams2.redhat.com\n\t[10.36.116.119])\n\tby smtp.corp.redhat.com (Postfix) with ESMTP id 15A9460E3E;\n\tMon, 25 Sep 2017 12:28:18 +0000 (UTC)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mx1.redhat.com 51D7BC0587EF","From":"Kevin Wolf ","To":"qemu-block@nongnu.org","Date":"Mon, 25 Sep 2017 14:28:08 +0200","Message-Id":"<20170925122808.14561-6-kwolf@redhat.com>","In-Reply-To":"<20170925122808.14561-1-kwolf@redhat.com>","References":"<20170925122808.14561-1-kwolf@redhat.com>","X-Scanned-By":"MIMEDefang 2.79 on 10.5.11.12","X-Greylist":"Sender IP whitelisted, not delayed by milter-greylist-4.5.16\n\t(mx1.redhat.com [10.5.110.32]);\n\tMon, 25 Sep 2017 12:28:20 +0000 (UTC)","X-detected-operating-system":"by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic]\n\t[fuzzy]","X-Received-From":"209.132.183.28","Subject":"[Qemu-devel] [PATCH 5/5] commit: Remove overlay_bs","X-BeenThere":"qemu-devel@nongnu.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"kwolf@redhat.com, qemu-devel@nongnu.org, mreitz@redhat.com","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"},"content":"We don't need to make any assumptions about the graph layout above the\ntop node of the commit operation any more. Remove the use of\nbdrv_find_overlay() and related variables from the commit job code.\n\nbdrv_drop_intermediate() doesn't use the 'active' parameter any more, so\nwe can just drop it.\n\nThe overlay node was previously added to the block job to get a\nBLK_PERM_GRAPH_MOD. We really need to respect those permissions in\nbdrv_drop_intermediate() now, but as long as we haven't figured out yet\nhow BLK_PERM_GRAPH_MOD is actually supposed to work, just leave a TODO\ncomment there.\n\nWith this change, it is now possible to perform another block job on an\noverlay node without conflicts. qemu-iotests 030 is changed accordingly.\n\nSigned-off-by: Kevin Wolf \n---\n include/block/block.h | 3 +--\n block.c | 6 +++--\n block/commit.c | 62 ++++++++++++--------------------------------------\n tests/qemu-iotests/030 | 4 ----\n 4 files changed, 20 insertions(+), 55 deletions(-)","diff":"diff --git a/include/block/block.h b/include/block/block.h\nindex 3c3af462e4..d5c2731a03 100644\n--- a/include/block/block.h\n+++ b/include/block/block.h\n@@ -315,8 +315,7 @@ int bdrv_commit(BlockDriverState *bs);\n int bdrv_change_backing_file(BlockDriverState *bs,\n const char *backing_file, const char *backing_fmt);\n void bdrv_register(BlockDriver *bdrv);\n-int bdrv_drop_intermediate(BlockDriverState *active, BlockDriverState *top,\n- BlockDriverState *base,\n+int bdrv_drop_intermediate(BlockDriverState *top, BlockDriverState *base,\n const char *backing_file_str);\n BlockDriverState *bdrv_find_overlay(BlockDriverState *active,\n BlockDriverState *bs);\ndiff --git a/block.c b/block.c\nindex e57fab501d..5f2d7222a5 100644\n--- a/block.c\n+++ b/block.c\n@@ -3491,8 +3491,8 @@ BlockDriverState *bdrv_find_base(BlockDriverState *bs)\n * if active == top, that is considered an error\n *\n */\n-int bdrv_drop_intermediate(BlockDriverState *active, BlockDriverState *top,\n- BlockDriverState *base, const char *backing_file_str)\n+int bdrv_drop_intermediate(BlockDriverState *top, BlockDriverState *base,\n+ const char *backing_file_str)\n {\n BdrvChild *c, *next;\n Error *local_err = NULL;\n@@ -3510,6 +3510,8 @@ int bdrv_drop_intermediate(BlockDriverState *active, BlockDriverState *top,\n }\n \n /* success - we can delete the intermediate states, and link top->base */\n+ /* TODO Check graph modification op blockers (BLK_PERM_GRAPH_MOD) once\n+ * we've figured out how they should work. */\n backing_file_str = backing_file_str ? backing_file_str : base->filename;\n \n QLIST_FOREACH_SAFE(c, &top->parents, next_parent, next) {\ndiff --git a/block/commit.c b/block/commit.c\nindex 610e1cd8f5..5036eec434 100644\n--- a/block/commit.c\n+++ b/block/commit.c\n@@ -36,13 +36,11 @@ enum {\n typedef struct CommitBlockJob {\n BlockJob common;\n RateLimit limit;\n- BlockDriverState *active;\n BlockDriverState *commit_top_bs;\n BlockBackend *top;\n BlockBackend *base;\n BlockdevOnError on_error;\n int base_flags;\n- int orig_overlay_flags;\n char *backing_file_str;\n } CommitBlockJob;\n \n@@ -81,18 +79,15 @@ static void commit_complete(BlockJob *job, void *opaque)\n {\n CommitBlockJob *s = container_of(job, CommitBlockJob, common);\n CommitCompleteData *data = opaque;\n- BlockDriverState *active = s->active;\n BlockDriverState *top = blk_bs(s->top);\n BlockDriverState *base = blk_bs(s->base);\n- BlockDriverState *overlay_bs = bdrv_find_overlay(active, s->commit_top_bs);\n+ BlockDriverState *commit_top_bs = s->commit_top_bs;\n int ret = data->ret;\n bool remove_commit_top_bs = false;\n \n- /* Make sure overlay_bs and top stay around until bdrv_set_backing_hd() */\n+ /* Make sure commit_top_bs and top stay around until bdrv_replace_node() */\n bdrv_ref(top);\n- if (overlay_bs) {\n- bdrv_ref(overlay_bs);\n- }\n+ bdrv_ref(commit_top_bs);\n \n /* Remove base node parent that still uses BLK_PERM_WRITE/RESIZE before\n * the normal backing chain can be restored. */\n@@ -100,9 +95,9 @@ static void commit_complete(BlockJob *job, void *opaque)\n \n if (!block_job_is_cancelled(&s->common) && ret == 0) {\n /* success */\n- ret = bdrv_drop_intermediate(active, s->commit_top_bs, base,\n+ ret = bdrv_drop_intermediate(s->commit_top_bs, base,\n s->backing_file_str);\n- } else if (overlay_bs) {\n+ } else {\n /* XXX Can (or should) we somehow keep 'consistent read' blocked even\n * after the failed/cancelled commit job is gone? If we already wrote\n * something to base, the intermediate images aren't valid any more. */\n@@ -115,9 +110,6 @@ static void commit_complete(BlockJob *job, void *opaque)\n if (s->base_flags != bdrv_get_flags(base)) {\n bdrv_reopen(base, s->base_flags, NULL);\n }\n- if (overlay_bs && s->orig_overlay_flags != bdrv_get_flags(overlay_bs)) {\n- bdrv_reopen(overlay_bs, s->orig_overlay_flags, NULL);\n- }\n g_free(s->backing_file_str);\n blk_unref(s->top);\n \n@@ -134,10 +126,13 @@ static void commit_complete(BlockJob *job, void *opaque)\n * filter driver from the backing chain. Do this as the final step so that\n * the 'consistent read' permission can be granted. */\n if (remove_commit_top_bs) {\n- bdrv_set_backing_hd(overlay_bs, top, &error_abort);\n+ bdrv_child_try_set_perm(commit_top_bs->backing, 0, BLK_PERM_ALL,\n+ &error_abort);\n+ bdrv_replace_node(commit_top_bs, backing_bs(commit_top_bs),\n+ &error_abort);\n }\n \n- bdrv_unref(overlay_bs);\n+ bdrv_unref(commit_top_bs);\n bdrv_unref(top);\n }\n \n@@ -283,10 +278,8 @@ void commit_start(const char *job_id, BlockDriverState *bs,\n {\n CommitBlockJob *s;\n BlockReopenQueue *reopen_queue = NULL;\n- int orig_overlay_flags;\n int orig_base_flags;\n BlockDriverState *iter;\n- BlockDriverState *overlay_bs;\n BlockDriverState *commit_top_bs = NULL;\n Error *local_err = NULL;\n int ret;\n@@ -297,31 +290,19 @@ void commit_start(const char *job_id, BlockDriverState *bs,\n return;\n }\n \n- overlay_bs = bdrv_find_overlay(bs, top);\n-\n- if (overlay_bs == NULL) {\n- error_setg(errp, \"Could not find overlay image for %s:\", top->filename);\n- return;\n- }\n-\n s = block_job_create(job_id, &commit_job_driver, bs, 0, BLK_PERM_ALL,\n speed, BLOCK_JOB_DEFAULT, NULL, NULL, errp);\n if (!s) {\n return;\n }\n \n- orig_base_flags = bdrv_get_flags(base);\n- orig_overlay_flags = bdrv_get_flags(overlay_bs);\n-\n- /* convert base & overlay_bs to r/w, if necessary */\n+ /* convert base to r/w, if necessary */\n+ orig_base_flags = bdrv_get_flags(base);\n if (!(orig_base_flags & BDRV_O_RDWR)) {\n reopen_queue = bdrv_reopen_queue(reopen_queue, base, NULL,\n orig_base_flags | BDRV_O_RDWR);\n }\n- if (!(orig_overlay_flags & BDRV_O_RDWR)) {\n- reopen_queue = bdrv_reopen_queue(reopen_queue, overlay_bs, NULL,\n- orig_overlay_flags | BDRV_O_RDWR);\n- }\n+\n if (reopen_queue) {\n bdrv_reopen_multiple(bdrv_get_aio_context(bs), reopen_queue, &local_err);\n if (local_err != NULL) {\n@@ -382,14 +363,6 @@ void commit_start(const char *job_id, BlockDriverState *bs,\n goto fail;\n }\n \n- /* overlay_bs must be blocked because it needs to be modified to\n- * update the backing image string. */\n- ret = block_job_add_bdrv(&s->common, \"overlay of top\", overlay_bs,\n- BLK_PERM_GRAPH_MOD, BLK_PERM_ALL, errp);\n- if (ret < 0) {\n- goto fail;\n- }\n-\n s->base = blk_new(BLK_PERM_CONSISTENT_READ\n | BLK_PERM_WRITE\n | BLK_PERM_RESIZE,\n@@ -408,13 +381,8 @@ void commit_start(const char *job_id, BlockDriverState *bs,\n goto fail;\n }\n \n- s->active = bs;\n-\n- s->base_flags = orig_base_flags;\n- s->orig_overlay_flags = orig_overlay_flags;\n-\n+ s->base_flags = orig_base_flags;\n s->backing_file_str = g_strdup(backing_file_str);\n-\n s->on_error = on_error;\n \n trace_commit_start(bs, base, top, s);\n@@ -429,7 +397,7 @@ fail:\n blk_unref(s->top);\n }\n if (commit_top_bs) {\n- bdrv_set_backing_hd(overlay_bs, top, &error_abort);\n+ bdrv_replace_node(commit_top_bs, top, &error_abort);\n }\n block_job_early_fail(&s->common);\n }\ndiff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030\nindex d745cb4cde..18838948fa 100755\n--- a/tests/qemu-iotests/030\n+++ b/tests/qemu-iotests/030\n@@ -287,10 +287,6 @@ class TestParallelOps(iotests.QMPTestCase):\n result = self.vm.qmp('block-stream', device='node6', base=self.imgs[4], job_id='stream-node6-v2')\n self.assert_qmp(result, 'error/class', 'GenericError')\n \n- # This fails because block-commit needs to block node6, the overlay of the 'top' image\n- result = self.vm.qmp('block-stream', device='node7', base=self.imgs[5], job_id='stream-node6-v3')\n- self.assert_qmp(result, 'error/class', 'GenericError')\n-\n # This fails because block-commit currently blocks the active layer even if it's not used\n result = self.vm.qmp('block-stream', device='drive0', base=self.imgs[5], job_id='stream-drive0')\n self.assert_qmp(result, 'error/class', 'GenericError')\n","prefixes":["5/5"]}