[{"id":1774017,"web_url":"http://patchwork.ozlabs.org/comment/1774017/","msgid":"","list_archive_url":null,"date":"2017-09-23T12:01:01","subject":"Re: [Qemu-devel] [PATCH v9 18/20] qcow2: Switch store_bitmap_data()\n\tto byte-based iteration","submitter":{"id":66592,"url":"http://patchwork.ozlabs.org/api/people/66592/","name":"Vladimir Sementsov-Ogievskiy","email":"vsementsov@virtuozzo.com"},"content":"19.09.2017 23:19, 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\nReviewed-by: Vladimir Sementsov-Ogievskiy\n\n\n\n>\n> ---\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),","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 3xzpqP1Mc4z9t4B\n\tfor ;\n\tSat, 23 Sep 2017 22:01:50 +1000 (AEST)","from localhost ([::1]:34789 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1dvj7w-0007dG-4f\n\tfor incoming@patchwork.ozlabs.org; Sat, 23 Sep 2017 08:01:44 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:35275)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1dvj7S-0007c3-1T\n\tfor qemu-devel@nongnu.org; Sat, 23 Sep 2017 08:01:15 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1dvj7N-0003kQ-1z\n\tfor qemu-devel@nongnu.org; Sat, 23 Sep 2017 08:01:14 -0400","from mailhub.sw.ru ([195.214.232.25]:10524 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 1dvj7M-0003jf-Hx\n\tfor qemu-devel@nongnu.org; Sat, 23 Sep 2017 08:01:09 -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 v8NC11LB016659;\n\tSat, 23 Sep 2017 15:01:01 +0300 (MSK)"],"To":"Eric Blake , qemu-devel@nongnu.org","References":"<20170919201910.25656-1-eblake@redhat.com>\n\t<20170919201910.25656-19-eblake@redhat.com>","From":"Vladimir Sementsov-Ogievskiy ","Message-ID":"","Date":"Sat, 23 Sep 2017 15:01:01 +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":"<20170919201910.25656-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 v9 18/20] 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":"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"}}]