Re: [PATCH v3 04/17] block/io: use int64_t bytes in driver wrappers

[Alberto Garcia]

On Thu 30 Apr 2020 01:10:20 PM CEST, 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 wrappers parameters which are already 64bit to
> signed type.
>
> Patch-correctness audit by Eric Blake:
>
>   bdrv_driver_preadv
>
>     Remains 64 bits, the question is whether any caller could pass in
>     something larger than 63 bits.  Callers are:
>
>     bdrv_co_do_copy_on_readv() - passes 'int64_t pnum', but sets pnum
>       in a fragmenting loop, MAX_BOUNCE_BUFFER when copy-on-read is
>       needed, or max_transfer bounded by BDRV_REQUEST_MAX_BYTES
>       otherwise
>     bdrv_aligned_preadv() - passes 'unsigned int bytes' further limited
>       by fragmenting loop on max_transfer <= INT_MAX
>
>     Input is bounded to < 2G, internal use of 'bytes' is:
>
>     drv->bdrv_co_preadv_part(uint64_t) - safe
>     qemu_iovec_init_slice(size_t) - potential truncation on 32-bit
>       platform [*]
>     drv->bdrv_co_preadv(uint64_t) - safe
>     drv->bdrv_aio_preadv(uint64_t) - safe
>     drv->bdrv_co_readv(int) after assertion of <2G - safe
>
>   bdrv_driver_pwritev
>
>     Remains 64 bits, the question is whether any caller could pass in
>     something larger than 63.  Callers are:
>
>     bdrv_co_do_copy_on_readv() - passes 'int64_t pnum', but set in a
>       fragmenting loop bounded by MAX_BOUNCE_BUFFER
>     bdrv_co_do_pwrite_zeroes() - passes 'int num'
>     bdrv_aligned_pwritev() - passes 'unsigned int bytes' further
>       limited by fragmenting loop on max_transfer <= INT_MAX
>
>     Input is bounded to < 2G, internal use of 'bytes' is:
>
>     drv->bdrv_co_pwritev_part(uint64_t) - safe
>     qemu_iovec_init_slice(size_t) - potential truncation on 32-bit
>       platform [*]
>     drv->bdrv_co_pwritev(uint64_t) - safe
>     drv->bdrv_aio_pwritev(uint64_t) - safe
>     drv->bdrv_co_writev(int) after assertion of <2G - safe
>
>   bdrv_driver_pwritev_compressed
>
>     bdrv_aligned_pwritev() - passes 'unsigned int bytes'
>
>     Input is bounded to < 4G, internal use of 'bytes' is:
>
>     drv->bdrv_co_pwritev_compressed_part(uint64_t) - safe
>     drv->bdrv_co_pwritev_compressed(uint64_t) - safe
>     qemu_iovec_init_slice(size_t) - potential truncation on 32-bit
>       platform [*]
>
> [*]: QEMUIOVector is in-RAM data, so size_t seems a valid type for
> it's operation. To avoid truncations we should require max_transfer to
> be not greater than SIZE_MAX, limiting RAM-occupying operations and
> QEMUIOVector usage. Still, 64bit discard and write_zeroes (which
> doesn't use QEMUIOVector) should work even on 32bit machines, not being
> limited by max_transfer.
>
> For now, we safe anyway, as all input goes through
> bdrv_aligned_pwritev() and bdrv_aligned_preadv(), which are limiting
> max_transfer to INT_MAX.
>
> Series: 64bit-block-status
> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> Reviewed-by: Eric Blake <address@hidden>

Reviewed-by: Alberto Garcia <address@hidden>

Berto