In case when get_image_offset() returns -1, we do zero out the
corresponding chunk of qiov. So, this should be reported as ZERO.

Note that this changes visible output of "qemu-img map --output=json"
and "qemu-io -c map" commands. For qemu-img map, the change is obvious:
we just mark as zero what is really zero. For qemu-io it's less
obvious: what was unallocated now is allocated.

There is an inconsistency in understanding of unallocated regions in
Qemu: backing-supporting format-drivers return 0 block-status to report
go-to-backing logic for this area. Some protocol-drivers (iscsi) return
0 to report fs-unallocated-non-zero status (i.e., don't occupy space on
disk, read result is undefined).

BDRV_BLOCK_ALLOCATED is defined as something more close to
go-to-backing logic. Still it is calculated as ZERO | DATA, so 0 from
iscsi is treated as unallocated. It doesn't influence backing-chain
behavior, as iscsi can't have backing file. But it does influence
"qemu-io -c map".

We should solve this inconsistency at some future point. Now, let's
just make backing-not-supporting format drivers (vdi in the previous
patch and vpc now) to behave more like backing-supporting drivers
and not report 0 block-status. More over, returning ZERO status is
absolutely valid thing, and again, corresponds to how the other
format-drivers (backing-supporting) work.

After block-status update, it never reports 0, so setting
unallocated_blocks_are_zero doesn't make sense (as the only user of it
is bdrv_co_block_status and it checks unallocated_blocks_are_zero only
for unallocated areas). Drop it.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 block/vpc.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/block/vpc.c b/block/vpc.c
index c055591641..01fcd37e3c 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -606,7 +606,6 @@ static int vpc_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
         bdi->cluster_size = s->block_size;
     }
 
-    bdi->unallocated_blocks_are_zero = true;
     return 0;
 }
 
@@ -745,7 +744,7 @@ static int coroutine_fn vpc_co_block_status(BlockDriverState *bs,
     image_offset = get_image_offset(bs, offset, false, NULL);
     allocated = (image_offset != -1);
     *pnum = 0;
-    ret = 0;
+    ret = BDRV_BLOCK_ZERO;
 
     do {
         /* All sectors in a block are contiguous (without using the bitmap) */
-- 
2.18.0

On 28.05.20 11:43, Vladimir Sementsov-Ogievskiy wrote:
> In case when get_image_offset() returns -1, we do zero out the
> corresponding chunk of qiov. So, this should be reported as ZERO.
> 
> Note that this changes visible output of "qemu-img map --output=json"
> and "qemu-io -c map" commands. For qemu-img map, the change is obvious:
> we just mark as zero what is really zero. For qemu-io it's less
> obvious: what was unallocated now is allocated.
> 
> There is an inconsistency in understanding of unallocated regions in
> Qemu: backing-supporting format-drivers return 0 block-status to report
> go-to-backing logic for this area. Some protocol-drivers (iscsi) return
> 0 to report fs-unallocated-non-zero status (i.e., don't occupy space on
> disk, read result is undefined).
> 
> BDRV_BLOCK_ALLOCATED is defined as something more close to
> go-to-backing logic. Still it is calculated as ZERO | DATA, so 0 from
> iscsi is treated as unallocated. It doesn't influence backing-chain
> behavior, as iscsi can't have backing file. But it does influence
> "qemu-io -c map".
> 
> We should solve this inconsistency at some future point. Now, let's
> just make backing-not-supporting format drivers (vdi in the previous
> patch and vpc now) to behave more like backing-supporting drivers
> and not report 0 block-status. More over, returning ZERO status is
> absolutely valid thing, and again, corresponds to how the other
> format-drivers (backing-supporting) work.
> 
> After block-status update, it never reports 0, so setting
> unallocated_blocks_are_zero doesn't make sense (as the only user of it
> is bdrv_co_block_status and it checks unallocated_blocks_are_zero only
> for unallocated areas). Drop it.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>  block/vpc.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)

This breaks iotest 146, I’m afraid.  Now everything is reported as
allocated, which is of course what we kind of want, but I suppose it
renders the test a bit useless.

It seems to me like the best thing to do would be to replace the
“qemu-io -o $opts -c map” calls by
“qemu-img map --output=json --image-opts $opts”, so that we get
“zero: true” instead of “not allocated” in the output.

A bit of a problem with that approach are the “allocated” entries,
because those are split into 2 MB chunks, so the output gets long.  But,
well.

I’ve attached a diff to that effect.  Would you be OK with squashing
that in?

Max

06.07.2020 11:28, Max Reitz wrote:
> On 28.05.20 11:43, Vladimir Sementsov-Ogievskiy wrote:
>> In case when get_image_offset() returns -1, we do zero out the
>> corresponding chunk of qiov. So, this should be reported as ZERO.
>>
>> Note that this changes visible output of "qemu-img map --output=json"
>> and "qemu-io -c map" commands. For qemu-img map, the change is obvious:
>> we just mark as zero what is really zero. For qemu-io it's less
>> obvious: what was unallocated now is allocated.
>>
>> There is an inconsistency in understanding of unallocated regions in
>> Qemu: backing-supporting format-drivers return 0 block-status to report
>> go-to-backing logic for this area. Some protocol-drivers (iscsi) return
>> 0 to report fs-unallocated-non-zero status (i.e., don't occupy space on
>> disk, read result is undefined).
>>
>> BDRV_BLOCK_ALLOCATED is defined as something more close to
>> go-to-backing logic. Still it is calculated as ZERO | DATA, so 0 from
>> iscsi is treated as unallocated. It doesn't influence backing-chain
>> behavior, as iscsi can't have backing file. But it does influence
>> "qemu-io -c map".
>>
>> We should solve this inconsistency at some future point. Now, let's
>> just make backing-not-supporting format drivers (vdi in the previous
>> patch and vpc now) to behave more like backing-supporting drivers
>> and not report 0 block-status. More over, returning ZERO status is
>> absolutely valid thing, and again, corresponds to how the other
>> format-drivers (backing-supporting) work.
>>
>> After block-status update, it never reports 0, so setting
>> unallocated_blocks_are_zero doesn't make sense (as the only user of it
>> is bdrv_co_block_status and it checks unallocated_blocks_are_zero only
>> for unallocated areas). Drop it.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> ---
>>   block/vpc.c | 3 +--
>>   1 file changed, 1 insertion(+), 2 deletions(-)
> 
> This breaks iotest 146, I’m afraid.  Now everything is reported as
> allocated, which is of course what we kind of want, but I suppose it
> renders the test a bit useless.
> 
> It seems to me like the best thing to do would be to replace the
> “qemu-io -o $opts -c map” calls by
> “qemu-img map --output=json --image-opts $opts”, so that we get
> “zero: true” instead of “not allocated” in the output.
> 
> A bit of a problem with that approach are the “allocated” entries,
> because those are split into 2 MB chunks, so the output gets long.  But,
> well.
> 
> I’ve attached a diff to that effect.  Would you be OK with squashing
> that in?

Yes, this looks reasonable, thanks a lot!


-- 
Best regards,
Vladimir