Re: [Qemu-devel] [PATCH 1/3] block: Ignore allocation size in underlying file

["Benoît Canet" <benoit.canet@irqsave.net>]

The Saturday 11 Oct 2014 à 11:44:20 (+0200), Max Reitz wrote :
> Am 10.10.2014 um 13:50 schrieb Benoît Canet:
> >The Saturday 16 Aug 2014 à 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 which
> >>information could be obtained be overwritten.
> >>
> >>Signed-off-by: Max Reitz <mreitz@redhat.com>
> >>---
> >>  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 = bdrv_co_get_block_status(bs->file, ret >> BDRV_SECTOR_BITS,
> >>-                                        *pnum, pnum);
> >>-        if (ret2 >= 0) {
> >>+                                        *pnum, &backing_pnum);
> >>+        if (ret2 >= 0 && backing_pnum >= *pnum) {
> >About backing_pnum >= *pnum.
> >
> >The documentation of bdrv_co_get_block_status says:
> >
> >  * 'nb_sectors' is the max value 'pnum' should be set to.  If nb_sectors 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_num,
> >                                                      int nb_sectors, int *pnum)
> >
> >So clearly after the bdrv_co_get_block_status *pnum >= backing_pnum.
> >
> >This means that  backing_pnum > *pnum will never happen.
> >
> >I think either this test is wrong or the doc is wrong.
> 
> Thank you for confusing me, I had to think quite a while about this. *g*
> 
> The condition is not for error checking. If it was, it would be the wrong
> order (the condition should be true on success, that's why it's "ret2 >= 0"
> and not "ret2 < 0", so it should then be "backing_pnum <= *pnum"). So what
> this is testing is whether all sectors in the underlying file in the queried
> range are read as zero. But if "backing_pnum < *pnum" that is not the case,
> 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 >=
> *pnum. This is not about whether *pnum > backing_pnum, but more about
> whether backing_pnum == *pnum (but >= would be fine, too, if
> bdrv_co_get_block_status() supported it, so that's why I wrote it that way).
> 
> However, I'm starting to think about whether it would be better, for the
> backing_pnum < *pnum case, not to not set the zero flag, but rather simply
> set *pnum = backing_pnum. And this in turn would be pretty equivalent to
> just omitting this patch, because:
> 
> If we get to this point where we query the underlying file and it returns a
> certain number of sectors is zero; then we therefore want to set *pnum =
> backing_pnum (both if backing_pnum < *pnum and if backing_pnum == *pnum;
> backing_pnum > *pnum cannot happen, as you pointed out). On the other hand,
> if the sectors are not reported to be zero, but backing_pnum < *pnum, we
> want to shorten *pnum accordingly as well because this may indicate that
> after another backing_pnum sectors, we arrive at a hole in the file.
> 
> 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 beyond the
> EOF. I think this might actually happen with qcow2, if one cluster simply
> 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_pnum
> always overwrites *pnum; except for when backing_pnum is zero (which should
> only happen at or after the EOF) in which case the zero flag should be set
> and *pnum should be left as it was.
> 
> And now in all honesty: Thanks for confusing me, I guess I can think better
> when I'm confused. :-)
> 

You better have killer english skills to sumarize this in a nice commit message :)
I'll read the next version.

Best regards

Benoît

> Max
> 
> >Best regards
> >
> >Benoît
> >
> >
> >>              /* Ignore errors.  This is just providing extra information, it
> >>               * is useful but not necessary.
> >>               */
> >>-- 
> >>2.0.4
> >>
> >>
> 
>