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 <ebl...@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
>
