[{"id":1775769,"web_url":"http://patchwork.ozlabs.org/comment/1775769/","msgid":"","list_archive_url":null,"date":"2017-09-26T18:31:17","subject":"Re: [Qemu-devel] [PATCH v4 02/23] block: Add flag to avoid wasted\n\twork in bdrv_is_allocated()","submitter":{"id":64343,"url":"http://patchwork.ozlabs.org/api/people/64343/","name":"John Snow","email":"jsnow@redhat.com"},"content":"On 09/13/2017 12:03 PM, Eric Blake wrote:\n> Not all callers care about which BDS owns the mapping for a given\n> range of the file. In particular, bdrv_is_allocated() cares more\n> about finding the largest run of allocated data from the guest\n> perspective, whether or not that data is consecutive from the\n> host perspective. Therefore, doing subsequent refinements such\n> as checking how much of the format-layer allocation also satisfies\n> BDRV_BLOCK_ZERO at the protocol layer is wasted work - in the best\n> case, it just costs extra CPU cycles during a single\n> bdrv_is_allocated(), but in the worst case, it results in a smaller\n> *pnum, and forces callers to iterate through more status probes when\n> visiting the entire file for even more extra CPU cycles.\n> \n> This patch only optimizes the block layer. But subsequent patches\n> will tweak the driver callback to be byte-based, and in the process,\n> can also pass this hint through to the driver.\n> \n> Signed-off-by: Eric Blake \n> \n> ---\n> v4: only context changes\n> v3: s/allocation/mapping/ and flip sense of bool\n> v2: new patch\n> ---\n> block/io.c | 52 ++++++++++++++++++++++++++++++++++++++--------------\n> 1 file changed, 38 insertions(+), 14 deletions(-)\n> \n> diff --git a/block/io.c b/block/io.c\n> index f250029395..6509c804d4 100644\n> --- a/block/io.c\n> +++ b/block/io.c\n> @@ -1709,6 +1709,7 @@ typedef struct BdrvCoGetBlockStatusData {\n> int nb_sectors;\n> int *pnum;\n> int64_t ret;\n> + bool mapping;\n> bool done;\n> } BdrvCoGetBlockStatusData;\n> \n> @@ -1743,6 +1744,11 @@ int64_t coroutine_fn bdrv_co_get_block_status_from_backing(BlockDriverState *bs,\n> * Drivers not implementing the functionality are assumed to not support\n> * backing files, hence all their sectors are reported as allocated.\n> *\n> + * If 'mapping' is true, the caller is querying for mapping purposes,\n> + * and the result should include BDRV_BLOCK_OFFSET_VALID where\n> + * possible; otherwise, the result may omit that bit particularly if\n> + * it allows for a larger value in 'pnum'.\n> + *\n> * If 'sector_num' is beyond the end of the disk image the return value is\n> * BDRV_BLOCK_EOF and 'pnum' is set to 0.\n> *\n> @@ -1759,6 +1765,7 @@ int64_t coroutine_fn bdrv_co_get_block_status_from_backing(BlockDriverState *bs,\n> * is allocated in.\n> */\n> static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,\n> + bool mapping,\n> int64_t sector_num,\n> int nb_sectors, int *pnum,\n> BlockDriverState **file)\n> @@ -1817,14 +1824,15 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,\n> \n> if (ret & BDRV_BLOCK_RAW) {\n> assert(ret & BDRV_BLOCK_OFFSET_VALID && local_file);\n> - ret = bdrv_co_get_block_status(local_file, ret >> BDRV_SECTOR_BITS,\n> + ret = bdrv_co_get_block_status(local_file, mapping,\n> + ret >> BDRV_SECTOR_BITS,\n> *pnum, pnum, &local_file);\n> goto out;\n> }\n> \n> if (ret & (BDRV_BLOCK_DATA | BDRV_BLOCK_ZERO)) {\n> ret |= BDRV_BLOCK_ALLOCATED;\n> - } else {\n> + } else if (mapping) {\n> if (bdrv_unallocated_blocks_are_zero(bs)) {\n> ret |= BDRV_BLOCK_ZERO;\n> } else if (bs->backing) {\n> @@ -1836,12 +1844,13 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,\n> }\n> }\n> \n> - if (local_file && local_file != bs &&\n> + if (mapping && local_file && local_file != bs &&\n\nTentatively this looks OK to me, but I have to admit I'm a little shaky\non this portion because I've not really investigated this function too\nmuch. I am at the very least convinced that when mapping is true that\nthe function is equivalent and that existing callers don't have their\nbehavior changed too much.\n\nBenefit of the doubt:\n\nReviewed-by: John Snow \n\n> (ret & BDRV_BLOCK_DATA) && !(ret & BDRV_BLOCK_ZERO) &&\n> (ret & BDRV_BLOCK_OFFSET_VALID)) {\n> int file_pnum;\n> \n> - ret2 = bdrv_co_get_block_status(local_file, ret >> BDRV_SECTOR_BITS,\n> + ret2 = bdrv_co_get_block_status(local_file, mapping,\n> + ret >> BDRV_SECTOR_BITS,\n> *pnum, &file_pnum, NULL);\n> if (ret2 >= 0) {\n> /* Ignore errors. This is just providing extra information, it\n> @@ -1876,6 +1885,7 @@ out:\n> \n> static int64_t coroutine_fn bdrv_co_get_block_status_above(BlockDriverState *bs,\n> BlockDriverState *base,\n> + bool mapping,\n> int64_t sector_num,\n> int nb_sectors,\n> int *pnum,\n> @@ -1887,7 +1897,8 @@ static int64_t coroutine_fn bdrv_co_get_block_status_above(BlockDriverState *bs,\n> \n> assert(bs != base);\n> for (p = bs; p != base; p = backing_bs(p)) {\n> - ret = bdrv_co_get_block_status(p, sector_num, nb_sectors, pnum, file);\n> + ret = bdrv_co_get_block_status(p, mapping, sector_num, nb_sectors,\n> + pnum, file);\n> if (ret < 0) {\n> break;\n> }\n> @@ -1917,6 +1928,7 @@ static void coroutine_fn bdrv_get_block_status_above_co_entry(void *opaque)\n> BdrvCoGetBlockStatusData *data = opaque;\n> \n> data->ret = bdrv_co_get_block_status_above(data->bs, data->base,\n> + data->mapping,\n> data->sector_num,\n> data->nb_sectors,\n> data->pnum,\n> @@ -1929,11 +1941,12 @@ static void coroutine_fn bdrv_get_block_status_above_co_entry(void *opaque)\n> *\n> * See bdrv_co_get_block_status_above() for details.\n> */\n> -int64_t bdrv_get_block_status_above(BlockDriverState *bs,\n> - BlockDriverState *base,\n> - int64_t sector_num,\n> - int nb_sectors, int *pnum,\n> - BlockDriverState **file)\n> +static int64_t bdrv_common_block_status_above(BlockDriverState *bs,\n> + BlockDriverState *base,\n> + bool mapping,\n> + int64_t sector_num,\n> + int nb_sectors, int *pnum,\n> + BlockDriverState **file)\n> {\n> Coroutine *co;\n> BdrvCoGetBlockStatusData data = {\n> @@ -1943,6 +1956,7 @@ int64_t bdrv_get_block_status_above(BlockDriverState *bs,\n> .sector_num = sector_num,\n> .nb_sectors = nb_sectors,\n> .pnum = pnum,\n> + .mapping = mapping,\n> .done = false,\n> };\n> \n> @@ -1958,6 +1972,16 @@ int64_t bdrv_get_block_status_above(BlockDriverState *bs,\n> return data.ret;\n> }\n> \n> +int64_t bdrv_get_block_status_above(BlockDriverState *bs,\n> + BlockDriverState *base,\n> + int64_t sector_num,\n> + int nb_sectors, int *pnum,\n> + BlockDriverState **file)\n> +{\n> + return bdrv_common_block_status_above(bs, base, true, sector_num,\n> + nb_sectors, pnum, file);\n> +}\n> +\n> int64_t bdrv_get_block_status(BlockDriverState *bs,\n> int64_t sector_num,\n> int nb_sectors, int *pnum,\n> @@ -1970,15 +1994,15 @@ int64_t bdrv_get_block_status(BlockDriverState *bs,\n> int coroutine_fn bdrv_is_allocated(BlockDriverState *bs, int64_t offset,\n> int64_t bytes, int64_t *pnum)\n> {\n> - int64_t sector_num = offset >> BDRV_SECTOR_BITS;\n> - int nb_sectors = bytes >> BDRV_SECTOR_BITS;\n> int64_t ret;\n> int psectors;\n> \n> assert(QEMU_IS_ALIGNED(offset, BDRV_SECTOR_SIZE));\n> assert(QEMU_IS_ALIGNED(bytes, BDRV_SECTOR_SIZE) && bytes < INT_MAX);\n> - ret = bdrv_get_block_status(bs, sector_num, nb_sectors, &psectors,\n> - NULL);\n> + ret = bdrv_common_block_status_above(bs, backing_bs(bs), false,\n> + offset >> BDRV_SECTOR_BITS,\n> + bytes >> BDRV_SECTOR_BITS, &psectors,\n> + NULL);\n> if (ret < 0) {\n> return ret;\n> }\n>","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-mx01.extmail.prod.ext.phx2.redhat.com;\n\tdmarc=none (p=none dis=none) header.from=redhat.com","ext-mx01.extmail.prod.ext.phx2.redhat.com;\n\tspf=fail smtp.mailfrom=jsnow@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 3y1qL13DyCz9t3m\n\tfor ;\n\tWed, 27 Sep 2017 04:31:53 +1000 (AEST)","from localhost ([::1]:50656 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1dwue7-0003C1-Ih\n\tfor incoming@patchwork.ozlabs.org; Tue, 26 Sep 2017 14:31:51 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:44424)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1dwudm-0003AR-06\n\tfor qemu-devel@nongnu.org; Tue, 26 Sep 2017 14:31:31 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1dwudk-0002v3-Kv\n\tfor qemu-devel@nongnu.org; Tue, 26 Sep 2017 14:31:29 -0400","from mx1.redhat.com ([209.132.183.28]:38060)\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 1dwudf-0002mx-Es; Tue, 26 Sep 2017 14:31:23 -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 8C05281DF7;\n\tTue, 26 Sep 2017 18:31:22 +0000 (UTC)","from [10.18.17.130] (dhcp-17-130.bos.redhat.com [10.18.17.130])\n\tby smtp.corp.redhat.com (Postfix) with ESMTP id 7DF2B4139;\n\tTue, 26 Sep 2017 18:31:17 +0000 (UTC)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mx1.redhat.com 8C05281DF7","To":"Eric Blake , qemu-devel@nongnu.org","References":"<20170913160333.23622-1-eblake@redhat.com>\n\t<20170913160333.23622-3-eblake@redhat.com>","From":"John Snow ","Message-ID":"","Date":"Tue, 26 Sep 2017 14:31:17 -0400","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":"<20170913160333.23622-3-eblake@redhat.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-US","Content-Transfer-Encoding":"7bit","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.25]);\n\tTue, 26 Sep 2017 18:31:22 +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 v4 02/23] block: Add flag to avoid wasted\n\twork in bdrv_is_allocated()","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, famz@redhat.com, Stefan Hajnoczi ,\n\tqemu-block@nongnu.org, Max Reitz ","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}},{"id":1777083,"web_url":"http://patchwork.ozlabs.org/comment/1777083/","msgid":"","list_archive_url":null,"date":"2017-09-28T14:58:37","subject":"Re: [Qemu-devel] [PATCH v4 02/23] block: Add flag to avoid wasted\n\twork in bdrv_is_allocated()","submitter":{"id":6591,"url":"http://patchwork.ozlabs.org/api/people/6591/","name":"Eric Blake","email":"eblake@redhat.com"},"content":"On 09/26/2017 01:31 PM, John Snow wrote:\n> \n> \n> On 09/13/2017 12:03 PM, Eric Blake wrote:\n>> Not all callers care about which BDS owns the mapping for a given\n>> range of the file. In particular, bdrv_is_allocated() cares more\n>> about finding the largest run of allocated data from the guest\n>> perspective, whether or not that data is consecutive from the\n>> host perspective. Therefore, doing subsequent refinements such\n>> as checking how much of the format-layer allocation also satisfies\n>> BDRV_BLOCK_ZERO at the protocol layer is wasted work - in the best\n>> case, it just costs extra CPU cycles during a single\n>> bdrv_is_allocated(), but in the worst case, it results in a smaller\n>> *pnum, and forces callers to iterate through more status probes when\n>> visiting the entire file for even more extra CPU cycles.\n>>\n>> This patch only optimizes the block layer. But subsequent patches\n>> will tweak the driver callback to be byte-based, and in the process,\n>> can also pass this hint through to the driver.\n>>\n>> Signed-off-by: Eric Blake \n>>\n\n>> *\n>> + * If 'mapping' is true, the caller is querying for mapping purposes,\n>> + * and the result should include BDRV_BLOCK_OFFSET_VALID where\n>> + * possible; otherwise, the result may omit that bit particularly if\n>> + * it allows for a larger value in 'pnum'.\n\nI decided one more tweak to the comment will help:\n\n+ * If 'mapping' is true, the caller is querying for mapping purposes,\n+ * and the result should include BDRV_BLOCK_OFFSET_VALID and\n+ * BDRV_BLOCK_ZERO where possible; otherwise, the result may omit those\n+ * bits particularly if it allows for a larger value in 'pnum'.\n\n\n>> @@ -1836,12 +1844,13 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,\n>> }\n>> }\n>>\n>> - if (local_file && local_file != bs &&\n>> + if (mapping && local_file && local_file != bs &&\n> \n> Tentatively this looks OK to me, but I have to admit I'm a little shaky\n> on this portion because I've not really investigated this function too\n> much. I am at the very least convinced that when mapping is true that\n> the function is equivalent and that existing callers don't have their\n> behavior changed too much.\n> \n> Benefit of the doubt:\n> \n> Reviewed-by: John Snow \n\nThen I'll tentatively keep your R-b even with the comment tweak, unless\nyou say otherwise :)","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-mx02.extmail.prod.ext.phx2.redhat.com;\n\tdmarc=none (p=none dis=none) header.from=redhat.com","ext-mx02.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 3y2yWy6fPDz9t48\n\tfor ;\n\tFri, 29 Sep 2017 00:59:25 +1000 (AEST)","from localhost ([::1]:59673 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1dxaHZ-0001qu-PE\n\tfor incoming@patchwork.ozlabs.org; Thu, 28 Sep 2017 10:59:21 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:57809)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1dxaHB-0001pk-QI\n\tfor qemu-devel@nongnu.org; Thu, 28 Sep 2017 10:59:01 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1dxaH7-0008KL-RF\n\tfor qemu-devel@nongnu.org; Thu, 28 Sep 2017 10:58:57 -0400","from mx1.redhat.com ([209.132.183.28]:48328)\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 1dxaH2-0008HY-4Q; Thu, 28 Sep 2017 10:58:48 -0400","from smtp.corp.redhat.com\n\t(int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14])\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 0CBA4935D6;\n\tThu, 28 Sep 2017 14:58:47 +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 D72936BEF3;\n\tThu, 28 Sep 2017 14:58:38 +0000 (UTC)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mx1.redhat.com 0CBA4935D6","To":"John Snow , qemu-devel@nongnu.org","References":"<20170913160333.23622-1-eblake@redhat.com>\n\t<20170913160333.23622-3-eblake@redhat.com>\n\t","From":"Eric Blake ","Openpgp":"url=http://people.redhat.com/eblake/eblake.gpg","Organization":"Red Hat, Inc.","Message-ID":"","Date":"Thu, 28 Sep 2017 09:58:37 -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":"","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\";\n\tboundary=\"D2olUSFlmPnm62DQB3awdaoX20SK8JujV\"","X-Scanned-By":"MIMEDefang 2.79 on 10.5.11.14","X-Greylist":"Sender IP whitelisted, not delayed by milter-greylist-4.5.16\n\t(mx1.redhat.com [10.5.110.26]);\n\tThu, 28 Sep 2017 14:58:47 +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 v4 02/23] block: Add flag to avoid wasted\n\twork in bdrv_is_allocated()","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, famz@redhat.com, Stefan Hajnoczi ,\n\tqemu-block@nongnu.org, Max Reitz ","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}}]