Message ID | 1422965567-8611-1-git-send-email-pl@kamp.de |
---|---|
State | New |
Headers | show |
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 <pl@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 <den@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 :) Den
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 <pl@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 <den@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
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
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 <pl@kamp.de> --- block.c | 19 ++++++++----------- hw/block/virtio-blk.c | 4 ++-- include/block/block.h | 3 +++ 3 files changed, 13 insertions(+), 13 deletions(-)