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 <address@hidden>
> ---
>   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