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

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

29.04.2020 18:50, Eric Blake wrote:
> 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@virtuozzo.com>
>> Reviewed-by: Stefan Hajnoczi <stefanha@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).

No, it's not here... But of course converting write-threshold.c should be separate patch.

I think that if we have BdrvTrackedRequest object, it must be valid, and offset + bytes must fit into INT64_MAX. And object-creator is responsible for this, and tracked_request_begin assert this thing.


-- 
Best regards,
Vladimir