[v2] 64bit block-layer

[PATCH v2 01/17] block/throttle-groups: throttle_group_co_io_limits_intercept(): 64bit bytes

Posted by Vladimir Sementsov-Ogievskiy 5 years, 9 months ago

The function is called from 64bit io handlers, and bytes is just passed
to throttle_account() which is 64bit too (unsigned though). So, let's
convert intermediate argument to 64bit too.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 include/block/throttle-groups.h | 2 +-
 block/throttle-groups.c         | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/block/throttle-groups.h b/include/block/throttle-groups.h
index 712a8e64b4..f921994b8a 100644
--- a/include/block/throttle-groups.h
+++ b/include/block/throttle-groups.h
@@ -76,7 +76,7 @@ void throttle_group_unregister_tgm(ThrottleGroupMember *tgm);
 void throttle_group_restart_tgm(ThrottleGroupMember *tgm);
 
 void coroutine_fn throttle_group_co_io_limits_intercept(ThrottleGroupMember *tgm,
-                                                        unsigned int bytes,
+                                                        int64_t bytes,
                                                         bool is_write);
 void throttle_group_attach_aio_context(ThrottleGroupMember *tgm,
                                        AioContext *new_context);
diff --git a/block/throttle-groups.c b/block/throttle-groups.c
index 37695b0cd7..37d1b7a8b8 100644
--- a/block/throttle-groups.c
+++ b/block/throttle-groups.c
@@ -358,7 +358,7 @@ static void schedule_next_request(ThrottleGroupMember *tgm, bool is_write)
  * @is_write:  the type of operation (read/write)
  */
 void coroutine_fn throttle_group_co_io_limits_intercept(ThrottleGroupMember *tgm,
-                                                        unsigned int bytes,
+                                                        int64_t bytes,
                                                         bool is_write)
 {
     bool must_wait;
-- 
2.21.0

Re: [PATCH v2 01/17] block/throttle-groups: throttle_group_co_io_limits_intercept(): 64bit bytes

Posted by Philippe Mathieu-Daudé 5 years, 9 months ago

On 4/27/20 10:23 AM, Vladimir Sementsov-Ogievskiy wrote:
> The function is called from 64bit io handlers, and bytes is just passed
> to throttle_account() which is 64bit too (unsigned though). So, let's
> convert intermediate argument to 64bit too.

What is the meaning of negative bytes in this function?

> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   include/block/throttle-groups.h | 2 +-
>   block/throttle-groups.c         | 2 +-
>   2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/include/block/throttle-groups.h b/include/block/throttle-groups.h
> index 712a8e64b4..f921994b8a 100644
> --- a/include/block/throttle-groups.h
> +++ b/include/block/throttle-groups.h
> @@ -76,7 +76,7 @@ void throttle_group_unregister_tgm(ThrottleGroupMember *tgm);
>   void throttle_group_restart_tgm(ThrottleGroupMember *tgm);
>   
>   void coroutine_fn throttle_group_co_io_limits_intercept(ThrottleGroupMember *tgm,
> -                                                        unsigned int bytes,
> +                                                        int64_t bytes,
>                                                           bool is_write);
>   void throttle_group_attach_aio_context(ThrottleGroupMember *tgm,
>                                          AioContext *new_context);
> diff --git a/block/throttle-groups.c b/block/throttle-groups.c
> index 37695b0cd7..37d1b7a8b8 100644
> --- a/block/throttle-groups.c
> +++ b/block/throttle-groups.c
> @@ -358,7 +358,7 @@ static void schedule_next_request(ThrottleGroupMember *tgm, bool is_write)
>    * @is_write:  the type of operation (read/write)
>    */
>   void coroutine_fn throttle_group_co_io_limits_intercept(ThrottleGroupMember *tgm,
> -                                                        unsigned int bytes,
> +                                                        int64_t bytes,
>                                                           bool is_write)
>   {
>       bool must_wait;
>

Re: [PATCH v2 01/17] block/throttle-groups: throttle_group_co_io_limits_intercept(): 64bit bytes

Posted by Eric Blake 5 years, 9 months ago

On 4/27/20 5:05 AM, Philippe Mathieu-Daudé wrote:
> On 4/27/20 10:23 AM, Vladimir Sementsov-Ogievskiy wrote:
>> The function is called from 64bit io handlers, and bytes is just passed
>> to throttle_account() which is 64bit too (unsigned though). So, let's
>> convert intermediate argument to 64bit too.
> 
> What is the meaning of negative bytes in this function?

An error.  It is more for consistency, in that we really cannot access 
more than 63 bits of size information (off_t is signed, and network 
protocols don't magically add a 64th bit to that underlying inherent 
limitation of files and block devices).

It may be worth adding assert(bytes >= 0), if that makes it easier to 
prove we are only dealing with positive lengths.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH v2 01/17] block/throttle-groups: throttle_group_co_io_limits_intercept(): 64bit bytes

Posted by Eric Blake 5 years, 9 months ago

On 4/27/20 3:23 AM, Vladimir Sementsov-Ogievskiy wrote:
> The function is called from 64bit io handlers, and bytes is just passed
> to throttle_account() which is 64bit too (unsigned though). So, let's
> convert intermediate argument to 64bit too.

My audit for this patch:

Caller has 32-bit, this patch now causes widening which is safe:
block/block-backend.c: blk_do_preadv() passes 'unsigned int'
block/block-backend.c: blk_do_pwritev_part() passes 'unsigned int'
block/throttle.c: throttle_co_pwrite_zeroes() passes 'int'
block/throttle.c: throttle_co_pdiscard() passes 'int'

Caller has 64-bit, this patch fixes potential bug where pre-patch could 
narrow, except it's easy enough to trace that callers are still capped 
at 2G actions:
block/throttle.c: throttle_co_preadv() passes 'uint64_t'
block/throttle.c: throttle_co_pwritev() passes 'uint64_t'

Implementation in question: block/throttle-groups.c 
throttle_group_co_io_limits_intercept() takes 'unsigned int bytes' and 
uses it:
argument to util/throttle.c throttle_account(uint64_t)

All safe: it patches a latent bug, and does not introduce any 64-bit 
gotchas once throttle_co_p{read,write}v are relaxed, and assuming 
throttle_account() is not buggy.

> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   include/block/throttle-groups.h | 2 +-
>   block/throttle-groups.c         | 2 +-
>   2 files changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH v2 01/17] block/throttle-groups: throttle_group_co_io_limits_intercept(): 64bit bytes

