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

[Vladimir Sementsov-Ogievskiy]

07.06.2021 21:13, Eric Blake wrote:
> 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?

I don't think so.. We just call glfs_discard_async() function which is not part 
of Qemu. So we shouldn't assume any extra restrictions except for argument 
types I think. byte is size_t, so maximum is SIZE_MAX.

> > +++ 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?

Yes, will add, we are storing it to list.num which is uint32_t and don't want 
it to be overflowed

> > +++ 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?

anyway, block/sheepdog.c is absent now :)


--
Best regards,
Vladimir