Re: [Qemu-devel] [PATCH v4 03/15] block: introduce BDRV_REQ_ALLOCATE flag

[Eric Blake]

On 08/01/2017 09:19 AM, 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 has to return -ENOTSUP if allocation cannot be done efficiently
> (i.e. without falling back to writing actual buffers)
> 
> Signed-off-by: Anton Nefedov <address@hidden>
> ---
>  include/block/block.h     |  6 +++++-
>  include/block/block_int.h |  2 +-
>  block/io.c                | 20 +++++++++++++++++---
>  3 files changed, 23 insertions(+), 5 deletions(-)

Reviewed-by: Eric Blake <address@hidden>

You might want the commit message to be a bit more verbose...

> 
> diff --git a/include/block/block.h b/include/block/block.h
> index 7fe0125..828da67 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -65,9 +65,13 @@ typedef enum {
>      BDRV_REQ_NO_SERIALISING     = 0x8,
>      BDRV_REQ_FUA                = 0x10,
>      BDRV_REQ_WRITE_COMPRESSED   = 0x20,
> +    /* 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.
> +     */
> +    BDRV_REQ_ALLOCATE           = 0x40,
>  
>      /* Mask of valid flags */
> -    BDRV_REQ_MASK               = 0x3f,
> +    BDRV_REQ_MASK               = 0x7f,
>  } BdrvRequestFlags;
>  
>  typedef struct BlockSizes {
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 9b94b32..9b64411 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -585,7 +585,7 @@ struct BlockDriverState {
>      /* Flags honored during pwrite (so far: BDRV_REQ_FUA) */
>      unsigned int supported_write_flags;
>      /* Flags honored during pwrite_zeroes (so far: BDRV_REQ_FUA,
> -     * BDRV_REQ_MAY_UNMAP) */
> +     * BDRV_REQ_MAY_UNMAP, BDRV_REQ_ALLOCATE) */
>      unsigned int supported_zero_flags;

...in addition to adding the new flag here and documenting its semantics
for drivers...

>  
>      /* the following member gives a name to every node on the bs graph. */
> diff --git a/block/io.c b/block/io.c
> index 375fc66..04d495e 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -1245,7 +1245,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;

...you also made sure that anywhere the flag is in use you avoid a slow
fallback...

> @@ -1639,6 +1645,14 @@ int coroutine_fn bdrv_co_pwrite_zeroes(BdrvChild 
> *child, int64_t offset,
>  {
>      trace_bdrv_co_pwrite_zeroes(child->bs, offset, bytes, flags);
>  
> +    assert(!(flags & BDRV_REQ_MAY_UNMAP && flags & BDRV_REQ_ALLOCATE));
> +
> +    if (flags & BDRV_REQ_ALLOCATE &&
> +        !(child->bs->supported_zero_flags & BDRV_REQ_ALLOCATE))
> +    {
> +        return -ENOTSUP;

...as well as providing a sane default to make the flag always trigger
-ENOTSUP until individual drivers implement something in later patches.

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

signature.asc
Description: OpenPGP digital signature