[{"id":1776547,"web_url":"http://patchwork.ozlabs.org/comment/1776547/","msgid":"<6cc0f8a8-6c3f-fd53-6167-82244b1633fa@redhat.com>","list_archive_url":null,"date":"2017-09-27T19:05:44","subject":"Re: [Qemu-devel] [PATCH v4 13/23] qemu-img: Simplify logic in\n\timg_compare()","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> As long as we are querying the status for a chunk smaller than\n> the known image size, we are guaranteed that a successful return\n> will have set pnum to a non-zero size (pnum is zero only for\n> queries beyond the end of the file). Use that to slightly\n> simplify the calculation of the current chunk size being compared.\n> Likewise, we don't have to shrink the amount of data operated on\n> until we know we have to read the file, and therefore have to fit\n> in the bounds of our buffer. Also, note that 'total_sectors_over'\n> is equivalent to 'progress_base'.\n> \n> With these changes in place, sectors_to_process() is now dead code,\n> and can be removed.\n> \n> Signed-off-by: Eric Blake \n> \n> ---\n> v3: new patch\n> ---\n> qemu-img.c | 40 +++++++++++-----------------------------\n> 1 file changed, 11 insertions(+), 29 deletions(-)\n> \n> diff --git a/qemu-img.c b/qemu-img.c\n> index b91133b922..f8423e9b3f 100644\n> --- a/qemu-img.c\n> +++ b/qemu-img.c\n> @@ -1171,11 +1171,6 @@ static int64_t sectors_to_bytes(int64_t sectors)\n> return sectors << BDRV_SECTOR_BITS;\n> }\n> \n> -static int64_t sectors_to_process(int64_t total, int64_t from)\n> -{\n> - return MIN(total - from, IO_BUF_SIZE >> BDRV_SECTOR_BITS);\n> -}\n> -\n> /*\n> * Check if passed sectors are empty (not allocated or contain only 0 bytes)\n> *\n> @@ -1372,13 +1367,9 @@ static int img_compare(int argc, char **argv)\n> goto out;\n> }\n> \n> - for (;;) {\n> + while (sector_num < total_sectors) {\n> int64_t status1, status2;\n> \n> - nb_sectors = sectors_to_process(total_sectors, sector_num);\n> - if (nb_sectors <= 0) {\n> - break;\n> - }\n> status1 = bdrv_block_status_above(bs1, NULL,\n> sector_num * BDRV_SECTOR_SIZE,\n> (total_sectors1 - sector_num) *\n> @@ -1402,14 +1393,9 @@ static int img_compare(int argc, char **argv)\n> goto out;\n> }\n> allocated2 = status2 & BDRV_BLOCK_ALLOCATED;\n> - if (pnum1) {\n> - nb_sectors = MIN(nb_sectors,\n> - DIV_ROUND_UP(pnum1, BDRV_SECTOR_SIZE));\n> - }\n> - if (pnum2) {\n> - nb_sectors = MIN(nb_sectors,\n> - DIV_ROUND_UP(pnum2, BDRV_SECTOR_SIZE));\n> - }\n> +\n> + assert(pnum1 && pnum2);\n> + nb_sectors = DIV_ROUND_UP(MIN(pnum1, pnum2), BDRV_SECTOR_SIZE);\n\nIn the apocalyptic future where non-sector sized returns are possible,\ndoes this math make sense?\n\ne.g. say the return is zeroes, but it's not aligned anymore, so we\nassume we have an extra half a sector's worth of zeroes here.\n\n> \n> if (strict) {\n> if ((status1 & ~BDRV_BLOCK_OFFSET_MASK) !=\n> @@ -1422,9 +1408,10 @@ static int img_compare(int argc, char **argv)\n> }\n> }\n> if ((status1 & BDRV_BLOCK_ZERO) && (status2 & BDRV_BLOCK_ZERO)) {\n> - nb_sectors = DIV_ROUND_UP(MIN(pnum1, pnum2), BDRV_SECTOR_SIZE);\n> + /* nothing to do */\n> } else if (allocated1 == allocated2) {\n> if (allocated1) {\n> + nb_sectors = MIN(nb_sectors, IO_BUF_SIZE >> BDRV_SECTOR_BITS);\n> ret = blk_pread(blk1, sector_num << BDRV_SECTOR_BITS, buf1,\n> nb_sectors << BDRV_SECTOR_BITS);\n> if (ret < 0) {\n> @@ -1453,7 +1440,7 @@ static int img_compare(int argc, char **argv)\n> }\n> }\n> } else {\n> -\n> + nb_sectors = MIN(nb_sectors, IO_BUF_SIZE >> BDRV_SECTOR_BITS);\n> if (allocated1) {\n> ret = check_empty_sectors(blk1, sector_num, nb_sectors,\n> filename1, buf1, quiet);\n> @@ -1476,30 +1463,24 @@ static int img_compare(int argc, char **argv)\n> \n> if (total_sectors1 != total_sectors2) {\n> BlockBackend *blk_over;\n> - int64_t total_sectors_over;\n> const char *filename_over;\n> \n> qprintf(quiet, \"Warning: Image size mismatch!\\n\");\n> if (total_sectors1 > total_sectors2) {\n> - total_sectors_over = total_sectors1;\n> blk_over = blk1;\n> filename_over = filename1;\n> } else {\n> - total_sectors_over = total_sectors2;\n> blk_over = blk2;\n> filename_over = filename2;\n> }\n> \n> - for (;;) {\n> + while (sector_num < progress_base) {\n> int64_t count;\n> \n> - nb_sectors = sectors_to_process(total_sectors_over, sector_num);\n> - if (nb_sectors <= 0) {\n> - break;\n> - }\n> ret = bdrv_is_allocated_above(blk_bs(blk_over), NULL,\n> sector_num * BDRV_SECTOR_SIZE,\n> - nb_sectors * BDRV_SECTOR_SIZE,\n> + (progress_base - sector_num) *\n> + BDRV_SECTOR_SIZE,\n> &count);\n> if (ret < 0) {\n> ret = 3;\n> @@ -1513,6 +1494,7 @@ static int img_compare(int argc, char **argv)\n> assert(QEMU_IS_ALIGNED(count, BDRV_SECTOR_SIZE));\n> nb_sectors = count >> BDRV_SECTOR_BITS;\n> if (ret) {\n> + nb_sectors = MIN(nb_sectors, IO_BUF_SIZE >> BDRV_SECTOR_BITS);\n> ret = check_empty_sectors(blk_over, sector_num, nb_sectors,\n> filename_over, buf1, quiet);\n> if (ret) {\n> \n\nRest looks right to me.","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-mx09.extmail.prod.ext.phx2.redhat.com;\n\tdmarc=none (p=none dis=none) header.from=redhat.com","ext-mx09.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 3y2S5v5yPdz9t49\n\tfor ;\n\tThu, 28 Sep 2017 05:08:35 +1000 (AEST)","from localhost ([::1]:56000 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1dxHhB-0003eJ-SP\n\tfor incoming@patchwork.ozlabs.org; Wed, 27 Sep 2017 15:08:33 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:33380)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1dxHee-0002Ls-W6\n\tfor qemu-devel@nongnu.org; Wed, 27 Sep 2017 15:05:58 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1dxHed-0001WR-GS\n\tfor qemu-devel@nongnu.org; Wed, 27 Sep 2017 15:05:56 -0400","from mx1.redhat.com ([209.132.183.28]:51396)\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 1dxHeY-0001TJ-M6; Wed, 27 Sep 2017 15:05:50 -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 A46132887AD;\n\tWed, 27 Sep 2017 19:05:49 +0000 (UTC)","from [10.10.124.80] (ovpn-124-80.rdu2.redhat.com [10.10.124.80])\n\tby smtp.corp.redhat.com (Postfix) with ESMTP id AE5A117C2D;\n\tWed, 27 Sep 2017 19:05:44 +0000 (UTC)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mx1.redhat.com A46132887AD","To":"Eric Blake , qemu-devel@nongnu.org","References":"<20170913160333.23622-1-eblake@redhat.com>\n\t<20170913160333.23622-14-eblake@redhat.com>","From":"John Snow ","Message-ID":"<6cc0f8a8-6c3f-fd53-6167-82244b1633fa@redhat.com>","Date":"Wed, 27 Sep 2017 15:05:44 -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-14-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.38]);\n\tWed, 27 Sep 2017 19:05:49 +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 13/23] qemu-img: Simplify logic in\n\timg_compare()","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, qemu-block@nongnu.org,\n\tMax Reitz ","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}},{"id":1776555,"web_url":"http://patchwork.ozlabs.org/comment/1776555/","msgid":"<363ffebf-61a1-609b-a37f-76ba5c07513c@redhat.com>","list_archive_url":null,"date":"2017-09-27T19:15:50","subject":"Re: [Qemu-devel] [PATCH v4 13/23] qemu-img: Simplify logic in\n\timg_compare()","submitter":{"id":6591,"url":"http://patchwork.ozlabs.org/api/people/6591/","name":"Eric Blake","email":"eblake@redhat.com"},"content":"On 09/27/2017 02:05 PM, John Snow wrote:\n> \n> \n> On 09/13/2017 12:03 PM, Eric Blake wrote:\n>> As long as we are querying the status for a chunk smaller than\n>> the known image size, we are guaranteed that a successful return\n>> will have set pnum to a non-zero size (pnum is zero only for\n>> queries beyond the end of the file). Use that to slightly\n>> simplify the calculation of the current chunk size being compared.\n>> Likewise, we don't have to shrink the amount of data operated on\n>> until we know we have to read the file, and therefore have to fit\n>> in the bounds of our buffer. Also, note that 'total_sectors_over'\n>> is equivalent to 'progress_base'.\n>>\n>> With these changes in place, sectors_to_process() is now dead code,\n>> and can be removed.\n>>\n>> Signed-off-by: Eric Blake \n>>\n\n>> @@ -1402,14 +1393,9 @@ static int img_compare(int argc, char **argv)\n>> goto out;\n>> }\n>> allocated2 = status2 & BDRV_BLOCK_ALLOCATED;\n>> - if (pnum1) {\n>> - nb_sectors = MIN(nb_sectors,\n>> - DIV_ROUND_UP(pnum1, BDRV_SECTOR_SIZE));\n>> - }\n>> - if (pnum2) {\n>> - nb_sectors = MIN(nb_sectors,\n>> - DIV_ROUND_UP(pnum2, BDRV_SECTOR_SIZE));\n>> - }\n>> +\n>> + assert(pnum1 && pnum2);\n>> + nb_sectors = DIV_ROUND_UP(MIN(pnum1, pnum2), BDRV_SECTOR_SIZE);\n> \n> In the apocalyptic future where non-sector sized returns are possible,\n> does this math make sense?\n> \n> e.g. say the return is zeroes, but it's not aligned anymore, so we\n> assume we have an extra half a sector's worth of zeroes here.\n\nNot introduced in this patch, but a good question for 12/23. We want to\nround up rather than down to ensure that we don't inf-loop on a partial\nsector response; but at the same time, you're right that if we got a\nreport of a half-sector zero and we widen it, we can't guarantee that\nthe second half is zero.\n\nOn the bright side, this rounding goes away when later patches switch\nimg_compare to be byte-based, later in this series. But you're right\nthat it is probably smarter to have 12/23 assert that things are already\naligned (and thus we don't need to round in the first place).","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=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 3y2SH64b6Dz9t49\n\tfor ;\n\tThu, 28 Sep 2017 05:16:33 +1000 (AEST)","from localhost ([::1]:56040 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1dxHos-0006Ke-Q5\n\tfor incoming@patchwork.ozlabs.org; Wed, 27 Sep 2017 15:16:30 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:36460)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1dxHoW-0006I7-6d\n\tfor qemu-devel@nongnu.org; Wed, 27 Sep 2017 15:16:11 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1dxHoU-0006WM-TK\n\tfor qemu-devel@nongnu.org; Wed, 27 Sep 2017 15:16:08 -0400","from mx1.redhat.com ([209.132.183.28]:53490)\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 1dxHoN-0006TO-1D; Wed, 27 Sep 2017 15:15:59 -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 EC258ED479;\n\tWed, 27 Sep 2017 19:15:57 +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 B0BB818B4D;\n\tWed, 27 Sep 2017 19:15:51 +0000 (UTC)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mx1.redhat.com EC258ED479","To":"John Snow , qemu-devel@nongnu.org","References":"<20170913160333.23622-1-eblake@redhat.com>\n\t<20170913160333.23622-14-eblake@redhat.com>\n\t<6cc0f8a8-6c3f-fd53-6167-82244b1633fa@redhat.com>","From":"Eric Blake ","Openpgp":"url=http://people.redhat.com/eblake/eblake.gpg","Organization":"Red Hat, Inc.","Message-ID":"<363ffebf-61a1-609b-a37f-76ba5c07513c@redhat.com>","Date":"Wed, 27 Sep 2017 14:15:50 -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":"<6cc0f8a8-6c3f-fd53-6167-82244b1633fa@redhat.com>","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\";\n\tboundary=\"HtjuKaid71cuhs28lfdvlmCkK5gsK1tSP\"","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\tWed, 27 Sep 2017 19:15:58 +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 13/23] qemu-img: Simplify logic in\n\timg_compare()","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, qemu-block@nongnu.org,\n\tMax Reitz ","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}}]