Re: [PATCH v4 1/4] block: add max_pwrite_zeroes_fast to BlockLimits

[Eric Blake]

On 6/11/20 11:26 AM, Vladimir Sementsov-Ogievskiy wrote:
> The NBD spec was recently updated to clarify that max_block doesn't
> relate to NBD_CMD_WRITE_ZEROES with NBD_CMD_FLAG_FAST_ZERO (which
> mirrors Qemu flag BDRV_REQ_NO_FALLBACK). To drop the restriction we
> need new max_pwrite_zeroes_fast.
>
> Default value of new max_pwrite_zeroes_fast is zero and it means
> use max_pwrite_zeroes. So this commit semantically changes nothing.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   include/block/block_int.h |  8 ++++++++
>   block/io.c                | 17 ++++++++++++-----
>   2 files changed, 20 insertions(+), 5 deletions(-)

Hmm, this is an optimization, rather than a correctness issue.  I'm 
sorry I didn't review it sooner, but at this point, I think it is better 
as 5.2 material.

> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 791de6a59c..277e32fe31 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -626,6 +626,14 @@ typedef struct BlockLimits {
>        * pwrite_zeroes_alignment. May be 0 if no inherent 32-bit limit */
>       int32_t max_pwrite_zeroes;
>   
> +    /*
> +     * Maximum number of bytes that can zeroed at once if flag
> +     * BDRV_REQ_NO_FALLBACK specified. Must be multiple of
> +     * pwrite_zeroes_alignment.
> +     * If 0, max_pwrite_zeroes is used for no-fallback case.
> +     */
> +    int64_t max_pwrite_zeroes_fast;

Nice that this is 64-bit off the bat (I know you have another series 
about converting more stuff to 64-bit).

> +
>       /* Optimal alignment for write zeroes requests in bytes. A power
>        * of 2 is best but not mandatory.  Must be a multiple of
>        * bl.request_alignment, and must be less than max_pwrite_zeroes
> diff --git a/block/io.c b/block/io.c
> index df8f2a98d4..0af62a53fd 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -1774,12 +1774,13 @@ static int coroutine_fn 
> bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,
>       bool need_flush = false;
>       int head = 0;
>       int tail = 0;
> -
> -    int max_write_zeroes = MIN_NON_ZERO(bs->bl.max_pwrite_zeroes, INT_MAX);
> +    int max_write_zeroes;

32-bit...

>       int alignment = MAX(bs->bl.pwrite_zeroes_alignment,
>                           bs->bl.request_alignment);
>       int max_transfer = MIN_NON_ZERO(bs->bl.max_transfer, MAX_BOUNCE_BUFFER);
>   
> +    assert(alignment % bs->bl.request_alignment == 0);

Would this look any better using the QEMU_IS_ALIGNED macro?

> +
>       if (!drv) {
>           return -ENOMEDIUM;
>       }
> @@ -1788,12 +1789,18 @@ static int coroutine_fn 
> bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,
>           return -ENOTSUP;
>       }
>   
> -    assert(alignment % bs->bl.request_alignment == 0);
> -    head = offset % alignment;
> -    tail = (offset + bytes) % alignment;
> +    if ((flags & BDRV_REQ_NO_FALLBACK) && bs->bl.max_pwrite_zeroes_fast) {
> +        max_write_zeroes = bs->bl.max_pwrite_zeroes_fast;

...but you try to assign something that may be 64-bit into it.  Risk of 
overflow.  Maybe we should get your 64-bit cleanup series in first.

> +    } else {
> +        max_write_zeroes = bs->bl.max_pwrite_zeroes;
> +    }
> +    max_write_zeroes = MIN_NON_ZERO(max_write_zeroes, INT_MAX);
>       max_write_zeroes = QEMU_ALIGN_DOWN(max_write_zeroes, alignment);
>       assert(max_write_zeroes >= bs->bl.request_alignment);
>   
> +    head = offset % alignment;
> +    tail = (offset + bytes) % alignment;
> +
>       while (bytes > 0 && !ret) {
>           int num = bytes;
>   

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