block: Swap request limit definitions
diff mbox

Message ID 20170212014724.10618-1-mreitz@redhat.com
State New
Headers show

Commit Message

Max Reitz Feb. 12, 2017, 1:47 a.m. UTC
Defining BDRV_REQUEST_MAX_SECTORS based on BDRV_REQUEST_MAX_BYTES is
simpler than the other way around.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 include/block/block.h | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

Comments

Fam Zheng Feb. 13, 2017, 5:52 a.m. UTC | #1
On Sun, 02/12 02:47, Max Reitz wrote:
> Defining BDRV_REQUEST_MAX_SECTORS based on BDRV_REQUEST_MAX_BYTES is
> simpler than the other way around.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  include/block/block.h | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/include/block/block.h b/include/block/block.h
> index 4e81f2069b..101ef33f6b 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -114,9 +114,8 @@ typedef struct HDGeometry {
>  #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)
> -#define BDRV_REQUEST_MAX_BYTES (BDRV_REQUEST_MAX_SECTORS << BDRV_SECTOR_BITS)
> +#define BDRV_REQUEST_MAX_BYTES      MIN(SIZE_MAX, INT_MAX)
> +#define BDRV_REQUEST_MAX_SECTORS    (BDRV_REQUEST_MAX_BYTES >> BDRV_SECTOR_BITS)
>  
>  /*
>   * Allocation status flags
> -- 
> 2.11.0
> 
> 

Reviewed-by: Fam Zheng <famz@redhat.com>
Alberto Garcia Feb. 13, 2017, 8:39 a.m. UTC | #2
On Sun 12 Feb 2017 02:47:24 AM CET, Max Reitz <mreitz@redhat.com> wrote:

> -#define BDRV_REQUEST_MAX_SECTORS MIN(SIZE_MAX >> BDRV_SECTOR_BITS, \
> -                                     INT_MAX >> BDRV_SECTOR_BITS)
> -#define BDRV_REQUEST_MAX_BYTES (BDRV_REQUEST_MAX_SECTORS << BDRV_SECTOR_BITS)
> +#define BDRV_REQUEST_MAX_BYTES      MIN(SIZE_MAX, INT_MAX)
> +#define BDRV_REQUEST_MAX_SECTORS    (BDRV_REQUEST_MAX_BYTES >> BDRV_SECTOR_BITS)

I'm just pointing it out because I don't know if this can cause
problems, but this patch would make BDRV_REQUEST_MAX_BYTES not a
multiple of the sector size (INT_MAX is actually a prime number).

Berto
Max Reitz Feb. 13, 2017, 5:13 p.m. UTC | #3
On 13.02.2017 09:39, Alberto Garcia wrote:
> On Sun 12 Feb 2017 02:47:24 AM CET, Max Reitz <mreitz@redhat.com> wrote:
> 
>> -#define BDRV_REQUEST_MAX_SECTORS MIN(SIZE_MAX >> BDRV_SECTOR_BITS, \
>> -                                     INT_MAX >> BDRV_SECTOR_BITS)
>> -#define BDRV_REQUEST_MAX_BYTES (BDRV_REQUEST_MAX_SECTORS << BDRV_SECTOR_BITS)
>> +#define BDRV_REQUEST_MAX_BYTES      MIN(SIZE_MAX, INT_MAX)
>> +#define BDRV_REQUEST_MAX_SECTORS    (BDRV_REQUEST_MAX_BYTES >> BDRV_SECTOR_BITS)
> 
> I'm just pointing it out because I don't know if this can cause
> problems, but this patch would make BDRV_REQUEST_MAX_BYTES not a
> multiple of the sector size (INT_MAX is actually a prime number).

Very good point. I don't think this could be an issue, though. For one
thing, the use of BDRV_REQUEST_MAX_BYTES is very limited.

Apart from that, I guess the main issue would be that if you send an
unaligned head/tail the automatic round-up would exceed
BDRV_REQUEST_MAX_SECTORS. But this is a pre-existing issue, actually:
When you issue a request with an unaligned head and 2G - 512 bytes
length, bdrv_co_pwritev() will align it to 2G (which is greater than
INT_MAX).

(Can be tested trivially with:

$ qemu-io -c 'write 511 2147483136' --image-opts driver=null-co,size=4G

and some debugging code in block/io.c)

You can make the request even bigger by increasing the
bs->bl.request_alignment.

Not a big issue, though, as bdrv_aligned_p{read,write}v() actually take
an unsigned int and break down requests greater than INT_MAX
automatically. As long as bs->bl.request_alignment stays under 1G or so
we'll be fine (i.e. as long as we don't get an unsigned int overflow) --
and when it breaks it won't be because BDRV_REQUEST_MAX_BYTES is not
aligned to sectors.

Max
Alberto Garcia Feb. 14, 2017, 9:52 a.m. UTC | #4
On Mon 13 Feb 2017 06:13:38 PM CET, Max Reitz wrote:

>>> -#define BDRV_REQUEST_MAX_SECTORS MIN(SIZE_MAX >> BDRV_SECTOR_BITS, \
>>> -                                     INT_MAX >> BDRV_SECTOR_BITS)
>>> -#define BDRV_REQUEST_MAX_BYTES (BDRV_REQUEST_MAX_SECTORS << BDRV_SECTOR_BITS)
>>> +#define BDRV_REQUEST_MAX_BYTES      MIN(SIZE_MAX, INT_MAX)
>>> +#define BDRV_REQUEST_MAX_SECTORS    (BDRV_REQUEST_MAX_BYTES >> BDRV_SECTOR_BITS)
>> 
>> I'm just pointing it out because I don't know if this can cause
>> problems, but this patch would make BDRV_REQUEST_MAX_BYTES not a
>> multiple of the sector size (INT_MAX is actually a prime number).
>
> Very good point. I don't think this could be an issue, though. For one
> thing, the use of BDRV_REQUEST_MAX_BYTES is very limited.

Ok, but then I wonder what's the benefit of increasing
BDRV_REQUEST_MAX_BYTES.

Berto
Max Reitz Feb. 15, 2017, 1:42 p.m. UTC | #5
On 14.02.2017 10:52, Alberto Garcia wrote:
> On Mon 13 Feb 2017 06:13:38 PM CET, Max Reitz wrote:
> 
>>>> -#define BDRV_REQUEST_MAX_SECTORS MIN(SIZE_MAX >> BDRV_SECTOR_BITS, \
>>>> -                                     INT_MAX >> BDRV_SECTOR_BITS)
>>>> -#define BDRV_REQUEST_MAX_BYTES (BDRV_REQUEST_MAX_SECTORS << BDRV_SECTOR_BITS)
>>>> +#define BDRV_REQUEST_MAX_BYTES      MIN(SIZE_MAX, INT_MAX)
>>>> +#define BDRV_REQUEST_MAX_SECTORS    (BDRV_REQUEST_MAX_BYTES >> BDRV_SECTOR_BITS)
>>>
>>> I'm just pointing it out because I don't know if this can cause
>>> problems, but this patch would make BDRV_REQUEST_MAX_BYTES not a
>>> multiple of the sector size (INT_MAX is actually a prime number).
>>
>> Very good point. I don't think this could be an issue, though. For one
>> thing, the use of BDRV_REQUEST_MAX_BYTES is very limited.
> 
> Ok, but then I wonder what's the benefit of increasing
> BDRV_REQUEST_MAX_BYTES.

The benefit is that the definition looks cleaner.

Max
Kevin Wolf Feb. 15, 2017, 4:44 p.m. UTC | #6
Am 15.02.2017 um 14:42 hat Max Reitz geschrieben:
> On 14.02.2017 10:52, Alberto Garcia wrote:
> > On Mon 13 Feb 2017 06:13:38 PM CET, Max Reitz wrote:
> > 
> >>>> -#define BDRV_REQUEST_MAX_SECTORS MIN(SIZE_MAX >> BDRV_SECTOR_BITS, \
> >>>> -                                     INT_MAX >> BDRV_SECTOR_BITS)
> >>>> -#define BDRV_REQUEST_MAX_BYTES (BDRV_REQUEST_MAX_SECTORS << BDRV_SECTOR_BITS)
> >>>> +#define BDRV_REQUEST_MAX_BYTES      MIN(SIZE_MAX, INT_MAX)
> >>>> +#define BDRV_REQUEST_MAX_SECTORS    (BDRV_REQUEST_MAX_BYTES >> BDRV_SECTOR_BITS)
> >>>
> >>> I'm just pointing it out because I don't know if this can cause
> >>> problems, but this patch would make BDRV_REQUEST_MAX_BYTES not a
> >>> multiple of the sector size (INT_MAX is actually a prime number).
> >>
> >> Very good point. I don't think this could be an issue, though. For one
> >> thing, the use of BDRV_REQUEST_MAX_BYTES is very limited.
> > 
> > Ok, but then I wonder what's the benefit of increasing
> > BDRV_REQUEST_MAX_BYTES.
> 
> The benefit is that the definition looks cleaner.

Whatever way we want to write it, I think MAX_BYTES = MAX_SECTORS * 512
should be a given. Everything else is bound to confuse people and
introduce bugs sooner or later.

Kevin
Max Reitz Feb. 15, 2017, 4:48 p.m. UTC | #7
On 15.02.2017 17:44, Kevin Wolf wrote:
> Am 15.02.2017 um 14:42 hat Max Reitz geschrieben:
>> On 14.02.2017 10:52, Alberto Garcia wrote:
>>> On Mon 13 Feb 2017 06:13:38 PM CET, Max Reitz wrote:
>>>
>>>>>> -#define BDRV_REQUEST_MAX_SECTORS MIN(SIZE_MAX >> BDRV_SECTOR_BITS, \
>>>>>> -                                     INT_MAX >> BDRV_SECTOR_BITS)
>>>>>> -#define BDRV_REQUEST_MAX_BYTES (BDRV_REQUEST_MAX_SECTORS << BDRV_SECTOR_BITS)
>>>>>> +#define BDRV_REQUEST_MAX_BYTES      MIN(SIZE_MAX, INT_MAX)
>>>>>> +#define BDRV_REQUEST_MAX_SECTORS    (BDRV_REQUEST_MAX_BYTES >> BDRV_SECTOR_BITS)
>>>>>
>>>>> I'm just pointing it out because I don't know if this can cause
>>>>> problems, but this patch would make BDRV_REQUEST_MAX_BYTES not a
>>>>> multiple of the sector size (INT_MAX is actually a prime number).
>>>>
>>>> Very good point. I don't think this could be an issue, though. For one
>>>> thing, the use of BDRV_REQUEST_MAX_BYTES is very limited.
>>>
>>> Ok, but then I wonder what's the benefit of increasing
>>> BDRV_REQUEST_MAX_BYTES.
>>
>> The benefit is that the definition looks cleaner.
> 
> Whatever way we want to write it, I think MAX_BYTES = MAX_SECTORS * 512
> should be a given. Everything else is bound to confuse people and
> introduce bugs sooner or later.

Probably only sooner and not later, considering we are switching to byte
granularity overall anyway. And if something confuses people, I'd argue
it's the fact that we still have sector granularity all over the place
and not that your requests can be a bit bigger if you submit them in
bytes than if you submit them in sectors.

Anyway, if MAX_BYTES should be a multiple of the sector size, then I
can't think of a much better way to write this than what we currently
have and this patch is unneeded.

Max
Kevin Wolf Feb. 15, 2017, 5:10 p.m. UTC | #8
Am 15.02.2017 um 17:48 hat Max Reitz geschrieben:
> On 15.02.2017 17:44, Kevin Wolf wrote:
> > Am 15.02.2017 um 14:42 hat Max Reitz geschrieben:
> >> On 14.02.2017 10:52, Alberto Garcia wrote:
> >>> On Mon 13 Feb 2017 06:13:38 PM CET, Max Reitz wrote:
> >>>
> >>>>>> -#define BDRV_REQUEST_MAX_SECTORS MIN(SIZE_MAX >> BDRV_SECTOR_BITS, \
> >>>>>> -                                     INT_MAX >> BDRV_SECTOR_BITS)
> >>>>>> -#define BDRV_REQUEST_MAX_BYTES (BDRV_REQUEST_MAX_SECTORS << BDRV_SECTOR_BITS)
> >>>>>> +#define BDRV_REQUEST_MAX_BYTES      MIN(SIZE_MAX, INT_MAX)
> >>>>>> +#define BDRV_REQUEST_MAX_SECTORS    (BDRV_REQUEST_MAX_BYTES >> BDRV_SECTOR_BITS)
> >>>>>
> >>>>> I'm just pointing it out because I don't know if this can cause
> >>>>> problems, but this patch would make BDRV_REQUEST_MAX_BYTES not a
> >>>>> multiple of the sector size (INT_MAX is actually a prime number).
> >>>>
> >>>> Very good point. I don't think this could be an issue, though. For one
> >>>> thing, the use of BDRV_REQUEST_MAX_BYTES is very limited.
> >>>
> >>> Ok, but then I wonder what's the benefit of increasing
> >>> BDRV_REQUEST_MAX_BYTES.
> >>
> >> The benefit is that the definition looks cleaner.
> > 
> > Whatever way we want to write it, I think MAX_BYTES = MAX_SECTORS * 512
> > should be a given. Everything else is bound to confuse people and
> > introduce bugs sooner or later.
> 
> Probably only sooner and not later, considering we are switching to byte
> granularity overall anyway. And if something confuses people, I'd argue
> it's the fact that we still have sector granularity all over the place
> and not that your requests can be a bit bigger if you submit them in
> bytes than if you submit them in sectors.
> 
> Anyway, if MAX_BYTES should be a multiple of the sector size, then I
> can't think of a much better way to write this than what we currently
> have and this patch is unneeded.

Maybe we can just get rid of BDRV_REQUEST_MAX_SECTORS? Or do we need to
do a few more conversion before that?

Kevin
Max Reitz Feb. 15, 2017, 5:15 p.m. UTC | #9
On 15.02.2017 18:10, Kevin Wolf wrote:
> Am 15.02.2017 um 17:48 hat Max Reitz geschrieben:
>> On 15.02.2017 17:44, Kevin Wolf wrote:
>>> Am 15.02.2017 um 14:42 hat Max Reitz geschrieben:
>>>> On 14.02.2017 10:52, Alberto Garcia wrote:
>>>>> On Mon 13 Feb 2017 06:13:38 PM CET, Max Reitz wrote:
>>>>>
>>>>>>>> -#define BDRV_REQUEST_MAX_SECTORS MIN(SIZE_MAX >> BDRV_SECTOR_BITS, \
>>>>>>>> -                                     INT_MAX >> BDRV_SECTOR_BITS)
>>>>>>>> -#define BDRV_REQUEST_MAX_BYTES (BDRV_REQUEST_MAX_SECTORS << BDRV_SECTOR_BITS)
>>>>>>>> +#define BDRV_REQUEST_MAX_BYTES      MIN(SIZE_MAX, INT_MAX)
>>>>>>>> +#define BDRV_REQUEST_MAX_SECTORS    (BDRV_REQUEST_MAX_BYTES >> BDRV_SECTOR_BITS)
>>>>>>>
>>>>>>> I'm just pointing it out because I don't know if this can cause
>>>>>>> problems, but this patch would make BDRV_REQUEST_MAX_BYTES not a
>>>>>>> multiple of the sector size (INT_MAX is actually a prime number).
>>>>>>
>>>>>> Very good point. I don't think this could be an issue, though. For one
>>>>>> thing, the use of BDRV_REQUEST_MAX_BYTES is very limited.
>>>>>
>>>>> Ok, but then I wonder what's the benefit of increasing
>>>>> BDRV_REQUEST_MAX_BYTES.
>>>>
>>>> The benefit is that the definition looks cleaner.
>>>
>>> Whatever way we want to write it, I think MAX_BYTES = MAX_SECTORS * 512
>>> should be a given. Everything else is bound to confuse people and
>>> introduce bugs sooner or later.
>>
>> Probably only sooner and not later, considering we are switching to byte
>> granularity overall anyway. And if something confuses people, I'd argue
>> it's the fact that we still have sector granularity all over the place
>> and not that your requests can be a bit bigger if you submit them in
>> bytes than if you submit them in sectors.
>>
>> Anyway, if MAX_BYTES should be a multiple of the sector size, then I
>> can't think of a much better way to write this than what we currently
>> have and this patch is unneeded.
> 
> Maybe we can just get rid of BDRV_REQUEST_MAX_SECTORS? Or do we need to
> do a few more conversion before that?

We probably could, but it wouldn't make much sense. We have many places
that use BDRV_REQUEST_MAX_SECTORS without multiplying it by 512 and if
we decided to use *_MAX_BYTES dividing it by 512 in every one of those
places, we could just as well define a central macro for that -- which
would be *_MAX_SECTORS, so...

Max

Patch
diff mbox

diff --git a/include/block/block.h b/include/block/block.h
index 4e81f2069b..101ef33f6b 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -114,9 +114,8 @@  typedef struct HDGeometry {
 #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)
-#define BDRV_REQUEST_MAX_BYTES (BDRV_REQUEST_MAX_SECTORS << BDRV_SECTOR_BITS)
+#define BDRV_REQUEST_MAX_BYTES      MIN(SIZE_MAX, INT_MAX)
+#define BDRV_REQUEST_MAX_SECTORS    (BDRV_REQUEST_MAX_BYTES >> BDRV_SECTOR_BITS)
 
 /*
  * Allocation status flags