[GEDI] [PATCH v2 02/17] block: use int64_t as bytes type in tracked requests

[Eric Blake]

On 4/27/20 3:23 AM, Vladimir Sementsov-Ogievskiy wrote:
> We are generally moving to int64_t for both offset and bytes parameters
> on all io paths. Convert tracked requests now.

As mentioned elsewhere in the thread, this states 'what' but not 'why'; 
adding a bit more of the 'why' can be useful when revisiting this commit 
in the future.

> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov at virtuozzo.com>
> Reviewed-by: Stefan Hajnoczi <stefanha at redhat.com>
> ---
>   include/block/block_int.h |  4 ++--
>   block/io.c                | 11 ++++++-----
>   2 files changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 4c3587ea19..c8daba608b 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -70,12 +70,12 @@ enum BdrvTrackedRequestType {
>   typedef struct BdrvTrackedRequest {
>       BlockDriverState *bs;
>       int64_t offset;
> -    uint64_t bytes;
> +    int64_t bytes;
>       enum BdrvTrackedRequestType type;
>   
>       bool serialising;
>       int64_t overlap_offset;
> -    uint64_t overlap_bytes;
> +    int64_t overlap_bytes;

unsigned values have defined wraparound semantics, signed values have a 
lower maximum and require careful handling to avoid undefined overflow. 
So we have to check all clients for any surprises.

block/file-posix.c:raw_do_pwrite_zeroes() -
         assert(req->offset + req->bytes >= offset + bytes);
pre-patch: assert(int64_t + uint64_t >= int64_t + int)
            assert(uint64_t >= int64_t) - unsigned compare
post-patch: assert(int64_t >= int64_t) - signed compare
Risky if adding req->bytes could ever overflow 63 bits but still fit in 
64 bits, but I couldn't find any way to trigger that.  I think we're 
safe because the block layer never calls a driver's .pwrite_zeroes with 
a bytes that would overflow 63 bits.

block/write-threshold.c:bdrv_write_threshold_exceeded() -
         if ((req->offset + req->bytes) > bs->write_threshold_offset) {
pre-patch: ((int64_t + uint64_t) > uint64_t) - unsigned compare
post-patch: (int64_t > uint64_t) - still unsigned compare

Perhaps that function should be changed to return int64_t, but probably 
as a different patch in the series (I didn't read ahead yet to see if 
you already did).

>   
>       QLIST_ENTRY(BdrvTrackedRequest) list;
>       Coroutine *co; /* owner, used for deadlock detection */
> diff --git a/block/io.c b/block/io.c
> index aba67f66b9..7cbb80bd24 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -692,10 +692,11 @@ static void tracked_request_end(BdrvTrackedRequest *req)
>   static void tracked_request_begin(BdrvTrackedRequest *req,
>                                     BlockDriverState *bs,
>                                     int64_t offset,
> -                                  uint64_t bytes,
> +                                  int64_t bytes,
>                                     enum BdrvTrackedRequestType type)
>   {
> -    assert(bytes <= INT64_MAX && offset <= INT64_MAX - bytes);
> +    assert(offset >= 0 && bytes >= 0 &&
> +           bytes <= INT64_MAX && offset <= INT64_MAX - bytes);

This part is nice - it makes it very easy to prove all other uses of 
BdrvTrackedRequest.bytes were already in range (both pre- and post-patch).

>   
>       *req = (BdrvTrackedRequest){
>           .bs = bs,
> @@ -716,7 +717,7 @@ static void tracked_request_begin(BdrvTrackedRequest *req,
>   }
>   
>   static bool tracked_request_overlaps(BdrvTrackedRequest *req,
> -                                     int64_t offset, uint64_t bytes)
> +                                     int64_t offset, int64_t bytes)
>   {
>       /*        aaaa   bbbb */
>       if (offset >= req->overlap_offset + req->overlap_bytes) {
> @@ -773,8 +774,8 @@ bool bdrv_mark_request_serialising(BdrvTrackedRequest *req, uint64_t align)
>   {
>       BlockDriverState *bs = req->bs;
>       int64_t overlap_offset = req->offset & ~(align - 1);

While here, should we use QEMU_ALIGN_DOWN instead of open-coding?

> -    uint64_t overlap_bytes = ROUND_UP(req->offset + req->bytes, align)
> -                               - overlap_offset;
> +    int64_t overlap_bytes =
> +            ROUND_UP(req->offset + req->bytes, align) - overlap_offset;
>       bool waited;
>   
>       qemu_co_mutex_lock(&bs->reqs_lock);
> 

Looking through uses of BdrvTrackedRequest in io.c, I couldn't find any 
other surprises (it seems everything starts with tracked_request_begin, 
and while you did switch between unsigned and signed, you did not switch 
width, so it's easier to reason about once we know things don't overflow).

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


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