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