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

[Fam Zheng <famz@redhat.com>]

On Thu, 06/23 16:37, 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.  The BlockLimits type is now completely
> byte-based; and in iscsi.c, sector_limits_lun2qemu() is no
> longer needed.
> 
> pdiscard_alignment is made unsigned (we use power-of-2 alignments
> as bitmasks, where unsigned is easier to think about) while
> leaving max_pdiscard signed (since we still have an 'int'
> interface); this is comparable to what commit cf081fc did for
> write zeroes limits.  We may later want to make everything an
> unsigned 64-bit limit - but that requires a bigger code audit.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> 
> ---
> v3: split out write_zeroes wording tweaks, improve commit message
> v2: rebase nbd and iscsi limits across earlier improvements
> ---
>  include/block/block_int.h | 14 ++++++++++----
>  block/io.c                | 16 +++++++++-------
>  block/iscsi.c             | 19 ++++++-------------
>  block/nbd.c               |  2 +-
>  qemu-img.c                |  3 ++-
>  5 files changed, 28 insertions(+), 26 deletions(-)
> 
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 7a4a00f..388ef80 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -324,11 +324,17 @@ 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;
> 
> -    /* 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

s/max_discard/max_pdiscard/

> +     * 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), should be multiple of
> diff --git a/block/io.c b/block/io.c
> index 8ca9d43..0f15d05 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -2368,19 +2368,21 @@ int coroutine_fn bdrv_co_discard(BlockDriverState *bs, int64_t sector_num,
>          goto out;
>      }
> 
> -    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;

Or just

               num = discard_alignment - sector_num % discard_alignment;

without the if.

Otherwise looks good,

Reviewed-by: Fam Zheng <famz@redhat.com>

>          }
> 
>          /* limit request size */
> diff --git a/block/iscsi.c b/block/iscsi.c
> index 368687d..0d16c31 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) {