Posted by Vladimir Sementsov-Ogievskiy 5 years, 9 months ago

29.04.2020 1:09, Eric Blake wrote:
> On 4/27/20 3:23 AM, Vladimir Sementsov-Ogievskiy wrote:
>> The function is called from 64bit io handlers, and bytes is just passed
>> to throttle_account() which is 64bit too (unsigned though). So, let's
>> convert intermediate argument to 64bit too.
> 
> My audit for this patch:
> 
> Caller has 32-bit, this patch now causes widening which is safe:
> block/block-backend.c: blk_do_preadv() passes 'unsigned int'
> block/block-backend.c: blk_do_pwritev_part() passes 'unsigned int'
> block/throttle.c: throttle_co_pwrite_zeroes() passes 'int'
> block/throttle.c: throttle_co_pdiscard() passes 'int'
> 
> Caller has 64-bit, this patch fixes potential bug where pre-patch could narrow, except it's easy enough to trace that callers are still capped at 2G actions:
> block/throttle.c: throttle_co_preadv() passes 'uint64_t'
> block/throttle.c: throttle_co_pwritev() passes 'uint64_t'
> 
> Implementation in question: block/throttle-groups.c throttle_group_co_io_limits_intercept() takes 'unsigned int bytes' and uses it:
> argument to util/throttle.c throttle_account(uint64_t)
> 
> All safe: it patches a latent bug, and does not introduce any 64-bit gotchas once throttle_co_p{read,write}v are relaxed, and assuming throttle_account() is not buggy.

Should I add this all to commit message?

> 
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   include/block/throttle-groups.h | 2 +-
>>   block/throttle-groups.c         | 2 +-
>>   2 files changed, 2 insertions(+), 2 deletions(-)
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 

Thanks for careful review!

-- 
Best regards,
Vladimir

Re: [PATCH v2 01/17] block/throttle-groups: throttle_group_co_io_limits_intercept(): 64bit bytes

Posted by Eric Blake 5 years, 9 months ago

On 4/29/20 12:05 AM, Vladimir Sementsov-Ogievskiy wrote:
> 29.04.2020 1:09, Eric Blake wrote:
>> On 4/27/20 3:23 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> The function is called from 64bit io handlers, and bytes is just passed
>>> to throttle_account() which is 64bit too (unsigned though). So, let's
>>> convert intermediate argument to 64bit too.
>>
>> My audit for this patch:
>>
>> Caller has 32-bit, this patch now causes widening which is safe:
>> block/block-backend.c: blk_do_preadv() passes 'unsigned int'
>> block/block-backend.c: blk_do_pwritev_part() passes 'unsigned int'
>> block/throttle.c: throttle_co_pwrite_zeroes() passes 'int'
>> block/throttle.c: throttle_co_pdiscard() passes 'int'
>>
>> Caller has 64-bit, this patch fixes potential bug where pre-patch 
>> could narrow, except it's easy enough to trace that callers are still 
>> capped at 2G actions:
>> block/throttle.c: throttle_co_preadv() passes 'uint64_t'
>> block/throttle.c: throttle_co_pwritev() passes 'uint64_t'
>>
>> Implementation in question: block/throttle-groups.c 
>> throttle_group_co_io_limits_intercept() takes 'unsigned int bytes' and 
>> uses it:
>> argument to util/throttle.c throttle_account(uint64_t)
>>
>> All safe: it patches a latent bug, and does not introduce any 64-bit 
>> gotchas once throttle_co_p{read,write}v are relaxed, and assuming 
>> throttle_account() is not buggy.
> 
> Should I add this all to commit message?

It never hurts to provide such rationale ;)

> 
>>
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> ---
>>>   include/block/throttle-groups.h | 2 +-
>>>   block/throttle-groups.c         | 2 +-
>>>   2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>
> 
> Thanks for careful review!
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org