[{"id":1774945,"web_url":"http://patchwork.ozlabs.org/comment/1774945/","msgid":"<59f25063-4338-dc44-be0b-bc682e72d4ee@redhat.com>","list_archive_url":null,"date":"2017-09-25T19:38:59","subject":"Re: [Qemu-devel] [PATCH 2/5] commit: Support multiple roots above\n\ttop node","submitter":{"id":6591,"url":"http://patchwork.ozlabs.org/api/people/6591/","name":"Eric Blake","email":"eblake@redhat.com"},"content":"On 09/25/2017 07:28 AM, Kevin Wolf wrote:\n> This changes the commit block job to support operation in a graph where\n> there is more than a single active layer that references the top node.\n> \n> This involves inserting the commit filter node not only on the path\n> between the given active node and the top node, but between the top node\n> and all of its parents.\n> \n> On completion, bdrv_drop_intermediate() must consider all parents for\n> updating the backing file link. These parents may be backing files\n> themselves and such read-only; reopen them temporarily if necessary.\n\ns/such/as such/\n\n> Previously this was achieved by the bdrv_reopen() calls in the commit\n> block job that made overlay_bs read-write for the whole duration of the\n> block job, even though write access is only needed on completion.\n\nDo we know, at the start of the operation, enough to reject attempts\nthat will eventually fail because we cannot obtain write access at\ncompletion, without having to wait for all the intermediate work before\nthe completion phase is reached? If not, that might be a bit of a\nregression (where a command used to fail up front, we now waste a lot of\neffort, and possibly modify the backing chain irreversably, before\nfinding out we still fail because we lack write access).\n\n> \n> Now that we consider all parents, overlay_bs is meaningless. It is left\n> in place in this commit, but we'll remove it soon.\n> \n> Signed-off-by: Kevin Wolf \n> ---\n> block.c | 68 ++++++++++++++++++++++++++++++++++------------------------\n> block/commit.c | 2 +-\n> 2 files changed, 41 insertions(+), 29 deletions(-)\n> \n> /* success - we can delete the intermediate states, and link top->base */\n> - if (new_top_bs->backing->role->update_filename) {\n> - backing_file_str = backing_file_str ? backing_file_str : base->filename;\n> - ret = new_top_bs->backing->role->update_filename(new_top_bs->backing,\n> - base, backing_file_str,\n> - &local_err);\n> - if (ret < 0) {\n> - bdrv_set_backing_hd(new_top_bs, top, &error_abort);\n> + backing_file_str = backing_file_str ? backing_file_str : base->filename;\n> +\n> + QLIST_FOREACH_SAFE(c, &top->parents, next_parent, next) {\n> + /* Check whether we are allowed to switch c from top to base */\n> + GSList *ignore_children = g_slist_prepend(NULL, c);\n> + bdrv_check_update_perm(base, NULL, c->perm, c->shared_perm,\n> + ignore_children, &local_err);\n> + if (local_err) {\n> + ret = -EPERM;\n> + error_report_err(local_err);\n> goto exit;\n> }\n> - }\n> + g_slist_free(ignore_children);\n> \n\nAnd it looks like you DO check up front that permissions are sufficient\nfor later on.","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-mx03.extmail.prod.ext.phx2.redhat.com;\n\tdmarc=none (p=none dis=none) header.from=redhat.com","ext-mx03.extmail.prod.ext.phx2.redhat.com;\n\tspf=fail smtp.mailfrom=eblake@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 3y1Dtt233kz9t3x\n\tfor ;\n\tTue, 26 Sep 2017 05:39:48 +1000 (AEST)","from localhost ([::1]:43978 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1dwZEI-0003S7-0H\n\tfor incoming@patchwork.ozlabs.org; Mon, 25 Sep 2017 15:39:46 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:47945)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1dwZDi-0003Lr-Hp\n\tfor qemu-devel@nongnu.org; Mon, 25 Sep 2017 15:39:12 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1dwZDh-0004BD-0r\n\tfor qemu-devel@nongnu.org; Mon, 25 Sep 2017 15:39:10 -0400","from mx1.redhat.com ([209.132.183.28]:60078)\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 1dwZDa-00049K-IG; Mon, 25 Sep 2017 15:39:02 -0400","from smtp.corp.redhat.com\n\t(int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15])\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 5EE2780F75;\n\tMon, 25 Sep 2017 19:39:01 +0000 (UTC)","from [10.10.124.97] (ovpn-124-97.rdu2.redhat.com [10.10.124.97])\n\tby smtp.corp.redhat.com (Postfix) with ESMTP id 91A655D761;\n\tMon, 25 Sep 2017 19:39:00 +0000 (UTC)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mx1.redhat.com 5EE2780F75","To":"Kevin Wolf , qemu-block@nongnu.org","References":"<20170925122808.14561-1-kwolf@redhat.com>\n\t<20170925122808.14561-3-kwolf@redhat.com>","From":"Eric Blake ","Openpgp":"url=http://people.redhat.com/eblake/eblake.gpg","Organization":"Red Hat, Inc.","Message-ID":"<59f25063-4338-dc44-be0b-bc682e72d4ee@redhat.com>","Date":"Mon, 25 Sep 2017 14:38:59 -0500","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.3.0","MIME-Version":"1.0","In-Reply-To":"<20170925122808.14561-3-kwolf@redhat.com>","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\";\n\tboundary=\"P6qgJCaLt8qLn8jae4Lt6AFWLMWdgGipc\"","X-Scanned-By":"MIMEDefang 2.79 on 10.5.11.15","X-Greylist":"Sender IP whitelisted, not delayed by milter-greylist-4.5.16\n\t(mx1.redhat.com [10.5.110.27]);\n\tMon, 25 Sep 2017 19:39:01 +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","X-Content-Filtered-By":"Mailman/MimeDel 2.1.21","Subject":"Re: [Qemu-devel] [PATCH 2/5] commit: Support multiple roots above\n\ttop node","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":"qemu-devel@nongnu.org, mreitz@redhat.com","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}},{"id":1775741,"web_url":"http://patchwork.ozlabs.org/comment/1775741/","msgid":"<20170926173547.GD14717@localhost.localdomain>","list_archive_url":null,"date":"2017-09-26T17:35:47","subject":"Re: [Qemu-devel] [PATCH 2/5] commit: Support multiple roots above\n\ttop node","submitter":{"id":2714,"url":"http://patchwork.ozlabs.org/api/people/2714/","name":"Kevin Wolf","email":"kwolf@redhat.com"},"content":"Am 25.09.2017 um 21:38 hat Eric Blake geschrieben:\n> On 09/25/2017 07:28 AM, Kevin Wolf wrote:\n> > This changes the commit block job to support operation in a graph where\n> > there is more than a single active layer that references the top node.\n> > \n> > This involves inserting the commit filter node not only on the path\n> > between the given active node and the top node, but between the top node\n> > and all of its parents.\n> > \n> > On completion, bdrv_drop_intermediate() must consider all parents for\n> > updating the backing file link. These parents may be backing files\n> > themselves and such read-only; reopen them temporarily if necessary.\n> \n> s/such/as such/\n\nYes, thanks.\n\n> > Previously this was achieved by the bdrv_reopen() calls in the commit\n> > block job that made overlay_bs read-write for the whole duration of the\n> > block job, even though write access is only needed on completion.\n> \n> Do we know, at the start of the operation, enough to reject attempts\n> that will eventually fail because we cannot obtain write access at\n> completion, without having to wait for all the intermediate work before\n> the completion phase is reached? If not, that might be a bit of a\n> regression (where a command used to fail up front, we now waste a lot of\n> effort, and possibly modify the backing chain irreversably, before\n> finding out we still fail because we lack write access).\n\nIt is kind of a change in behaviour, but I wouldn't call it a\nregression. The reason is that today I'm looking at it with a completely\ndifferent perspective than when the command was added.\n\nOriginally, we assumed a more or less static block node graph, which in\nfact was only a mostly degenerated tree - a backing file chain. Block\njobs were kind of an exceptional thing and we allowed only one per\nchain. In this restricted setting, checking at the start of the\noperation made sense because we wouldn't allow things to change while\nthe job was running anyway.\n\nA big part of the recent blockdev work, however, is about getting rid of\narbitrary restrictions like this. We want to allow running two commit\noperations in the same backing chain as long as they don't conflict. We\ndon't want to prevent adding new users to an existing node unless there\nis a good reason.\n\nIf you consider this, the set of nodes that will get their backing file\nrewritten at the end of the block job isn't known yet when the job\nstarts; and even if we knew it, keeping the nodes writable all the time\nwhile the job runs feels like it could easily conflict with other\ncallers of bdrv_reopen().\n\n> > Now that we consider all parents, overlay_bs is meaningless. It is left\n> > in place in this commit, but we'll remove it soon.\n> > \n> > Signed-off-by: Kevin Wolf \n> > ---\n> > block.c | 68 ++++++++++++++++++++++++++++++++++------------------------\n> > block/commit.c | 2 +-\n> > 2 files changed, 41 insertions(+), 29 deletions(-)\n> > \n> > /* success - we can delete the intermediate states, and link top->base */\n> > - if (new_top_bs->backing->role->update_filename) {\n> > - backing_file_str = backing_file_str ? backing_file_str : base->filename;\n> > - ret = new_top_bs->backing->role->update_filename(new_top_bs->backing,\n> > - base, backing_file_str,\n> > - &local_err);\n> > - if (ret < 0) {\n> > - bdrv_set_backing_hd(new_top_bs, top, &error_abort);\n> > + backing_file_str = backing_file_str ? backing_file_str : base->filename;\n> > +\n> > + QLIST_FOREACH_SAFE(c, &top->parents, next_parent, next) {\n> > + /* Check whether we are allowed to switch c from top to base */\n> > + GSList *ignore_children = g_slist_prepend(NULL, c);\n> > + bdrv_check_update_perm(base, NULL, c->perm, c->shared_perm,\n> > + ignore_children, &local_err);\n> > + if (local_err) {\n> > + ret = -EPERM;\n> > + error_report_err(local_err);\n> > goto exit;\n> > }\n> > - }\n> > + g_slist_free(ignore_children);\n> > \n> \n> And it looks like you DO check up front that permissions are sufficient\n> for later on.\n\nThis is already during completion. The completion code uses the\ntransactional nature of permission updates to make sure that the\nin-memory graph stays consistent with the on-disk backing file links\neven in case of errors, but it's not really upfront in the sense of\nchecking when the block job is started. (You also need to hold the BQL\nbetween starting and completing a permission change transaction, so just\nmoving this code wouldn't work.)\n\nKevin","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-mx04.extmail.prod.ext.phx2.redhat.com;\n\tdmarc=none (p=none dis=none) header.from=redhat.com","ext-mx04.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 3y1p645PG2z9sPr\n\tfor ;\n\tWed, 27 Sep 2017 03:36:28 +1000 (AEST)","from localhost ([::1]:50476 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1dwtmU-0003hs-Ui\n\tfor incoming@patchwork.ozlabs.org; Tue, 26 Sep 2017 13:36:26 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:58912)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1dwtm3-0003ez-Mf\n\tfor qemu-devel@nongnu.org; Tue, 26 Sep 2017 13:36:01 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1dwtm2-0001yz-3I\n\tfor qemu-devel@nongnu.org; Tue, 26 Sep 2017 13:35:59 -0400","from mx1.redhat.com ([209.132.183.28]:34546)\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 1dwtlv-0001mI-6B; Tue, 26 Sep 2017 13:35:51 -0400","from smtp.corp.redhat.com\n\t(int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15])\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 0B01A7EA80;\n\tTue, 26 Sep 2017 17:35:50 +0000 (UTC)","from localhost.localdomain (ovpn-117-39.ams2.redhat.com\n\t[10.36.117.39])\n\tby smtp.corp.redhat.com (Postfix) with ESMTPS id E1F6F62463;\n\tTue, 26 Sep 2017 17:35:48 +0000 (UTC)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mx1.redhat.com 0B01A7EA80","Date":"Tue, 26 Sep 2017 19:35:47 +0200","From":"Kevin Wolf ","To":"Eric Blake ","Message-ID":"<20170926173547.GD14717@localhost.localdomain>","References":"<20170925122808.14561-1-kwolf@redhat.com>\n\t<20170925122808.14561-3-kwolf@redhat.com>\n\t<59f25063-4338-dc44-be0b-bc682e72d4ee@redhat.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha1;\n\tprotocol=\"application/pgp-signature\"; boundary=\"pvezYHf7grwyp3Bc\"","Content-Disposition":"inline","In-Reply-To":"<59f25063-4338-dc44-be0b-bc682e72d4ee@redhat.com>","User-Agent":"Mutt/1.9.0 (2017-09-02)","X-Scanned-By":"MIMEDefang 2.79 on 10.5.11.15","X-Greylist":"Sender IP whitelisted, not delayed by milter-greylist-4.5.16\n\t(mx1.redhat.com [10.5.110.28]);\n\tTue, 26 Sep 2017 17:35:50 +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":"Re: [Qemu-devel] [PATCH 2/5] commit: Support multiple roots above\n\ttop node","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":"qemu-devel@nongnu.org, qemu-block@nongnu.org, mreitz@redhat.com","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}}]