[{"id":1765354,"web_url":"http://patchwork.ozlabs.org/comment/1765354/","msgid":"<20170908132741.GJ3283@localhost.localdomain>","list_archive_url":null,"date":"2017-09-08T13:27:41","subject":"Re: [Qemu-devel] [PATCH v6 16/18] qcow2: Switch store_bitmap_data()\n\tto byte-based iteration","submitter":{"id":2714,"url":"http://patchwork.ozlabs.org/api/people/2714/","name":"Kevin Wolf","email":"kwolf@redhat.com"},"content":"Am 30.08.2017 um 23:05 hat Eric Blake geschrieben:\n> Now that we have adjusted the majority of the calls this function\n> makes to be byte-based, it is easier to read the code if it makes\n> passes over the image using bytes rather than sectors.\n> \n> Signed-off-by: Eric Blake \n> Reviewed-by: John Snow \n> Reviewed-by: Vladimir Sementsov-Ogievskiy \n> \n> ---\n> v5: no change\n> v4: new patch\n> ---\n> block/qcow2-bitmap.c | 26 +++++++++++---------------\n> 1 file changed, 11 insertions(+), 15 deletions(-)\n> \n> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c\n> index b807298484..63d845e35f 100644\n> --- a/block/qcow2-bitmap.c\n> +++ b/block/qcow2-bitmap.c\n> @@ -1072,10 +1072,9 @@ static uint64_t *store_bitmap_data(BlockDriverState *bs,\n> {\n> int ret;\n> BDRVQcow2State *s = bs->opaque;\n> - int64_t sector;\n> - uint64_t limit, sbc;\n> + int64_t offset;\n> + uint64_t limit;\n> uint64_t bm_size = bdrv_dirty_bitmap_size(bitmap);\n> - uint64_t bm_sectors = DIV_ROUND_UP(bm_size, BDRV_SECTOR_SIZE);\n> const char *bm_name = bdrv_dirty_bitmap_name(bitmap);\n> uint8_t *buf = NULL;\n> BdrvDirtyBitmapIter *dbi;\n> @@ -1100,18 +1099,17 @@ static uint64_t *store_bitmap_data(BlockDriverState *bs,\n> dbi = bdrv_dirty_iter_new(bitmap);\n> buf = g_malloc(s->cluster_size);\n> limit = bytes_covered_by_bitmap_cluster(s, bitmap);\n> - sbc = limit >> BDRV_SECTOR_BITS;\n> assert(DIV_ROUND_UP(bm_size, limit) == tb_size);\n> \n> - while ((sector = bdrv_dirty_iter_next(dbi) >> BDRV_SECTOR_BITS) != -1) {\n> - uint64_t cluster = sector / sbc;\n> + while ((offset = bdrv_dirty_iter_next(dbi)) != -1) {\n\nDon't you have to multiply both sides of the equation? This would be\noffset != -512, which points out that the previous patch to convert\nbdrv_dirty_iter_next() to byte-based gave it a really awkward interface.\n\n> + uint64_t cluster = offset / limit;\n> uint64_t end, write_size;\n> int64_t off;\n> \n> - sector = cluster * sbc;\n> - end = MIN(bm_sectors, sector + sbc);\n> - write_size = bdrv_dirty_bitmap_serialization_size(bitmap,\n> - sector * BDRV_SECTOR_SIZE, (end - sector) * BDRV_SECTOR_SIZE);\n> + offset = cluster * limit;\n\nYou just had cluster = offset / limit, so in other words, align down\noffset? If so, this is how it should be written.\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 3xpdS949LJz9s4s\n\tfor ;\n\tFri, 8 Sep 2017 23:28:22 +1000 (AEST)","from localhost ([::1]:45469 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1dqJKV-0006yu-JF\n\tfor incoming@patchwork.ozlabs.org; Fri, 08 Sep 2017 09:28:19 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:34295)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1dqJK7-0006uI-6X\n\tfor qemu-devel@nongnu.org; Fri, 08 Sep 2017 09:27:56 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1dqJK6-0001kM-8w\n\tfor qemu-devel@nongnu.org; Fri, 08 Sep 2017 09:27:55 -0400","from mx1.redhat.com ([209.132.183.28]:51840)\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 1dqJJz-0001fF-Pk; Fri, 08 Sep 2017 09:27:47 -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 C65AB5F795;\n\tFri, 8 Sep 2017 13:27:46 +0000 (UTC)","from localhost.localdomain (ovpn-116-113.ams2.redhat.com\n\t[10.36.116.113])\n\tby smtp.corp.redhat.com (Postfix) with ESMTPS id 2BE1D60F89;\n\tFri, 8 Sep 2017 13:27:43 +0000 (UTC)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mx1.redhat.com C65AB5F795","Date":"Fri, 8 Sep 2017 15:27:41 +0200","From":"Kevin Wolf ","To":"Eric Blake ","Message-ID":"<20170908132741.GJ3283@localhost.localdomain>","References":"<20170830210542.2153-1-eblake@redhat.com>\n\t<20170830210542.2153-17-eblake@redhat.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20170830210542.2153-17-eblake@redhat.com>","User-Agent":"Mutt/1.8.3 (2017-05-23)","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.39]);\n\tFri, 08 Sep 2017 13:27:46 +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 v6 16/18] qcow2: Switch store_bitmap_data()\n\tto byte-based iteration","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":"vsementsov@virtuozzo.com, jsnow@redhat.com, qemu-devel@nongnu.org,\n\tqemu-block@nongnu.org, Max Reitz ","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}},{"id":1765401,"web_url":"http://patchwork.ozlabs.org/comment/1765401/","msgid":"<3e1edd8f-dcfa-561e-a5e1-99ef89b1ec81@redhat.com>","list_archive_url":null,"date":"2017-09-08T14:09:24","subject":"Re: [Qemu-devel] [PATCH v6 16/18] qcow2: Switch store_bitmap_data()\n\tto byte-based iteration","submitter":{"id":6591,"url":"http://patchwork.ozlabs.org/api/people/6591/","name":"Eric Blake","email":"eblake@redhat.com"},"content":"On 09/08/2017 08:27 AM, Kevin Wolf wrote:\n> Am 30.08.2017 um 23:05 hat Eric Blake geschrieben:\n>> Now that we have adjusted the majority of the calls this function\n>> makes to be byte-based, it is easier to read the code if it makes\n>> passes over the image using bytes rather than sectors.\n>>\n>> Signed-off-by: Eric Blake \n>> Reviewed-by: John Snow \n>> Reviewed-by: Vladimir Sementsov-Ogievskiy \n>>\n\n>>\n>> - while ((sector = bdrv_dirty_iter_next(dbi) >> BDRV_SECTOR_BITS) != -1) {\n>> - uint64_t cluster = sector / sbc;\n>> + while ((offset = bdrv_dirty_iter_next(dbi)) != -1) {\n> \n> Don't you have to multiply both sides of the equation? This would be\n> offset != -512, which points out that the previous patch to convert\n> bdrv_dirty_iter_next() to byte-based gave it a really awkward interface.\n\nI think what I really need to do is change '!= -1' to '< 0', as that's\nmuch easier to reason about when scaling is present.\n\n> \n>> + uint64_t cluster = offset / limit;\n>> uint64_t end, write_size;\n>> int64_t off;\n>>\n>> - sector = cluster * sbc;\n>> - end = MIN(bm_sectors, sector + sbc);\n>> - write_size = bdrv_dirty_bitmap_serialization_size(bitmap,\n>> - sector * BDRV_SECTOR_SIZE, (end - sector) * BDRV_SECTOR_SIZE);\n>> + offset = cluster * limit;\n> \n> You just had cluster = offset / limit, so in other words, align down\n> offset? If so, this is how it should be written.\n\nThanks for the close reviews; looks like I have enough things to do a\nrespin.","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-mx05.extmail.prod.ext.phx2.redhat.com;\n\tdmarc=none (p=none dis=none) header.from=redhat.com","ext-mx05.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 3xpfNV3tJYz9s76\n\tfor ;\n\tSat, 9 Sep 2017 00:10:17 +1000 (AEST)","from localhost ([::1]:45629 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1dqJz5-0001C2-5o\n\tfor incoming@patchwork.ozlabs.org; Fri, 08 Sep 2017 10:10:15 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:46417)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1dqJyZ-0001AG-Nl\n\tfor qemu-devel@nongnu.org; Fri, 08 Sep 2017 10:09:47 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1dqJyV-00027o-8G\n\tfor qemu-devel@nongnu.org; Fri, 08 Sep 2017 10:09:43 -0400","from mx1.redhat.com ([209.132.183.28]:40328)\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 1dqJyM-00020Q-OX; Fri, 08 Sep 2017 10:09:30 -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 C855E634;\n\tFri, 8 Sep 2017 14:09:29 +0000 (UTC)","from [10.10.120.228] (ovpn-120-228.rdu2.redhat.com [10.10.120.228])\n\tby smtp.corp.redhat.com (Postfix) with ESMTP id CA6CF60C14;\n\tFri, 8 Sep 2017 14:09:25 +0000 (UTC)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mx1.redhat.com C855E634","To":"Kevin Wolf ","References":"<20170830210542.2153-1-eblake@redhat.com>\n\t<20170830210542.2153-17-eblake@redhat.com>\n\t<20170908132741.GJ3283@localhost.localdomain>","From":"Eric Blake ","Openpgp":"url=http://people.redhat.com/eblake/eblake.gpg","Organization":"Red Hat, Inc.","Message-ID":"<3e1edd8f-dcfa-561e-a5e1-99ef89b1ec81@redhat.com>","Date":"Fri, 8 Sep 2017 09:09:24 -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":"<20170908132741.GJ3283@localhost.localdomain>","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\";\n\tboundary=\"kKmBgP08dlBwqnI2oXeQTNGtfFdameNjB\"","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.29]);\n\tFri, 08 Sep 2017 14:09:29 +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 v6 16/18] qcow2: Switch store_bitmap_data()\n\tto byte-based iteration","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":"vsementsov@virtuozzo.com, jsnow@redhat.com, qemu-devel@nongnu.org,\n\tqemu-block@nongnu.org, Max Reitz ","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}},{"id":1765406,"web_url":"http://patchwork.ozlabs.org/comment/1765406/","msgid":"<20170908141629.GK3283@localhost.localdomain>","list_archive_url":null,"date":"2017-09-08T14:16:29","subject":"Re: [Qemu-devel] [PATCH v6 16/18] qcow2: Switch store_bitmap_data()\n\tto byte-based iteration","submitter":{"id":2714,"url":"http://patchwork.ozlabs.org/api/people/2714/","name":"Kevin Wolf","email":"kwolf@redhat.com"},"content":"Am 08.09.2017 um 16:09 hat Eric Blake geschrieben:\n> On 09/08/2017 08:27 AM, Kevin Wolf wrote:\n> > Am 30.08.2017 um 23:05 hat Eric Blake geschrieben:\n> >> Now that we have adjusted the majority of the calls this function\n> >> makes to be byte-based, it is easier to read the code if it makes\n> >> passes over the image using bytes rather than sectors.\n> >>\n> >> Signed-off-by: Eric Blake \n> >> Reviewed-by: John Snow \n> >> Reviewed-by: Vladimir Sementsov-Ogievskiy \n> >>\n> \n> >>\n> >> - while ((sector = bdrv_dirty_iter_next(dbi) >> BDRV_SECTOR_BITS) != -1) {\n> >> - uint64_t cluster = sector / sbc;\n> >> + while ((offset = bdrv_dirty_iter_next(dbi)) != -1) {\n> > \n> > Don't you have to multiply both sides of the equation? This would be\n> > offset != -512, which points out that the previous patch to convert\n> > bdrv_dirty_iter_next() to byte-based gave it a really awkward interface.\n> \n> I think what I really need to do is change '!= -1' to '< 0', as that's\n> much easier to reason about when scaling is present.\n\nHm, I think I would prefer a special case for -1 in bdrv_dirty_iter_next()\nso that it returns -1 after the last entry. Even if you check for < 0,\n-512 is still an odd return value to signal the end.\n\nThough I think after the final patch, we're back to -1 anyway, so it's\nnot that important.\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-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=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 3xpfXn2wByz9s7G\n\tfor ;\n\tSat, 9 Sep 2017 00:17:28 +1000 (AEST)","from localhost ([::1]:45662 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1dqK61-0004zV-SZ\n\tfor incoming@patchwork.ozlabs.org; Fri, 08 Sep 2017 10:17:25 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:48442)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1dqK5X-0004ya-6K\n\tfor qemu-devel@nongnu.org; Fri, 08 Sep 2017 10:16:56 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1dqK5T-00026E-5i\n\tfor qemu-devel@nongnu.org; Fri, 08 Sep 2017 10:16:55 -0400","from mx1.redhat.com ([209.132.183.28]:37724)\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 1dqK5J-0001iq-4j; Fri, 08 Sep 2017 10:16:41 -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 A5ABB80C20;\n\tFri, 8 Sep 2017 14:16:39 +0000 (UTC)","from localhost.localdomain (ovpn-116-113.ams2.redhat.com\n\t[10.36.116.113])\n\tby smtp.corp.redhat.com (Postfix) with ESMTPS id 6586D619DB;\n\tFri, 8 Sep 2017 14:16:30 +0000 (UTC)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mx1.redhat.com A5ABB80C20","Date":"Fri, 8 Sep 2017 16:16:29 +0200","From":"Kevin Wolf ","To":"Eric Blake ","Message-ID":"<20170908141629.GK3283@localhost.localdomain>","References":"<20170830210542.2153-1-eblake@redhat.com>\n\t<20170830210542.2153-17-eblake@redhat.com>\n\t<20170908132741.GJ3283@localhost.localdomain>\n\t<3e1edd8f-dcfa-561e-a5e1-99ef89b1ec81@redhat.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha1;\n\tprotocol=\"application/pgp-signature\"; boundary=\"mP3DRpeJDSE+ciuQ\"","Content-Disposition":"inline","In-Reply-To":"<3e1edd8f-dcfa-561e-a5e1-99ef89b1ec81@redhat.com>","User-Agent":"Mutt/1.8.3 (2017-05-23)","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.26]);\n\tFri, 08 Sep 2017 14:16:39 +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 v6 16/18] qcow2: Switch store_bitmap_data()\n\tto byte-based iteration","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":"vsementsov@virtuozzo.com, jsnow@redhat.com, qemu-devel@nongnu.org,\n\tqemu-block@nongnu.org, Max Reitz ","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}}]