[PATCH 0/8] drop unallocated_blocks_are_zero

[PATCH 0/8] drop unallocated_blocks_are_zero

[Vladimir Sementsov-Ogievskiy]

Hi all!

This is first step to block-status refactoring, and solves most simple
problem mentioned in my investigation of block-status described in
the thread "backing chain & block status & filters":
  https://lists.gnu.org/archive/html/qemu-devel/2020-04/msg04706.html


unallocated_blocks_are_zero doesn't simplify all the logic about
block-status, and happily it's not needed, as shown by the following
patches. So, let's get rid of it.

Vladimir Sementsov-Ogievskiy (8):
  block/vdi: return ZERO block-status when appropriate
  block/vpc: return ZERO block-status when appropriate
  block/crypto: drop unallocated_blocks_are_zero
  block/iscsi: drop unallocated_blocks_are_zero
  block/file-posix: drop unallocated_blocks_are_zero
  block/vhdx: drop unallocated_blocks_are_zero
  qemu-img: convert: don't use unallocated_blocks_are_zero
  block: drop unallocated_blocks_are_zero

 include/block/block.h     |  6 ------
 include/block/block_int.h | 13 ++++++++++++-
 block.c                   | 15 ---------------
 block/crypto.c            |  1 -
 block/file-posix.c        |  3 ---
 block/io.c                |  8 ++++----
 block/iscsi.c             |  1 -
 block/qcow2.c             |  1 -
 block/qed.c               |  1 -
 block/vdi.c               |  3 +--
 block/vhdx.c              |  3 ---
 block/vpc.c               |  3 +--
 qemu-img.c                |  4 +---
 13 files changed, 19 insertions(+), 43 deletions(-)

-- 
2.21.0

[PATCH 1/8] block/vdi: return ZERO block-status when appropriate

[Vladimir Sementsov-Ogievskiy]

In case of !VDI_IS_ALLOCATED[], we do zero out the corresponding chunk
of qiov. So, this should be reported as ZERO.

After block-status update, it never reports 0, so setting
unallocated_blocks_are_zero doesn't make sense. Drop it.

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

diff --git a/block/vdi.c b/block/vdi.c
index 0c7835ae70..83471528d2 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -334,7 +334,6 @@ static int vdi_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
     logout("\n");
     bdi->cluster_size = s->block_size;
     bdi->vm_state_offset = 0;
-    bdi->unallocated_blocks_are_zero = true;
     return 0;
 }
 
