From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49488) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1avNuT-0003t9-9q for qemu-devel@nongnu.org; Wed, 27 Apr 2016 07:45:38 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1avNuS-0007Qr-0L for qemu-devel@nongnu.org; Wed, 27 Apr 2016 07:45:37 -0400 References: <1461392787-26897-1-git-send-email-rkx1209dev@gmail.com> <1461392787-26897-2-git-send-email-rkx1209dev@gmail.com> From: Max Reitz Message-ID: <5720A656.5050809@redhat.com> Date: Wed, 27 Apr 2016 13:45:26 +0200 MIME-Version: 1.0 In-Reply-To: <1461392787-26897-2-git-send-email-rkx1209dev@gmail.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="V2jAeXWnDlGPJSxnCp9Vodi5TgBKist1A" Subject: Re: [Qemu-devel] [PATCH v4 1/1] qemu-img: check block status of backing file when converting. List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Ren Kimura Cc: kwolf@redhat.com, qemu-block@nongnu.org, qemu-devel@nongnu.org This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --V2jAeXWnDlGPJSxnCp9Vodi5TgBKist1A Content-Type: multipart/mixed; boundary="qTowPXc7ljjKiQiiXuUgkEaMrj4mCNvkR" From: Max Reitz To: Ren Kimura Cc: kwolf@redhat.com, qemu-block@nongnu.org, qemu-devel@nongnu.org Message-ID: <5720A656.5050809@redhat.com> Subject: Re: [PATCH v4 1/1] qemu-img: check block status of backing file when converting. References: <1461392787-26897-1-git-send-email-rkx1209dev@gmail.com> <1461392787-26897-2-git-send-email-rkx1209dev@gmail.com> In-Reply-To: <1461392787-26897-2-git-send-email-rkx1209dev@gmail.com> --qTowPXc7ljjKiQiiXuUgkEaMrj4mCNvkR Content-Type: text/plain; charset=iso-8859-15 Content-Transfer-Encoding: quoted-printable On 23.04.2016 08:26, Ren Kimura wrote: > When converting images, check the block status of its backing file chai= n > to avoid needlessly reading zeros. >=20 > Signed-off-by: Ren Kimura > --- > qemu-img.c | 31 +++++++++++++++++++++++++++++-- > 1 file changed, 29 insertions(+), 2 deletions(-) >=20 > diff --git a/qemu-img.c b/qemu-img.c > index 06264d9..b771227 100644 > --- a/qemu-img.c > +++ b/qemu-img.c > @@ -1451,6 +1451,22 @@ static void convert_select_part(ImgConvertState = *s, int64_t sector_num) > } > } > =20 > +static int64_t get_backing_status(BlockDriverState *bs, > + int64_t sector_num, > + int nb_sectors, int *pnum) > +{ > + while (bs->backing) { > + int64_t ret; > + BlockDriverState *file; > + bs =3D bs->backing->bs; > + ret =3D bdrv_get_block_status(bs, sector_num, nb_sectors, pnum= , &file); > + if (ret < 0 || ret & (BDRV_BLOCK_DATA | BDRV_BLOCK_ZERO)) { > + return ret; > + } Denis' suggestion was to basically drop this function and call bdrv_get_block_status_above() instead. I wouldn't strictly oppose keeping this function because of the BDRV_BLOCK_{DATA,ZERO} vs. BDRV_BLOCK_ALLOCATED issue I talked about befo= re. One thing we can definitely learn from bdrv_get_block_status_above(), though, is that a nb_sectors =3D MIN(nb_sectors, *pnum); is missing here (see bdrv_co_get_block_status_above()). (Sorry for not noticing this before) > + } > + return -1; > +} > + > static int convert_iteration_sectors(ImgConvertState *s, int64_t secto= r_num) > { > int64_t ret; > @@ -1477,10 +1493,21 @@ static int convert_iteration_sectors(ImgConvert= State *s, int64_t sector_num) > } else if (!s->target_has_backing) { > /* Without a target backing file we must copy over the con= tents of > * the backing file as well. */ > - /* TODO Check block status of the backing file chain to av= oid > + /* Check block status of the backing file chain to avoid > * needlessly reading zeroes and limiting the iteration to= the > * buffer size */ > - s->status =3D BLK_DATA; > + ret =3D get_backing_status(blk_bs(s->src[s->src_cur]), > + sector_num - s->src_cur_offset, > + n, &n); In any case, I think I agree with Denis that just calling ret =3D bdrv_get_block_status_above(blk_bs(s->src[s->src_cur]), NULL, sector_num - s->src_cur_offset, n, &n, &file); would be sufficient. (Sorry for not noticing this either...) Max > + if (ret < 0) { > + return ret; > + } > + > + if (ret & BDRV_BLOCK_ZERO) { > + s->status =3D BLK_ZERO; > + } else { > + s->status =3D BLK_DATA; > + } > } else { > s->status =3D BLK_BACKING_FILE; > } >=20 --qTowPXc7ljjKiQiiXuUgkEaMrj4mCNvkR-- --V2jAeXWnDlGPJSxnCp9Vodi5TgBKist1A 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 iQEcBAEBCAAGBQJXIKZWAAoJEDuxQgLoOKytdB4H/0rFzhphOXf6b0U+5wNPxD0w ZX5mKqCm+vpvV0RPYYTbJuTTAsdrbd4wLM8MBaB9pedHn2MwAFAZQms8uWGMEG3+ YIhkgzfOu6RV55cwga4QF3aaIri0NrrR2GlC7413GnJqSWUM7wYEJ/YrvXGPvK2g aZ58SEYQYVqqII+ZO9WXinNks4ibaoQEIyfEByukHw5k2phnvfOdg2POLY542OSj +NCTJB/6njNolEdDntHPXBNCFywY7m798MgQifDjW70juloI59gl4ib17jAThA4j aYTUIX0FXd7xzAGEoBglWdj8JKjLeqNtcG1SRH5lT+HgIvqO4yqGOGreVybiaJo= =S4cX -----END PGP SIGNATURE----- --V2jAeXWnDlGPJSxnCp9Vodi5TgBKist1A--