Re: [PATCH 2/3] block/io: convert generic io path to use int64_t parameters
22.04.2020 18:50, Stefan Hajnoczi wrote:
On Mon, Mar 30, 2020 at 05:18:17PM +0300, Vladimir Sementsov-Ogievskiy wrote:
@@ -875,9 +875,9 @@ static bool coroutine_fn
bdrv_wait_serialising_requests(BdrvTrackedRequest *self
}
static int bdrv_check_byte_request(BlockDriverState *bs, int64_t offset,
- size_t size)
+ int64_t bytes)
{
-if (size > BDRV_REQUEST_MAX_BYTES) {
+if (offset < 0 || bytes < 0 || bytes > BDRV_REQUEST_MAX_BYTES) {
return -EIO;
}
@@ -885,10 +885,6 @@ static int bdrv_check_byte_request(BlockDriverState *bs, int64_t offset,
return -ENOMEDIUM;
}
-if (offset < 0) {
-return -EIO;
-}
-
return 0;
}
Moving this if statement was unnecessary. I'm not sure if there are
cases where callers will now see EIO instead of ENOMEDIUM and change
their behavior :(.
More conservative code changes are easier to review and merge because
they are less risky.
Hmm, would be a bit strange to just add "bytes < 0" to the first "if" keeping "offset
< 0" in the third.
And strange to keep "bytes > BDRV_REQUEST_MAX_BYTES" in the first, if add "bytes <
0" to the third..
@@ -1743,7 +1740,7 @@ int coroutine_fn bdrv_co_preadv_part(BdrvChild *child,
}
static int coroutine_fn bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,
-int64_t offset, int bytes, BdrvRequestFlags flags)
+int64_t offset, int64_t bytes, BdrvRequestFlags flags)
{
BlockDriver *drv = bs->drv;
QEMUIOVector qiov;
@@ -1773,7 +1770,7 @@ static int coroutine_fn
bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,
assert(max_write_zeroes >= bs->bl.request_alignment);
while (bytes > 0 && !ret) {
-int num = bytes;
+int64_t num = bytes;
/* Align request. Block drivers can expect the "bulk" of the request
* to be aligned, and that unaligned requests do not cross cluster
@@ -1799,6 +1796,7 @@ static int coroutine_fn
bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,
ret = -ENOTSUP;
/* First try the efficient write zeroes operation */
if (drv->bdrv_co_pwrite_zeroes) {
+assert(num <= INT_MAX);
max_write_zeroes already enforces this, so the assertion is always
false:
int max_write_zeroes = MIN_NON_ZERO(bs->bl.max_pwrite_zeroes, INT_MAX);
...
/* limit request size */
if (num > max_write_zeroes) {
num = max_write_zeroes;
}
You are right, I'll drop it.
--
Best regards,
Vladimir
Re: [PATCH 2/3] block/io: convert generic io path to use int64_t parameters
On Mon, Mar 30, 2020 at 05:18:17PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> @@ -875,9 +875,9 @@ static bool coroutine_fn
> bdrv_wait_serialising_requests(BdrvTrackedRequest *self
> }
>
> static int bdrv_check_byte_request(BlockDriverState *bs, int64_t offset,
> - size_t size)
> + int64_t bytes)
> {
> -if (size > BDRV_REQUEST_MAX_BYTES) {
> +if (offset < 0 || bytes < 0 || bytes > BDRV_REQUEST_MAX_BYTES) {
> return -EIO;
> }
>
> @@ -885,10 +885,6 @@ static int bdrv_check_byte_request(BlockDriverState *bs,
> int64_t offset,
> return -ENOMEDIUM;
> }
>
> -if (offset < 0) {
> -return -EIO;
> -}
> -
> return 0;
> }
Moving this if statement was unnecessary. I'm not sure if there are
cases where callers will now see EIO instead of ENOMEDIUM and change
their behavior :(.
More conservative code changes are easier to review and merge because
they are less risky.
> @@ -1743,7 +1740,7 @@ int coroutine_fn bdrv_co_preadv_part(BdrvChild *child,
> }
>
> static int coroutine_fn bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,
> -int64_t offset, int bytes, BdrvRequestFlags flags)
> +int64_t offset, int64_t bytes, BdrvRequestFlags flags)
> {
> BlockDriver *drv = bs->drv;
> QEMUIOVector qiov;
> @@ -1773,7 +1770,7 @@ static int coroutine_fn
> bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,
> assert(max_write_zeroes >= bs->bl.request_alignment);
>
> while (bytes > 0 && !ret) {
> -int num = bytes;
> +int64_t num = bytes;
>
> /* Align request. Block drivers can expect the "bulk" of the request
> * to be aligned, and that unaligned requests do not cross cluster
> @@ -1799,6 +1796,7 @@ static int coroutine_fn
> bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,
> ret = -ENOTSUP;
> /* First try the efficient write zeroes operation */
> if (drv->bdrv_co_pwrite_zeroes) {
> +assert(num <= INT_MAX);
max_write_zeroes already enforces this, so the assertion is always
false:
int max_write_zeroes = MIN_NON_ZERO(bs->bl.max_pwrite_zeroes, INT_MAX);
...
/* limit request size */
if (num > max_write_zeroes) {
num = max_write_zeroes;
}
signature.asc
Description: PGP signature
[PATCH 2/3] block/io: convert generic io path to use int64_t parameters
We are generally moving to int64_t for both offset and bytes paramaters
on all io paths. Convert generic io path now.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
include/block/block.h | 8 ++--
include/block/block_int.h | 20 -
block/blkverify.c | 2 +-
block/io.c| 86 +++
block/trace-events| 12 +++---
5 files changed, 64 insertions(+), 64 deletions(-)
diff --git a/include/block/block.h b/include/block/block.h
index b05995fe9c..fa5c7285b6 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -333,7 +333,7 @@ int bdrv_pwrite_sync(BdrvChild *child, int64_t offset,
* because it may allocate memory for the entire region.
*/
int coroutine_fn bdrv_co_pwrite_zeroes(BdrvChild *child, int64_t offset,
- int bytes, BdrvRequestFlags flags);
+ int64_t bytes, BdrvRequestFlags flags);
BlockDriverState *bdrv_find_backing_image(BlockDriverState *bs,
const char *backing_file);
void bdrv_refresh_filename(BlockDriverState *bs);
@@ -731,8 +731,8 @@ void bdrv_unregister_buf(BlockDriverState *bs, void *host);
*
* Returns: 0 if succeeded; negative error code if failed.
**/
-int coroutine_fn bdrv_co_copy_range(BdrvChild *src, uint64_t src_offset,
-BdrvChild *dst, uint64_t dst_offset,
-uint64_t bytes, BdrvRequestFlags
read_flags,
+int coroutine_fn bdrv_co_copy_range(BdrvChild *src, int64_t src_offset,
+BdrvChild *dst, int64_t dst_offset,
+int64_t bytes, BdrvRequestFlags read_flags,
BdrvRequestFlags write_flags);
#endif
diff --git a/include/block/block_int.h b/include/block/block_int.h
index c8daba608b..28aea2bcfd 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -972,16 +972,16 @@ extern BlockDriver bdrv_raw;
extern BlockDriver bdrv_qcow2;
int coroutine_fn bdrv_co_preadv(BdrvChild *child,
-int64_t offset, unsigned int bytes, QEMUIOVector *qiov,
+int64_t offset, int64_t bytes, QEMUIOVector *qiov,
BdrvRequestFlags flags);
int coroutine_fn bdrv_co_preadv_part(BdrvChild *child,
-int64_t offset, unsigned int bytes,
+int64_t offset, int64_t bytes,
QEMUIOVector *qiov, size_t qiov_offset, BdrvRequestFlags flags);
int coroutine_fn bdrv_co_pwritev(BdrvChild *child,
-int64_t offset, unsigned int bytes, QEMUIOVector *qiov,
+int64_t offset, int64_t bytes, QEMUIOVector *qiov,
BdrvRequestFlags flags);
int coroutine_fn bdrv_co_pwritev_part(BdrvChild *child,
-int64_t offset, unsigned int bytes,
+int64_t offset, int64_t bytes,
QEMUIOVector *qiov, size_t qiov_offset, BdrvRequestFlags flags);
static inline int coroutine_fn bdrv_co_pread(BdrvChild *child,
@@ -1315,14 +1315,14 @@ void bdrv_dec_in_flight(BlockDriverState *bs);
void blockdev_close_all_bdrv_states(void);
-int coroutine_fn bdrv_co_copy_range_from(BdrvChild *src, uint64_t src_offset,
- BdrvChild *dst, uint64_t dst_offset,
- uint64_t bytes,
+int coroutine_fn bdrv_co_copy_range_from(BdrvChild *src, int64_t src_offset,
+ BdrvChild *dst, int64_t dst_offset,
+ int64_t bytes,
BdrvRequestFlags read_flags,
BdrvRequestFlags write_flags);
-int coroutine_fn bdrv_co_copy_range_to(BdrvChild *src, uint64_t src_offset,
- BdrvChild *dst, uint64_t dst_offset,
- uint64_t bytes,
+int coroutine_fn bdrv_co_copy_range_to(BdrvChild *src, int64_t src_offset,
+ BdrvChild *dst, int64_t dst_offset,
+ int64_t bytes,
BdrvRequestFlags read_flags,
BdrvRequestFlags write_flags);
diff --git a/block/blkverify.c b/block/blkverify.c
index ba6b1853ae..667e60d832 100644
--- a/block/blkverify.c
+++ b/block/blkverify.c
@@ -31,7 +31,7 @@ typedef struct BlkverifyRequest {
uint64_t bytes;
int flags;
-int (*request_fn)(BdrvChild *, int64_t, unsigned int, QEMUIOVector *,
+int (*request_fn)(BdrvChild *, int64_t, int64_t, QEMUIOVector *,
BdrvRequestFlags);
int ret;/* test image result */
diff --git a/block/io.c b/block/io.c
index 7cbb80bd24..79e3014489 100644
--- a/block/io.c
+++ b/block/io.c
@@ -42,7 +42,7 @@
static void bdrv_parent_cb_resize(BlockDriverState *bs);
static int coroutine_fn bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,
-int64_t offset, int bytes, BdrvRequestFlags flags);
+int64_t offset, int64_t bytes,
