Re: [PATCH v5 10/11] block: use int64_t instead of int in driver discard handlers

[Eric Blake <eblake@redhat.com>]

On Wed, May 05, 2021 at 10:50:00AM +0300, 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 discard handlers bytes parameter to int64_t.
> 
> The only caller of all updated function is bdrv_co_pdiscard in
> block/io.c. It is already prepared to work with 64bit requests, but
> pass at most max(bs->bl.max_pdiscard, INT_MAX) to the driver.
> 
> Let's look at all updated functions:
> 
> backup-top: pass to bdrv_co_pdiscard which is 64bit

and to backup_top_cbw, but that is also 64-bit

> 
> blkdebug: all calculations are still OK, thanks to
>   bdrv_check_qiov_request().
>   both rule_check and bdrv_co_pdiscard are 64bit
> 
> blklogwrites: pass to blk_loc_writes_co_log which is 64bit
> 
> blkreply, copy-on-read, filter-compress: pass to bdrv_co_pdiscard, OK

blkreplay

> 
> file-posix: one handler calls raw_account_discard() is 64bit and both
>   handlers calls raw_do_pdiscard(). Update raw_do_pdiscard, which pass
>   to RawPosixAIOData::aio_nbytes, which is 64bit (and calls
>   raw_account_discard())
> 
> gluster: somehow, third argument of glfs_discard_async is size_t.
>   Let's set max_pdiscard accordingly.
> 
> iscsi: iscsi_allocmap_set_invalid is 64bit,
>   !is_byte_request_lun_aligned is 64bit.
>   list.num is uint32_t. Let's clarify max_pdiscard and
>   pdiscard_alignment.

The patch tweaks max_pdiscard, but doesn't change pdiscard_alignment.

> 
> mirror_top, preallocate: pass to bdrv_mirror_top_do_write() which is
>   64bit

file is mirror.c, not mirror-top.c.  But it matches the BlockDriver
bdrv_mirror_top name.  preallocate does not call
bdrv_mirror_top_do_write, so it's probably worth separating that line
out.

> 
> nbd: protocol limitation. max_pdiscard is alredy set strict enough,
>   keep it as is for now.
> 
> nvmd: buf.nlb is uint32_t and we do shift. So, add corresponding limits
>   to nvme_refresh_limits().

nvme

> 
> qcow2: calculations are still OK, thanks to bdrv_check_qiov_request(),
>   qcow2_cluster_discard() is 64bit.
> 
> raw-format: raw_adjust_offset() is 64bit, bdrv_co_pdiscard too.
> 
> sheepdog: the format is deprecated. Don't care and just make old
>   INT_MAX limit to be explicit
> 
> throttle: pass to bdrv_co_pdiscard() which is 64bit and to
>   throttle_group_co_io_limits_intercept() which is 64bit as well.
> 
> test-block-iothread: bytes argument is unused
> 
> Great! Now all drivers are prepared to 64bit discard requests or has
> explicit max_pdiscard limit.

are prepared to handle 64-bit discard requests, or else have explicit
max_pdiscard limits.

> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  include/block/block_int.h        |  2 +-
>  block/backup-top.c               |  2 +-
>  block/blkdebug.c                 |  2 +-
>  block/blklogwrites.c             |  4 ++--
>  block/blkreplay.c                |  2 +-
>  block/copy-on-read.c             |  2 +-
>  block/file-posix.c               |  7 ++++---
>  block/filter-compress.c          |  2 +-
>  block/gluster.c                  |  7 +++++--
>  block/iscsi.c                    | 10 +++++-----
>  block/mirror.c                   |  2 +-
>  block/nbd.c                      |  6 ++++--
>  block/nvme.c                     | 14 +++++++++++++-
>  block/preallocate.c              |  2 +-
>  block/qcow2.c                    |  2 +-
>  block/raw-format.c               |  2 +-
>  block/sheepdog.c                 | 15 ++++++++++++++-
>  block/throttle.c                 |  2 +-
>  tests/unit/test-block-iothread.c |  2 +-
>  block/trace-events               |  4 ++--
>  20 files changed, 61 insertions(+), 30 deletions(-)
> 

> +++ b/block/gluster.c
> @@ -891,6 +891,7 @@ out:
>  static void qemu_gluster_refresh_limits(BlockDriverState *bs, Error **errp)
>  {
>      bs->bl.max_transfer = GLUSTER_MAX_TRANSFER;
> +    bs->bl.max_pdiscard = SIZE_MAX;

We probably want this to be MIN(GLUSTER_MAX_TRANSFER, SIZE_MAX). Also,
do we want to round it down to alignment boundaries?

> +++ b/block/iscsi.c
> @@ -1141,7 +1141,8 @@ iscsi_getlength(BlockDriverState *bs)
>  }
>  
>  static int
> -coroutine_fn iscsi_co_pdiscard(BlockDriverState *bs, int64_t offset, int bytes)
> +coroutine_fn iscsi_co_pdiscard(BlockDriverState *bs, int64_t offset,
> +                               int64_t bytes)
>  {
>      IscsiLun *iscsilun = bs->opaque;
>      struct IscsiTask iTask;

Did you want to add some sort of assert(bytes / iscsilun->block_size
<= UINT32_MAX), or a comment that we are relying on bl.max_pdiscard?

> +++ b/block/sheepdog.c

> +static void sd_refresh_limits(BlockDriverState *bs, Error **errp)
> +{
> +    bs->bl.max_pdiscard = INT_MAX;

Do we want to round this down to alignment?

Looks close!

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