From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41353) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xd1lH-0008BW-7o for qemu-devel@nongnu.org; Sat, 11 Oct 2014 14:51:33 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Xd1lB-0008TJ-35 for qemu-devel@nongnu.org; Sat, 11 Oct 2014 14:51:27 -0400 Received: from lputeaux-656-01-25-125.w80-12.abo.wanadoo.fr ([80.12.84.125]:47598 helo=paradis.irqsave.net) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xd1lA-0008TD-QK for qemu-devel@nongnu.org; Sat, 11 Oct 2014 14:51:21 -0400 Date: Sat, 11 Oct 2014 20:48:49 +0200 From: =?iso-8859-1?Q?Beno=EEt?= Canet Message-ID: <20141011184849.GA13202@irqsave.net> References: <1408215258-12545-1-git-send-email-mreitz@redhat.com> <1408215258-12545-2-git-send-email-mreitz@redhat.com> <20141010115011.GB10091@irqsave.net> <5438FBF4.7070504@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <5438FBF4.7070504@redhat.com> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 1/3] block: Ignore allocation size in underlying file List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Max Reitz Cc: =?iso-8859-1?Q?Beno=EEt?= Canet , Kevin Wolf , qemu-devel@nongnu.org, Stefan Hajnoczi The Saturday 11 Oct 2014 =E0 11:44:20 (+0200), Max Reitz wrote : > Am 10.10.2014 um 13:50 schrieb Beno=EEt Canet: > >The Saturday 16 Aug 2014 =E0 20:54:16 (+0200), Max Reitz wrote : > >>When falling through to the underlying file in > >>bdrv_co_get_block_status(), do not let the number of sectors for whic= h > >>information could be obtained be overwritten. > >> > >>Signed-off-by: Max Reitz > >>--- > >> block.c | 6 ++++-- > >> 1 file changed, 4 insertions(+), 2 deletions(-) > >> > >>diff --git a/block.c b/block.c > >>index 3e252a2..c922664 100644 > >>--- a/block.c > >>+++ b/block.c > >>@@ -3991,9 +3991,11 @@ static int64_t coroutine_fn bdrv_co_get_block_= status(BlockDriverState *bs, > >> if (bs->file && > >> (ret & BDRV_BLOCK_DATA) && !(ret & BDRV_BLOCK_ZERO) && > >> (ret & BDRV_BLOCK_OFFSET_VALID)) { > >>+ int backing_pnum; > >>+ > >> ret2 =3D bdrv_co_get_block_status(bs->file, ret >> BDRV_SEC= TOR_BITS, > >>- *pnum, pnum); > >>- if (ret2 >=3D 0) { > >>+ *pnum, &backing_pnum); > >>+ if (ret2 >=3D 0 && backing_pnum >=3D *pnum) { > >About backing_pnum >=3D *pnum. > > > >The documentation of bdrv_co_get_block_status says: > > > > * 'nb_sectors' is the max value 'pnum' should be set to. If nb_sect= ors goes > > * beyond the end of the disk image it will be clamped. > > */ > >static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState = *bs, > > int64_t sector_n= um, > > int nb_sectors, = int *pnum) > > > >So clearly after the bdrv_co_get_block_status *pnum >=3D backing_pnum. > > > >This means that backing_pnum > *pnum will never happen. > > > >I think either this test is wrong or the doc is wrong. >=20 > Thank you for confusing me, I had to think quite a while about this. *g= * >=20 > The condition is not for error checking. If it was, it would be the wro= ng > order (the condition should be true on success, that's why it's "ret2 >= =3D 0" > and not "ret2 < 0", so it should then be "backing_pnum <=3D *pnum"). So= what > this is testing is whether all sectors in the underlying file in the qu= eried > range are read as zero. But if "backing_pnum < *pnum" that is not the c= ase, > some clusters are not zero. So we may not set the zero flag if backing_= pnum > < *pnum; or as it reads in the code, we may only set it if backing_pnum= >=3D > *pnum. This is not about whether *pnum > backing_pnum, but more about > whether backing_pnum =3D=3D *pnum (but >=3D would be fine, too, if > bdrv_co_get_block_status() supported it, so that's why I wrote it that = way). >=20 > However, I'm starting to think about whether it would be better, for th= e > backing_pnum < *pnum case, not to not set the zero flag, but rather sim= ply > set *pnum =3D backing_pnum. And this in turn would be pretty equivalent= to > just omitting this patch, because: >=20 > If we get to this point where we query the underlying file and it retur= ns a > certain number of sectors is zero; then we therefore want to set *pnum = =3D > backing_pnum (both if backing_pnum < *pnum and if backing_pnum =3D=3D *= pnum; > backing_pnum > *pnum cannot happen, as you pointed out). On the other h= and, > if the sectors are not reported to be zero, but backing_pnum < *pnum, w= e > want to shorten *pnum accordingly as well because this may indicate tha= t > after another backing_pnum sectors, we arrive at a hole in the file. >=20 > There is only one point I can imagine where it makes sense not to let > backing_pnum overwrite *pnum: And that's if bdrv_co_get_block_status() > reported BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID with an offset beyon= d the > EOF. I think this might actually happen with qcow2, if one cluster simp= ly > lies beyond the EOF (which is perfectly valid). So I conclude that this > patch has its use after all but needs to be modified so that backing_pn= um > always overwrites *pnum; except for when backing_pnum is zero (which sh= ould > only happen at or after the EOF) in which case the zero flag should be = set > and *pnum should be left as it was. >=20 > And now in all honesty: Thanks for confusing me, I guess I can think be= tter > when I'm confused. :-) >=20 You better have killer english skills to sumarize this in a nice commit m= essage :) I'll read the next version. Best regards Beno=EEt > Max >=20 > >Best regards > > > >Beno=EEt > > > > > >> /* Ignore errors. This is just providing extra informa= tion, it > >> * is useful but not necessary. > >> */ > >>--=20 > >>2.0.4 > >> > >> >=20 >=20