We are going to convert .bdrv_co_preadv_part and .bdrv_co_pwritev_part
to int64_t type for offset and bytes parameters (as it's already done
for generic block/io.c layer).
In qcow2 .bdrv_co_preadv_part is used in some places, so let's add
corresponding checks and assertions.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
block: use int64_t instead of uint64_t in driver read handlers
We are generally moving to int64_t for both offset and bytes parameters
on all io paths.
Main motivation is realization of 64-bit write_zeroes operation for
fast zeroing large disk chunks, up to the whole disk.
We chose signed type, to be consistent with off_t (which is signed) and
with possibility for signed return type (where negative value means
error).
So, convert driver read handlers parameters which are already 64bit to
signed type.
While being here, convert also flags parameter to be BdrvRequestFlags.
Now let's consider all callers. Simple
git grep '\->bdrv_\(aio\|co\)_preadv\(_part\)\?'
shows that's there three callers of driver function:
bdrv_driver_preadv() in block/io.c, passes int64_t, checked by
bdrv_check_qiov_request() to be non-negative.
qcow2_load_vmstate() does bdrv_check_qiov_request().
do_perform_cow_read() has uint64_t argument. And a lot of things in
qcow2 driver are uint64_t, so converting it is big job. But we must
not work with requests that doesn't satisfy bdrv_check_qiov_request(),
so let's just assert it here.
Still, the functions may be called directly, not only by drv->...
Let's check:
git grep '\.bdrv_\(aio\|co\)_preadv\(_part\)\?\s*=' | \
awk '{print $4}' | sed 's/,//' | sed 's/&//' | sort | uniq | \
while read func; do git grep "$func(" | \
grep -v "$func(BlockDriverState"; done
The only one such caller:
QEMUIOVector qiov = QEMU_IOVEC_INIT_BUF(qiov, &data, 1);
...
ret = bdrv_replace_test_co_preadv(bs, 0, 1, &qiov, 0);
in tesTS/unit/test-bdrv-drain.c, and it's OK obviously.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
include/block/block_int.h | 11 ++++++-----
block/backup-top.c | 4 ++--
block/blkdebug.c | 4 ++--
block/blklogwrites.c | 4 ++--
block/blkreplay.c | 2 +-
block/blkverify.c | 4 ++--
block/bochs.c | 4 ++--
block/cloop.c | 4 ++--
block/commit.c | 2 +-
block/copy-on-read.c | 4 ++--
block/crypto.c | 4 ++--
block/curl.c | 3 ++-
block/dmg.c | 4 ++--
block/file-posix.c | 6 +++---
block/file-win32.c | 4 ++--
block/filter-compress.c | 4 ++--
block/mirror.c | 2 +-
block/nbd.c | 5 +++--
block/nfs.c | 6 +++---
block/null.c | 9 +++++----
block/nvme.c | 5 +++--
block/preallocate.c | 4 ++--
block/qcow.c | 6 +++---
block/qcow2-cluster.c | 14 +++++++++++++-
block/qcow2.c | 5 +++--
block/quorum.c | 4 ++--
block/raw-format.c | 20 ++++++++++----------
block/rbd.c | 5 +++--
block/throttle.c | 5 +++--
block/vdi.c | 4 ++--
block/vmdk.c | 4 ++--
block/vpc.c | 4 ++--
block/vvfat.c | 4 ++--
tests/unit/test-bdrv-drain.c | 16 +++++++++-------
tests/unit/test-block-iothread.c | 19 ++++++++++++++-----
35 files changed, 120 insertions(+), 89 deletions(-)
diff --git a/include/block/block_int.h b/include/block/block_int.h
index db7a909ea9..e6bcf74e46 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -234,8 +234,8 @@ struct BlockDriver {
/* aio */
BlockAIOCB *(*bdrv_aio_preadv)(BlockDriverState *bs,
- uint64_t offset, uint64_t bytes, QEMUIOVector *qiov, int flags,
- BlockCompletionFunc *cb, void *opaque);
+ int64_t offset, int64_t bytes, QEMUIOVector *qiov,
+ BdrvRequestFlags flags, BlockCompletionFunc *cb, void *opaque);
BlockAIOCB *(*bdrv_aio_pwritev)(BlockDriverState *bs,
uint64_t offset, uint64_t bytes, QEMUIOVector *qiov, int flags,
BlockCompletionFunc *cb, void *opaque);
@@ -264,10 +264,11 @@ struct BlockDriver {
* The buffer in @qiov may point directly to guest memory.
*/
int coroutine_fn (*bdrv_co_preadv)(BlockDriverState *bs,
- uint64_t offset, uint64_t bytes, QEMUIOVector *qiov, int flags);
+ int64_t offset, int64_t bytes, QEMUIOVector *qiov,
+ BdrvRequestFlags flags);
int coroutine_fn (*bdrv_co_preadv_part)(BlockDriverState *bs,
- uint64_t offset, uint64_t bytes,
- QEMUIOVector *qiov, size_t qiov_offset, int flags);
+ int64_t offset, int64_t bytes,
+ QEMUIOVector *qiov, size_t qiov_offset, BdrvRequestFlags flags);
int coroutine_fn (*bdrv_co_writev)(BlockDriverState *bs,
int64_t sector_num, int nb_sectors, QEMUIOVector *qiov, int flags);
/**
diff --git a/block/backup-top.c b/block/backup-top.c
index 589e8b651d..9779493a4b 100644
--- a/block/backup-top.c
+++ b/block/backup-top.c
@@ -42,8 +42,8 @@ typedef struct BDRVBackupTopState {
} BDRVBackupTopState;
static coroutine_fn int backup_top_co_preadv(
- BlockDriverState *bs, uint64_t offset, uint64_t bytes,
- QEMUIOVector *qiov, int flags)
+ BlockDriverState *bs, int64_t offset, int64_t bytes,
+ QEMUIOVector *qiov, BdrvRequestFlags flags)
{
BDRVBackupTopState *s = bs->opaque;
diff --git a/block/blkdebug.c b/block/blkdebug.c
index 2c0b9b0ee8..b26fecf222 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -614,8 +614,8 @@ static int rule_check(BlockDriverState *bs, uint64_t offset, uint64_t bytes,
}
static int coroutine_fn
-blkdebug_co_preadv(BlockDriverState *bs, uint64_t offset, uint64_t bytes,
- QEMUIOVector *qiov, int flags)
+blkdebug_co_preadv(BlockDriverState *bs, int64_t offset, int64_t bytes,
+ QEMUIOVector *qiov, BdrvRequestFlags flags)
{
int err;
diff --git a/block/blklogwrites.c b/block/blklogwrites.c
index b7579370a3..de3d4ba2b6 100644
--- a/block/blklogwrites.c
+++ b/block/blklogwrites.c
@@ -301,8 +301,8 @@ static void blk_log_writes_refresh_limits(BlockDriverState *bs, Error **errp)
}
static int coroutine_fn
-blk_log_writes_co_preadv(BlockDriverState *bs, uint64_t offset, uint64_t bytes,
- QEMUIOVector *qiov, int flags)
+blk_log_writes_co_preadv(BlockDriverState *bs, int64_t offset, int64_t bytes,
+ QEMUIOVector *qiov, BdrvRequestFlags flags)
{
return bdrv_co_preadv(bs->file, offset, bytes, qiov, flags);
}
diff --git a/block/blkreplay.c b/block/blkreplay.c
index 4a247752fd..13ea2166bb 100644
--- a/block/blkreplay.c
+++ b/block/blkreplay.c
@@ -72,7 +72,7 @@ static void block_request_create(uint64_t reqid, BlockDriverState *bs,
}
static int coroutine_fn blkreplay_co_preadv(BlockDriverState *bs,
- uint64_t offset, uint64_t bytes, QEMUIOVector *qiov, int flags)
+ int64_t offset, int64_t bytes, QEMUIOVector *qiov, BdrvRequestFlags flags)
{
uint64_t reqid = blkreplay_next_id();
int ret = bdrv_co_preadv(bs->file, offset, bytes, qiov, flags);
diff --git a/block/blkverify.c b/block/blkverify.c
index 188d7632fa..5e35744b8a 100644
--- a/block/blkverify.c
+++ b/block/blkverify.c
@@ -221,8 +221,8 @@ blkverify_co_prwv(BlockDriverState *bs, BlkverifyRequest *r, uint64_t offset,
}
static int coroutine_fn
-blkverify_co_preadv(BlockDriverState *bs, uint64_t offset, uint64_t bytes,
- QEMUIOVector *qiov, int flags)
+blkverify_co_preadv(BlockDriverState *bs, int64_t offset, int64_t bytes,
+ QEMUIOVector *qiov, BdrvRequestFlags flags)
{
BlkverifyRequest r;
QEMUIOVector raw_qiov;
diff --git a/block/bochs.c b/block/bochs.c
index 2f010ab40a..4d68658087 100644
--- a/block/bochs.c
+++ b/block/bochs.c
@@ -238,8 +238,8 @@ static int64_t seek_to_sector(BlockDriverState *bs, int64_t sector_num)
}
static int coroutine_fn
-bochs_co_preadv(BlockDriverState *bs, uint64_t offset, uint64_t bytes,
- QEMUIOVector *qiov, int flags)
+bochs_co_preadv(BlockDriverState *bs, int64_t offset, int64_t bytes,
+ QEMUIOVector *qiov, BdrvRequestFlags flags)
{
BDRVBochsState *s = bs->opaque;
uint64_t sector_num = offset >> BDRV_SECTOR_BITS;
diff --git a/block/cloop.c b/block/cloop.c
index c99192a57f..b8c6d0eccd 100644
--- a/block/cloop.c
+++ b/block/cloop.c
@@ -245,8 +245,8 @@ static inline int cloop_read_block(BlockDriverState *bs, int block_num)
}
static int coroutine_fn
-cloop_co_preadv(BlockDriverState *bs, uint64_t offset, uint64_t bytes,
- QEMUIOVector *qiov, int flags)
+cloop_co_preadv(BlockDriverState *bs, int64_t offset, int64_t bytes,
+ QEMUIOVector *qiov, BdrvRequestFlags flags)
{
BDRVCloopState *s = bs->opaque;
uint64_t sector_num = offset >> BDRV_SECTOR_BITS;
diff --git a/block/commit.c b/block/commit.c
index dd9ba87349..04a1bb43c1 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -212,7 +212,7 @@ static const BlockJobDriver commit_job_driver = {
};
static int coroutine_fn bdrv_commit_top_preadv(BlockDriverState *bs,
- uint64_t offset, uint64_t bytes, QEMUIOVector *qiov, int flags)
+ int64_t offset, int64_t bytes, QEMUIOVector *qiov, BdrvRequestFlags flags)
{
return bdrv_co_preadv(bs->backing, offset, bytes, qiov, flags);
}
diff --git a/block/copy-on-read.c b/block/copy-on-read.c
index 9cad9e1b8c..afb2a77b08 100644
--- a/block/copy-on-read.c
+++ b/block/copy-on-read.c
@@ -141,10 +141,10 @@ static int64_t cor_getlength(BlockDriverState *bs)
static int coroutine_fn cor_co_preadv_part(BlockDriverState *bs,
- uint64_t offset, uint64_t bytes,
+ int64_t offset, int64_t bytes,
QEMUIOVector *qiov,
size_t qiov_offset,
- int flags)
+ BdrvRequestFlags flags)
{
int64_t n;
int local_flags;
diff --git a/block/crypto.c b/block/crypto.c
index 1d30fde38e..a732a36d10 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -397,8 +397,8 @@ static int block_crypto_reopen_prepare(BDRVReopenState *state,
#define BLOCK_CRYPTO_MAX_IO_SIZE (1024 * 1024)
static coroutine_fn int
-block_crypto_co_preadv(BlockDriverState *bs, uint64_t offset, uint64_t bytes,
- QEMUIOVector *qiov, int flags)
+block_crypto_co_preadv(BlockDriverState *bs, int64_t offset, int64_t bytes,
+ QEMUIOVector *qiov, BdrvRequestFlags flags)
{
BlockCrypto *crypto = bs->opaque;
uint64_t cur_bytes; /* number of bytes in current iteration */
diff --git a/block/curl.c b/block/curl.c
index 50e741a0d7..4a8ae2b269 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -896,7 +896,8 @@ out:
}
static int coroutine_fn curl_co_preadv(BlockDriverState *bs,
- uint64_t offset, uint64_t bytes, QEMUIOVector *qiov, int flags)
+ int64_t offset, int64_t bytes, QEMUIOVector *qiov,
+ BdrvRequestFlags flags)
{
CURLAIOCB acb = {
.co = qemu_coroutine_self(),
diff --git a/block/dmg.c b/block/dmg.c
index ef35a505f2..447901fbb8 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -689,8 +689,8 @@ static inline int dmg_read_chunk(BlockDriverState *bs, uint64_t sector_num)
}
static int coroutine_fn
-dmg_co_preadv(BlockDriverState *bs, uint64_t offset, uint64_t bytes,
- QEMUIOVector *qiov, int flags)
+dmg_co_preadv(BlockDriverState *bs, int64_t offset, int64_t bytes,
+ QEMUIOVector *qiov, BdrvRequestFlags flags)
{
BDRVDMGState *s = bs->opaque;
uint64_t sector_num = offset >> BDRV_SECTOR_BITS;
diff --git a/block/file-posix.c b/block/file-posix.c
index 20e14f8e96..25da79cdd5 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -2033,9 +2033,9 @@ static int coroutine_fn raw_co_prw(BlockDriverState *bs, uint64_t offset,
return raw_thread_pool_submit(bs, handle_aiocb_rw, &acb);
}
-static int coroutine_fn raw_co_preadv(BlockDriverState *bs, uint64_t offset,
- uint64_t bytes, QEMUIOVector *qiov,
- int flags)
+static int coroutine_fn raw_co_preadv(BlockDriverState *bs, int64_t offset,
+ int64_t bytes, QEMUIOVector *qiov,
+ BdrvRequestFlags flags)
{
return raw_co_prw(bs, offset, bytes, qiov, QEMU_AIO_READ);
}
diff --git a/block/file-win32.c b/block/file-win32.c
index 2642088bd6..15076350f2 100644
--- a/block/file-win32.c
+++ b/block/file-win32.c
@@ -436,8 +436,8 @@ fail:
}
static BlockAIOCB *raw_aio_preadv(BlockDriverState *bs,
- uint64_t offset, uint64_t bytes,
- QEMUIOVector *qiov, int flags,
+ int64_t offset, int64_t bytes,
+ QEMUIOVector *qiov, BdrvRequestFlags flags,
BlockCompletionFunc *cb, void *opaque)
{
BDRVRawState *s = bs->opaque;
diff --git a/block/filter-compress.c b/block/filter-compress.c
index 5136371bf8..54a16c6c64 100644
--- a/block/filter-compress.c
+++ b/block/filter-compress.c
@@ -63,10 +63,10 @@ static int64_t compress_getlength(BlockDriverState *bs)
static int coroutine_fn compress_co_preadv_part(BlockDriverState *bs,
- uint64_t offset, uint64_t bytes,
+ int64_t offset, int64_t bytes,
QEMUIOVector *qiov,
size_t qiov_offset,
- int flags)
+ BdrvRequestFlags flags)
{
return bdrv_co_preadv_part(bs->file, offset, bytes, qiov, qiov_offset,
flags);
diff --git a/block/mirror.c b/block/mirror.c
index 6af02a57c4..1587cda13c 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1381,7 +1381,7 @@ static void coroutine_fn active_write_settle(MirrorOp *op)
}
static int coroutine_fn bdrv_mirror_top_preadv(BlockDriverState *bs,
- uint64_t offset, uint64_t bytes, QEMUIOVector *qiov, int flags)
+ int64_t offset, int64_t bytes, QEMUIOVector *qiov, BdrvRequestFlags flags)
{
return bdrv_co_preadv(bs->backing, offset, bytes, qiov, flags);
}
diff --git a/block/nbd.c b/block/nbd.c
index c26dc5a54f..a2b6aa1bca 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -1534,8 +1534,9 @@ static int nbd_co_request(BlockDriverState *bs, NBDRequest *request,
return ret ? ret : request_ret;
}
-static int nbd_client_co_preadv(BlockDriverState *bs, uint64_t offset,
- uint64_t bytes, QEMUIOVector *qiov, int flags)
+static int nbd_client_co_preadv(BlockDriverState *bs, int64_t offset,
+ int64_t bytes, QEMUIOVector *qiov,
+ BdrvRequestFlags flags)
{
int ret, request_ret;
Error *local_err = NULL;
diff --git a/block/nfs.c b/block/nfs.c
index 8c1968bb41..eee8f706ba 100644
--- a/block/nfs.c
+++ b/block/nfs.c
@@ -265,9 +265,9 @@ nfs_co_generic_cb(int ret, struct nfs_context *nfs, void *data,
nfs_co_generic_bh_cb, task);
}
-static int coroutine_fn nfs_co_preadv(BlockDriverState *bs, uint64_t offset,
- uint64_t bytes, QEMUIOVector *iov,
- int flags)
+static int coroutine_fn nfs_co_preadv(BlockDriverState *bs, int64_t offset,
+ int64_t bytes, QEMUIOVector *iov,
+ BdrvRequestFlags flags)
{
NFSClient *client = bs->opaque;
NFSRPC task;
diff --git a/block/null.c b/block/null.c
index cc9b1d4ea7..343dbb580d 100644
--- a/block/null.c
+++ b/block/null.c
@@ -116,8 +116,9 @@ static coroutine_fn int null_co_common(BlockDriverState *bs)
}
static coroutine_fn int null_co_preadv(BlockDriverState *bs,
- uint64_t offset, uint64_t bytes,
- QEMUIOVector *qiov, int flags)
+ int64_t offset, int64_t bytes,
+ QEMUIOVector *qiov,
+ BdrvRequestFlags flags)
{
BDRVNullState *s = bs->opaque;
@@ -187,8 +188,8 @@ static inline BlockAIOCB *null_aio_common(BlockDriverState *bs,
}
static BlockAIOCB *null_aio_preadv(BlockDriverState *bs,
- uint64_t offset, uint64_t bytes,
- QEMUIOVector *qiov, int flags,
+ int64_t offset, int64_t bytes,
+ QEMUIOVector *qiov, BdrvRequestFlags flags,
BlockCompletionFunc *cb,
void *opaque)
{
diff --git a/block/nvme.c b/block/nvme.c
index 2b5421e7aa..58342c6409 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -1221,8 +1221,9 @@ static int nvme_co_prw(BlockDriverState *bs, uint64_t offset, uint64_t bytes,
}
static coroutine_fn int nvme_co_preadv(BlockDriverState *bs,
- uint64_t offset, uint64_t bytes,
- QEMUIOVector *qiov, int flags)
+ int64_t offset, int64_t bytes,
+ QEMUIOVector *qiov,
+ BdrvRequestFlags flags)
{
return nvme_co_prw(bs, offset, bytes, qiov, false, flags);
}
diff --git a/block/preallocate.c b/block/preallocate.c
index b619206304..5709443612 100644
--- a/block/preallocate.c
+++ b/block/preallocate.c
@@ -227,8 +227,8 @@ static void preallocate_reopen_abort(BDRVReopenState *state)
}
static coroutine_fn int preallocate_co_preadv_part(
- BlockDriverState *bs, uint64_t offset, uint64_t bytes,
- QEMUIOVector *qiov, size_t qiov_offset, int flags)
+ BlockDriverState *bs, int64_t offset, int64_t bytes,
+ QEMUIOVector *qiov, size_t qiov_offset, BdrvRequestFlags flags)
{
return bdrv_co_preadv_part(bs->file, offset, bytes, qiov, qiov_offset,
flags);
diff --git a/block/qcow.c b/block/qcow.c
index f8919a44d1..1151b8d79b 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -617,9 +617,9 @@ static void qcow_refresh_limits(BlockDriverState *bs, Error **errp)
bs->bl.request_alignment = BDRV_SECTOR_SIZE;
}
-static coroutine_fn int qcow_co_preadv(BlockDriverState *bs, uint64_t offset,
- uint64_t bytes, QEMUIOVector *qiov,
- int flags)
+static coroutine_fn int qcow_co_preadv(BlockDriverState *bs, int64_t offset,
+ int64_t bytes, QEMUIOVector *qiov,
+ BdrvRequestFlags flags)
{
BDRVQcowState *s = bs->opaque;
int offset_in_cluster;
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index bd0597842f..70adea9f59 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -505,7 +505,19 @@ static int coroutine_fn do_perform_cow_read(BlockDriverState *bs,
return -ENOMEDIUM;
}
- /* Call .bdrv_co_readv() directly instead of using the public block-layer
+ /*
+ * We never deal with requests that doesn't satisfy
+ * bdrv_check_qiov_request(), and aligning requests to clusters never break
+ * this condition. So, do some assertions before calling
+ * bs->drv->bdrv_co_preadv_part() which has int64_t arguments.
+ */
+ assert(src_cluster_offset <= INT64_MAX);
+ assert(src_cluster_offset + offset_in_cluster <= INT64_MAX);
+ assert(qiov->size <= INT64_MAX);
+ bdrv_check_qiov_request(src_cluster_offset + offset_in_cluster, qiov->size,
+ qiov, 0, &error_abort);
+ /*
+ * Call .bdrv_co_readv() directly instead of using the public block-layer
* interface. This avoids double I/O throttling and request tracking,
* which can lead to deadlock when block layer copy-on-read is enabled.
*/
diff --git a/block/qcow2.c b/block/qcow2.c
index b57acda010..34c5e1c073 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2281,9 +2281,10 @@ static coroutine_fn int qcow2_co_preadv_task_entry(AioTask *task)
}
static coroutine_fn int qcow2_co_preadv_part(BlockDriverState *bs,
- uint64_t offset, uint64_t bytes,
+ int64_t offset, int64_t bytes,
QEMUIOVector *qiov,
- size_t qiov_offset, int flags)
+ size_t qiov_offset,
+ BdrvRequestFlags flags)
{
BDRVQcow2State *s = bs->opaque;
int ret = 0;
diff --git a/block/quorum.c b/block/quorum.c
index cfc1436abb..1eba2ce7ff 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -663,8 +663,8 @@ static int read_fifo_child(QuorumAIOCB *acb)
return ret;
}
-static int quorum_co_preadv(BlockDriverState *bs, uint64_t offset,
- uint64_t bytes, QEMUIOVector *qiov, int flags)
+static int quorum_co_preadv(BlockDriverState *bs, int64_t offset, int64_t bytes,
+ QEMUIOVector *qiov, BdrvRequestFlags flags)
{
BDRVQuorumState *s = bs->opaque;
QuorumAIOCB *acb = quorum_aio_get(bs, qiov, offset, bytes, flags);
diff --git a/block/raw-format.c b/block/raw-format.c
index 7717578ed6..e3f459b2cb 100644
--- a/block/raw-format.c
+++ b/block/raw-format.c
@@ -181,8 +181,8 @@ static void raw_reopen_abort(BDRVReopenState *state)
}
/* Check and adjust the offset, against 'offset' and 'size' options. */
-static inline int raw_adjust_offset(BlockDriverState *bs, uint64_t *offset,
- uint64_t bytes, bool is_write)
+static inline int raw_adjust_offset(BlockDriverState *bs, int64_t *offset,
+ int64_t bytes, bool is_write)
{
BDRVRawState *s = bs->opaque;
@@ -201,9 +201,9 @@ static inline int raw_adjust_offset(BlockDriverState *bs, uint64_t *offset,
return 0;
}
-static int coroutine_fn raw_co_preadv(BlockDriverState *bs, uint64_t offset,
- uint64_t bytes, QEMUIOVector *qiov,
- int flags)
+static int coroutine_fn raw_co_preadv(BlockDriverState *bs, int64_t offset,
+ int64_t bytes, QEMUIOVector *qiov,
+ BdrvRequestFlags flags)
{
int ret;
@@ -259,7 +259,7 @@ static int coroutine_fn raw_co_pwritev(BlockDriverState *bs, uint64_t offset,
qiov = &local_qiov;
}
- ret = raw_adjust_offset(bs, &offset, bytes, true);
+ ret = raw_adjust_offset(bs, (int64_t *)&offset, bytes, true);
if (ret) {
goto fail;
}
@@ -294,7 +294,7 @@ static int coroutine_fn raw_co_pwrite_zeroes(BlockDriverState *bs,
{
int ret;
- ret = raw_adjust_offset(bs, (uint64_t *)&offset, bytes, true);
+ ret = raw_adjust_offset(bs, (int64_t *)&offset, bytes, true);
if (ret) {
return ret;
}
@@ -306,7 +306,7 @@ static int coroutine_fn raw_co_pdiscard(BlockDriverState *bs,
{
int ret;
- ret = raw_adjust_offset(bs, (uint64_t *)&offset, bytes, true);
+ ret = raw_adjust_offset(bs, &offset, bytes, true);
if (ret) {
return ret;
}
@@ -541,7 +541,7 @@ static int coroutine_fn raw_co_copy_range_from(BlockDriverState *bs,
{
int ret;
- ret = raw_adjust_offset(bs, &src_offset, bytes, false);
+ ret = raw_adjust_offset(bs, (int64_t *)&src_offset, bytes, false);
if (ret) {
return ret;
}
@@ -560,7 +560,7 @@ static int coroutine_fn raw_co_copy_range_to(BlockDriverState *bs,
{
int ret;
- ret = raw_adjust_offset(bs, &dst_offset, bytes, true);
+ ret = raw_adjust_offset(bs, (int64_t *)&dst_offset, bytes, true);
if (ret) {
return ret;
}
diff --git a/block/rbd.c b/block/rbd.c
index 9071a00e3f..69299e5f6f 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -992,8 +992,9 @@ failed:
}
static BlockAIOCB *qemu_rbd_aio_preadv(BlockDriverState *bs,
- uint64_t offset, uint64_t bytes,
- QEMUIOVector *qiov, int flags,
+ int64_t offset, int64_t bytes,
+ QEMUIOVector *qiov,
+ BdrvRequestFlags flags,
BlockCompletionFunc *cb,
void *opaque)
{
diff --git a/block/throttle.c b/block/throttle.c
index b685166ad4..20362b5fe5 100644
--- a/block/throttle.c
+++ b/block/throttle.c
@@ -112,8 +112,9 @@ static int64_t throttle_getlength(BlockDriverState *bs)
}
static int coroutine_fn throttle_co_preadv(BlockDriverState *bs,
- uint64_t offset, uint64_t bytes,
- QEMUIOVector *qiov, int flags)
+ int64_t offset, int64_t bytes,
+ QEMUIOVector *qiov,
+ BdrvRequestFlags flags)
{
ThrottleGroupMember *tgm = bs->opaque;
diff --git a/block/vdi.c b/block/vdi.c
index 5627e7d764..c0e71bfa53 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -544,8 +544,8 @@ static int coroutine_fn vdi_co_block_status(BlockDriverState *bs,
}
static int coroutine_fn
-vdi_co_preadv(BlockDriverState *bs, uint64_t offset, uint64_t bytes,
- QEMUIOVector *qiov, int flags)
+vdi_co_preadv(BlockDriverState *bs, int64_t offset, int64_t bytes,
+ QEMUIOVector *qiov, BdrvRequestFlags flags)
{
BDRVVdiState *s = bs->opaque;
QEMUIOVector local_qiov;
diff --git a/block/vmdk.c b/block/vmdk.c
index 4499f136bd..78592160d0 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -1888,8 +1888,8 @@ static int vmdk_read_extent(VmdkExtent *extent, int64_t cluster_offset,
}
static int coroutine_fn
-vmdk_co_preadv(BlockDriverState *bs, uint64_t offset, uint64_t bytes,
- QEMUIOVector *qiov, int flags)
+vmdk_co_preadv(BlockDriverState *bs, int64_t offset, int64_t bytes,
+ QEMUIOVector *qiov, BdrvRequestFlags flags)
{
BDRVVmdkState *s = bs->opaque;
int ret;
diff --git a/block/vpc.c b/block/vpc.c
index 17a705b482..29c8517ff8 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -608,8 +608,8 @@ static int vpc_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
}
static int coroutine_fn
-vpc_co_preadv(BlockDriverState *bs, uint64_t offset, uint64_t bytes,
- QEMUIOVector *qiov, int flags)
+vpc_co_preadv(BlockDriverState *bs, int64_t offset, int64_t bytes,
+ QEMUIOVector *qiov, BdrvRequestFlags flags)
{
BDRVVPCState *s = bs->opaque;
int ret;
diff --git a/block/vvfat.c b/block/vvfat.c
index 54807f82ca..8ac90166c5 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -1522,8 +1522,8 @@ static int vvfat_read(BlockDriverState *bs, int64_t sector_num,
}
static int coroutine_fn
-vvfat_co_preadv(BlockDriverState *bs, uint64_t offset, uint64_t bytes,
- QEMUIOVector *qiov, int flags)
+vvfat_co_preadv(BlockDriverState *bs, int64_t offset, int64_t bytes,
+ QEMUIOVector *qiov, BdrvRequestFlags flags)
{
int ret;
BDRVVVFATState *s = bs->opaque;
diff --git a/tests/unit/test-bdrv-drain.c b/tests/unit/test-bdrv-drain.c
index 8a29e33e00..2627d9781e 100644
--- a/tests/unit/test-bdrv-drain.c
+++ b/tests/unit/test-bdrv-drain.c
@@ -65,8 +65,9 @@ static void co_reenter_bh(void *opaque)
}
static int coroutine_fn bdrv_test_co_preadv(BlockDriverState *bs,
- uint64_t offset, uint64_t bytes,
- QEMUIOVector *qiov, int flags)
+ int64_t offset, int64_t bytes,
+ QEMUIOVector *qiov,
+ BdrvRequestFlags flags)
{
BDRVTestState *s = bs->opaque;
@@ -1105,8 +1106,9 @@ static void bdrv_test_top_close(BlockDriverState *bs)
}
static int coroutine_fn bdrv_test_top_co_preadv(BlockDriverState *bs,
- uint64_t offset, uint64_t bytes,
- QEMUIOVector *qiov, int flags)
+ int64_t offset, int64_t bytes,
+ QEMUIOVector *qiov,
+ BdrvRequestFlags flags)
{
BDRVTestTopState *tts = bs->opaque;
return bdrv_co_preadv(tts->wait_child, offset, bytes, qiov, flags);
@@ -1854,10 +1856,10 @@ static void bdrv_replace_test_close(BlockDriverState *bs)
* Set .has_read to true and return success.
*/
static int coroutine_fn bdrv_replace_test_co_preadv(BlockDriverState *bs,
- uint64_t offset,
- uint64_t bytes,
+ int64_t offset,
+ int64_t bytes,
QEMUIOVector *qiov,
- int flags)
+ BdrvRequestFlags flags)
{
BDRVReplaceTestState *s = bs->opaque;
diff --git a/tests/unit/test-block-iothread.c b/tests/unit/test-block-iothread.c
index 8cf172cb7a..b3f8f3ff8e 100644
--- a/tests/unit/test-block-iothread.c
+++ b/tests/unit/test-block-iothread.c
@@ -31,9 +31,18 @@
#include "qemu/main-loop.h"
#include "iothread.h"
-static int coroutine_fn bdrv_test_co_prwv(BlockDriverState *bs,
- uint64_t offset, uint64_t bytes,
- QEMUIOVector *qiov, int flags)
+static int coroutine_fn bdrv_test_co_preadv(BlockDriverState *bs,
+ int64_t offset, int64_t bytes,
+ QEMUIOVector *qiov,
+ BdrvRequestFlags flags)
+{
+ return 0;
+}
+
+static int coroutine_fn bdrv_test_co_pwritev(BlockDriverState *bs,
+ uint64_t offset, uint64_t bytes,
+ QEMUIOVector *qiov,
+ int flags)
{
return 0;
}
@@ -66,8 +75,8 @@ static BlockDriver bdrv_test = {
.format_name = "test",
.instance_size = 1,
- .bdrv_co_preadv = bdrv_test_co_prwv,
- .bdrv_co_pwritev = bdrv_test_co_prwv,
+ .bdrv_co_preadv = bdrv_test_co_preadv,
+ .bdrv_co_pwritev = bdrv_test_co_pwritev,
.bdrv_co_pdiscard = bdrv_test_co_pdiscard,
.bdrv_co_truncate = bdrv_test_co_truncate,
.bdrv_co_block_status = bdrv_test_co_block_status,
--
2.29.2
On 3/24/21 3:51 PM, Vladimir Sementsov-Ogievskiy wrote: > We are going to convert .bdrv_co_preadv_part and .bdrv_co_pwritev_part > to int64_t type for offset and bytes parameters (as it's already done > for generic block/io.c layer). > > In qcow2 .bdrv_co_preadv_part is used in some places, so let's add > corresponding checks and assertions. > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Unusual line, especially since... > > block: use int64_t instead of uint64_t in driver read handlers > > We are generally moving to int64_t for both offset and bytes parameters > on all io paths. > > Main motivation is realization of 64-bit write_zeroes operation for > fast zeroing large disk chunks, up to the whole disk. > > We chose signed type, to be consistent with off_t (which is signed) and > with possibility for signed return type (where negative value means > error). > > So, convert driver read handlers parameters which are already 64bit to > signed type. > > While being here, convert also flags parameter to be BdrvRequestFlags. > > Now let's consider all callers. Simple > > git grep '\->bdrv_\(aio\|co\)_preadv\(_part\)\?' > > shows that's there three callers of driver function: > > bdrv_driver_preadv() in block/io.c, passes int64_t, checked by > bdrv_check_qiov_request() to be non-negative. > > qcow2_load_vmstate() does bdrv_check_qiov_request(). > > do_perform_cow_read() has uint64_t argument. And a lot of things in > qcow2 driver are uint64_t, so converting it is big job. But we must > not work with requests that doesn't satisfy bdrv_check_qiov_request(), s/doesn't/don't/ > so let's just assert it here. > > Still, the functions may be called directly, not only by drv->... > Let's check: > > git grep '\.bdrv_\(aio\|co\)_preadv\(_part\)\?\s*=' | \ > awk '{print $4}' | sed 's/,//' | sed 's/&//' | sort | uniq | \ > while read func; do git grep "$func(" | \ > grep -v "$func(BlockDriverState"; done > > The only one such caller: > > QEMUIOVector qiov = QEMU_IOVEC_INIT_BUF(qiov, &data, 1); > ... > ret = bdrv_replace_test_co_preadv(bs, 0, 1, &qiov, 0); > > in tesTS/unit/test-bdrv-drain.c, and it's OK obviously. > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> ...it is repeated here. I'm fine dropping the first. > --- > include/block/block_int.h | 11 ++++++----- > block/backup-top.c | 4 ++-- > 35 files changed, 120 insertions(+), 89 deletions(-) > > diff --git a/include/block/block_int.h b/include/block/block_int.h > index db7a909ea9..e6bcf74e46 100644 > --- a/include/block/block_int.h > +++ b/include/block/block_int.h > @@ -234,8 +234,8 @@ struct BlockDriver { > > /* aio */ > BlockAIOCB *(*bdrv_aio_preadv)(BlockDriverState *bs, > - uint64_t offset, uint64_t bytes, QEMUIOVector *qiov, int flags, > - BlockCompletionFunc *cb, void *opaque); > + int64_t offset, int64_t bytes, QEMUIOVector *qiov, > + BdrvRequestFlags flags, BlockCompletionFunc *cb, void *opaque); > BlockAIOCB *(*bdrv_aio_pwritev)(BlockDriverState *bs, > uint64_t offset, uint64_t bytes, QEMUIOVector *qiov, int flags, > BlockCompletionFunc *cb, void *opaque); > @@ -264,10 +264,11 @@ struct BlockDriver { > * The buffer in @qiov may point directly to guest memory. > */ > int coroutine_fn (*bdrv_co_preadv)(BlockDriverState *bs, > - uint64_t offset, uint64_t bytes, QEMUIOVector *qiov, int flags); > + int64_t offset, int64_t bytes, QEMUIOVector *qiov, > + BdrvRequestFlags flags); > int coroutine_fn (*bdrv_co_preadv_part)(BlockDriverState *bs, > - uint64_t offset, uint64_t bytes, > - QEMUIOVector *qiov, size_t qiov_offset, int flags); > + int64_t offset, int64_t bytes, > + QEMUIOVector *qiov, size_t qiov_offset, BdrvRequestFlags flags); Lots of fallout, but I'm in favor of this signature change. > +++ b/block/blkdebug.c > @@ -614,8 +614,8 @@ static int rule_check(BlockDriverState *bs, uint64_t offset, uint64_t bytes, > } > > static int coroutine_fn > -blkdebug_co_preadv(BlockDriverState *bs, uint64_t offset, uint64_t bytes, > - QEMUIOVector *qiov, int flags) > +blkdebug_co_preadv(BlockDriverState *bs, int64_t offset, int64_t bytes, > + QEMUIOVector *qiov, BdrvRequestFlags flags) > { > int err; Still calls rule_check() with uint64_t parameters, but since we've asserted the caller was within range, no behavior change. > +++ b/block/blkverify.c > @@ -221,8 +221,8 @@ blkverify_co_prwv(BlockDriverState *bs, BlkverifyRequest *r, uint64_t offset, > } > > static int coroutine_fn > -blkverify_co_preadv(BlockDriverState *bs, uint64_t offset, uint64_t bytes, > - QEMUIOVector *qiov, int flags) > +blkverify_co_preadv(BlockDriverState *bs, int64_t offset, int64_t bytes, > + QEMUIOVector *qiov, BdrvRequestFlags flags) > { Similarly for the call to blkverify_co_prwv(), and probably elsewhere in the patch. > +++ b/block/crypto.c > @@ -397,8 +397,8 @@ static int block_crypto_reopen_prepare(BDRVReopenState *state, > #define BLOCK_CRYPTO_MAX_IO_SIZE (1024 * 1024) > > static coroutine_fn int > -block_crypto_co_preadv(BlockDriverState *bs, uint64_t offset, uint64_t bytes, > - QEMUIOVector *qiov, int flags) > +block_crypto_co_preadv(BlockDriverState *bs, int64_t offset, int64_t bytes, > + QEMUIOVector *qiov, BdrvRequestFlags flags) > { > BlockCrypto *crypto = bs->opaque; > uint64_t cur_bytes; /* number of bytes in current iteration */ We could perhaps change the type of local variables like cur_bytes and bytes_done; but it doesn't affect semantics. > +++ b/block/curl.c > @@ -896,7 +896,8 @@ out: > } > > static int coroutine_fn curl_co_preadv(BlockDriverState *bs, > - uint64_t offset, uint64_t bytes, QEMUIOVector *qiov, int flags) > + int64_t offset, int64_t bytes, QEMUIOVector *qiov, > + BdrvRequestFlags flags) > { > CURLAIOCB acb = { > .co = qemu_coroutine_self(), Likewise changing the type of CURLAIOCB.offset and .bytes. Cleanups like that can be followups. > +++ b/block/file-posix.c > @@ -2033,9 +2033,9 @@ static int coroutine_fn raw_co_prw(BlockDriverState *bs, uint64_t offset, > return raw_thread_pool_submit(bs, handle_aiocb_rw, &acb); > } > > -static int coroutine_fn raw_co_preadv(BlockDriverState *bs, uint64_t offset, > - uint64_t bytes, QEMUIOVector *qiov, > - int flags) > +static int coroutine_fn raw_co_preadv(BlockDriverState *bs, int64_t offset, > + int64_t bytes, QEMUIOVector *qiov, > + BdrvRequestFlags flags) > { > return raw_co_prw(bs, offset, bytes, qiov, QEMU_AIO_READ); Similarly for fixing the signature of raw_co_prw() (after the counterpart change to raw_co_pwritev) > +++ b/block/nvme.c > @@ -1221,8 +1221,9 @@ static int nvme_co_prw(BlockDriverState *bs, uint64_t offset, uint64_t bytes, > } > > static coroutine_fn int nvme_co_preadv(BlockDriverState *bs, > - uint64_t offset, uint64_t bytes, > - QEMUIOVector *qiov, int flags) > + int64_t offset, int64_t bytes, > + QEMUIOVector *qiov, > + BdrvRequestFlags flags) > { > return nvme_co_prw(bs, offset, bytes, qiov, false, flags); > } And for nvme_co_prw(). > +++ b/block/qcow2.c > @@ -2281,9 +2281,10 @@ static coroutine_fn int qcow2_co_preadv_task_entry(AioTask *task) > } > > static coroutine_fn int qcow2_co_preadv_part(BlockDriverState *bs, > - uint64_t offset, uint64_t bytes, > + int64_t offset, int64_t bytes, > QEMUIOVector *qiov, > - size_t qiov_offset, int flags) > + size_t qiov_offset, > + BdrvRequestFlags flags) > { > BDRVQcow2State *s = bs->opaque; > int ret = 0; Unrelated to this patch; the loop sets cur_bytes = MIN(bytes, INT_MAX); but it would be smarter to choose a cluster-aligned max instead of INT_MAX. It doesn't matter as long as the block layer has pre-clamped requests to be < 32 bit to begin with, and then our later call to qcow2_get_host_offset() further clamps it down to actual cluster boundaries. But it does look odd, compared to computing the original MIN() to an aligned boundary in the first place, whether or not we ever change the block layer to allow individual drivers to opt in to reading more than 2G in one transaction. qcow2_add_task() is another internal function worth improving in a followup. > +++ b/block/quorum.c > @@ -663,8 +663,8 @@ static int read_fifo_child(QuorumAIOCB *acb) > return ret; > } > > -static int quorum_co_preadv(BlockDriverState *bs, uint64_t offset, > - uint64_t bytes, QEMUIOVector *qiov, int flags) > +static int quorum_co_preadv(BlockDriverState *bs, int64_t offset, int64_t bytes, > + QEMUIOVector *qiov, BdrvRequestFlags flags) > { > BDRVQuorumState *s = bs->opaque; > QuorumAIOCB *acb = quorum_aio_get(bs, qiov, offset, bytes, flags); and quorum_aio_get(). > diff --git a/block/raw-format.c b/block/raw-format.c > index 7717578ed6..e3f459b2cb 100644 > --- a/block/raw-format.c > +++ b/block/raw-format.c > @@ -181,8 +181,8 @@ static void raw_reopen_abort(BDRVReopenState *state) > } > > /* Check and adjust the offset, against 'offset' and 'size' options. */ > -static inline int raw_adjust_offset(BlockDriverState *bs, uint64_t *offset, > - uint64_t bytes, bool is_write) > +static inline int raw_adjust_offset(BlockDriverState *bs, int64_t *offset, > + int64_t bytes, bool is_write) > { > BDRVRawState *s = bs->opaque; > > @@ -201,9 +201,9 @@ static inline int raw_adjust_offset(BlockDriverState *bs, uint64_t *offset, > return 0; > } > > -static int coroutine_fn raw_co_preadv(BlockDriverState *bs, uint64_t offset, > - uint64_t bytes, QEMUIOVector *qiov, > - int flags) > +static int coroutine_fn raw_co_preadv(BlockDriverState *bs, int64_t offset, > + int64_t bytes, QEMUIOVector *qiov, > + BdrvRequestFlags flags) > { > int ret; > > @@ -259,7 +259,7 @@ static int coroutine_fn raw_co_pwritev(BlockDriverState *bs, uint64_t offset, > qiov = &local_qiov; > } > > - ret = raw_adjust_offset(bs, &offset, bytes, true); > + ret = raw_adjust_offset(bs, (int64_t *)&offset, bytes, true); Ugly type-punning; thankfully it's transient until the counterpart patch to driver write handlers. > if (ret) { > goto fail; > } > @@ -294,7 +294,7 @@ static int coroutine_fn raw_co_pwrite_zeroes(BlockDriverState *bs, > { > int ret; > > - ret = raw_adjust_offset(bs, (uint64_t *)&offset, bytes, true); > + ret = raw_adjust_offset(bs, (int64_t *)&offset, bytes, true); And now you should lose this cast... > if (ret) { > return ret; > } > @@ -306,7 +306,7 @@ static int coroutine_fn raw_co_pdiscard(BlockDriverState *bs, > { > int ret; > > - ret = raw_adjust_offset(bs, (uint64_t *)&offset, bytes, true); > + ret = raw_adjust_offset(bs, &offset, bytes, true); ...like you did here. > if (ret) { > return ret; > } > @@ -541,7 +541,7 @@ static int coroutine_fn raw_co_copy_range_from(BlockDriverState *bs, > { > int ret; > > - ret = raw_adjust_offset(bs, &src_offset, bytes, false); > + ret = raw_adjust_offset(bs, (int64_t *)&src_offset, bytes, false); > if (ret) { > return ret; > } > @@ -560,7 +560,7 @@ static int coroutine_fn raw_co_copy_range_to(BlockDriverState *bs, > { > int ret; > > - ret = raw_adjust_offset(bs, &dst_offset, bytes, true); > + ret = raw_adjust_offset(bs, (int64_t *)&dst_offset, bytes, true); Also transient casts. Easy enough to fix my nits for qcow2 and the commit message. Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
11.05.2021 22:22, Eric Blake wrote: > On 3/24/21 3:51 PM, Vladimir Sementsov-Ogievskiy wrote: >> We are going to convert .bdrv_co_preadv_part and .bdrv_co_pwritev_part >> to int64_t type for offset and bytes parameters (as it's already done >> for generic block/io.c layer). >> >> In qcow2 .bdrv_co_preadv_part is used in some places, so let's add >> corresponding checks and assertions. >> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > > Unusual line, especially since... > >> >> block: use int64_t instead of uint64_t in driver read handlers >> >> We are generally moving to int64_t for both offset and bytes parameters >> on all io paths. >> >> Main motivation is realization of 64-bit write_zeroes operation for >> fast zeroing large disk chunks, up to the whole disk. >> >> We chose signed type, to be consistent with off_t (which is signed) and >> with possibility for signed return type (where negative value means >> error). >> >> So, convert driver read handlers parameters which are already 64bit to >> signed type. >> >> While being here, convert also flags parameter to be BdrvRequestFlags. >> >> Now let's consider all callers. Simple >> >> git grep '\->bdrv_\(aio\|co\)_preadv\(_part\)\?' >> >> shows that's there three callers of driver function: >> >> bdrv_driver_preadv() in block/io.c, passes int64_t, checked by >> bdrv_check_qiov_request() to be non-negative. >> >> qcow2_load_vmstate() does bdrv_check_qiov_request(). >> >> do_perform_cow_read() has uint64_t argument. And a lot of things in >> qcow2 driver are uint64_t, so converting it is big job. But we must >> not work with requests that doesn't satisfy bdrv_check_qiov_request(), > > s/doesn't/don't/ > >> so let's just assert it here. >> >> Still, the functions may be called directly, not only by drv->... >> Let's check: >> >> git grep '\.bdrv_\(aio\|co\)_preadv\(_part\)\?\s*=' | \ >> awk '{print $4}' | sed 's/,//' | sed 's/&//' | sort | uniq | \ >> while read func; do git grep "$func(" | \ >> grep -v "$func(BlockDriverState"; done >> >> The only one such caller: >> >> QEMUIOVector qiov = QEMU_IOVEC_INIT_BUF(qiov, &data, 1); >> ... >> ret = bdrv_replace_test_co_preadv(bs, 0, 1, &qiov, 0); >> >> in tesTS/unit/test-bdrv-drain.c, and it's OK obviously. >> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > > ...it is repeated here. I'm fine dropping the first. > >> --- >> include/block/block_int.h | 11 ++++++----- >> block/backup-top.c | 4 ++-- > >> 35 files changed, 120 insertions(+), 89 deletions(-) >> >> diff --git a/include/block/block_int.h b/include/block/block_int.h >> index db7a909ea9..e6bcf74e46 100644 >> --- a/include/block/block_int.h >> +++ b/include/block/block_int.h >> @@ -234,8 +234,8 @@ struct BlockDriver { >> >> /* aio */ >> BlockAIOCB *(*bdrv_aio_preadv)(BlockDriverState *bs, >> - uint64_t offset, uint64_t bytes, QEMUIOVector *qiov, int flags, >> - BlockCompletionFunc *cb, void *opaque); >> + int64_t offset, int64_t bytes, QEMUIOVector *qiov, >> + BdrvRequestFlags flags, BlockCompletionFunc *cb, void *opaque); >> BlockAIOCB *(*bdrv_aio_pwritev)(BlockDriverState *bs, >> uint64_t offset, uint64_t bytes, QEMUIOVector *qiov, int flags, >> BlockCompletionFunc *cb, void *opaque); >> @@ -264,10 +264,11 @@ struct BlockDriver { >> * The buffer in @qiov may point directly to guest memory. >> */ >> int coroutine_fn (*bdrv_co_preadv)(BlockDriverState *bs, >> - uint64_t offset, uint64_t bytes, QEMUIOVector *qiov, int flags); >> + int64_t offset, int64_t bytes, QEMUIOVector *qiov, >> + BdrvRequestFlags flags); >> int coroutine_fn (*bdrv_co_preadv_part)(BlockDriverState *bs, >> - uint64_t offset, uint64_t bytes, >> - QEMUIOVector *qiov, size_t qiov_offset, int flags); >> + int64_t offset, int64_t bytes, >> + QEMUIOVector *qiov, size_t qiov_offset, BdrvRequestFlags flags); > > Lots of fallout, but I'm in favor of this signature change. > > >> +++ b/block/blkdebug.c >> @@ -614,8 +614,8 @@ static int rule_check(BlockDriverState *bs, uint64_t offset, uint64_t bytes, >> } >> >> static int coroutine_fn >> -blkdebug_co_preadv(BlockDriverState *bs, uint64_t offset, uint64_t bytes, >> - QEMUIOVector *qiov, int flags) >> +blkdebug_co_preadv(BlockDriverState *bs, int64_t offset, int64_t bytes, >> + QEMUIOVector *qiov, BdrvRequestFlags flags) >> { >> int err; > > Still calls rule_check() with uint64_t parameters, but since we've > asserted the caller was within range, no behavior change. > >> +++ b/block/blkverify.c >> @@ -221,8 +221,8 @@ blkverify_co_prwv(BlockDriverState *bs, BlkverifyRequest *r, uint64_t offset, >> } >> >> static int coroutine_fn >> -blkverify_co_preadv(BlockDriverState *bs, uint64_t offset, uint64_t bytes, >> - QEMUIOVector *qiov, int flags) >> +blkverify_co_preadv(BlockDriverState *bs, int64_t offset, int64_t bytes, >> + QEMUIOVector *qiov, BdrvRequestFlags flags) >> { > > Similarly for the call to blkverify_co_prwv(), and probably elsewhere in > the patch. > >> +++ b/block/crypto.c >> @@ -397,8 +397,8 @@ static int block_crypto_reopen_prepare(BDRVReopenState *state, >> #define BLOCK_CRYPTO_MAX_IO_SIZE (1024 * 1024) >> >> static coroutine_fn int >> -block_crypto_co_preadv(BlockDriverState *bs, uint64_t offset, uint64_t bytes, >> - QEMUIOVector *qiov, int flags) >> +block_crypto_co_preadv(BlockDriverState *bs, int64_t offset, int64_t bytes, >> + QEMUIOVector *qiov, BdrvRequestFlags flags) >> { >> BlockCrypto *crypto = bs->opaque; >> uint64_t cur_bytes; /* number of bytes in current iteration */ > > We could perhaps change the type of local variables like cur_bytes and > bytes_done; but it doesn't affect semantics. > >> +++ b/block/curl.c >> @@ -896,7 +896,8 @@ out: >> } >> >> static int coroutine_fn curl_co_preadv(BlockDriverState *bs, >> - uint64_t offset, uint64_t bytes, QEMUIOVector *qiov, int flags) >> + int64_t offset, int64_t bytes, QEMUIOVector *qiov, >> + BdrvRequestFlags flags) >> { >> CURLAIOCB acb = { >> .co = qemu_coroutine_self(), > > Likewise changing the type of CURLAIOCB.offset and .bytes. Cleanups > like that can be followups. > >> +++ b/block/file-posix.c >> @@ -2033,9 +2033,9 @@ static int coroutine_fn raw_co_prw(BlockDriverState *bs, uint64_t offset, >> return raw_thread_pool_submit(bs, handle_aiocb_rw, &acb); >> } >> >> -static int coroutine_fn raw_co_preadv(BlockDriverState *bs, uint64_t offset, >> - uint64_t bytes, QEMUIOVector *qiov, >> - int flags) >> +static int coroutine_fn raw_co_preadv(BlockDriverState *bs, int64_t offset, >> + int64_t bytes, QEMUIOVector *qiov, >> + BdrvRequestFlags flags) >> { >> return raw_co_prw(bs, offset, bytes, qiov, QEMU_AIO_READ); > > Similarly for fixing the signature of raw_co_prw() (after the > counterpart change to raw_co_pwritev) > >> +++ b/block/nvme.c >> @@ -1221,8 +1221,9 @@ static int nvme_co_prw(BlockDriverState *bs, uint64_t offset, uint64_t bytes, >> } >> >> static coroutine_fn int nvme_co_preadv(BlockDriverState *bs, >> - uint64_t offset, uint64_t bytes, >> - QEMUIOVector *qiov, int flags) >> + int64_t offset, int64_t bytes, >> + QEMUIOVector *qiov, >> + BdrvRequestFlags flags) >> { >> return nvme_co_prw(bs, offset, bytes, qiov, false, flags); >> } > > And for nvme_co_prw(). > >> +++ b/block/qcow2.c >> @@ -2281,9 +2281,10 @@ static coroutine_fn int qcow2_co_preadv_task_entry(AioTask *task) >> } >> >> static coroutine_fn int qcow2_co_preadv_part(BlockDriverState *bs, >> - uint64_t offset, uint64_t bytes, >> + int64_t offset, int64_t bytes, >> QEMUIOVector *qiov, >> - size_t qiov_offset, int flags) >> + size_t qiov_offset, >> + BdrvRequestFlags flags) >> { >> BDRVQcow2State *s = bs->opaque; >> int ret = 0; > > Unrelated to this patch; the loop sets cur_bytes = MIN(bytes, INT_MAX); > but it would be smarter to choose a cluster-aligned max instead of > INT_MAX. It doesn't matter as long as the block layer has pre-clamped > requests to be < 32 bit to begin with, and then our later call to > qcow2_get_host_offset() further clamps it down to actual cluster > boundaries. But it does look odd, compared to computing the original > MIN() to an aligned boundary in the first place, whether or not we ever > change the block layer to allow individual drivers to opt in to reading > more than 2G in one transaction. > > qcow2_add_task() is another internal function worth improving in a followup. > >> +++ b/block/quorum.c >> @@ -663,8 +663,8 @@ static int read_fifo_child(QuorumAIOCB *acb) >> return ret; >> } >> >> -static int quorum_co_preadv(BlockDriverState *bs, uint64_t offset, >> - uint64_t bytes, QEMUIOVector *qiov, int flags) >> +static int quorum_co_preadv(BlockDriverState *bs, int64_t offset, int64_t bytes, >> + QEMUIOVector *qiov, BdrvRequestFlags flags) >> { >> BDRVQuorumState *s = bs->opaque; >> QuorumAIOCB *acb = quorum_aio_get(bs, qiov, offset, bytes, flags); > > and quorum_aio_get(). > >> diff --git a/block/raw-format.c b/block/raw-format.c >> index 7717578ed6..e3f459b2cb 100644 >> --- a/block/raw-format.c >> +++ b/block/raw-format.c >> @@ -181,8 +181,8 @@ static void raw_reopen_abort(BDRVReopenState *state) >> } >> >> /* Check and adjust the offset, against 'offset' and 'size' options. */ >> -static inline int raw_adjust_offset(BlockDriverState *bs, uint64_t *offset, >> - uint64_t bytes, bool is_write) >> +static inline int raw_adjust_offset(BlockDriverState *bs, int64_t *offset, >> + int64_t bytes, bool is_write) >> { >> BDRVRawState *s = bs->opaque; >> >> @@ -201,9 +201,9 @@ static inline int raw_adjust_offset(BlockDriverState *bs, uint64_t *offset, >> return 0; >> } >> >> -static int coroutine_fn raw_co_preadv(BlockDriverState *bs, uint64_t offset, >> - uint64_t bytes, QEMUIOVector *qiov, >> - int flags) >> +static int coroutine_fn raw_co_preadv(BlockDriverState *bs, int64_t offset, >> + int64_t bytes, QEMUIOVector *qiov, >> + BdrvRequestFlags flags) >> { >> int ret; >> >> @@ -259,7 +259,7 @@ static int coroutine_fn raw_co_pwritev(BlockDriverState *bs, uint64_t offset, >> qiov = &local_qiov; >> } >> >> - ret = raw_adjust_offset(bs, &offset, bytes, true); >> + ret = raw_adjust_offset(bs, (int64_t *)&offset, bytes, true); > > Ugly type-punning; thankfully it's transient until the counterpart patch > to driver write handlers. > >> if (ret) { >> goto fail; >> } >> @@ -294,7 +294,7 @@ static int coroutine_fn raw_co_pwrite_zeroes(BlockDriverState *bs, >> { >> int ret; >> >> - ret = raw_adjust_offset(bs, (uint64_t *)&offset, bytes, true); >> + ret = raw_adjust_offset(bs, (int64_t *)&offset, bytes, true); > > And now you should lose this cast... > >> if (ret) { >> return ret; >> } >> @@ -306,7 +306,7 @@ static int coroutine_fn raw_co_pdiscard(BlockDriverState *bs, >> { >> int ret; >> >> - ret = raw_adjust_offset(bs, (uint64_t *)&offset, bytes, true); >> + ret = raw_adjust_offset(bs, &offset, bytes, true); > > ...like you did here. > >> if (ret) { >> return ret; >> } >> @@ -541,7 +541,7 @@ static int coroutine_fn raw_co_copy_range_from(BlockDriverState *bs, >> { >> int ret; >> >> - ret = raw_adjust_offset(bs, &src_offset, bytes, false); >> + ret = raw_adjust_offset(bs, (int64_t *)&src_offset, bytes, false); >> if (ret) { >> return ret; >> } >> @@ -560,7 +560,7 @@ static int coroutine_fn raw_co_copy_range_to(BlockDriverState *bs, >> { >> int ret; >> >> - ret = raw_adjust_offset(bs, &dst_offset, bytes, true); >> + ret = raw_adjust_offset(bs, (int64_t *)&dst_offset, bytes, true); > > Also transient casts. > > Easy enough to fix my nits for qcow2 and the commit message. > Reviewed-by: Eric Blake <eblake@redhat.com> > But you said that qcow2 note is unrelated to that patch. So, as I understand, only commit message and raw_co_pwrite_zeroes (drop extra case) should be fixed here, yes? -- Best regards, Vladimir
© 2016 - 2024 Red Hat, Inc.