From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33722) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aXtaJ-0007Ju-Mh for qemu-devel@nongnu.org; Mon, 22 Feb 2016 11:44:22 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aXtaE-0007va-5s for qemu-devel@nongnu.org; Mon, 22 Feb 2016 11:43:43 -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> From: Max Reitz Message-ID: <56CB3AB2.4090809@redhat.com> Date: Mon, 22 Feb 2016 17:43:30 +0100 MIME-Version: 1.0 In-Reply-To: <20160222125011.GE5387@noname.str.redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="am67kIUGNeuHG7Ag3agvpgO5d6xLxuOQv" 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) --am67kIUGNeuHG7Ag3agvpgO5d6xLxuOQv Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: quoted-printable 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 to= >> be fully allocated. Right now, this is not the case if the source imag= e >> 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, becaus= e >> 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 wh= at >> 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 >=20 > 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. >=20 > The following change should do the same thing (it passes your test case= > anyway) and is more contained to the actual writeout of image data. Well, I briefly considered making @buf non-const in convert_write(), but 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. 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 basically what my patch does. But why does it then not just use BLK_DATA but this new status? 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 li= ke: > if (s->status =3D=3D BLK_DATA || > (!s->min_sparse && s->status =3D=3D BLK_ZERO)) 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.* And if you pull the memset() out of convert_write() and add a new status, what you end up with is basically my patch. *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. Max >=20 > Kevin >=20 >=20 > diff --git a/qemu-img.c b/qemu-img.c > index 2edb139..3b234cf 100644 > --- a/qemu-img.c > +++ b/qemu-img.c > @@ -1540,14 +1540,21 @@ static int convert_read(ImgConvertState *s, int= 64_t sector_num, int nb_sectors, > } > =20 > static int convert_write(ImgConvertState *s, int64_t sector_num, int n= b_sectors, > - const uint8_t *buf) > + uint8_t *buf) > { > int ret; > =20 > while (nb_sectors > 0) { > int n =3D nb_sectors; > + int status =3D s->status; > =20 > - switch (s->status) { > + if (!s->min_sparse && status =3D=3D BLK_ZERO) { > + n =3D MIN(n, s->buf_sectors); > + memset(buf, 0, n * BDRV_SECTOR_SIZE); > + status =3D BLK_DATA; > + } > + > + switch (status) { > case BLK_BACKING_FILE: > /* If we have a backing file, leave clusters unallocated t= hat are > * unallocated in the source image, so that the backing fi= le is > @@ -1602,7 +1609,9 @@ static int convert_write(ImgConvertState *s, int6= 4_t sector_num, int nb_sectors, > =20 > sector_num +=3D n; > nb_sectors -=3D n; > - buf +=3D n * BDRV_SECTOR_SIZE; > + if (s->status =3D=3D BLK_DATA) { > + buf +=3D n * BDRV_SECTOR_SIZE; > + } > } > =20 > return 0; >=20 --am67kIUGNeuHG7Ag3agvpgO5d6xLxuOQv 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 iQEcBAEBCAAGBQJWyzqyAAoJEDuxQgLoOKytgeMH/i1aTvkSfZ3E2UQMePwKLdvA pE7tiEjviz5orW3G0dzHiKCWZKOrLVUyR0TlWMRoQ/krw50OsvZ2ySQkxNYJulxc jVsayo7mzHYw+B1un/xqZgD121gp3uPQ8OGBx+8Pze7hfGUl8ICzSObMwb2Zrwr+ JmVO8kCEcHXNadyR/DdKmH9jQBS0XX5eies0q9241LzT/GrDqYihbTNud1Wdpw55 uVE2E4rl7821jNNtFnH7Qs30gYbbWPj9k3/4s2OacMB/tFieZi2FXRCkfWzVzxuf I17PkAzcXgpnPS8/qCROTtzNkdHt2y1UATomrskoYJIWhOTomY7ldTjtC2MOhZo= =EHv5 -----END PGP SIGNATURE----- --am67kIUGNeuHG7Ag3agvpgO5d6xLxuOQv--