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

[Eric Blake <eblake@redhat.com>]

On 5/20/20 3:35 AM, 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>
> ---
>   block/io.c    | 39 ++++++++++++++++++++++++++++++---------
>   block/qcow2.c | 16 ++++++++++++++--
>   2 files changed, 44 insertions(+), 11 deletions(-)

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


> +++ b/block/qcow2.c
> @@ -3827,8 +3827,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
> +     * example, zeros which comes from the region which is unallocated in

s/comes/come/

> +     * the whole backing chain, and zeros which comes because of short backing

and again, also s/of/of a/

> +     * 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,
> 

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