[{"id":1783568,"web_url":"http://patchwork.ozlabs.org/comment/1783568/","msgid":"<20171010094738.GF4177@dhcp-200-186.str.redhat.com>","list_archive_url":null,"date":"2017-10-10T09:47:38","subject":"Re: [Qemu-devel] [PATCH 10/18] block/mirror: Make source the file\n\tchild","submitter":{"id":2714,"url":"http://patchwork.ozlabs.org/api/people/2714/","name":"Kevin Wolf","email":"kwolf@redhat.com"},"content":"Am 13.09.2017 um 20:19 hat Max Reitz geschrieben:\n> Regarding the source BDS, the mirror BDS is arguably a filter node.\n> Therefore, the source BDS should be its \"file\" child.\n> \n> Signed-off-by: Max Reitz \n\nTODO: Justification why this doesn't break things like\nbdrv_is_allocated_above() that iterate through the backing chain.\n\n> block/mirror.c | 127 ++++++++++++++++++++++++++++++++++-----------\n> block/qapi.c | 25 ++++++---\n> tests/qemu-iotests/141.out | 4 +-\n> 3 files changed, 119 insertions(+), 37 deletions(-)\n> \n> diff --git a/block/qapi.c b/block/qapi.c\n> index 7fa2437923..ee792d0cbc 100644\n> --- a/block/qapi.c\n> +++ b/block/qapi.c\n> @@ -147,9 +147,13 @@ BlockDeviceInfo *bdrv_block_device_info(BlockBackend *blk,\n> \n> /* Skip automatically inserted nodes that the user isn't aware of for\n> * query-block (blk != NULL), but not for query-named-block-nodes */\n> - while (blk && bs0->drv && bs0->implicit) {\n> - bs0 = backing_bs(bs0);\n> - assert(bs0);\n> + while (blk && bs0 && bs0->drv && bs0->implicit) {\n> + if (bs0->backing) {\n> + bs0 = backing_bs(bs0);\n> + } else {\n> + assert(bs0->file);\n> + bs0 = bs0->file->bs;\n> + }\n> }\n> }\n\nMaybe backing_bs() should skip filters? If so, need to show that all\nexisting users of backing_bs() really want to skip filters. If not,\nexplain why the missing backing_bs() callers don't need it (I'm quite\nsure that some do).\n\nAlso, if we attack this at the backing_bs() level, we need to audit\ncode that it doesn't directly use bs->backing.\n\n> @@ -1135,44 +1146,88 @@ static const BlockJobDriver commit_active_job_driver = {\n> .drain = mirror_drain,\n> };\n> \n> +static void source_child_inherit_fmt_options(int *child_flags,\n> + QDict *child_options,\n> + int parent_flags,\n> + QDict *parent_options)\n> +{\n> + child_backing.inherit_options(child_flags, child_options,\n> + parent_flags, parent_options);\n> +}\n> +\n> +static char *source_child_get_parent_desc(BdrvChild *c)\n> +{\n> + return child_backing.get_parent_desc(c);\n> +}\n> +\n> +static void source_child_cb_drained_begin(BdrvChild *c)\n> +{\n> + BlockDriverState *bs = c->opaque;\n> + MirrorBDSOpaque *s = bs->opaque;\n> +\n> + if (s && s->job) {\n> + block_job_drained_begin(&s->job->common);\n> + }\n> + bdrv_drained_begin(bs);\n> +}\n> +\n> +static void source_child_cb_drained_end(BdrvChild *c)\n> +{\n> + BlockDriverState *bs = c->opaque;\n> + MirrorBDSOpaque *s = bs->opaque;\n> +\n> + if (s && s->job) {\n> + block_job_drained_end(&s->job->common);\n> + }\n> + bdrv_drained_end(bs);\n> +}\n> +\n> +static BdrvChildRole source_child_role = {\n> + .inherit_options = source_child_inherit_fmt_options,\n> + .get_parent_desc = source_child_get_parent_desc,\n> + .drained_begin = source_child_cb_drained_begin,\n> + .drained_end = source_child_cb_drained_end,\n> +};\n\nWouldn't it make much more sense to use a standard child role and just\nimplement BlockDriver callbacks for .bdrv_drained_begin/end? It seems\nthat master still only has .bdrv_co_drain (which is begin), but one of\nManos' pending series adds the missing end callback.\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-mx10.extmail.prod.ext.phx2.redhat.com;\n\tdmarc=none (p=none dis=none) header.from=redhat.com","ext-mx10.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 3yBC4w5Gzqz9tXl\n\tfor ;\n\tTue, 10 Oct 2017 20:49:36 +1100 (AEDT)","from localhost ([::1]:33806 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1e1rAM-00017X-Sx\n\tfor incoming@patchwork.ozlabs.org; Tue, 10 Oct 2017 05:49:34 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:46566)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1e1r8w-0000ZW-AU\n\tfor qemu-devel@nongnu.org; Tue, 10 Oct 2017 05:48:12 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1e1r8q-0002eA-Di\n\tfor qemu-devel@nongnu.org; Tue, 10 Oct 2017 05:48:06 -0400","from mx1.redhat.com ([209.132.183.28]:33366)\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 1e1r8k-0002Zo-I5; Tue, 10 Oct 2017 05:47:54 -0400","from smtp.corp.redhat.com\n\t(int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13])\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 7A08F72644;\n\tTue, 10 Oct 2017 09:47:53 +0000 (UTC)","from dhcp-200-186.str.redhat.com (dhcp-200-186.str.redhat.com\n\t[10.33.200.186])\n\tby smtp.corp.redhat.com (Postfix) with ESMTPS id 92B4C6061D;\n\tTue, 10 Oct 2017 09:47:39 +0000 (UTC)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mx1.redhat.com 7A08F72644","Date":"Tue, 10 Oct 2017 11:47:38 +0200","From":"Kevin Wolf ","To":"Max Reitz ","Message-ID":"<20171010094738.GF4177@dhcp-200-186.str.redhat.com>","References":"<20170913181910.29688-1-mreitz@redhat.com>\n\t<20170913181910.29688-11-mreitz@redhat.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20170913181910.29688-11-mreitz@redhat.com>","User-Agent":"Mutt/1.9.1 (2017-09-22)","X-Scanned-By":"MIMEDefang 2.79 on 10.5.11.13","X-Greylist":"Sender IP whitelisted, not delayed by milter-greylist-4.5.16\n\t(mx1.redhat.com [10.5.110.39]);\n\tTue, 10 Oct 2017 09:47:53 +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 10/18] block/mirror: Make source the file\n\tchild","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":"Stefan Hajnoczi , John Snow ,\n\tFam Zheng , qemu-devel@nongnu.org, qemu-block@nongnu.org","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}},{"id":1784554,"web_url":"http://patchwork.ozlabs.org/comment/1784554/","msgid":"<3cc56061-d4e4-a07c-cc24-c8162a87fa0b@redhat.com>","list_archive_url":null,"date":"2017-10-11T12:02:56","subject":"Re: [Qemu-devel] [PATCH 10/18] block/mirror: Make source the file\n\tchild","submitter":{"id":36836,"url":"http://patchwork.ozlabs.org/api/people/36836/","name":"Max Reitz","email":"mreitz@redhat.com"},"content":"On 2017-10-10 11:47, Kevin Wolf wrote:\n> Am 13.09.2017 um 20:19 hat Max Reitz geschrieben:\n>> Regarding the source BDS, the mirror BDS is arguably a filter node.\n>> Therefore, the source BDS should be its \"file\" child.\n>>\n>> Signed-off-by: Max Reitz \n> \n> TODO: Justification why this doesn't break things like\n> bdrv_is_allocated_above() that iterate through the backing chain.\n\nEr, well, yes.\n\n>> block/mirror.c | 127 ++++++++++++++++++++++++++++++++++-----------\n>> block/qapi.c | 25 ++++++---\n>> tests/qemu-iotests/141.out | 4 +-\n>> 3 files changed, 119 insertions(+), 37 deletions(-)\n>>\n>> diff --git a/block/qapi.c b/block/qapi.c\n>> index 7fa2437923..ee792d0cbc 100644\n>> --- a/block/qapi.c\n>> +++ b/block/qapi.c\n>> @@ -147,9 +147,13 @@ BlockDeviceInfo *bdrv_block_device_info(BlockBackend *blk,\n>> \n>> /* Skip automatically inserted nodes that the user isn't aware of for\n>> * query-block (blk != NULL), but not for query-named-block-nodes */\n>> - while (blk && bs0->drv && bs0->implicit) {\n>> - bs0 = backing_bs(bs0);\n>> - assert(bs0);\n>> + while (blk && bs0 && bs0->drv && bs0->implicit) {\n>> + if (bs0->backing) {\n>> + bs0 = backing_bs(bs0);\n>> + } else {\n>> + assert(bs0->file);\n>> + bs0 = bs0->file->bs;\n>> + }\n>> }\n>> }\n> \n> Maybe backing_bs() should skip filters? If so, need to show that all\n> existing users of backing_bs() really want to skip filters. If not,\n> explain why the missing backing_bs() callers don't need it (I'm quite\n> sure that some do).\n\nArguably any BDS is a filter BDS regarding its backing BDS. So maybe I\ncould add a new function filter_child_bs().\n\n> Also, if we attack this at the backing_bs() level, we need to audit\n> code that it doesn't directly use bs->backing.\n\nYup.\n\nMaybe the best idea is to leave this patch for a follow-up...\n\n>> @@ -1135,44 +1146,88 @@ static const BlockJobDriver commit_active_job_driver = {\n>> .drain = mirror_drain,\n>> };\n>> \n>> +static void source_child_inherit_fmt_options(int *child_flags,\n>> + QDict *child_options,\n>> + int parent_flags,\n>> + QDict *parent_options)\n>> +{\n>> + child_backing.inherit_options(child_flags, child_options,\n>> + parent_flags, parent_options);\n>> +}\n>> +\n>> +static char *source_child_get_parent_desc(BdrvChild *c)\n>> +{\n>> + return child_backing.get_parent_desc(c);\n>> +}\n>> +\n>> +static void source_child_cb_drained_begin(BdrvChild *c)\n>> +{\n>> + BlockDriverState *bs = c->opaque;\n>> + MirrorBDSOpaque *s = bs->opaque;\n>> +\n>> + if (s && s->job) {\n>> + block_job_drained_begin(&s->job->common);\n>> + }\n>> + bdrv_drained_begin(bs);\n>> +}\n>> +\n>> +static void source_child_cb_drained_end(BdrvChild *c)\n>> +{\n>> + BlockDriverState *bs = c->opaque;\n>> + MirrorBDSOpaque *s = bs->opaque;\n>> +\n>> + if (s && s->job) {\n>> + block_job_drained_end(&s->job->common);\n>> + }\n>> + bdrv_drained_end(bs);\n>> +}\n>> +\n>> +static BdrvChildRole source_child_role = {\n>> + .inherit_options = source_child_inherit_fmt_options,\n>> + .get_parent_desc = source_child_get_parent_desc,\n>> + .drained_begin = source_child_cb_drained_begin,\n>> + .drained_end = source_child_cb_drained_end,\n>> +};\n> \n> Wouldn't it make much more sense to use a standard child role and just\n> implement BlockDriver callbacks for .bdrv_drained_begin/end? It seems\n> that master still only has .bdrv_co_drain (which is begin), but one of\n> Manos' pending series adds the missing end callback.\n\nOK then. :-)\n\nMax","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=mreitz@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 3yBt1W4hBZz9s7m\n\tfor ;\n\tWed, 11 Oct 2017 23:03:58 +1100 (AEDT)","from localhost ([::1]:40719 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1e2Fjq-0007tc-0d\n\tfor incoming@patchwork.ozlabs.org; Wed, 11 Oct 2017 08:03:50 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:34039)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1e2FjM-0007oz-7M\n\tfor qemu-devel@nongnu.org; Wed, 11 Oct 2017 08:03:26 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1e2FjL-0000WM-5k\n\tfor qemu-devel@nongnu.org; Wed, 11 Oct 2017 08:03:20 -0400","from mx1.redhat.com ([209.132.183.28]:28210)\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 1e2FjE-0000M6-JG; Wed, 11 Oct 2017 08:03:12 -0400","from smtp.corp.redhat.com\n\t(int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13])\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 50F61C2D0D24;\n\tWed, 11 Oct 2017 12:03:11 +0000 (UTC)","from dresden.str.redhat.com (unknown [10.40.205.86])\n\tby smtp.corp.redhat.com (Postfix) with ESMTPS id 5C23853C76;\n\tWed, 11 Oct 2017 12:02:58 +0000 (UTC)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mx1.redhat.com 50F61C2D0D24","To":"Kevin Wolf ","References":"<20170913181910.29688-1-mreitz@redhat.com>\n\t<20170913181910.29688-11-mreitz@redhat.com>\n\t<20171010094738.GF4177@dhcp-200-186.str.redhat.com>","From":"Max Reitz ","Message-ID":"<3cc56061-d4e4-a07c-cc24-c8162a87fa0b@redhat.com>","Date":"Wed, 11 Oct 2017 14:02:56 +0200","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":"<20171010094738.GF4177@dhcp-200-186.str.redhat.com>","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\";\n\tboundary=\"nULmsXhCrDhh4KoneO10HLonwF8aSbTBu\"","X-Scanned-By":"MIMEDefang 2.79 on 10.5.11.13","X-Greylist":"Sender IP whitelisted, not delayed by milter-greylist-4.5.16\n\t(mx1.redhat.com [10.5.110.32]);\n\tWed, 11 Oct 2017 12:03:11 +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 10/18] block/mirror: Make source the file\n\tchild","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":"Stefan Hajnoczi , John Snow ,\n\tFam Zheng , qemu-devel@nongnu.org, qemu-block@nongnu.org","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}}]