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

[Eric Blake]

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 write handlers parameters which are already 64bit to
> signed type.
> 
> While being here, convert also flags parameter to be BdrvRequestFlags.
> 
> Now let's consider all callers. Simple
> 
>   git grep '\->bdrv_\(aio\|co\)_pwritev\(_part\)\?'
> 
> shows that's there two callers of driver function:
> 
>  bdrv_driver_pwritev() in block/io.c, passes int64_t, checked by
>    bdrv_check_qiov_request() to be non-negative.
> 
>  qcow2_save_vmstate() does bdrv_check_qiov_request().
> 
> Still, the functions may be called directly, not only by drv->...
> Let's check:
> 
> git grep '\.bdrv_\(aio\|co\)_pwritev\(_part\)\?\s*=' | \
> awk '{print $4}' | sed 's/,//' | sed 's/&//' | sort | uniq | \
> while read func; do git grep "$func(" | \
> grep -v "$func(BlockDriverState"; done
> 
> shows several callers:

...
Thanks for recording the audit.

> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  include/block/block_int.h        | 16 ++++++++--------
>  block/backup-top.c               |  6 +++---

>  30 files changed, 95 insertions(+), 86 deletions(-)
> 
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index e6bcf74e46..928369e0bc 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -237,8 +237,8 @@ struct BlockDriver {
>          int64_t offset, int64_t bytes, QEMUIOVector *qiov,
>          BdrvRequestFlags flags, BlockCompletionFunc *cb, void *opaque);
>      BlockAIOCB *(*bdrv_aio_pwritev)(BlockDriverState *bs,
> -        uint64_t offset, uint64_t bytes, QEMUIOVector *qiov, int flags,
> -        BlockCompletionFunc *cb, void *opaque);
> +        int64_t offset, int64_t bytes, QEMUIOVector *qiov,
> +        BdrvRequestFlags flags, BlockCompletionFunc *cb, void *opaque);
>      BlockAIOCB *(*bdrv_aio_flush)(BlockDriverState *bs,
>          BlockCompletionFunc *cb, void *opaque);
>      BlockAIOCB *(*bdrv_aio_pdiscard)(BlockDriverState *bs,
> @@ -287,10 +287,11 @@ struct BlockDriver {
>       * The buffer in @qiov may point directly to guest memory.
>       */
>      int coroutine_fn (*bdrv_co_pwritev)(BlockDriverState *bs,
> -        uint64_t offset, uint64_t bytes, QEMUIOVector *qiov, int flags);
> +        int64_t offset, int64_t bytes, QEMUIOVector *qiov,
> +        BdrvRequestFlags flags);
>      int coroutine_fn (*bdrv_co_pwritev_part)(BlockDriverState *bs,
> -        uint64_t offset, uint64_t bytes,
> -        QEMUIOVector *qiov, size_t qiov_offset, int flags);
> +        int64_t offset, int64_t bytes, QEMUIOVector *qiov, size_t 
> qiov_offset,
> +        BdrvRequestFlags flags);
>  
>      /*
>       * Efficiently zero a region of the disk image.  Typically an image 
> format
> @@ -428,10 +429,9 @@ struct BlockDriver {
>                                        Error **errp);
>  
>      int coroutine_fn (*bdrv_co_pwritev_compressed)(BlockDriverState *bs,
> -        uint64_t offset, uint64_t bytes, QEMUIOVector *qiov);
> +        int64_t offset, int64_t bytes, QEMUIOVector *qiov);
>      int coroutine_fn (*bdrv_co_pwritev_compressed_part)(BlockDriverState *bs,
> -        uint64_t offset, uint64_t bytes, QEMUIOVector *qiov,
> -        size_t qiov_offset);
> +        int64_t offset, int64_t bytes, QEMUIOVector *qiov, size_t 
> qiov_offset);

Someday it may be nice to convert remaining drivers of the old callbacks
to use the newer ones instead, for fewer callbacks to manage here.  But
not the problem of this patch.

> +++ b/block/backup-top.c
> @@ -97,9 +97,9 @@ static int coroutine_fn 
> backup_top_co_pwrite_zeroes(BlockDriverState *bs,
>  }
>  
>  static coroutine_fn int backup_top_co_pwritev(BlockDriverState *bs,
> -                                              uint64_t offset,
> -                                              uint64_t bytes,
> -                                              QEMUIOVector *qiov, int flags)
> +                                              int64_t offset, int64_t bytes,
> +                                              QEMUIOVector *qiov,
> +                                              BdrvRequestFlags flags)
>  {
>      int ret = backup_top_cbw(bs, offset, bytes, flags);

We should clean up the signature of backup_top_cbw() in a followup, now
that pdiscard, pwrite_zeroes, pwritev all pass int64_t.

> +++ b/block/blkdebug.c
> @@ -635,8 +635,8 @@ blkdebug_co_preadv(BlockDriverState *bs, int64_t offset, 
> int64_t bytes,
>  }
>  
>  static int coroutine_fn
> -blkdebug_co_pwritev(BlockDriverState *bs, uint64_t offset, uint64_t bytes,
> -                    QEMUIOVector *qiov, int flags)
> +blkdebug_co_pwritev(BlockDriverState *bs, int64_t offset, int64_t bytes,
> +                    QEMUIOVector *qiov, BdrvRequestFlags flags)
>  {
>      int err;

Similarly for rule_check().  I probably called out a lot of the same
spots in 3/11.

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

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