Re: [PATCH v7 1/5] block/io: fix bdrv_co_block_status_above

[Eric Blake <eblake@redhat.com>]

On 9/24/20 2:39 PM, Vladimir Sementsov-Ogievskiy wrote:
> bdrv_co_block_status_above has several design problems with handling
> short backing files:
> 
> 1. With want_zeros=true, it may return ret with BDRV_BLOCK_ZERO but
> without BDRV_BLOCK_ALLOCATED flag, when actually short backing file
> which produces these after-EOF zeros is inside requested backing
> sequence.
> 
> 2. With want_zero=false, it may return pnum=0 prior to actual EOF,
> because of EOF of short backing file.
> 
> Fix these things, making logic about short backing files clearer.
> 
> With fixed bdrv_block_status_above we also have to improve is_zero in
> qcow2 code, otherwise iotest 154 will fail, because with this patch we
> stop to merge zeros of different types (produced by fully unallocated
> in the whole backing chain regions vs produced by short backing files).
> 
> Note also, that this patch leaves for another day the general problem
> around block-status: misuse of BDRV_BLOCK_ALLOCATED as is-fs-allocated
> vs go-to-backing.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Reviewed-by: Alberto Garcia <berto@igalia.com>
> ---
>   block/io.c    | 68 ++++++++++++++++++++++++++++++++++++++++-----------
>   block/qcow2.c | 16 ++++++++++--
>   2 files changed, 68 insertions(+), 16 deletions(-)
> 
> diff --git a/block/io.c b/block/io.c
> index 449b99b92c..4697e67a85 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -2350,34 +2350,74 @@ bdrv_co_common_block_status_above(BlockDriverState *bs,
>                                     int64_t *map,
>                                     BlockDriverState **file)
>   {
> +    int ret;
>       BlockDriverState *p;
> -    int ret = 0;

The shuffle of these lines is odd, but not a show-stopper.

The new flow is definitely easier to read.  Thanks for doing this!

> +++ b/block/qcow2.c
> @@ -3860,8 +3860,20 @@ static bool is_zero(BlockDriverState *bs, int64_t offset, int64_t bytes)
>       if (!bytes) {
>           return true;
>       }
> -    res = bdrv_block_status_above(bs, NULL, offset, bytes, &nr, NULL, NULL);
> -    return res >= 0 && (res & BDRV_BLOCK_ZERO) && nr == bytes;
> +
> +    /*
> +     * bdrv_block_status_above doesn't merge different types of zeros, for

We're inconsistent on whether we spell the noun 'zeros' or 'zeroes'; but 
I don't care enough to make you change this.

> +     * example, zeros which come from the region which is unallocated in
> +     * the whole backing chain, and zeros which comes because of a short

s/comes/come/

> +     * backing file. So, we need a loop.
> +     */
> +    do {
> +        res = bdrv_block_status_above(bs, NULL, offset, bytes, &nr, NULL, NULL);
> +        offset += nr;
> +        bytes -= nr;
> +    } while (res >= 0 && (res & BDRV_BLOCK_ZERO) && nr && bytes);
> +
> +    return res >= 0 && (res & BDRV_BLOCK_ZERO) && bytes == 0;
>   }
>   
>   static coroutine_fn int qcow2_co_pwrite_zeroes(BlockDriverState *bs,
> 

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

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