Re: [Qemu-devel] [PATCH] block: introduce BDRV_REQUEST_MAX_SECTORS

[Denis V. Lunev]

On 03/02/15 15:12, Peter Lieven wrote:
> we check and adjust request sizes at several places with
> sometimes inconsistent checks or default values:
>   INT_MAX
>   INT_MAX >> BDRV_SECTOR_BITS
>   UINT_MAX >> BDRV_SECTOR_BITS
>   SIZE_MAX >> BDRV_SECTOR_BITS
>
> This patches introdocues a macro for the maximal allowed sectors
> per request and uses it at several places.
>
> Signed-off-by: Peter Lieven <address@hidden>
> ---
>   block.c               | 19 ++++++++-----------
>   hw/block/virtio-blk.c |  4 ++--
>   include/block/block.h |  3 +++
>   3 files changed, 13 insertions(+), 13 deletions(-)
>
> diff --git a/block.c b/block.c
> index 8272ef9..4e58b35 100644
> --- a/block.c
> +++ b/block.c
> @@ -2671,7 +2671,7 @@ static int bdrv_check_byte_request(BlockDriverState *bs, 
> int64_t offset,
>   static int bdrv_check_request(BlockDriverState *bs, int64_t sector_num,
>                                 int nb_sectors)
>   {
> -    if (nb_sectors < 0 || nb_sectors > INT_MAX / BDRV_SECTOR_SIZE) {
> +    if (nb_sectors < 0 || nb_sectors > BDRV_REQUEST_MAX_SECTORS) {
>           return -EIO;
>       }
>   
> @@ -2758,7 +2758,7 @@ static int bdrv_rw_co(BlockDriverState *bs, int64_t sector_num, uint8_t *buf,
>           .iov_len = nb_sectors * BDRV_SECTOR_SIZE,
>       };
>   
> -    if (nb_sectors < 0 || nb_sectors > INT_MAX / BDRV_SECTOR_SIZE) {
> +    if (nb_sectors < 0 || nb_sectors > BDRV_REQUEST_MAX_SECTORS) {
>           return -EINVAL;
>       }
>   
> @@ -2826,13 +2826,10 @@ int bdrv_make_zero(BlockDriverState *bs, BdrvRequestFlags flags)
>       }
>   
>       for (;;) {
> -        nb_sectors = target_sectors - sector_num;
> +        nb_sectors = MIN(target_sectors - sector_num, 
> BDRV_REQUEST_MAX_SECTORS);
>           if (nb_sectors <= 0) {
>               return 0;
>           }
> -        if (nb_sectors > INT_MAX / BDRV_SECTOR_SIZE) {
> -            nb_sectors = INT_MAX / BDRV_SECTOR_SIZE;
> -        }
>           ret = bdrv_get_block_status(bs, sector_num, nb_sectors, &n);
>           if (ret < 0) {
>               error_report("error getting block status at sector %" PRId64 ": 
> %s",
> @@ -3167,7 +3164,7 @@ static int coroutine_fn bdrv_co_do_readv(BlockDriverState 
> *bs,
>       int64_t sector_num, int nb_sectors, QEMUIOVector *qiov,
>       BdrvRequestFlags flags)
>   {
> -    if (nb_sectors < 0 || nb_sectors > (UINT_MAX >> BDRV_SECTOR_BITS)) {
> +    if (nb_sectors < 0 || nb_sectors > BDRV_REQUEST_MAX_SECTORS) {
>           return -EINVAL;
>       }
>   
> @@ -3202,8 +3199,8 @@ static int coroutine_fn bdrv_co_do_write_zeroes(BlockDriverState *bs,
>       struct iovec iov = {0};
>       int ret = 0;
>   
> -    int max_write_zeroes = bs->bl.max_write_zeroes ?
> -                           bs->bl.max_write_zeroes : INT_MAX;
> +    int max_write_zeroes = MIN_NON_ZERO(bs->bl.max_write_zeroes,
> +                                        BDRV_REQUEST_MAX_SECTORS);
>   
>       while (nb_sectors > 0 && !ret) {
>           int num = nb_sectors;
> @@ -3458,7 +3455,7 @@ static int coroutine_fn 
> bdrv_co_do_writev(BlockDriverState *bs,
>       int64_t sector_num, int nb_sectors, QEMUIOVector *qiov,
>       BdrvRequestFlags flags)
>   {
> -    if (nb_sectors < 0 || nb_sectors > (INT_MAX >> BDRV_SECTOR_BITS)) {
> +    if (nb_sectors < 0 || nb_sectors > BDRV_REQUEST_MAX_SECTORS) {
>           return -EINVAL;
>       }
>   
> @@ -5120,7 +5117,7 @@ int coroutine_fn bdrv_co_discard(BlockDriverState *bs, int64_t sector_num,
>           return 0;
>       }
>   
> -    max_discard = bs->bl.max_discard ?  bs->bl.max_discard : INT_MAX;
> +    max_discard = MIN_NON_ZERO(bs->bl.max_discard, BDRV_REQUEST_MAX_SECTORS);
>       while (nb_sectors > 0) {
>           int ret;
>           int num = nb_sectors;
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index 8c51a29..1a8a176 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -381,7 +381,7 @@ void virtio_blk_submit_multireq(BlockBackend *blk, 
> MultiReqBuffer *mrb)
>       }
>   
>       max_xfer_len = blk_get_max_transfer_length(mrb->reqs[0]->dev->blk);
> -    max_xfer_len = MIN_NON_ZERO(max_xfer_len, INT_MAX);
> +    max_xfer_len = MIN_NON_ZERO(max_xfer_len, BDRV_REQUEST_MAX_SECTORS);
>   
>       qsort(mrb->reqs, mrb->num_reqs, sizeof(*mrb->reqs),
>             &multireq_compare);
> @@ -447,7 +447,7 @@ static bool virtio_blk_sect_range_ok(VirtIOBlock *dev,
>       uint64_t nb_sectors = size >> BDRV_SECTOR_BITS;
>       uint64_t total_sectors;
>   
> -    if (nb_sectors > INT_MAX) {
> +    if (nb_sectors > BDRV_REQUEST_MAX_SECTORS) {
>           return false;
>       }
>       if (sector & dev->sector_mask) {
> diff --git a/include/block/block.h b/include/block/block.h
> index 3082d2b..25a6d62 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -83,6 +83,9 @@ typedef enum {
>   #define BDRV_SECTOR_SIZE   (1ULL << BDRV_SECTOR_BITS)
>   #define BDRV_SECTOR_MASK   ~(BDRV_SECTOR_SIZE - 1)
>   
> +#define BDRV_REQUEST_MAX_SECTORS MIN(SIZE_MAX >> BDRV_SECTOR_BITS, \
> +                                     INT_MAX >> BDRV_SECTOR_BITS)
> +
>   /*
>    * Allocation status flags
>    * BDRV_BLOCK_DATA: data is read from bs->file or another file
Reviewed-by: Denis V. Lunev <address@hidden>

On the other hand the limitation to INT_MAX for a request size
(in bytes) is here.

bdrv_check_byte_request
    if (size > INT_MAX) {
        return -EIO;
    }

which means that all my patches from series
  [PATCH v3 0/2] fix max_discard for NBD/gluster block drivers
becomes unnecessary but this was not that obvious before this
clarification :)

Den