Re: [Qemu-devel] [PATCH v2 15/17] block: Switch discard length bounds to byte-based

[Fam Zheng <famz@redhat.com>]

On Tue, 06/14 15:30, Eric Blake wrote:
> Sector-based limits are awkward to think about; in our on-going
> quest to move to byte-based interfaces, convert max_discard and
> discard_alignment.  Rename them, using 'pdiscard' as an aid to
> track which remaining discard interfaces need conversion, and so
> that the compiler will help us catch the change in semantics
> across any rebased code.  In iscsi.c, sector_limits_lun2qemu()
> is no longer needed; and the BlockLimits type is now completely
> byte-based.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> 
> ---
> v2: rebase nbd and iscsi limits across earlier improvements
> ---
>  include/block/block_int.h | 21 +++++++++++++++------
>  block/io.c                | 16 +++++++++-------
>  block/iscsi.c             | 19 ++++++-------------
>  block/nbd.c               |  2 +-
>  qemu-img.c                |  3 ++-
>  5 files changed, 33 insertions(+), 28 deletions(-)
> 
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 2b45d57..0169019 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -324,18 +324,27 @@ struct BlockDriver {
>  };
> 
>  typedef struct BlockLimits {
> -    /* maximum number of sectors that can be discarded at once */
> -    int max_discard;
> +    /* maximum number of bytes that can be discarded at once (since it
> +     * is signed, it must be < 2G, if set), should be multiple of
> +     * pdiscard_alignment, but need not be power of 2. May be 0 if no
> +     * inherent 32-bit limit */
> +    int32_t max_pdiscard;

Why not use uint32_t?

> 
> -    /* optimal alignment for discard requests in sectors */
> -    int64_t discard_alignment;
> +    /* optimal alignment for discard requests in bytes, must be power
> +     * of 2, less than max_discard if that is set, and multiple of
> +     * bs->request_alignment. May be 0 if bs->request_alignment is
> +     * good enough */
> +    uint32_t pdiscard_alignment;
> 
>      /* maximum number of bytes that can zeroized at once (since it is
> -     * signed, it must be < 2G, if set) */
> +     * signed, it must be < 2G, if set), should be multiple of
> +     * pwrite_zeroes_alignment. May be 0 if no inherent 32-bit limit */
>      int32_t max_pwrite_zeroes;
> 
>      /* optimal alignment for write zeroes requests in bytes, must be
> -     * power of 2, and less than max_pwrite_zeroes if that is set */
> +     * power of 2, less than max_pwrite_zeroes if that is set, and
> +     * multiple of bs->request_alignment. May be 0 if
> +     * bs->request_alignment is good enough */
>      uint32_t pwrite_zeroes_alignment;
> 
>      /* optimal transfer length in bytes (must be power of 2, and
> diff --git a/block/io.c b/block/io.c
> index 5e38ab4..0b5c40d 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -2352,19 +2352,21 @@ int coroutine_fn bdrv_co_discard(BlockDriverState *bs, int64_t sector_num,
>                            BDRV_TRACKED_DISCARD);
>      bdrv_set_dirty(bs, sector_num, nb_sectors);
> 
> -    max_discard = MIN_NON_ZERO(bs->bl.max_discard, BDRV_REQUEST_MAX_SECTORS);
> +    max_discard = MIN_NON_ZERO(bs->bl.max_pdiscard >> BDRV_SECTOR_BITS,
> +                               BDRV_REQUEST_MAX_SECTORS);
>      while (nb_sectors > 0) {
>          int ret;
>          int num = nb_sectors;
> +        int discard_alignment = bs->bl.pdiscard_alignment >> BDRV_SECTOR_BITS;
> 
>          /* align request */
> -        if (bs->bl.discard_alignment &&
> -            num >= bs->bl.discard_alignment &&
> -            sector_num % bs->bl.discard_alignment) {
> -            if (num > bs->bl.discard_alignment) {
> -                num = bs->bl.discard_alignment;
> +        if (discard_alignment &&
> +            num >= discard_alignment &&
> +            sector_num % discard_alignment) {
> +            if (num > discard_alignment) {
> +                num = discard_alignment;
>              }
> -            num -= sector_num % bs->bl.discard_alignment;
> +            num -= sector_num % discard_alignment;
>          }
> 
>          /* limit request size */
> diff --git a/block/iscsi.c b/block/iscsi.c
> index 739d5ca..af9babf 100644
> --- a/block/iscsi.c
> +++ b/block/iscsi.c
> @@ -1696,13 +1696,6 @@ static void iscsi_close(BlockDriverState *bs)
>      memset(iscsilun, 0, sizeof(IscsiLun));
>  }
> 
> -static int sector_limits_lun2qemu(int64_t sector, IscsiLun *iscsilun)
> -{
> -    int limit = MIN(sector_lun2qemu(sector, iscsilun), INT_MAX / 2 + 1);
> -
> -    return limit < BDRV_REQUEST_MAX_SECTORS ? limit : 0;
> -}
> -
>  static void iscsi_refresh_limits(BlockDriverState *bs, Error **errp)
>  {
>      /* We don't actually refresh here, but just return data queried in
> @@ -1722,14 +1715,14 @@ static void iscsi_refresh_limits(BlockDriverState *bs, Error **errp)
>      }
> 
>      if (iscsilun->lbp.lbpu) {
> -        if (iscsilun->bl.max_unmap < 0xffffffff) {
> -            bs->bl.max_discard =
> -                sector_limits_lun2qemu(iscsilun->bl.max_unmap, iscsilun);
> +        if (iscsilun->bl.max_unmap < 0xffffffff / iscsilun->block_size) {
> +            bs->bl.max_pdiscard =
> +                iscsilun->bl.max_unmap * iscsilun->block_size;
>          }
> -        bs->bl.discard_alignment =
> -            sector_limits_lun2qemu(iscsilun->bl.opt_unmap_gran, iscsilun);
> +        bs->bl.pdiscard_alignment =
> +            iscsilun->bl.opt_unmap_gran * iscsilun->block_size;
>      } else {
> -        bs->bl.discard_alignment = iscsilun->block_size >> BDRV_SECTOR_BITS;
> +        bs->bl.pdiscard_alignment = iscsilun->block_size;
>      }
> 
>      if (iscsilun->bl.max_ws_len < 0xffffffff / iscsilun->block_size) {
> diff --git a/block/nbd.c b/block/nbd.c
> index f5511ea..08e5b67 100644
> --- a/block/nbd.c
> +++ b/block/nbd.c
> @@ -362,7 +362,7 @@ static int nbd_co_flush(BlockDriverState *bs)
> 
>  static void nbd_refresh_limits(BlockDriverState *bs, Error **errp)
>  {
> -    bs->bl.max_discard = NBD_MAX_SECTORS;
> +    bs->bl.max_pdiscard = NBD_MAX_BUFFER_SIZE;
>      bs->bl.max_transfer = NBD_MAX_BUFFER_SIZE;
>  }
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index cf9876d..1237d61 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -2084,7 +2084,8 @@ static int img_convert(int argc, char **argv)
>      bufsectors = MIN(32768,
>                       MAX(bufsectors,
>                           MAX(out_bs->bl.opt_transfer >> BDRV_SECTOR_BITS,
> -                             out_bs->bl.discard_alignment)));
> +                             out_bs->bl.pdiscard_alignment >>
> +                             BDRV_SECTOR_BITS)));
> 
>      if (skip_create) {
>          int64_t output_sectors = blk_nb_sectors(out_blk);
> -- 
> 2.5.5
>