Re: [PATCH v4 05/11] block: use int64_t instead of uint64_t in copy_range driver handlers

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 05/11] block: use int64_t instead of uint64_t in copy_range driver handlers
Date: Tue, 11 May 2021 16:14:51 -0500	[thread overview]
Message-ID: <0d5f5ba8-97f4-ce8d-311f-795458df6399@redhat.com> (raw)
In-Reply-To: <20210324205132.464899-6-vsementsov@virtuozzo.com>

On 3/24/21 3:51 PM, Vladimir Sementsov-Ogievskiy wrote:
> We are generally moving to int64_t for both offset and bytes parameters
> on all io paths.
> 
> Main motivation is realization of 64-bit write_zeroes operation for
> fast zeroing large disk chunks, up to the whole disk.
> 
> We chose signed type, to be consistent with off_t (which is signed) and
> with possibility for signed return type (where negative value means
> error).
> 
> So, convert driver copy_range handlers parameters which are already
> 64bit to signed type.
> 
> Now let's consider all callers. Simple
> 
>   git grep '\->bdrv_co_copy_range'
> 
> shows the only caller:
> 
>   bdrv_co_copy_range_internal(), which doesn bdrv_check_request32(),

s/doesn/does/

>   so everything is OK.
> 
> Still, the functions may be called directly, not only by drv->...
> Let's check:
> 
> git grep '\.bdrv_co_copy_range_\(from\|to\)\s*=' | \
> awk '{print $4}' | sed 's/,//' | sed 's/&//' | sort | uniq | \
> while read func; do git grep "$func(" | \
> grep -v "$func(BlockDriverState"; done
> 
> shows no more callers. So, we are done.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  include/block/block_int.h | 12 ++++++------
>  block/file-posix.c        | 10 +++++-----
>  block/iscsi.c             | 12 ++++++------
>  block/qcow2.c             | 12 ++++++------
>  block/raw-format.c        | 16 ++++++++--------
>  5 files changed, 31 insertions(+), 31 deletions(-)

Fewer drivers implement this, so easier review :)

> +++ b/block/qcow2.c
> @@ -3975,9 +3975,9 @@ static coroutine_fn int qcow2_co_pdiscard(BlockDriverState *bs,
>  
>  static int coroutine_fn
>  qcow2_co_copy_range_from(BlockDriverState *bs,
> -                         BdrvChild *src, uint64_t src_offset,
> -                         BdrvChild *dst, uint64_t dst_offset,
> -                         uint64_t bytes, BdrvRequestFlags read_flags,
> +                         BdrvChild *src, int64_t src_offset,
> +                         BdrvChild *dst, int64_t dst_offset,
> +                         int64_t bytes, BdrvRequestFlags read_flags,
>                           BdrvRequestFlags write_flags)
>  {
>      BDRVQcow2State *s = bs->opaque;

The use of cur_bytes = MIN(bytes, INT_MAX) looks odd, when we could
instead clamp to a nicer aligned boundary.  As noted before,
qcow2_get_host_offset() then further clamps cur_bytes, and the caller is
already splitting up requests larger than 2G, but copy_from is one of
those interfaces that is actually designed to have potentially nice
speedups with large byte ranges within the same filesystem (faster than
what is possible with usual pread/pwrite).  Then again, that's probably
more for file-posix (where we know the bytes are contiguous) than qcow2
(where we do have to worry about whether clusters are contiguous), so
we'll still have to keep a while(bytes) fragmenting loop in this
function.  Thoughts for a future patch, doesn't affect correctness of
this one.

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

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