qemu-devel
[Top][All Lists]
Advanced
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Denis V. Lunev
Subject: Re: [Qemu-devel] [PATCH] block: introduce BDRV_REQUEST_MAX_SECTORS
Date: Fri, 6 Feb 2015 12:09:02 +0300
User-agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:31.0) Gecko/20100101 Thunderbird/31.4.0 
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 <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 :)

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
reply via email to
[Prev in Thread] Current Thread [Next in Thread]