From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58661) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YJfIo-0008Ni-Ih for qemu-devel@nongnu.org; Fri, 06 Feb 2015 04:34:19 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YJfIl-0003fv-CE for qemu-devel@nongnu.org; Fri, 06 Feb 2015 04:34:18 -0500 Received: from mx2.parallels.com ([199.115.105.18]:38847) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YJfIl-0002w4-47 for qemu-devel@nongnu.org; Fri, 06 Feb 2015 04:34:15 -0500 Message-ID: <54D484AE.4040104@parallels.com> Date: Fri, 6 Feb 2015 12:09:02 +0300 From: "Denis V. Lunev" MIME-Version: 1.0 References: <1422965567-8611-1-git-send-email-pl@kamp.de> <54D0CD54.9080309@parallels.com> <54D0DB80.4070004@kamp.de> <54D0EA2D.2070900@parallels.com> <54D48010.9000908@kamp.de> In-Reply-To: <54D48010.9000908@kamp.de> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] block: introduce BDRV_REQUEST_MAX_SECTORS List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Lieven , qemu-devel@nongnu.org Cc: kwolf@redhat.com On 06/02/15 11:49, Peter Lieven wrote: > 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 >>>>> --- >>>>> 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 >>>> >>>> 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 > that's cool. Though in this case NBD patch will be necessary. Gluster code is correct. I'll re-spin it as some modification is necessary. >> 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