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 <address@hidden>
Reviewed-by: Stefan Hajnoczi <address@hidden>
---
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 <address@hidden>