@@ -536,7 +535,7 @@ static int coroutine_fn vdi_co_block_status(BlockDriverState *bs,
     *pnum = MIN(s->block_size - index_in_block, bytes);
     result = VDI_IS_ALLOCATED(bmap_entry);
     if (!result) {
-        return 0;
+        return BDRV_BLOCK_ZERO;
     }
 
     *map = s->header.offset_data + (uint64_t)bmap_entry * s->block_size +
-- 
2.21.0

[PATCH 2/8] block/vpc: return ZERO block-status when appropriate

[Vladimir Sementsov-Ogievskiy]

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

After block-status update, it never reports 0, so setting
unallocated_blocks_are_zero doesn't make sense. Drop it.

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

diff --git a/block/vpc.c b/block/vpc.c
index 2d1eade146..555f9d8587 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.21.0

[PATCH 3/8] block/crypto: drop unallocated_blocks_are_zero

[Vladimir Sementsov-Ogievskiy]

It's false by default, no needs to set it. We are going to drop this
variable at all, so drop it now here, it doesn't hurt.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/crypto.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/block/crypto.c b/block/crypto.c
index e02f343590..7685e61844 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -694,7 +694,6 @@ static int block_crypto_get_info_luks(BlockDriverState *bs,
         return ret;
     }
 
-    bdi->unallocated_blocks_are_zero = false;
     bdi->cluster_size = subbdi.cluster_size;
 
     return 0;
-- 
2.21.0

[PATCH 4/8] block/iscsi: drop unallocated_blocks_are_zero

[Vladimir Sementsov-Ogievskiy]

We set bdi->unallocated_blocks_are_zero = iscsilun->lbprz, but
iscsi_co_block_status doesn't return 0 in case of iscsilun->lbprz, it
returns ZERO when appropriate. So actually unallocated_blocks_are_zero
is useless. Drop it now.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/iscsi.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/block/iscsi.c b/block/iscsi.c
index a8b76979d8..767e3e75fd 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -2163,7 +2163,6 @@ static int coroutine_fn iscsi_co_truncate(BlockDriverState *bs, int64_t offset,
 static int iscsi_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
 {
     IscsiLun *iscsilun = bs->opaque;
-    bdi->unallocated_blocks_are_zero = iscsilun->lbprz;
     bdi->cluster_size = iscsilun->cluster_size;
     return 0;
 }
-- 
2.21.0

[PATCH 5/8] block/file-posix: drop unallocated_blocks_are_zero

[Vladimir Sementsov-Ogievskiy]

raw_co_block_status() in block/file-posix.c never returns 0, so
unallocated_blocks_are_zero is useless. Drop it.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/file-posix.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index 05e094be29..5c01735108 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -2878,9 +2878,6 @@ static int coroutine_fn raw_co_pwrite_zeroes(
 
 static int raw_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
 {
-    BDRVRawState *s = bs->opaque;
-
-    bdi->unallocated_blocks_are_zero = s->discard_zeroes;
     return 0;
 }
 
-- 
2.21.0

[PATCH 6/8] block/vhdx: drop unallocated_blocks_are_zero

[Vladimir Sementsov-Ogievskiy]

vhdx doesn't have .bdrv_co_block_status handler, so DATA|ALLOCATED is
always assumed for it. unallocated_blocks_are_zero is useless, drop it.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/vhdx.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/block/vhdx.c b/block/vhdx.c
index aedd782604..45963a3166 100644
--- a/block/vhdx.c
+++ b/block/vhdx.c
@@ -1164,9 +1164,6 @@ static int vhdx_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
 
     bdi->cluster_size = s->block_size;
 
-    bdi->unallocated_blocks_are_zero =
-        (s->params.data_bits & VHDX_PARAMS_HAS_PARENT) == 0;
-
     return 0;
 }
 
-- 
2.21.0

[PATCH 7/8] qemu-img: convert: don't use unallocated_blocks_are_zero

[Vladimir Sementsov-Ogievskiy]

qemu-img convert wants to distinguish ZERO which comes from short
backing files. unallocated_blocks_are_zero field of bdi is unrelated:
space after EOF is always considered to be zero anyway. So, just make
post_backing_zero true in case of short backing file.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 qemu-img.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index 6a4327aaba..4fe751878b 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1632,7 +1632,6 @@ typedef struct ImgConvertState {
     BlockBackend *target;
     bool has_zero_init;
     bool compressed;
-    bool unallocated_blocks_are_zero;
     bool target_is_new;
     bool target_has_backing;
     int64_t target_backing_sectors; /* negative if unknown */
@@ -1677,7 +1676,7 @@ static int convert_iteration_sectors(ImgConvertState *s, int64_t sector_num)
 
     if (s->target_backing_sectors >= 0) {
         if (sector_num >= s->target_backing_sectors) {
-            post_backing_zero = s->unallocated_blocks_are_zero;
+            post_backing_zero = true;
         } else if (sector_num + n > s->target_backing_sectors) {
             /* Split requests around target_backing_sectors (because
              * starting from there, zeros are handled differently) */
@@ -2580,7 +2579,6 @@ static int img_convert(int argc, char **argv)
     } else {
         s.compressed = s.compressed || bdi.needs_compressed_writes;
         s.cluster_sectors = bdi.cluster_size / BDRV_SECTOR_SIZE;
-        s.unallocated_blocks_are_zero = bdi.unallocated_blocks_are_zero;
     }
 
     ret = convert_do_copy(&s);
-- 
2.21.0

[PATCH 8/8] block: drop unallocated_blocks_are_zero

[Vladimir Sementsov-Ogievskiy]

Currently this field only set by qed and qcow2. But in fact, all
backing-supporting formats (parallels, qcow, qcow2, qed, vmdk) share
this semantics: on unallocated blocks, if there is no backing file they
just memset the buffer with zeroes.

So, document this behavior for .supports_backing and drop
.unallocated_blocks_are_zero

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 include/block/block.h     |  6 ------
 include/block/block_int.h | 13 ++++++++++++-
 block.c                   | 15 ---------------
 block/io.c                |  8 ++++----
 block/qcow2.c             |  1 -
 block/qed.c               |  1 -
 6 files changed, 16 insertions(+), 28 deletions(-)

diff --git a/include/block/block.h b/include/block/block.h
index 8b62429aa4..db1cb503ec 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -21,11 +21,6 @@ typedef struct BlockDriverInfo {
     /* offset at which the VM state can be saved (0 if not possible) */
     int64_t vm_state_offset;
     bool is_dirty;
-    /*
-     * True if unallocated blocks read back as zeroes. This is equivalent
-     * to the LBPRZ flag in the SCSI logical block provisioning page.
-     */
-    bool unallocated_blocks_are_zero;
     /*
      * True if this block driver only supports compressed writes
      */
@@ -431,7 +426,6 @@ int bdrv_co_pdiscard(BdrvChild *child, int64_t offset, int64_t bytes);
 int bdrv_has_zero_init_1(BlockDriverState *bs);
 int bdrv_has_zero_init(BlockDriverState *bs);
 int bdrv_has_zero_init_truncate(BlockDriverState *bs);
-bool bdrv_unallocated_blocks_are_zero(BlockDriverState *bs);
 bool bdrv_can_write_zeroes_with_unmap(BlockDriverState *bs);
 int bdrv_block_status(BlockDriverState *bs, int64_t offset,
                       int64_t bytes, int64_t *pnum, int64_t *map,
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 92335f33c7..c156b22c6b 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -115,7 +115,18 @@ struct BlockDriver {
      */
     bool bdrv_needs_filename;
 
-    /* Set if a driver can support backing files */
+    /*
+     * Set if a driver can support backing files. This also implies the
+     * following semantics:
+     *
+     *  - Return status 0 of .bdrv_co_block_status means that corresponding
+     *    blocks are not allocated in this layer of backing-chain
+     *  - For such (unallocated) blocks, read will:
+     *    - read from backing file if there is one and big enough
+     *    - fill buffer with zeroes if there is no backing file
+     *    - space after EOF of the backing file considered as zero
+     *      (corresponding part of read-buffer must be zeroed by driver)
+     */
     bool supports_backing;
 
     /* For handling image reopen for split or non-split files */
diff --git a/block.c b/block.c
index cf5c19b1db..0283fdecbc 100644
--- a/block.c
+++ b/block.c
@@ -5305,21 +5305,6 @@ int bdrv_has_zero_init_truncate(BlockDriverState *bs)
     return 0;
 }
 
-bool bdrv_unallocated_blocks_are_zero(BlockDriverState *bs)
-{
-    BlockDriverInfo bdi;
-
-    if (bs->backing) {
-        return false;
-    }
-
-    if (bdrv_get_info(bs, &bdi) == 0) {
-        return bdi.unallocated_blocks_are_zero;
-    }
-
-    return false;
-}
-
 bool bdrv_can_write_zeroes_with_unmap(BlockDriverState *bs)
 {
     if (!(bs->open_flags & BDRV_O_UNMAP)) {
diff --git a/block/io.c b/block/io.c
index a4f9714230..484adec5a1 100644
--- a/block/io.c
+++ b/block/io.c
@@ -2385,16 +2385,16 @@ static int coroutine_fn bdrv_co_block_status(BlockDriverState *bs,
 
     if (ret & (BDRV_BLOCK_DATA | BDRV_BLOCK_ZERO)) {
         ret |= BDRV_BLOCK_ALLOCATED;
-    } else if (want_zero) {
-        if (bdrv_unallocated_blocks_are_zero(bs)) {
-            ret |= BDRV_BLOCK_ZERO;
-        } else if (bs->backing) {
+    } else if (want_zero && bs->drv->supports_backing) {
+        if (bs->backing) {
             BlockDriverState *bs2 = bs->backing->bs;
             int64_t size2 = bdrv_getlength(bs2);
 
             if (size2 >= 0 && offset >= size2) {
                 ret |= BDRV_BLOCK_ZERO;
             }
+        } else {
+            ret |= BDRV_BLOCK_ZERO;
         }
     }
 
diff --git a/block/qcow2.c b/block/qcow2.c
index 2ba0b17c39..dc3c0aac2b 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -4858,7 +4858,6 @@ err:
 static int qcow2_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
 {
     BDRVQcow2State *s = bs->opaque;
-    bdi->unallocated_blocks_are_zero = true;
     bdi->cluster_size = s->cluster_size;
     bdi->vm_state_offset = qcow2_vm_state_offset(s);
     return 0;
diff --git a/block/qed.c b/block/qed.c
index b0fdb8f565..fb7833dc8b 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -1514,7 +1514,6 @@ static int bdrv_qed_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
     memset(bdi, 0, sizeof(*bdi));
     bdi->cluster_size = s->header.cluster_size;
     bdi->is_dirty = s->header.features & QED_F_NEED_CHECK;
-    bdi->unallocated_blocks_are_zero = true;
     return 0;
 }
 
-- 
2.21.0

Re: [PATCH 1/8] block/vdi: return ZERO block-status when appropriate

[Eric Blake]

On 5/6/20 4:25 AM, Vladimir Sementsov-Ogievskiy wrote:
> In case of !VDI_IS_ALLOCATED[], we do zero out the corresponding chunk
> of qiov. So, this should be reported as ZERO.

This part makes sense outright, since vdi does not allow backing files, 
so all reads of a VDI disk result in data rather than deferral to 
another layer, and we indeed read zero in this case.  However, it _is_ a 
behavior change: previously, 'qemu-io -c map' on a vdi image will show 
unallocated portions, but with this change, the entire image now shows 
as allocated.  You need to call out this fact in the commit message, 
documenting that it is intentional (it matches what we do for posix 
files: since neither files nor vdi support backing images, we guarantee 
that all read attempts will be satisfied by this layer rather than 
deferring to a backing layer, and thus can treat all data as allocated 
in this layer, regardless of whether it is sparse).

Do any existing iotests need an update to reflect change in output?  If 
not, should we have an iotest covering it?

> 
> After block-status update, it never reports 0, so setting
> unallocated_blocks_are_zero doesn't make sense. Drop it.

This is a harder claim. To prove it, we need to inspect all callers that 
use unallocated_blocks_are_zero, to see their intent.  Fortunately, the 
list is small - a mere 2 clients!

block/io.c:bdrv_co_block_status(): if .bdrv_co_block_status sets either 
_DATA or _ZERO, block status adds _ALLOCATED; but if the driver did not 
set either, we use bdrv_unallocated_blocks_are_zero() in order to set 
_ZERO but not _ALLOCATED.  This is the code that explains the behavior 
change mentioned above (now that vdi is reporting _ZERO instead of 0, 
block_status is appending _ALLOCATED instead of querying 
bdrv_unallocated_blocks_are_zero).  But you are correct that it does not 
change where _ZERO is reported.

qemu-img.c:img_convert(): calls bdrv_get_info() up front and caches what 
it learned from unallocated_blocks_are_zero about the target; later on, 
in convert_iteration_sectors(), it uses this information to optimize the 
case where the source reads as zero, but the target has a backing file 
and the range being written lies beyond the end of the backing file. 
That is, given the following scenario:

qemu-img convert input -B backing output
input:   ABCD-0E
backing: ACBD

the optimization lets us produce:
output:  -BC---E

instead of the larger:
output:  -BC--0E

Do we have any iotests that cover this particular scenario, to prove 
that our optimization does indeed result in a smaller image file without 
explicit zeros written in the tail?  Or put another way, if we were to 
disable the post_backing_zero optimization in 
convert_iteration_sectors(), would any iotests fail?  If not, we should 
really enhance our testsuite.

With that said, we could just as easily achieve the optimization of the 
conversion to the tail of a destination with short backing file by 
checking block_status to see whether the tail already reads as all 
zeroes, rather than relying on unallocated_blocks_are_zero.  Even if 
querying block_status is a slight pessimization in time, it would avoid 
regressing in the more important factor of artificially bloating the 
destination.  I would _really_ love to see a patch to qemu-img.c 
reworking the logic to avoid the use of unallocated_blocks_are_zero 
first, prior to removing the setting of that field in the various 
drivers.  Once bdrv_co_block_status() remains as the only client of the 
field, then I could accept this patch with a better commit message about 
the intentional change in block_status _ALLOCATION behavior.

> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   block/vdi.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/block/vdi.c b/block/vdi.c
> index 0c7835ae70..83471528d2 100644
> --- a/block/vdi.c
> +++ b/block/vdi.c
> @@ -334,7 +334,6 @@ static int vdi_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
>       logout("\n");
>       bdi->cluster_size = s->block_size;
>       bdi->vm_state_offset = 0;
> -    bdi->unallocated_blocks_are_zero = true;
>       return 0;
>   }
>   
> @@ -536,7 +535,7 @@ static int coroutine_fn vdi_co_block_status(BlockDriverState *bs,
>       *pnum = MIN(s->block_size - index_in_block, bytes);
>       result = VDI_IS_ALLOCATED(bmap_entry);
>       if (!result) {
> -        return 0;
> +        return BDRV_BLOCK_ZERO;
>       }
>   
>       *map = s->header.offset_data + (uint64_t)bmap_entry * s->block_size +
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 1/8] block/vdi: return ZERO block-status when appropriate

[Vladimir Sementsov-Ogievskiy]

06.05.2020 16:57, Eric Blake wrote:
> On 5/6/20 4:25 AM, Vladimir Sementsov-Ogievskiy wrote:
>> In case of !VDI_IS_ALLOCATED[], we do zero out the corresponding chunk
>> of qiov. So, this should be reported as ZERO.
> 
> This part makes sense outright, since vdi does not allow backing files, so all reads of a VDI disk result in data rather than deferral to another layer, and we indeed read zero in this case.  However, it _is_ a behavior change: previously, 'qemu-io -c map' on a vdi image will show unallocated portions, but with this change, the entire image now shows as allocated.  You need to call out this fact in the commit message, documenting that it is intentional (it matches what we do for posix files: since neither files nor vdi support backing images, we guarantee that all read attempts will be satisfied by this layer rather than deferring to a backing layer, and thus can treat all data as allocated in this layer, regardless of whether it is sparse).
> 
> Do any existing iotests need an update to reflect change in output?  If not, should we have an iotest covering it?

all passed for me on tmpfs (with some skips)..

> 
>>
>> After block-status update, it never reports 0, so setting
>> unallocated_blocks_are_zero doesn't make sense. Drop it.
> 
> This is a harder claim. To prove it, we need to inspect all callers that use unallocated_blocks_are_zero, to see their intent.  Fortunately, the list is small - a mere 2 clients!
> 
> block/io.c:bdrv_co_block_status(): if .bdrv_co_block_status sets either _DATA or _ZERO, block status adds _ALLOCATED; but if the driver did not set either, we use bdrv_unallocated_blocks_are_zero() in order to set _ZERO but not _ALLOCATED.  This is the code that explains the behavior change mentioned above (now that vdi is reporting _ZERO instead of 0, block_status is appending _ALLOCATED instead of querying bdrv_unallocated_blocks_are_zero).  But you are correct that it does not change where _ZERO is reported.
> 
> qemu-img.c:img_convert(): calls bdrv_get_info() up front and caches what it learned from unallocated_blocks_are_zero about the target; later on, in convert_iteration_sectors(), it uses this information to optimize the case where the source reads as zero, but the target has a backing file and the range being written lies beyond the end of the backing file. That is, given the following scenario:
> 
> qemu-img convert input -B backing output
> input:   ABCD-0E
> backing: ACBD
> 
> the optimization lets us produce:
> output:  -BC---E
> 
> instead of the larger:
> output:  -BC--0E
> 
> Do we have any iotests that cover this particular scenario, to prove that our optimization does indeed result in a smaller image file without explicit zeros written in the tail?  Or put another way, if we were to disable the post_backing_zero optimization in convert_iteration_sectors(), would any iotests fail?  If not, we should really enhance our testsuite.
> 
> With that said, we could just as easily achieve the optimization of the conversion to the tail of a destination with short backing file by checking block_status to see whether the tail already reads as all zeroes, rather than relying on unallocated_blocks_are_zero.  Even if querying block_status is a slight pessimization in time, it would avoid regressing in the more important factor of artificially bloating the destination.  I would _really_ love to see a patch to qemu-img.c reworking the logic to avoid the use of unallocated_blocks_are_zero first, prior to removing the setting of that field in the various drivers.  Once bdrv_co_block_status() remains as the only client of the field, then I could accept this patch with a better commit message about the intentional change in block_status _ALLOCATION behavior.

Actually unallocated_blocks_are_zero doesn't influence on this thing, you should not check unallocated_blocks_are_zero to understand how to read unallocated area above short backing (after backing EOF). It's always reads as zeroes. So, patch 7 just use "true" instead. But yes, I'd better move patch 7 to be the first one.

> 
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   block/vdi.c | 3 +--
>>   1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/block/vdi.c b/block/vdi.c
>> index 0c7835ae70..83471528d2 100644
>> --- a/block/vdi.c
>> +++ b/block/vdi.c
>> @@ -334,7 +334,6 @@ static int vdi_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
>>       logout("\n");
>>       bdi->cluster_size = s->block_size;
>>       bdi->vm_state_offset = 0;
>> -    bdi->unallocated_blocks_are_zero = true;
>>       return 0;
>>   }
>> @@ -536,7 +535,7 @@ static int coroutine_fn vdi_co_block_status(BlockDriverState *bs,
>>       *pnum = MIN(s->block_size - index_in_block, bytes);
>>       result = VDI_IS_ALLOCATED(bmap_entry);
>>       if (!result) {
>> -        return 0;
>> +        return BDRV_BLOCK_ZERO;
>>       }
>>       *map = s->header.offset_data + (uint64_t)bmap_entry * s->block_size +
>>
> 


-- 
Best regards,
Vladimir

Re: [PATCH 7/8] qemu-img: convert: don't use unallocated_blocks_are_zero

[Eric Blake]

On 5/6/20 4:25 AM, Vladimir Sementsov-Ogievskiy wrote:
> qemu-img convert wants to distinguish ZERO which comes from short
> backing files. unallocated_blocks_are_zero field of bdi is unrelated:
> space after EOF is always considered to be zero anyway. So, just make
> post_backing_zero true in case of short backing file.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   qemu-img.c | 4 +---
>   1 file changed, 1 insertion(+), 3 deletions(-)

This patch should come first in the series.  It would have saved me a 
lot of time in reviewing 1/8.

> 
> diff --git a/qemu-img.c b/qemu-img.c
> index 6a4327aaba..4fe751878b 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -1632,7 +1632,6 @@ typedef struct ImgConvertState {
>       BlockBackend *target;
>       bool has_zero_init;
>       bool compressed;
> -    bool unallocated_blocks_are_zero;
>       bool target_is_new;
>       bool target_has_backing;
>       int64_t target_backing_sectors; /* negative if unknown */
> @@ -1677,7 +1676,7 @@ static int convert_iteration_sectors(ImgConvertState *s, int64_t sector_num)
>   
>       if (s->target_backing_sectors >= 0) {
>           if (sector_num >= s->target_backing_sectors) {
> -            post_backing_zero = s->unallocated_blocks_are_zero;
> +            post_backing_zero = true;
>           } else if (sector_num + n > s->target_backing_sectors) {
>               /* Split requests around target_backing_sectors (because
>                * starting from there, zeros are handled differently) */
> @@ -2580,7 +2579,6 @@ static int img_convert(int argc, char **argv)
>       } else {
>           s.compressed = s.compressed || bdi.needs_compressed_writes;
>           s.cluster_sectors = bdi.cluster_size / BDRV_SECTOR_SIZE;
> -        s.unallocated_blocks_are_zero = bdi.unallocated_blocks_are_zero;
>       }

My question in 1/8 about whether we have iotest coverage of this 
optimization remains, but I concur that the block layer takes care of 
reading zero when encountering unallocated blocks beyond EOF of a short 
backing file, and therefore performing this optimization always rather 
than just when the driver claims that unallocated blocks read as zero 
should be safe.

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 7/8] qemu-img: convert: don't use unallocated_blocks_are_zero

[Vladimir Sementsov-Ogievskiy]

06.05.2020 17:49, Eric Blake wrote:
> On 5/6/20 4:25 AM, Vladimir Sementsov-Ogievskiy wrote:
>> qemu-img convert wants to distinguish ZERO which comes from short
>> backing files. unallocated_blocks_are_zero field of bdi is unrelated:
>> space after EOF is always considered to be zero anyway. So, just make
>> post_backing_zero true in case of short backing file.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   qemu-img.c | 4 +---
>>   1 file changed, 1 insertion(+), 3 deletions(-)
> 
> This patch should come first in the series.  It would have saved me a lot of time in reviewing 1/8.

I'm sorry :(

> 
>>
>> diff --git a/qemu-img.c b/qemu-img.c
>> index 6a4327aaba..4fe751878b 100644
>> --- a/qemu-img.c
>> +++ b/qemu-img.c
>> @@ -1632,7 +1632,6 @@ typedef struct ImgConvertState {
>>       BlockBackend *target;
>>       bool has_zero_init;
>>       bool compressed;
>> -    bool unallocated_blocks_are_zero;
>>       bool target_is_new;
>>       bool target_has_backing;
>>       int64_t target_backing_sectors; /* negative if unknown */
>> @@ -1677,7 +1676,7 @@ static int convert_iteration_sectors(ImgConvertState *s, int64_t sector_num)
>>       if (s->target_backing_sectors >= 0) {
>>           if (sector_num >= s->target_backing_sectors) {
>> -            post_backing_zero = s->unallocated_blocks_are_zero;
>> +            post_backing_zero = true;
>>           } else if (sector_num + n > s->target_backing_sectors) {
>>               /* Split requests around target_backing_sectors (because
>>                * starting from there, zeros are handled differently) */
>> @@ -2580,7 +2579,6 @@ static int img_convert(int argc, char **argv)
>>       } else {
>>           s.compressed = s.compressed || bdi.needs_compressed_writes;
>>           s.cluster_sectors = bdi.cluster_size / BDRV_SECTOR_SIZE;
>> -        s.unallocated_blocks_are_zero = bdi.unallocated_blocks_are_zero;
>>       }
> 
> My question in 1/8 about whether we have iotest coverage of this optimization remains, but I concur that the block layer takes care of reading zero when encountering unallocated blocks beyond EOF of a short backing file, and therefore performing this optimization always rather than just when the driver claims that unallocated blocks read as zero should be safe.
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 

Thanks!

-- 
Best regards,
Vladimir

Re: [PATCH 8/8] block: drop unallocated_blocks_are_zero

[Eric Blake]

On 5/6/20 4:25 AM, Vladimir Sementsov-Ogievskiy wrote:
> Currently this field only set by qed and qcow2.

Well, only after patches 1-6 (prior to then, it was also set in protocol 
drivers).  I think you might be able to hoist part of this patch earlier 
in the series, to make the changes to the protocol drivers easier to 
review, by rewording this sentence:

Currently, the only format drivers that set this field are qed and qcow2.

> But in fact, all
> backing-supporting formats (parallels, qcow, qcow2, qed, vmdk) share
> this semantics: on unallocated blocks, if there is no backing file they

s/this/these/

> just memset the buffer with zeroes.
> 
> So, document this behavior for .supports_backing and drop
> .unallocated_blocks_are_zero
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   include/block/block.h     |  6 ------
>   include/block/block_int.h | 13 ++++++++++++-
>   block.c                   | 15 ---------------
>   block/io.c                |  8 ++++----
>   block/qcow2.c             |  1 -
>   block/qed.c               |  1 -
>   6 files changed, 16 insertions(+), 28 deletions(-)
> 
> diff --git a/include/block/block.h b/include/block/block.h
> index 8b62429aa4..db1cb503ec 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -21,11 +21,6 @@ typedef struct BlockDriverInfo {
>       /* offset at which the VM state can be saved (0 if not possible) */
>       int64_t vm_state_offset;
>       bool is_dirty;
> -    /*
> -     * True if unallocated blocks read back as zeroes. This is equivalent
> -     * to the LBPRZ flag in the SCSI logical block provisioning page.
> -     */
> -    bool unallocated_blocks_are_zero;

You can't delete this field until all protocol drivers are cleaned up, 
so deferring this part of the change to the end of the series makes sense.

>       /*
>        * True if this block driver only supports compressed writes
>        */
> @@ -431,7 +426,6 @@ int bdrv_co_pdiscard(BdrvChild *child, int64_t offset, int64_t bytes);
>   int bdrv_has_zero_init_1(BlockDriverState *bs);
>   int bdrv_has_zero_init(BlockDriverState *bs);
>   int bdrv_has_zero_init_truncate(BlockDriverState *bs);
> -bool bdrv_unallocated_blocks_are_zero(BlockDriverState *bs);

Doing this cleanup makes sense: there is only one caller of this 
function pre-patch, and 0 callers post-patch - but whether the cleanup 
should be at the same time as you fix the one caller, or deferred to 
when you also clean up the field, is less important.

If I were writing the series:

1 - fix qemu-img.c to not use the field
2 - fix block_status to not use the function
3-n - fix protocol drivers that set the field to also return _ZERO
  during block status (but not delete the field at that time)
n+1 - delete unused function and field (from ALL drivers)

>   bool bdrv_can_write_zeroes_with_unmap(BlockDriverState *bs);
>   int bdrv_block_status(BlockDriverState *bs, int64_t offset,
>                         int64_t bytes, int64_t *pnum, int64_t *map,
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 92335f33c7..c156b22c6b 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -115,7 +115,18 @@ struct BlockDriver {
>        */
>       bool bdrv_needs_filename;
>   
> -    /* Set if a driver can support backing files */
> +    /*
> +     * Set if a driver can support backing files. This also implies the
> +     * following semantics:
> +     *
> +     *  - Return status 0 of .bdrv_co_block_status means that corresponding
> +     *    blocks are not allocated in this layer of backing-chain
> +     *  - For such (unallocated) blocks, read will:
> +     *    - read from backing file if there is one and big enough

s/and/and it is/

> +     *    - fill buffer with zeroes if there is no backing file
> +     *    - space after EOF of the backing file considered as zero
> +     *      (corresponding part of read-buffer must be zeroed by driver)

Does the driver actually have to do the zeroing?  Looking at qcow2.c, I see:
static coroutine_fn int qcow2_co_preadv_task(BlockDriverState *bs,
...

     case QCOW2_CLUSTER_UNALLOCATED:
         assert(bs->backing); /* otherwise handled in 
qcow2_co_preadv_part */

         BLKDBG_EVENT(bs->file, BLKDBG_READ_BACKING_AIO);
         return bdrv_co_preadv_part(bs->backing, offset, bytes,
                                    qiov, qiov_offset, 0);

which just defers to the block layer to handle read-beyond-EOF, rather 
than an explicit memset in the driver.

So maybe you can simplify to:
- For such (unallocated) blocks, read will:
   - fill buffer with zeros if there is no backing file
   - read from the backing file otherwise, where the block layer
     takes care of reading zeros beyond EOF if backing file is short

But the effect is the same.

> +++ b/block/io.c
> @@ -2385,16 +2385,16 @@ static int coroutine_fn bdrv_co_block_status(BlockDriverState *bs,
>   
>       if (ret & (BDRV_BLOCK_DATA | BDRV_BLOCK_ZERO)) {
>           ret |= BDRV_BLOCK_ALLOCATED;
> -    } else if (want_zero) {
> -        if (bdrv_unallocated_blocks_are_zero(bs)) {
> -            ret |= BDRV_BLOCK_ZERO;
> -        } else if (bs->backing) {
> +    } else if (want_zero && bs->drv->supports_backing) {
> +        if (bs->backing) {
>               BlockDriverState *bs2 = bs->backing->bs;
>               int64_t size2 = bdrv_getlength(bs2);
>   
>               if (size2 >= 0 && offset >= size2) {
>                   ret |= BDRV_BLOCK_ZERO;
>               }
> +        } else {
> +            ret |= BDRV_BLOCK_ZERO;
>           }

I like this part of the change.  But if it is done first in the series, 
it _does_ have a semantic impact on protocol drivers (previously, 
protocol drivers that return 0 but set the field 
.unallocated_blocks_are_zero will be changed to report _ZERO; after this 
patch, protocol drivers do not do that, because they don't support 
backing files, and it is now only backing files that do the _ZERO 
magic).  So doing _just_ this change, along with a better analysis of 
how it changes the semantics of 'qemu-io -c map' on protocol drivers 
while mentioning why that is okay, would make a better start to the 
series, rather than here at the end.  Of course, if you defer it to the 
end, then none of the protocol drivers set .unallocated_blocks_are_zero 
anyway, but that cost more review work on each altered protocol driver.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 8/8] block: drop unallocated_blocks_are_zero

[Vladimir Sementsov-Ogievskiy]

06.05.2020 18:14, Eric Blake wrote:
> On 5/6/20 4:25 AM, Vladimir Sementsov-Ogievskiy wrote:
>> Currently this field only set by qed and qcow2.
> 
> Well, only after patches 1-6 (prior to then, it was also set in protocol drivers).  I think you might be able to hoist part of this patch earlier in the series, to make the changes to the protocol drivers easier to review, by rewording this sentence:
> 
> Currently, the only format drivers that set this field are qed and qcow2.
> 
>> But in fact, all
>> backing-supporting formats (parallels, qcow, qcow2, qed, vmdk) share
>> this semantics: on unallocated blocks, if there is no backing file they
> 
> s/this/these/
> 
>> just memset the buffer with zeroes.
>>
>> So, document this behavior for .supports_backing and drop
>> .unallocated_blocks_are_zero
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   include/block/block.h     |  6 ------
>>   include/block/block_int.h | 13 ++++++++++++-
>>   block.c                   | 15 ---------------
>>   block/io.c                |  8 ++++----
>>   block/qcow2.c             |  1 -
>>   block/qed.c               |  1 -
>>   6 files changed, 16 insertions(+), 28 deletions(-)
>>
>> diff --git a/include/block/block.h b/include/block/block.h
>> index 8b62429aa4..db1cb503ec 100644
>> --- a/include/block/block.h
>> +++ b/include/block/block.h
>> @@ -21,11 +21,6 @@ typedef struct BlockDriverInfo {
>>       /* offset at which the VM state can be saved (0 if not possible) */
>>       int64_t vm_state_offset;
>>       bool is_dirty;
>> -    /*
>> -     * True if unallocated blocks read back as zeroes. This is equivalent
>> -     * to the LBPRZ flag in the SCSI logical block provisioning page.
>> -     */
>> -    bool unallocated_blocks_are_zero;
> 
> You can't delete this field until all protocol drivers are cleaned up, so deferring this part of the change to the end of the series makes sense.
> 
>>       /*
>>        * True if this block driver only supports compressed writes
>>        */
>> @@ -431,7 +426,6 @@ int bdrv_co_pdiscard(BdrvChild *child, int64_t offset, int64_t bytes);
>>   int bdrv_has_zero_init_1(BlockDriverState *bs);
>>   int bdrv_has_zero_init(BlockDriverState *bs);
>>   int bdrv_has_zero_init_truncate(BlockDriverState *bs);
>> -bool bdrv_unallocated_blocks_are_zero(BlockDriverState *bs);
> 
> Doing this cleanup makes sense: there is only one caller of this function pre-patch, and 0 callers post-patch - but whether the cleanup should be at the same time as you fix the one caller, or deferred to when you also clean up the field, is less important.
> 
> If I were writing the series:
> 
> 1 - fix qemu-img.c to not use the field
> 2 - fix block_status to not use the function
> 3-n - fix protocol drivers that set the field to also return _ZERO
>   during block status (but not delete the field at that time)
> n+1 - delete unused function and field (from ALL drivers)
> 
>>   bool bdrv_can_write_zeroes_with_unmap(BlockDriverState *bs);
>>   int bdrv_block_status(BlockDriverState *bs, int64_t offset,
>>                         int64_t bytes, int64_t *pnum, int64_t *map,
>> diff --git a/include/block/block_int.h b/include/block/block_int.h
>> index 92335f33c7..c156b22c6b 100644
>> --- a/include/block/block_int.h
>> +++ b/include/block/block_int.h
>> @@ -115,7 +115,18 @@ struct BlockDriver {
>>        */
>>       bool bdrv_needs_filename;
>> -    /* Set if a driver can support backing files */
>> +    /*
>> +     * Set if a driver can support backing files. This also implies the
>> +     * following semantics:
>> +     *
>> +     *  - Return status 0 of .bdrv_co_block_status means that corresponding
>> +     *    blocks are not allocated in this layer of backing-chain
>> +     *  - For such (unallocated) blocks, read will:
>> +     *    - read from backing file if there is one and big enough
> 
> s/and/and it is/
> 
>> +     *    - fill buffer with zeroes if there is no backing file
>> +     *    - space after EOF of the backing file considered as zero
>> +     *      (corresponding part of read-buffer must be zeroed by driver)
> 
> Does the driver actually have to do the zeroing?  Looking at qcow2.c, I see:
> static coroutine_fn int qcow2_co_preadv_task(BlockDriverState *bs,
> ...
> 
>      case QCOW2_CLUSTER_UNALLOCATED:
>          assert(bs->backing); /* otherwise handled in qcow2_co_preadv_part */
> 
>          BLKDBG_EVENT(bs->file, BLKDBG_READ_BACKING_AIO);
>          return bdrv_co_preadv_part(bs->backing, offset, bytes,
>                                     qiov, qiov_offset, 0);
> 
> which just defers to the block layer to handle read-beyond-EOF, rather than an explicit memset in the driver.

Hmm, yes.

> 
> So maybe you can simplify to:
> - For such (unallocated) blocks, read will:
>    - fill buffer with zeros if there is no backing file
>    - read from the backing file otherwise, where the block layer
>      takes care of reading zeros beyond EOF if backing file is short
> 

OK

> But the effect is the same.
> 
>> +++ b/block/io.c
>> @@ -2385,16 +2385,16 @@ static int coroutine_fn bdrv_co_block_status(BlockDriverState *bs,
>>       if (ret & (BDRV_BLOCK_DATA | BDRV_BLOCK_ZERO)) {
>>           ret |= BDRV_BLOCK_ALLOCATED;
>> -    } else if (want_zero) {
>> -        if (bdrv_unallocated_blocks_are_zero(bs)) {
>> -            ret |= BDRV_BLOCK_ZERO;
>> -        } else if (bs->backing) {
>> +    } else if (want_zero && bs->drv->supports_backing) {
>> +        if (bs->backing) {
>>               BlockDriverState *bs2 = bs->backing->bs;
>>               int64_t size2 = bdrv_getlength(bs2);
>>               if (size2 >= 0 && offset >= size2) {
>>                   ret |= BDRV_BLOCK_ZERO;
>>               }
>> +        } else {
>> +            ret |= BDRV_BLOCK_ZERO;
>>           }
> 
> I like this part of the change.  But if it is done first in the series, it _does_ have a semantic impact on protocol drivers (previously, protocol drivers that return 0 but set the field .unallocated_blocks_are_zero will be changed to report _ZERO; after this patch, protocol drivers do not do that, because they don't support backing files, and it is now only backing files that do the _ZERO magic).  So doing _just_ this change, along with a better analysis of how it changes the semantics of 'qemu-io -c map' on protocol drivers while mentioning why that is okay, would make a better start to the series, rather than here at the end.  Of course, if you defer it to the end, then none of the protocol drivers set .unallocated_blocks_are_zero anyway, but that cost more review work on each altered protocol driver.
> 

I understand the idea.. Ha, it looks like an optimization issue :) I use greedy algorithm, trying to make each patch to be a simple small step to the result. But greedy algorithm now always optimal as we know. Actually, here, making first step larger and more complicated, we achieve less total review-complexity. Good new experience for me, thanks, I'll try the proposed way.



-- 
Best regards,
Vladimir

Re: [PATCH 8/8] block: drop unallocated_blocks_are_zero

[Eric Blake]

On 5/6/20 4:25 AM, Vladimir Sementsov-Ogievskiy wrote:
> Currently this field only set by qed and qcow2. But in fact, all
> backing-supporting formats (parallels, qcow, qcow2, qed, vmdk) share
> this semantics: on unallocated blocks, if there is no backing file they
> just memset the buffer with zeroes.

In checking the behavior of all 5 .supports_backing drivers, I noticed 
that qed.c:qed_read_backing_file() does a lot of redundant work in 
computing the backing file size itself, when it could just defer to the 
block layer like all the other drivers do.  That would be a separate patch.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 2/8] block/vpc: return ZERO block-status when appropriate

[Eric Blake]

On 5/6/20 4:25 AM, 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.
> 
> After block-status update, it never reports 0, so setting
> unallocated_blocks_are_zero doesn't make sense. Drop it.

Same analysis as in patch 1 as to the lone two clients that cared, and 
the fact that we are changing 'qemu-io -c map' output by reporting data 
as allocated now.  But I concur that as there is never a backing file, 
the change is not a regression, but rather a bug fix.

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

While the commit message could be improved, the code change itself looks 
correct.

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 3/8] block/crypto: drop unallocated_blocks_are_zero

[Eric Blake]

On 5/6/20 4:25 AM, Vladimir Sementsov-Ogievskiy wrote:
> It's false by default, no needs to set it. We are going to drop this
> variable at all, so drop it now here, it doesn't hurt.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   block/crypto.c | 1 -
>   1 file changed, 1 deletion(-)

Trivially correct, regardless of clients.

Reviewed-by: Eric Blake <eblake@redhat.com>

> 
> diff --git a/block/crypto.c b/block/crypto.c
> index e02f343590..7685e61844 100644
> --- a/block/crypto.c
> +++ b/block/crypto.c
> @@ -694,7 +694,6 @@ static int block_crypto_get_info_luks(BlockDriverState *bs,
>           return ret;
>       }
>   
> -    bdi->unallocated_blocks_are_zero = false;
>       bdi->cluster_size = subbdi.cluster_size;
>   
>       return 0;
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 4/8] block/iscsi: drop unallocated_blocks_are_zero

[Eric Blake]

On 5/6/20 4:25 AM, Vladimir Sementsov-Ogievskiy wrote:
> We set bdi->unallocated_blocks_are_zero = iscsilun->lbprz, but
> iscsi_co_block_status doesn't return 0 in case of iscsilun->lbprz, it
> returns ZERO when appropriate. So actually unallocated_blocks_are_zero
> is useless. Drop it now.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   block/iscsi.c | 1 -
>   1 file changed, 1 deletion(-)

This one is easier to justify after removing the 2 clients.  But it's 
simpler than patch 1 in that because block_status never returned 0, this 
has no visible impact to 'qemu-io -c map' or similar, so it doesn't need 
the commit message justification about any change in behavior like patch 
1 needed.

Reviewed-by: Eric Blake <eblake@redhat.com>

> 
> diff --git a/block/iscsi.c b/block/iscsi.c
> index a8b76979d8..767e3e75fd 100644
> --- a/block/iscsi.c
> +++ b/block/iscsi.c
> @@ -2163,7 +2163,6 @@ static int coroutine_fn iscsi_co_truncate(BlockDriverState *bs, int64_t offset,
>   static int iscsi_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
>   {
>       IscsiLun *iscsilun = bs->opaque;
> -    bdi->unallocated_blocks_are_zero = iscsilun->lbprz;
>       bdi->cluster_size = iscsilun->cluster_size;
>       return 0;
>   }
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 5/8] block/file-posix: drop unallocated_blocks_are_zero

[Eric Blake]

On 5/6/20 4:25 AM, Vladimir Sementsov-Ogievskiy wrote:
> raw_co_block_status() in block/file-posix.c never returns 0, so
> unallocated_blocks_are_zero is useless. Drop it.

As in 4/8, you are correct that it had no impact on block_status, but it 
did affect qemu-img convert.  So again, removing the clients first makes 
this easier to justify as a cleanup patch.

That said...

> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   block/file-posix.c | 3 ---
>   1 file changed, 3 deletions(-)
> 
> diff --git a/block/file-posix.c b/block/file-posix.c
> index 05e094be29..5c01735108 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -2878,9 +2878,6 @@ static int coroutine_fn raw_co_pwrite_zeroes(
>   
>   static int raw_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
>   {
> -    BDRVRawState *s = bs->opaque;
> -
> -    bdi->unallocated_blocks_are_zero = s->discard_zeroes;
>       return 0;
>   }

the function now does nothing.  Hmm - why does bdrv_get_info() return 
-ENOTSUP if the driver does not implement this function?  Wouldn't it be 
better if the block layer could allow us to omit .bdrv_get_info and do 
the same thing, without us having to write a dummy function that does 
nothing but return 0?  As separate patches, of course, as it would 
require changing several existing bdrv_get_info() callers to behave 
sanely when getting an all-0 success return where they now deal with an 
-ENOTSUP return.

So in the meantime, yes, we need this placeholder.
Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 6/8] block/vhdx: drop unallocated_blocks_are_zero

[Eric Blake]

On 5/6/20 4:25 AM, Vladimir Sementsov-Ogievskiy wrote:
> vhdx doesn't have .bdrv_co_block_status handler, so DATA|ALLOCATED is
> always assumed for it. unallocated_blocks_are_zero is useless, drop it.
> 

After the analysis I did in patch 1, this is correct.  No behavior change.

Reviewed-by: Eric Blake <eblake@redhat.com>

> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   block/vhdx.c | 3 ---
>   1 file changed, 3 deletions(-)
> 
> diff --git a/block/vhdx.c b/block/vhdx.c
> index aedd782604..45963a3166 100644
> --- a/block/vhdx.c
> +++ b/block/vhdx.c
> @@ -1164,9 +1164,6 @@ static int vhdx_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
>   
>       bdi->cluster_size = s->block_size;
>   
> -    bdi->unallocated_blocks_are_zero =
> -        (s->params.data_bits & VHDX_PARAMS_HAS_PARENT) == 0;
> -
>       return 0;
>   }
>   
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 8/8] block: drop unallocated_blocks_are_zero

[Vladimir Sementsov-Ogievskiy]

06.05.2020 18:14, Eric Blake wrote:
> On 5/6/20 4:25 AM, Vladimir Sementsov-Ogievskiy wrote:
>> Currently this field only set by qed and qcow2.
> 
> Well, only after patches 1-6 (prior to then, it was also set in protocol drivers).  I think you might be able to hoist part of this patch earlier in the series, to make the changes to the protocol drivers easier to review, by rewording this sentence:
> 
> Currently, the only format drivers that set this field are qed and qcow2.
> 
>> But in fact, all
>> backing-supporting formats (parallels, qcow, qcow2, qed, vmdk) share
>> this semantics: on unallocated blocks, if there is no backing file they
> 
> s/this/these/
> 
>> just memset the buffer with zeroes.
>>
>> So, document this behavior for .supports_backing and drop
>> .unallocated_blocks_are_zero
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   include/block/block.h     |  6 ------
>>   include/block/block_int.h | 13 ++++++++++++-
>>   block.c                   | 15 ---------------
>>   block/io.c                |  8 ++++----
>>   block/qcow2.c             |  1 -
>>   block/qed.c               |  1 -
>>   6 files changed, 16 insertions(+), 28 deletions(-)
>>
>> diff --git a/include/block/block.h b/include/block/block.h
>> index 8b62429aa4..db1cb503ec 100644
>> --- a/include/block/block.h
>> +++ b/include/block/block.h
>> @@ -21,11 +21,6 @@ typedef struct BlockDriverInfo {
>>       /* offset at which the VM state can be saved (0 if not possible) */
>>       int64_t vm_state_offset;
>>       bool is_dirty;
>> -    /*
>> -     * True if unallocated blocks read back as zeroes. This is equivalent
>> -     * to the LBPRZ flag in the SCSI logical block provisioning page.
>> -     */
>> -    bool unallocated_blocks_are_zero;
> 
> You can't delete this field until all protocol drivers are cleaned up, so deferring this part of the change to the end of the series makes sense.
> 
>>       /*
>>        * True if this block driver only supports compressed writes
>>        */
>> @@ -431,7 +426,6 @@ int bdrv_co_pdiscard(BdrvChild *child, int64_t offset, int64_t bytes);
>>   int bdrv_has_zero_init_1(BlockDriverState *bs);
>>   int bdrv_has_zero_init(BlockDriverState *bs);
>>   int bdrv_has_zero_init_truncate(BlockDriverState *bs);
>> -bool bdrv_unallocated_blocks_are_zero(BlockDriverState *bs);
> 
> Doing this cleanup makes sense: there is only one caller of this function pre-patch, and 0 callers post-patch - but whether the cleanup should be at the same time as you fix the one caller, or deferred to when you also clean up the field, is less important.
> 
> If I were writing the series:
> 
> 1 - fix qemu-img.c to not use the field
> 2 - fix block_status to not use the function

Hmm stop. We still need patches 1,2 before modifying block_status, otherwise we'll still need to check unallocated_blocks_are_zero

> 3-n - fix protocol drivers that set the field to also return _ZERO
>   during block status (but not delete the field at that time)
> n+1 - delete unused function and field (from ALL drivers)
> 
>>   bool bdrv_can_write_zeroes_with_unmap(BlockDriverState *bs);
>>   int bdrv_block_status(BlockDriverState *bs, int64_t offset,
>>                         int64_t bytes, int64_t *pnum, int64_t *map,
>> diff --git a/include/block/block_int.h b/include/block/block_int.h
>> index 92335f33c7..c156b22c6b 100644
>> --- a/include/block/block_int.h
>> +++ b/include/block/block_int.h
>> @@ -115,7 +115,18 @@ struct BlockDriver {
>>        */
>>       bool bdrv_needs_filename;
>> -    /* Set if a driver can support backing files */
>> +    /*
>> +     * Set if a driver can support backing files. This also implies the
>> +     * following semantics:
>> +     *
>> +     *  - Return status 0 of .bdrv_co_block_status means that corresponding
>> +     *    blocks are not allocated in this layer of backing-chain
>> +     *  - For such (unallocated) blocks, read will:
>> +     *    - read from backing file if there is one and big enough
> 
> s/and/and it is/
> 
>> +     *    - fill buffer with zeroes if there is no backing file
>> +     *    - space after EOF of the backing file considered as zero
>> +     *      (corresponding part of read-buffer must be zeroed by driver)
> 
> Does the driver actually have to do the zeroing?  Looking at qcow2.c, I see:
> static coroutine_fn int qcow2_co_preadv_task(BlockDriverState *bs,
> ...
> 
>      case QCOW2_CLUSTER_UNALLOCATED:
>          assert(bs->backing); /* otherwise handled in qcow2_co_preadv_part */
> 
>          BLKDBG_EVENT(bs->file, BLKDBG_READ_BACKING_AIO);
>          return bdrv_co_preadv_part(bs->backing, offset, bytes,
>                                     qiov, qiov_offset, 0);
> 
> which just defers to the block layer to handle read-beyond-EOF, rather than an explicit memset in the driver.
> 
> So maybe you can simplify to:
> - For such (unallocated) blocks, read will:
>    - fill buffer with zeros if there is no backing file
>    - read from the backing file otherwise, where the block layer
>      takes care of reading zeros beyond EOF if backing file is short
> 
> But the effect is the same.
> 
>> +++ b/block/io.c
>> @@ -2385,16 +2385,16 @@ static int coroutine_fn bdrv_co_block_status(BlockDriverState *bs,
>>       if (ret & (BDRV_BLOCK_DATA | BDRV_BLOCK_ZERO)) {
>>           ret |= BDRV_BLOCK_ALLOCATED;
>> -    } else if (want_zero) {
>> -        if (bdrv_unallocated_blocks_are_zero(bs)) {
>> -            ret |= BDRV_BLOCK_ZERO;
>> -        } else if (bs->backing) {
>> +    } else if (want_zero && bs->drv->supports_backing) {
>> +        if (bs->backing) {
>>               BlockDriverState *bs2 = bs->backing->bs;
>>               int64_t size2 = bdrv_getlength(bs2);
>>               if (size2 >= 0 && offset >= size2) {
>>                   ret |= BDRV_BLOCK_ZERO;
>>               }
>> +        } else {
>> +            ret |= BDRV_BLOCK_ZERO;
>>           }
> 
> I like this part of the change.  But if it is done first in the series, it _does_ have a semantic impact on protocol drivers (previously, protocol drivers that return 0 but set the field .unallocated_blocks_are_zero will be changed to report _ZERO; after this patch, protocol drivers do not do that, because they don't support backing files, and it is now only backing files that do the _ZERO magic).  So doing _just_ this change, along with a better analysis of how it changes the semantics of 'qemu-io -c map' on protocol drivers while mentioning why that is okay, would make a better start to the series, rather than here at the end.  Of course, if you defer it to the end, then none of the protocol drivers set .unallocated_blocks_are_zero anyway, but that cost more review work on each altered protocol driver.
> 


-- 
Best regards,
Vladimir

Re: [PATCH 2/8] block/vpc: return ZERO block-status when appropriate

[Vladimir Sementsov-Ogievskiy]

07.05.2020 0:18, Eric Blake wrote:
> On 5/6/20 4:25 AM, 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.
>>
>> After block-status update, it never reports 0, so setting
>> unallocated_blocks_are_zero doesn't make sense. Drop it.
> 
> Same analysis as in patch 1 as to the lone two clients that cared, and the fact that we are changing 'qemu-io -c map' output by reporting data as allocated now.  But I concur that as there is never a backing file, the change is not a regression, but rather a bug fix.

Note that we have a problem with meaning of unallocated for protocol drivers. For example, iscsi block_status return 0, and it means "unallocated garbage", i.e. not occupying space, read may return any garbage. But vdi and vpc are format drivers, just don't support backing and they would better return ZERO status where appropriate.

> 
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   block/vpc.c | 3 +--
>>   1 file changed, 1 insertion(+), 2 deletions(-)
> 
> While the commit message could be improved, the code change itself looks correct.
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 


-- 
Best regards,
Vladimir

Re: [PATCH 1/8] block/vdi: return ZERO block-status when appropriate

[Vladimir Sementsov-Ogievskiy]

06.05.2020 16:57, Eric Blake wrote:
> On 5/6/20 4:25 AM, Vladimir Sementsov-Ogievskiy wrote:
>> In case of !VDI_IS_ALLOCATED[], we do zero out the corresponding chunk
>> of qiov. So, this should be reported as ZERO.
> 
> This part makes sense outright, since vdi does not allow backing files, so all reads of a VDI disk result in data rather than deferral to another layer, and we indeed read zero in this case.  However, it _is_ a behavior change: previously, 'qemu-io -c map' on a vdi image will show unallocated portions, but with this change, the entire image now shows as allocated.  You need to call out this fact in the commit message, documenting that it is intentional (it matches what we do for posix files

posix is bad example, as we do have protocol drivers which may return 0 block-status for purpose: it means not go-to-backing but fs-unallocated-non-zero (iscsi is an example of it). And we need to split these meanings I think, but it's a task for another series.

>: since neither files nor vdi support backing images, we guarantee that all read attempts will be satisfied by this layer rather than deferring to a backing layer, and thus can treat all data as allocated in this layer, regardless of whether it is sparse).
> 
> Do any existing iotests need an update to reflect change in output?  If not, should we have an iotest covering it?
> 
>>
>> After block-status update, it never reports 0, so setting
>> unallocated_blocks_are_zero doesn't make sense. Drop it.
> 
> This is a harder claim. To prove it, we need to inspect all callers that use unallocated_blocks_are_zero, to see their intent.  Fortunately, the list is small - a mere 2 clients!
> 
> block/io.c:bdrv_co_block_status(): if .bdrv_co_block_status sets either _DATA or _ZERO, block status adds _ALLOCATED; but if the driver did not set either, we use bdrv_unallocated_blocks_are_zero() in order to set _ZERO but not _ALLOCATED.  This is the code that explains the behavior change mentioned above (now that vdi is reporting _ZERO instead of 0, block_status is appending _ALLOCATED instead of querying bdrv_unallocated_blocks_are_zero).  But you are correct that it does not change where _ZERO is reported.
> 
> qemu-img.c:img_convert(): calls bdrv_get_info() up front and caches what it learned from unallocated_blocks_are_zero about the target; later on, in convert_iteration_sectors(), it uses this information to optimize the case where the source reads as zero, but the target has a backing file and the range being written lies beyond the end of the backing file. That is, given the following scenario:
> 
> qemu-img convert input -B backing output
> input:   ABCD-0E
> backing: ACBD
> 
> the optimization lets us produce:
> output:  -BC---E
> 
> instead of the larger:
> output:  -BC--0E
> 
> Do we have any iotests that cover this particular scenario, to prove that our optimization does indeed result in a smaller image file without explicit zeros written in the tail?  Or put another way, if we were to disable the post_backing_zero optimization in convert_iteration_sectors(), would any iotests fail?  If not, we should really enhance our testsuite.
> 
> With that said, we could just as easily achieve the optimization of the conversion to the tail of a destination with short backing file by checking block_status to see whether the tail already reads as all zeroes, rather than relying on unallocated_blocks_are_zero.  Even if querying block_status is a slight pessimization in time, it would avoid regressing in the more important factor of artificially bloating the destination.  I would _really_ love to see a patch to qemu-img.c reworking the logic to avoid the use of unallocated_blocks_are_zero first, prior to removing the setting of that field in the various drivers.  Once bdrv_co_block_status() remains as the only client of the field, then I could accept this patch with a better commit message about the intentional change in block_status _ALLOCATION behavior.
> 
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   block/vdi.c | 3 +--
>>   1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/block/vdi.c b/block/vdi.c
>> index 0c7835ae70..83471528d2 100644
>> --- a/block/vdi.c
>> +++ b/block/vdi.c
>> @@ -334,7 +334,6 @@ static int vdi_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
>>       logout("\n");
>>       bdi->cluster_size = s->block_size;
>>       bdi->vm_state_offset = 0;
>> -    bdi->unallocated_blocks_are_zero = true;
>>       return 0;
>>   }
>> @@ -536,7 +535,7 @@ static int coroutine_fn vdi_co_block_status(BlockDriverState *bs,
>>       *pnum = MIN(s->block_size - index_in_block, bytes);
>>       result = VDI_IS_ALLOCATED(bmap_entry);
>>       if (!result) {
>> -        return 0;
>> +        return BDRV_BLOCK_ZERO;
>>       }
>>       *map = s->header.offset_data + (uint64_t)bmap_entry * s->block_size +
>>
> 


-- 
Best regards,
Vladimir

Re: [PATCH 8/8] block: drop unallocated_blocks_are_zero

[Vladimir Sementsov-Ogievskiy]

07.05.2020 10:05, Vladimir Sementsov-Ogievskiy wrote:
> 06.05.2020 18:14, Eric Blake wrote:
>> On 5/6/20 4:25 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> Currently this field only set by qed and qcow2.
>>
>> Well, only after patches 1-6 (prior to then, it was also set in protocol drivers).  I think you might be able to hoist part of this patch earlier in the series, to make the changes to the protocol drivers easier to review, by rewording this sentence:
>>
>> Currently, the only format drivers that set this field are qed and qcow2.
>>
>>> But in fact, all
>>> backing-supporting formats (parallels, qcow, qcow2, qed, vmdk) share
>>> this semantics: on unallocated blocks, if there is no backing file they
>>
>> s/this/these/
>>
>>> just memset the buffer with zeroes.
>>>
>>> So, document this behavior for .supports_backing and drop
>>> .unallocated_blocks_are_zero
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> ---
>>>   include/block/block.h     |  6 ------
>>>   include/block/block_int.h | 13 ++++++++++++-
>>>   block.c                   | 15 ---------------
>>>   block/io.c                |  8 ++++----
>>>   block/qcow2.c             |  1 -
>>>   block/qed.c               |  1 -
>>>   6 files changed, 16 insertions(+), 28 deletions(-)
>>>
>>> diff --git a/include/block/block.h b/include/block/block.h
>>> index 8b62429aa4..db1cb503ec 100644
>>> --- a/include/block/block.h
>>> +++ b/include/block/block.h
>>> @@ -21,11 +21,6 @@ typedef struct BlockDriverInfo {
>>>       /* offset at which the VM state can be saved (0 if not possible) */
>>>       int64_t vm_state_offset;
>>>       bool is_dirty;
>>> -    /*
>>> -     * True if unallocated blocks read back as zeroes. This is equivalent
>>> -     * to the LBPRZ flag in the SCSI logical block provisioning page.
>>> -     */
>>> -    bool unallocated_blocks_are_zero;
>>
>> You can't delete this field until all protocol drivers are cleaned up, so deferring this part of the change to the end of the series makes sense.
>>
>>>       /*
>>>        * True if this block driver only supports compressed writes
>>>        */
>>> @@ -431,7 +426,6 @@ int bdrv_co_pdiscard(BdrvChild *child, int64_t offset, int64_t bytes);
>>>   int bdrv_has_zero_init_1(BlockDriverState *bs);
>>>   int bdrv_has_zero_init(BlockDriverState *bs);
>>>   int bdrv_has_zero_init_truncate(BlockDriverState *bs);
>>> -bool bdrv_unallocated_blocks_are_zero(BlockDriverState *bs);
>>
>> Doing this cleanup makes sense: there is only one caller of this function pre-patch, and 0 callers post-patch - but whether the cleanup should be at the same time as you fix the one caller, or deferred to when you also clean up the field, is less important.
>>
>> If I were writing the series:
>>
>> 1 - fix qemu-img.c to not use the field
>> 2 - fix block_status to not use the function
> 
> Hmm stop. We still need patches 1,2 before modifying block_status, otherwise we'll still need to check unallocated_blocks_are_zero


Hmm2. This just means that I need to put all commit messages about dropping unallocated_block_are_zero into one commit message rewriting the block_status. I doubt that it simplifies review: instead of analyzing format-by-format, you'll have to analyze all format at once.

> 
>> 3-n - fix protocol drivers that set the field to also return _ZERO
>>   during block status (but not delete the field at that time)
>> n+1 - delete unused function and field (from ALL drivers)
>>
>>>   bool bdrv_can_write_zeroes_with_unmap(BlockDriverState *bs);
>>>   int bdrv_block_status(BlockDriverState *bs, int64_t offset,
>>>                         int64_t bytes, int64_t *pnum, int64_t *map,
>>> diff --git a/include/block/block_int.h b/include/block/block_int.h
>>> index 92335f33c7..c156b22c6b 100644
>>> --- a/include/block/block_int.h
>>> +++ b/include/block/block_int.h
>>> @@ -115,7 +115,18 @@ struct BlockDriver {
>>>        */
>>>       bool bdrv_needs_filename;
>>> -    /* Set if a driver can support backing files */
>>> +    /*
>>> +     * Set if a driver can support backing files. This also implies the
>>> +     * following semantics:
>>> +     *
>>> +     *  - Return status 0 of .bdrv_co_block_status means that corresponding
>>> +     *    blocks are not allocated in this layer of backing-chain
>>> +     *  - For such (unallocated) blocks, read will:
>>> +     *    - read from backing file if there is one and big enough
>>
>> s/and/and it is/
>>
>>> +     *    - fill buffer with zeroes if there is no backing file
>>> +     *    - space after EOF of the backing file considered as zero
>>> +     *      (corresponding part of read-buffer must be zeroed by driver)
>>
>> Does the driver actually have to do the zeroing?  Looking at qcow2.c, I see:
>> static coroutine_fn int qcow2_co_preadv_task(BlockDriverState *bs,
>> ...
>>
>>      case QCOW2_CLUSTER_UNALLOCATED:
>>          assert(bs->backing); /* otherwise handled in qcow2_co_preadv_part */
>>
>>          BLKDBG_EVENT(bs->file, BLKDBG_READ_BACKING_AIO);
>>          return bdrv_co_preadv_part(bs->backing, offset, bytes,
>>                                     qiov, qiov_offset, 0);
>>
>> which just defers to the block layer to handle read-beyond-EOF, rather than an explicit memset in the driver.
>>
>> So maybe you can simplify to:
>> - For such (unallocated) blocks, read will:
>>    - fill buffer with zeros if there is no backing file
>>    - read from the backing file otherwise, where the block layer
>>      takes care of reading zeros beyond EOF if backing file is short
>>
>> But the effect is the same.
>>
>>> +++ b/block/io.c
>>> @@ -2385,16 +2385,16 @@ static int coroutine_fn bdrv_co_block_status(BlockDriverState *bs,
>>>       if (ret & (BDRV_BLOCK_DATA | BDRV_BLOCK_ZERO)) {
>>>           ret |= BDRV_BLOCK_ALLOCATED;
>>> -    } else if (want_zero) {
>>> -        if (bdrv_unallocated_blocks_are_zero(bs)) {
>>> -            ret |= BDRV_BLOCK_ZERO;
>>> -        } else if (bs->backing) {
>>> +    } else if (want_zero && bs->drv->supports_backing) {
>>> +        if (bs->backing) {
>>>               BlockDriverState *bs2 = bs->backing->bs;
>>>               int64_t size2 = bdrv_getlength(bs2);
>>>               if (size2 >= 0 && offset >= size2) {
>>>                   ret |= BDRV_BLOCK_ZERO;
>>>               }
>>> +        } else {
>>> +            ret |= BDRV_BLOCK_ZERO;
>>>           }
>>
>> I like this part of the change.  But if it is done first in the series, it _does_ have a semantic impact on protocol drivers (previously, protocol drivers that return 0 but set the field .unallocated_blocks_are_zero will be changed to report _ZERO; after this patch, protocol drivers do not do that, because they don't support backing files, and it is now only backing files that do the _ZERO magic).  So doing _just_ this change, along with a better analysis of how it changes the semantics of 'qemu-io -c map' on protocol drivers while mentioning why that is okay, would make a better start to the series, rather than here at the end.  Of course, if you defer it to the end, then none of the protocol drivers set .unallocated_blocks_are_zero anyway, but that cost more review work on each altered protocol driver.
>>
> 
> 


-- 
Best regards,
Vladimir

Re: [PATCH 8/8] block: drop unallocated_blocks_are_zero

[Vladimir Sementsov-Ogievskiy]

06.05.2020 18:14, Eric Blake wrote:
> On 5/6/20 4:25 AM, Vladimir Sementsov-Ogievskiy wrote:
>> Currently this field only set by qed and qcow2.
> 
> Well, only after patches 1-6 (prior to then, it was also set in protocol drivers).  I think you might be able to hoist part of this patch earlier in the series, to make the changes to the protocol drivers easier to review, by rewording this sentence:
> 
> Currently, the only format drivers that set this field are qed and qcow2.
> 
>> But in fact, all
>> backing-supporting formats (parallels, qcow, qcow2, qed, vmdk) share
>> this semantics: on unallocated blocks, if there is no backing file they
> 
> s/this/these/
> 
>> just memset the buffer with zeroes.
>>
>> So, document this behavior for .supports_backing and drop
>> .unallocated_blocks_are_zero
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   include/block/block.h     |  6 ------
>>   include/block/block_int.h | 13 ++++++++++++-
>>   block.c                   | 15 ---------------
>>   block/io.c                |  8 ++++----
>>   block/qcow2.c             |  1 -
>>   block/qed.c               |  1 -
>>   6 files changed, 16 insertions(+), 28 deletions(-)
>>
>> diff --git a/include/block/block.h b/include/block/block.h
>> index 8b62429aa4..db1cb503ec 100644
>> --- a/include/block/block.h
>> +++ b/include/block/block.h
>> @@ -21,11 +21,6 @@ typedef struct BlockDriverInfo {
>>       /* offset at which the VM state can be saved (0 if not possible) */
>>       int64_t vm_state_offset;
>>       bool is_dirty;
>> -    /*
>> -     * True if unallocated blocks read back as zeroes. This is equivalent
>> -     * to the LBPRZ flag in the SCSI logical block provisioning page.
>> -     */
>> -    bool unallocated_blocks_are_zero;
> 
> You can't delete this field until all protocol drivers are cleaned up, so deferring this part of the change to the end of the series makes sense.
> 
>>       /*
>>        * True if this block driver only supports compressed writes
>>        */
>> @@ -431,7 +426,6 @@ int bdrv_co_pdiscard(BdrvChild *child, int64_t offset, int64_t bytes);
>>   int bdrv_has_zero_init_1(BlockDriverState *bs);
>>   int bdrv_has_zero_init(BlockDriverState *bs);
>>   int bdrv_has_zero_init_truncate(BlockDriverState *bs);
>> -bool bdrv_unallocated_blocks_are_zero(BlockDriverState *bs);
> 
> Doing this cleanup makes sense: there is only one caller of this function pre-patch, and 0 callers post-patch - but whether the cleanup should be at the same time as you fix the one caller, or deferred to when you also clean up the field, is less important.
> 
> If I were writing the series:
> 
> 1 - fix qemu-img.c to not use the field
> 2 - fix block_status to not use the function
> 3-n - fix protocol drivers that set the field to also return _ZERO
>   during block status (but not delete the field at that time)
> n+1 - delete unused function and field (from ALL drivers)
> 
>>   bool bdrv_can_write_zeroes_with_unmap(BlockDriverState *bs);
>>   int bdrv_block_status(BlockDriverState *bs, int64_t offset,
>>                         int64_t bytes, int64_t *pnum, int64_t *map,
>> diff --git a/include/block/block_int.h b/include/block/block_int.h
>> index 92335f33c7..c156b22c6b 100644
>> --- a/include/block/block_int.h
>> +++ b/include/block/block_int.h
>> @@ -115,7 +115,18 @@ struct BlockDriver {
>>        */
>>       bool bdrv_needs_filename;
>> -    /* Set if a driver can support backing files */
>> +    /*
>> +     * Set if a driver can support backing files. This also implies the
>> +     * following semantics:
>> +     *
>> +     *  - Return status 0 of .bdrv_co_block_status means that corresponding
>> +     *    blocks are not allocated in this layer of backing-chain
>> +     *  - For such (unallocated) blocks, read will:
>> +     *    - read from backing file if there is one and big enough
> 
> s/and/and it is/
> 
>> +     *    - fill buffer with zeroes if there is no backing file
>> +     *    - space after EOF of the backing file considered as zero
>> +     *      (corresponding part of read-buffer must be zeroed by driver)
> 
> Does the driver actually have to do the zeroing?  Looking at qcow2.c, I see:
> static coroutine_fn int qcow2_co_preadv_task(BlockDriverState *bs,
> ...
> 
>      case QCOW2_CLUSTER_UNALLOCATED:
>          assert(bs->backing); /* otherwise handled in qcow2_co_preadv_part */
> 
>          BLKDBG_EVENT(bs->file, BLKDBG_READ_BACKING_AIO);
>          return bdrv_co_preadv_part(bs->backing, offset, bytes,
>                                     qiov, qiov_offset, 0);
> 
> which just defers to the block layer to handle read-beyond-EOF, rather than an explicit memset in the driver.
> 
> So maybe you can simplify to:
> - For such (unallocated) blocks, read will:
>    - fill buffer with zeros if there is no backing file
>    - read from the backing file otherwise, where the block layer
>      takes care of reading zeros beyond EOF if backing file is short
> 
> But the effect is the same.
> 
>> +++ b/block/io.c
>> @@ -2385,16 +2385,16 @@ static int coroutine_fn bdrv_co_block_status(BlockDriverState *bs,
>>       if (ret & (BDRV_BLOCK_DATA | BDRV_BLOCK_ZERO)) {
>>           ret |= BDRV_BLOCK_ALLOCATED;
>> -    } else if (want_zero) {
>> -        if (bdrv_unallocated_blocks_are_zero(bs)) {
>> -            ret |= BDRV_BLOCK_ZERO;
>> -        } else if (bs->backing) {
>> +    } else if (want_zero && bs->drv->supports_backing) {
>> +        if (bs->backing) {
>>               BlockDriverState *bs2 = bs->backing->bs;
>>               int64_t size2 = bdrv_getlength(bs2);
>>               if (size2 >= 0 && offset >= size2) {
>>                   ret |= BDRV_BLOCK_ZERO;
>>               }
>> +        } else {
>> +            ret |= BDRV_BLOCK_ZERO;
>>           }
> 
> I like this part of the change.  But if it is done first in the series, it _does_ have a semantic impact on protocol drivers (previously, protocol drivers that return 0 but set the field .unallocated_blocks_are_zero will be changed to report _ZERO; after this patch, protocol drivers do not do that, because they don't support backing files, and it is now only backing files that do the _ZERO magic).  So doing _just_ this change, along with a better analysis of how it changes the semantics of 'qemu-io -c map' on protocol drivers while mentioning why that is okay, would make a better start to the series, rather than here at the end.  Of course, if you defer it to the end, then none of the protocol drivers set .unallocated_blocks_are_zero anyway, but that cost more review work on each altered protocol driver.
> 

Doing just this change prior to patches 1/2  breaks final BDRV_BLOCK_ZERO produced for vdi and vpc.

-- 
Best regards,
Vladimir