From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52759) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aXuDh-0003Ep-J9 for qemu-devel@nongnu.org; Mon, 22 Feb 2016 12:24:26 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aXuDg-0003s2-9r for qemu-devel@nongnu.org; Mon, 22 Feb 2016 12:24:25 -0500 References: <1455989993-1917-1-git-send-email-mreitz@redhat.com> <1455989993-1917-2-git-send-email-mreitz@redhat.com> <20160222125011.GE5387@noname.str.redhat.com> <56CB3AB2.4090809@redhat.com> From: Max Reitz Message-ID: <56CB443E.8060301@redhat.com> Date: Mon, 22 Feb 2016 18:24:14 +0100 MIME-Version: 1.0 In-Reply-To: <56CB3AB2.4090809@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="FPsX2OqL9A9UoswXwkNsWcbmOV3nHPTxB" Subject: Re: [Qemu-devel] [PATCH 1/4] qemu-img: Fix preallocation with -S 0 for convert List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: Fam Zheng , qemu-devel@nongnu.org, qemu-block@nongnu.org This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --FPsX2OqL9A9UoswXwkNsWcbmOV3nHPTxB Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: quoted-printable On 22.02.2016 17:43, Max Reitz wrote: > On 22.02.2016 13:50, Kevin Wolf wrote: >> Am 20.02.2016 um 18:39 hat Max Reitz geschrieben: >>> When passing -S 0 to qemu-img convert, the target image is supposed t= o >>> be fully allocated. Right now, this is not the case if the source ima= ge >>> contains areas which bdrv_get_block_status() reports as being zero. >>> >>> This patch introduces a new ImgConvertBlockStatus named BLK_ZERO_DATA= >>> which is set by convert_iteration_sectors() if an area is detected to= be >>> zero and min_sparse is 0. Simply using BLK_DATA is not optimal, becau= se >>> knowing an area to be zero allows us to memset() the buffer to be >>> written directly rather than having to use blk_read(). >>> >>> Other than that, BLK_ZERO_DATA is handled exactly like BLK_DATA. >>> >>> This patch changes the reference output for iotest 122; contrary to w= hat >>> it assumed, -S 0 really should allocate everything in the output, not= >>> just areas that are filled with zeros (as opposed to being zeroed). >>> >>> Signed-off-by: Max Reitz >> >> I don't like how you touch the conversion code all over the place. >> Specifically, convert_iteration_sectors() and convert_read() (and >> consequently s->status) are supposed to be only about the source file.= >> -S 0 doesn't make a difference for what the source file looks like, so= >> we shouldn't need to change anything there. >> >> The following change should do the same thing (it passes your test cas= e >> anyway) and is more contained to the actual writeout of image data. >=20 > Well, I briefly considered making @buf non-const in convert_write(), bu= t > I discarded that idea, and I'm still not comfortable with that. If you > argue that convert_read() should only deal with the source image, I'll > argue that convert_write() should only deal with the target image. I > know that you're making @buf non-const because we need some scratch > buffer and, well, @buf is available, so why not use that? But it still > doesn't sit right with me. >=20 > So I'd like to pull that memset() out of convert_write() and just tell > convert_write() to write that buffer as data. In fact, that is basicall= y > what my patch does. But why does it then not just use BLK_DATA but this= > new status? >=20 > Because of two reasons: First, another issue with your patch is that > zeroed areas are not counted in the progress update. If we are writing > them as zeros, we should count them, however. Therefore, we need some > special-casing in convert_do_copy(). Effectively, we end up with stuff = like: >=20 >> if (s->status =3D=3D BLK_DATA || >> (!s->min_sparse && s->status =3D=3D BLK_ZERO)) >=20 > I found that combination of (min_sparse && BLK_ZERO) to be ugly, and > liked it much better if we could do that test a single time in > convert_read and be done with it. This is why I added the new status.* >=20 > And if you pull the memset() out of convert_write() and add a new > status, what you end up with is basically my patch. >=20 > *Note that I initially thought we'd have this test in many more places > than we actually would, as it turned out. I'll take a look at how much > simpler this patch becomes if I drop the BLK_ZERO_DATA status. Said patch would look like this: diff --git a/qemu-img.c b/qemu-img.c index 2edb139..b696ba4 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -1509,10 +1509,6 @@ static int convert_read(ImgConvertState *s, int64_t sector_num, int nb_sectors, int n; int ret; - if (s->status =3D=3D BLK_ZERO || s->status =3D=3D BLK_BACKING_FILE) = { - return 0; - } - assert(nb_sectors <=3D s->buf_sectors); while (nb_sectors > 0) { BlockBackend *blk; @@ -1650,7 +1646,8 @@ static int convert_do_copy(ImgConvertState *s) ret =3D n; goto fail; } - if (s->status =3D=3D BLK_DATA) { + if (s->status =3D=3D BLK_DATA || (!s->min_sparse && s->status =3D= =3D BLK_ZERO)) + { s->allocated_sectors +=3D n; } sector_num +=3D n; @@ -1670,17 +1667,24 @@ static int convert_do_copy(ImgConvertState *s) ret =3D n; goto fail; } - if (s->status =3D=3D BLK_DATA) { + if (s->status =3D=3D BLK_DATA || (!s->min_sparse && s->status =3D= =3D BLK_ZERO)) + { allocated_done +=3D n; qemu_progress_print(100.0 * allocated_done / s->allocated_sectors, 0); } - ret =3D convert_read(s, sector_num, n, buf); - if (ret < 0) { - error_report("error while reading sector %" PRId64 - ": %s", sector_num, strerror(-ret)); - goto fail; + if (s->status =3D=3D BLK_DATA) { + ret =3D convert_read(s, sector_num, n, buf); + if (ret < 0) { + error_report("error while reading sector %" PRId64 + ": %s", sector_num, strerror(-ret)); + goto fail; + } + } else if (!s->min_sparse && s->status =3D=3D BLK_ZERO) { + n =3D MIN(n, s->buf_sectors); + memset(buf, 0, n * BDRV_SECTOR_SIZE); + s->status =3D BLK_DATA; } ret =3D convert_write(s, sector_num, n, buf); I'd get the diffcount even smaller by keeping convert_read() unchanged and continuing to call it unconditionally, but I like that change in itself because I find it makes the logic clearer. I can be persuaded to split this patch into two, however (one pulling the condition out of convert_read(), and another one doing the rest of this patch). Max --FPsX2OqL9A9UoswXwkNsWcbmOV3nHPTxB Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQEcBAEBCAAGBQJWy0Q+AAoJEDuxQgLoOKytCEkH/1UWlV1HPv/Fl0hYYycsU/vf 1r9pb9GEStxaQn1wJ/lIZ12dGxHHv0pyQV8djPkqWZ64zpnwZu/24wLZpJCOiFzH /z1src/VgCyU03hAUL9sonsllAENyTZ0cyhEG5gHWcBHcrBWvbMXqBXe3Ssl699M X1zl5NagmJyWr/OBoWrVYZZns+7uthWNTmy2bEiRx24oW8dVk5pzVD4Q/ZJCkhKa IquwaBKqVp0n5+TNy9xtzJyHCxr0HEFaGesfPL6zJOcRujaZuPOMt5OC3XEaRL90 OouqWT52SMhxQkRICXcuLAAIsSKUAYMTxGvSaLkhxsJuYkRoaFus7rKy96sw3z0= =8h8D -----END PGP SIGNATURE----- --FPsX2OqL9A9UoswXwkNsWcbmOV3nHPTxB--