Re: [PATCH v4 06/11] block: make BlockLimits::max_pwrite_zeroes 64bit

From: Eric Blake <eblake@redhat.com>
To: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>,
	qemu-block@nongnu.org
Cc: fam@euphon.net, kwolf@redhat.com, integration@gluster.org,
	berto@igalia.com, stefanha@redhat.com, pavel.dovgaluk@ispras.ru,
	sw@weilnetz.de, pl@kamp.de, qemu-devel@nongnu.org,
	mreitz@redhat.com, jsnow@redhat.com, ronniesahlberg@gmail.com,
	pbonzini@redhat.com, namei.unix@gmail.com, dillaman@redhat.com,
	ari@tuxera.com
Subject: Re: [PATCH v4 06/11] block: make BlockLimits::max_pwrite_zeroes 64bit
Date: Tue, 11 May 2021 16:19:45 -0500	[thread overview]
Message-ID: <94bc702a-99a4-9a94-cf9b-6c67f4cf1fd2@redhat.com> (raw)
In-Reply-To: <20210324205132.464899-7-vsementsov@virtuozzo.com>

On 3/24/21 3:51 PM, Vladimir Sementsov-Ogievskiy wrote:
> We are going to support 64 bit write-zeroes requests. Now update the
> limit variable. It's absolutely safe. The variable is set in some
> drivers, and used in bdrv_co_do_pwrite_zeroes().
> 
> Update also max_write_zeroes variable in bdrv_co_do_pwrite_zeroes(), so
> that bdrv_co_do_pwrite_zeroes() is now prepared to 64bit requests. The
> remaining logic including num, offset and bytes variables is already
> supporting 64bit requests.
> 
> So the only thing that prevents 64 bit requests is limiting
> max_write_zeroes variable to INT_MAX in bdrv_co_do_pwrite_zeroes().
> We'll drop this limitation after updating all block drivers.
> 
> Ah, we also have bdrv_check_request32() in bdrv_co_pwritev_part(). It
> will be modified to do bdrv_check_request() for write-zeroes path.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  include/block/block_int.h | 7 +++----
>  block/io.c                | 2 +-
>  2 files changed, 4 insertions(+), 5 deletions(-)
> 

> +++ b/include/block/block_int.h
> @@ -676,10 +676,9 @@ typedef struct BlockLimits {
>       * that is set. May be 0 if bl.request_alignment is good enough */
>      uint32_t pdiscard_alignment;
>  
> -    /* Maximum number of bytes that can zeroized at once (since it is
> -     * signed, it must be < 2G, if set). Must be multiple of
> -     * pwrite_zeroes_alignment. May be 0 if no inherent 32-bit limit */
> -    int32_t max_pwrite_zeroes;
> +    /* Maximum number of bytes that can zeroized at once. Must be multiple of
> +     * pwrite_zeroes_alignment. May be 0 if no inherent 64-bit limit */

Is the comment still right?

Leaving as 0 is the easiest way for a driver to say "default limit", but
I would feel safer with the default being 2G-align rather than 63-bit
limit.  And it is a 63-bit limit, not 64-bit, if the driver opts in to
INT64_MAX.

> +    int64_t max_pwrite_zeroes;
>  
>      /* Optimal alignment for write zeroes requests in bytes. A power
>       * of 2 is best but not mandatory.  Must be a multiple of
> diff --git a/block/io.c b/block/io.c
> index 55095dd08e..79e600af27 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -1836,7 +1836,7 @@ static int coroutine_fn bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,
>      int head = 0;
>      int tail = 0;
>  
> -    int max_write_zeroes = MIN_NON_ZERO(bs->bl.max_pwrite_zeroes, INT_MAX);
> +    int64_t max_write_zeroes = MIN_NON_ZERO(bs->bl.max_pwrite_zeroes, INT_MAX);

You are correct that for now we have no behavior change; a driver opting
in to a larger limit will still be clamped until we revisit this patch
later to drop the MIN() - but I agree with your approach of keeping
MIN() here until all drivers are audited.

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