Am 03.02.2015 um 16:33 schrieb Denis V. Lunev:
> On 03/02/15 17:30, Peter Lieven wrote:
>> Am 03.02.2015 um 14:29 schrieb 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 <p...@kamp.de>
>>>> ---
>>>> 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 <d...@openvz.org>
>>>
>>> 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 :)
>>
>> No, not completely. If we run into the check above the request fails.
>> If you set max_write_zeroes the request is appropiately trimmed.
>> So if the driver allows less than BDRV_REQUEST_MAX_SECTORS sectors
>> you still have to set it in the BlockLimits.
>>
>> Peter
>>
> your point will be correct after the following patch applied
>
> diff --git a/block.c b/block.c
> index 4e58b35..49e0073 100644
> --- a/block.c
> +++ b/block.c
> @@ -2647,7 +2647,7 @@ static int bdrv_check_byte_request(BlockDriverState
> *bs, i
> {
> int64_t len;
>
> - if (size > INT_MAX) {
> + if (size > BDRV_REQUEST_MAX_SECTORS << BDRV_SECTOR_BITS) {
> return -EIO;
> }
You are right, this will still fail if SIZE_MAX < INT_MAX. I would send a v2 of
my
patch and add the above. Then Kevin can remove your 2 patches, right?
Peter
>
> without we will fail at entry point inside bdrv_check_byte_request().
> My patch applies the limit of UINT32_MAX over the value which
> is not greater than INT_MAX in the current codebase.
> bdrv_check_byte_request() is performed at the very-very beginning
> of the processing, f.e
>
> bdrv_co_discard
> bdrv_check_request
> bdrv_check_byte_request <-- (INT_MAX limit applied)
> max_discard = MIN_NON_ZERO(bs->bl.max_discard, BDRV_REQUEST_MAX_SECTORS);
>
> Den
