| Message ID | 20200528094405.145708-5-vsementsov@virtuozzo.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drop unallocated_blocks_are_zero | expand |
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!
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) */