[{"id":1774806,"web_url":"http://patchwork.ozlabs.org/comment/1774806/","msgid":"","list_archive_url":null,"date":"2017-09-25T15:22:09","subject":"Re: [Qemu-devel] [PATCH v10 18/20] qcow2: Switch\n\tstore_bitmap_data() to byte-based iteration","submitter":{"id":66592,"url":"http://patchwork.ozlabs.org/api/people/66592/","name":"Vladimir Sementsov-Ogievskiy","email":"vsementsov@virtuozzo.com"},"content":"25.09.2017 17:55, Eric Blake wrote:\n> Reviewed-by: Vladimir Sementsov-Ogievskiy\n\nstupid thunderbird bug always stole my whitespace =(","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=)","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 3y17Kx4vP9z9tX8\n\tfor ;\n\tTue, 26 Sep 2017 01:29:25 +1000 (AEST)","from localhost ([::1]:43046 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1dwVJz-0008Lk-QL\n\tfor incoming@patchwork.ozlabs.org; Mon, 25 Sep 2017 11:29:23 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:36897)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1dwVD8-0003OL-U6\n\tfor qemu-devel@nongnu.org; Mon, 25 Sep 2017 11:22:19 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1dwVD5-0006WG-Oy\n\tfor qemu-devel@nongnu.org; Mon, 25 Sep 2017 11:22:18 -0400","from mailhub.sw.ru ([195.214.232.25]:2458 helo=relay.sw.ru)\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 1dwVD5-0006VV-Cy\n\tfor qemu-devel@nongnu.org; Mon, 25 Sep 2017 11:22:15 -0400","from [172.16.24.243] (msk-vpn.virtuozzo.com [195.214.232.6])\n\tby relay.sw.ru (8.13.4/8.13.4) with ESMTP id v8PFM9RM009429;\n\tMon, 25 Sep 2017 18:22:09 +0300 (MSK)"],"To":"Eric Blake , qemu-devel@nongnu.org","References":"<20170925145526.32690-1-eblake@redhat.com>\n\t<20170925145526.32690-19-eblake@redhat.com>","From":"Vladimir Sementsov-Ogievskiy ","Message-ID":"","Date":"Mon, 25 Sep 2017 18:22:09 +0300","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":"<20170925145526.32690-19-eblake@redhat.com>","Content-Language":"en-US","X-detected-operating-system":"by eggs.gnu.org: OpenBSD 3.x [fuzzy]","X-Received-From":"195.214.232.25","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"7bit","X-Content-Filtered-By":"Mailman/MimeDel 2.1.21","Subject":"Re: [Qemu-devel] [PATCH v10 18/20] qcow2: Switch\n\tstore_bitmap_data() to 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":"kwolf@redhat.com, famz@redhat.com, jsnow@redhat.com,\n\tqemu-block@nongnu.org, Max Reitz ","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}},{"id":1774980,"web_url":"http://patchwork.ozlabs.org/comment/1774980/","msgid":"","list_archive_url":null,"date":"2017-09-25T21:00:12","subject":"Re: [Qemu-devel] [PATCH v10 18/20] qcow2: Switch\n\tstore_bitmap_data() to byte-based iteration","submitter":{"id":64343,"url":"http://patchwork.ozlabs.org/api/people/64343/","name":"John Snow","email":"jsnow@redhat.com"},"content":"On 09/25/2017 10:55 AM, Eric Blake wrote:\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> iotests 165 was rather weak - on a default 64k-cluster image, where\n> bitmap granularity also defaults to 64k bytes, a single cluster of\n> the bitmap table thus covers (64*1024*8) bits which each cover 64k\n> bytes, or 32G of image space. But the test only uses a 1G image,\n> so it cannot trigger any more than one loop of the code in\n> store_bitmap_data(); and it was writing to the first cluster. In\n> order to test that we are properly aligning which portions of the\n> bitmap are being written to the file, we really want to test a case\n> where the first dirty bit returned by bdrv_dirty_iter_next() is not\n> aligned to the start of a cluster, which we can do by modifying the\n> test to write data that doesn't happen to fall in the first cluster\n> of the image.\n> \n> Signed-off-by: Eric Blake \n> Reviewed-by: Vladimir Sementsov-Ogievskiy\n> \n> ---\n> v10: no change, add R-b\n> v9: update iotests to show why aligning down is needed [Kevin], R-b dropped\n> v8: no change\n> v7: rebase to earlier change, make rounding of offset obvious (no semantic\n> change, so R-b kept) [Kevin]\n> v5-v6: no change\n> v4: new patch\n> ---\n> block/qcow2-bitmap.c | 31 ++++++++++++++++---------------\n> tests/qemu-iotests/165 | 2 +-\n> 2 files changed, 17 insertions(+), 16 deletions(-)\n> \n> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c\n> index 692ce0de88..df957c66d5 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,22 @@ 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) >= 0) {\n> - uint64_t cluster = sector / sbc;\n> + while ((offset = bdrv_dirty_iter_next(dbi)) >= 0) {\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> + /*\n> + * We found the first dirty offset, but want to write out the\n> + * entire cluster of the bitmap that includes that offset,\n> + * including any leading zero bits.\n> + */\n> + offset = QEMU_ALIGN_DOWN(offset, limit);\n> + end = MIN(bm_size, offset + limit);\n> + write_size = bdrv_dirty_bitmap_serialization_size(bitmap, offset,\n> + end - offset);\n> assert(write_size <= s->cluster_size);\n> \n> off = qcow2_alloc_clusters(bs, s->cluster_size);\n> @@ -1123,9 +1126,7 @@ static uint64_t *store_bitmap_data(BlockDriverState *bs,\n> }\n> tb[cluster] = off;\n> \n> - bdrv_dirty_bitmap_serialize_part(bitmap, buf,\n> - sector * BDRV_SECTOR_SIZE,\n> - (end - sector) * BDRV_SECTOR_SIZE);\n> + bdrv_dirty_bitmap_serialize_part(bitmap, buf, offset, end - offset);\n> if (write_size < s->cluster_size) {\n> memset(buf + write_size, 0, s->cluster_size - write_size);\n> }\n> @@ -1143,11 +1144,11 @@ static uint64_t *store_bitmap_data(BlockDriverState *bs,\n> goto fail;\n> }\n> \n> - if (end >= bm_sectors) {\n> + if (end >= bm_size) {\n> break;\n> }\n> \n> - bdrv_set_dirty_iter(dbi, end * BDRV_SECTOR_SIZE);\n> + bdrv_set_dirty_iter(dbi, end);\n> }\n> \n> *bitmap_table_size = tb_size;\n> diff --git a/tests/qemu-iotests/165 b/tests/qemu-iotests/165\n> index 74d7b79a0b..a3932db3de 100755\n> --- a/tests/qemu-iotests/165\n> +++ b/tests/qemu-iotests/165\n> @@ -27,7 +27,7 @@ disk = os.path.join(iotests.test_dir, 'disk')\n> disk_size = 0x40000000 # 1G\n> \n> # regions for qemu_io: (start, count) in bytes\n> -regions1 = ((0, 0x100000),\n> +regions1 = ((0x0fff00, 0x10000),\n> (0x200000, 0x100000))\n> \n> regions2 = ((0x10000000, 0x20000),\n> \n\nReviewed-by: John Snow \n\nJust need to figure out who stages it. Either Fam, Kevin or myself. I'll\njust assume Kevin has higher precedence here with his qcow2 maintainership.\n\nIf otherwise, let me know and I'll do it.\n\n--js","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=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 3y1GjW3lbyz9t30\n\tfor ;\n\tTue, 26 Sep 2017 07:01:51 +1000 (AEST)","from localhost ([::1]:44269 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1dwaVh-0008BD-Lt\n\tfor incoming@patchwork.ozlabs.org; Mon, 25 Sep 2017 17:01:49 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:37109)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1dwaUN-0007fP-6L\n\tfor qemu-devel@nongnu.org; Mon, 25 Sep 2017 17:00:30 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1dwaUJ-00071f-2v\n\tfor qemu-devel@nongnu.org; Mon, 25 Sep 2017 17:00:27 -0400","from mx1.redhat.com ([209.132.183.28]:46290)\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 1dwaUE-0006zx-28; Mon, 25 Sep 2017 17:00:18 -0400","from smtp.corp.redhat.com\n\t(int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11])\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 D7E867E43B;\n\tMon, 25 Sep 2017 21:00:16 +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 406ED6047C;\n\tMon, 25 Sep 2017 21:00:13 +0000 (UTC)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mx1.redhat.com D7E867E43B","To":"Eric Blake , qemu-devel@nongnu.org","References":"<20170925145526.32690-1-eblake@redhat.com>\n\t<20170925145526.32690-19-eblake@redhat.com>","From":"John Snow ","Message-ID":"","Date":"Mon, 25 Sep 2017 17:00:12 -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":"<20170925145526.32690-19-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.11","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 21:00:17 +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 v10 18/20] qcow2: Switch\n\tstore_bitmap_data() to 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":"kwolf@redhat.com, vsementsov@virtuozzo.com, famz@redhat.com,\n\tqemu-block@nongnu.org, Max Reitz ","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}},{"id":1775080,"web_url":"http://patchwork.ozlabs.org/comment/1775080/","msgid":"<20170926013014.GB31924@lemon.lan>","list_archive_url":null,"date":"2017-09-26T01:30:14","subject":"Re: [Qemu-devel] [PATCH v10 18/20] qcow2: Switch\n\tstore_bitmap_data() to byte-based iteration","submitter":{"id":24872,"url":"http://patchwork.ozlabs.org/api/people/24872/","name":"Fam Zheng","email":"famz@redhat.com"},"content":"On Mon, 09/25 09:55, Eric Blake wrote:\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> iotests 165 was rather weak - on a default 64k-cluster image, where\n> bitmap granularity also defaults to 64k bytes, a single cluster of\n> the bitmap table thus covers (64*1024*8) bits which each cover 64k\n> bytes, or 32G of image space. But the test only uses a 1G image,\n> so it cannot trigger any more than one loop of the code in\n> store_bitmap_data(); and it was writing to the first cluster. In\n> order to test that we are properly aligning which portions of the\n> bitmap are being written to the file, we really want to test a case\n> where the first dirty bit returned by bdrv_dirty_iter_next() is not\n> aligned to the start of a cluster, which we can do by modifying the\n> test to write data that doesn't happen to fall in the first cluster\n> of the image.\n> \n> Signed-off-by: Eric Blake \n> Reviewed-by: Vladimir Sementsov-Ogievskiy\n\nReviewed-by: Fam Zheng ","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=famz@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 3y1Ngv07VKz9t30\n\tfor ;\n\tTue, 26 Sep 2017 11:30:50 +1000 (AEST)","from localhost ([::1]:45134 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1dwei1-0005F2-4z\n\tfor incoming@patchwork.ozlabs.org; Mon, 25 Sep 2017 21:30:49 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:41083)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1dwehh-0005D7-4R\n\tfor qemu-devel@nongnu.org; Mon, 25 Sep 2017 21:30:30 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1dwehf-0002PN-VD\n\tfor qemu-devel@nongnu.org; Mon, 25 Sep 2017 21:30:29 -0400","from mx1.redhat.com ([209.132.183.28]:40784)\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 1dwehY-0002HC-Kb; Mon, 25 Sep 2017 21:30:20 -0400","from smtp.corp.redhat.com\n\t(int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11])\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 AD8A27E442;\n\tTue, 26 Sep 2017 01:30:19 +0000 (UTC)","from localhost (ovpn-12-91.pek2.redhat.com [10.72.12.91])\n\tby smtp.corp.redhat.com (Postfix) with ESMTP id C7C5767547;\n\tTue, 26 Sep 2017 01:30:16 +0000 (UTC)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mx1.redhat.com AD8A27E442","Date":"Tue, 26 Sep 2017 09:30:14 +0800","From":"Fam Zheng ","To":"Eric Blake ","Message-ID":"<20170926013014.GB31924@lemon.lan>","References":"<20170925145526.32690-1-eblake@redhat.com>\n\t<20170925145526.32690-19-eblake@redhat.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20170925145526.32690-19-eblake@redhat.com>","User-Agent":"Mutt/1.8.3 (2017-05-23)","X-Scanned-By":"MIMEDefang 2.79 on 10.5.11.11","X-Greylist":"Sender IP whitelisted, not delayed by milter-greylist-4.5.16\n\t(mx1.redhat.com [10.5.110.27]);\n\tTue, 26 Sep 2017 01:30:19 +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 v10 18/20] qcow2: Switch\n\tstore_bitmap_data() to 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":"kwolf@redhat.com, vsementsov@virtuozzo.com, qemu-block@nongnu.org,\n\tqemu-devel@nongnu.org, Max Reitz , jsnow@redhat.com","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}}]