Re: [Qemu-devel] [PATCH v11 04/10] block: introduce BDRV_REQ_ALLOCATE flag

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

18.12.2018 10:57, Anton Nefedov wrote:
> The flag is supposed to indicate that the region of the disk image has
> to be sufficiently allocated so it reads as zeroes.
> 
> The call with the flag set must return -ENOTSUP if allocation cannot
> be done efficiently.
> This has to be made sure of by both
>    - the drivers that support the flag
>    - and the common block layer (so it will not fall back to any slowpath
>      (like writing zero buffers) in case the driver does not support
>      the flag).
> 
> Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
> ---
>   include/block/block.h     | 10 +++++++++-
>   include/block/block_int.h |  3 ++-
>   block/io.c                | 14 +++++++++++++-
>   3 files changed, 24 insertions(+), 3 deletions(-)
> 
> diff --git a/include/block/block.h b/include/block/block.h
> index f70a843b72..643d32f4b8 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -83,8 +83,16 @@ typedef enum {
>        */
>       BDRV_REQ_SERIALISING        = 0x80,
>   
> +    /*
> +     * The BDRV_REQ_ALLOCATE flag is used to indicate that the driver has to
> +     * efficiently allocate the space so it reads as zeroes, or return an error.
> +     * If this flag is set then BDRV_REQ_ZERO_WRITE must also be set.
> +     * This flag cannot be set together with BDRV_REQ_MAY_UNMAP.
> +     */
> +    BDRV_REQ_ALLOCATE           = 0x100,
> +
>       /* Mask of valid flags */
> -    BDRV_REQ_MASK               = 0xff,
> +    BDRV_REQ_MASK               = 0x1ff,
>   } BdrvRequestFlags;
>   
>   typedef struct BlockSizes {
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index f605622216..833129d912 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -724,7 +724,8 @@ struct BlockDriverState {
>        * their children. */
>       unsigned int supported_write_flags;
>       /* Flags honored during pwrite_zeroes (so far: BDRV_REQ_FUA,
> -     * BDRV_REQ_MAY_UNMAP, BDRV_REQ_WRITE_UNCHANGED) */
> +     * BDRV_REQ_MAY_UNMAP, BDRV_REQ_WRITE_UNCHANGED, BDRV_REQ_ALLOCATE)
> +     */
>       unsigned int supported_zero_flags;
>   
>       /* the following member gives a name to every node on the bs graph. */
> diff --git a/block/io.c b/block/io.c
> index bd9d688f8b..66006a089d 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -1534,7 +1534,7 @@ static int coroutine_fn bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,
>               assert(!bs->supported_zero_flags);
>           }
>   
> -        if (ret == -ENOTSUP) {
> +        if (ret == -ENOTSUP && !(flags & BDRV_REQ_ALLOCATE)) {
>               /* Fall back to bounce buffer if write zeroes is unsupported */
>               BdrvRequestFlags write_flags = flags & ~BDRV_REQ_ZERO_WRITE;
>   
> @@ -1773,6 +1773,9 @@ static int coroutine_fn bdrv_co_do_zero_pwritev(BdrvChild *child,
>   
>       assert(flags & BDRV_REQ_ZERO_WRITE);
>       if (head_padding_bytes || tail_padding_bytes) {
> +        if (flags & BDRV_REQ_ALLOCATE) {
> +            return -ENOTSUP;
> +        }
>           buf = qemu_blockalign(bs, align);
>           iov = (struct iovec) {
>               .iov_base   = buf,
> @@ -1858,6 +1861,9 @@ int coroutine_fn bdrv_co_pwritev(BdrvChild *child,
>       bool use_local_qiov = false;
>       int ret;
>   
> +    assert(!((flags & BDRV_REQ_ALLOCATE) && (flags & BDRV_REQ_MAY_UNMAP)));
> +    assert(!((flags & BDRV_REQ_ALLOCATE) && !(flags & BDRV_REQ_ZERO_WRITE)));
> +
>       trace_bdrv_co_pwritev(child->bs, offset, bytes, flags);
>   
>       if (!bs->drv) {
> @@ -1980,6 +1986,12 @@ int coroutine_fn bdrv_co_pwrite_zeroes(BdrvChild *child, int64_t offset,
>   {
>       trace_bdrv_co_pwrite_zeroes(child->bs, offset, bytes, flags);
>   
> +    if ((flags & BDRV_REQ_ALLOCATE) &&
> +        !(child->bs->supported_zero_flags & BDRV_REQ_ALLOCATE))
> +    {
> +        return -ENOTSUP;
> +    }

Why don't we have this check in bdrv_co_pwritev?
However, similar check about UNMAP is here too, so:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

> +
>       if (!(child->bs->open_flags & BDRV_O_UNMAP)) {
>           flags &= ~BDRV_REQ_MAY_UNMAP;
>       }
> 


-- 
Best regards,
Vladimir