[Qemu-devel] [PATCH] block: Swap request limit definitions

[Qemu-devel] [PATCH] block: Swap request limit definitions

[Max Reitz]

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

Re: [Qemu-devel] [PATCH] block: Swap request limit definitions

[Fam Zheng]

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>

Re: [Qemu-devel] [Qemu-block] [PATCH] block: Swap request limit definitions

[Alberto Garcia]

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

Re: [Qemu-devel] [Qemu-block] [PATCH] block: Swap request limit definitions

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 1812 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH] block: Swap request limit definitions

[Alberto Garcia]

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

Re: [Qemu-devel] [Qemu-block] [PATCH] block: Swap request limit definitions

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 999 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH] block: Swap request limit definitions

[Kevin Wolf]

[-- Attachment #1: Type: text/plain, Size: 1265 bytes --]

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

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH] block: Swap request limit definitions

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 1826 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH] block: Swap request limit definitions

[Kevin Wolf]

[-- Attachment #1: Type: text/plain, Size: 2063 bytes --]

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

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH] block: Swap request limit definitions

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 2441 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]