From 69b55e03f7e65a36eb954d0b7d4698b258df2708 Mon Sep 17 00:00:00 2001 From: Vladimir Sementsov-Ogievskiy Date: Fri, 11 Dec 2020 21:39:19 +0300 Subject: block: refactor bdrv_check_request: add errp It's better to pass &error_abort than just assert that result is 0: on crash, we'll immediately see the reason in the backtrace. Signed-off-by: Vladimir Sementsov-Ogievskiy Message-Id: <20201211183934.169161-2-vsementsov@virtuozzo.com> Reviewed-by: Eric Blake [eblake: fix iotest 206 fallout] Signed-off-by: Eric Blake --- include/block/block_int.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'include') diff --git a/include/block/block_int.h b/include/block/block_int.h index d01fc23720..5bbbf9ee0a 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -93,7 +93,7 @@ typedef struct BdrvTrackedRequest { struct BdrvTrackedRequest *waiting_for; } BdrvTrackedRequest; -int bdrv_check_request(int64_t offset, int64_t bytes); +int bdrv_check_request(int64_t offset, int64_t bytes, Error **errp); struct BlockDriver { const char *format_name; -- cgit v1.2.3 From 4c002cef0e9abe7135d7916c51abce47f7fc1ee2 Mon Sep 17 00:00:00 2001 From: Vladimir Sementsov-Ogievskiy Date: Fri, 11 Dec 2020 21:39:20 +0300 Subject: util/iov: make qemu_iovec_init_extended() honest Actually, we can't extend the io vector in all cases. Handle possible MAX_IOV and size_t overflows. For now add assertion to callers (actually they rely on success anyway) and fix them in the following patch. Add also some additional good assertions to qemu_iovec_init_slice() while being here. Signed-off-by: Vladimir Sementsov-Ogievskiy Message-Id: <20201211183934.169161-3-vsementsov@virtuozzo.com> Reviewed-by: Eric Blake Signed-off-by: Eric Blake --- include/qemu/iov.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'include') diff --git a/include/qemu/iov.h b/include/qemu/iov.h index b6b283a5e5..9330746680 100644 --- a/include/qemu/iov.h +++ b/include/qemu/iov.h @@ -222,7 +222,7 @@ static inline void *qemu_iovec_buf(QEMUIOVector *qiov) void qemu_iovec_init(QEMUIOVector *qiov, int alloc_hint); void qemu_iovec_init_external(QEMUIOVector *qiov, struct iovec *iov, int niov); -void qemu_iovec_init_extended( +int qemu_iovec_init_extended( QEMUIOVector *qiov, void *head_buf, size_t head_len, QEMUIOVector *mid_qiov, size_t mid_offset, size_t mid_len, -- cgit v1.2.3 From 801625e69d947b0f9291d77bb1deb7545525c66d Mon Sep 17 00:00:00 2001 From: Vladimir Sementsov-Ogievskiy Date: Fri, 11 Dec 2020 21:39:24 +0300 Subject: block/throttle-groups: throttle_group_co_io_limits_intercept(): 64bit bytes The function is called from 64bit io handlers, and bytes is just passed to throttle_account() which is 64bit too (unsigned though). So, let's convert intermediate argument to 64bit too. This patch is a first in the 64-bit-blocklayer series, so 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). Patch-correctness audit by Eric Blake: Caller has 32-bit, this patch now causes widening which is safe: block/block-backend.c: blk_do_preadv() passes 'unsigned int' block/block-backend.c: blk_do_pwritev_part() passes 'unsigned int' block/throttle.c: throttle_co_pwrite_zeroes() passes 'int' block/throttle.c: throttle_co_pdiscard() passes 'int' Caller has 64-bit, this patch fixes potential bug where pre-patch could narrow, except it's easy enough to trace that callers are still capped at 2G actions: block/throttle.c: throttle_co_preadv() passes 'uint64_t' block/throttle.c: throttle_co_pwritev() passes 'uint64_t' Implementation in question: block/throttle-groups.c throttle_group_co_io_limits_intercept() takes 'unsigned int bytes' and uses it: argument to util/throttle.c throttle_account(uint64_t) All safe: it patches a latent bug, and does not introduce any 64-bit gotchas once throttle_co_p{read,write}v are relaxed, and assuming throttle_account() is not buggy. Signed-off-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Eric Blake Reviewed-by: Alberto Garcia Message-Id: <20201211183934.169161-7-vsementsov@virtuozzo.com> Signed-off-by: Eric Blake --- include/block/throttle-groups.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'include') diff --git a/include/block/throttle-groups.h b/include/block/throttle-groups.h index 8bf7d233fa..9541b32432 100644 --- a/include/block/throttle-groups.h +++ b/include/block/throttle-groups.h @@ -77,7 +77,7 @@ void throttle_group_unregister_tgm(ThrottleGroupMember *tgm); void throttle_group_restart_tgm(ThrottleGroupMember *tgm); void coroutine_fn throttle_group_co_io_limits_intercept(ThrottleGroupMember *tgm, - unsigned int bytes, + int64_t bytes, bool is_write); void throttle_group_attach_aio_context(ThrottleGroupMember *tgm, AioContext *new_context); -- cgit v1.2.3 From 8024726459f37ffcceebfc9e50ff8e5cc362f274 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Wed, 3 Feb 2021 08:14:15 -0600 Subject: block: use int64_t as bytes type in tracked requests 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). All requests in block/io must not overflow BDRV_MAX_LENGTH, all external users of BdrvTrackedRequest already have corresponding assertions, so we are safe. Add some assertions still. Signed-off-by: Vladimir Sementsov-Ogievskiy Message-Id: <20201211183934.169161-9-vsementsov@virtuozzo.com> Reviewed-by: Eric Blake Signed-off-by: Eric Blake --- include/block/block_int.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'include') diff --git a/include/block/block_int.h b/include/block/block_int.h index 5bbbf9ee0a..7f41f0990c 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -79,12 +79,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; QLIST_ENTRY(BdrvTrackedRequest) list; Coroutine *co; /* owner, used for deadlock detection */ -- cgit v1.2.3 From 37e9403ea87a473f96744af7583dbb3eaef8d0f6 Mon Sep 17 00:00:00 2001 From: Vladimir Sementsov-Ogievskiy Date: Fri, 11 Dec 2020 21:39:32 +0300 Subject: block/io: support int64_t bytes in bdrv_co_p{read,write}v_part() 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, prepare bdrv_co_preadv_part() and bdrv_co_pwritev_part() and their remaining dependencies now. bdrv_pad_request() is updated simultaneously, as pointer to bytes passed to it both from bdrv_co_pwritev_part() and bdrv_co_preadv_part(). So, all callers of bdrv_pad_request() are updated to pass 64bit bytes. bdrv_pad_request() is already good for 64bit requests, add corresponding assertion. Look at bdrv_co_preadv_part() and bdrv_co_pwritev_part(). Type is widening, so callers are safe. Let's look inside the functions. In bdrv_co_preadv_part() and bdrv_aligned_pwritev() we only pass bytes to other already int64_t interfaces (and some obviously safe calculations), it's OK. In bdrv_co_do_zero_pwritev() aligned_bytes may become large now, still it's passed to bdrv_aligned_pwritev which supports int64_t bytes. Signed-off-by: Vladimir Sementsov-Ogievskiy Message-Id: <20201211183934.169161-15-vsementsov@virtuozzo.com> Reviewed-by: Eric Blake Signed-off-by: Eric Blake --- include/block/block_int.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'include') diff --git a/include/block/block_int.h b/include/block/block_int.h index 7f41f0990c..f2ad8aa771 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -1035,13 +1035,13 @@ int coroutine_fn bdrv_co_preadv(BdrvChild *child, int64_t offset, unsigned int 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, 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, -- cgit v1.2.3 From e9e52efdc53bf7746bdb3c21f1a9ee5da298c6a2 Mon Sep 17 00:00:00 2001 From: Vladimir Sementsov-Ogievskiy Date: Fri, 11 Dec 2020 21:39:33 +0300 Subject: block/io: support int64_t bytes in read/write wrappers 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). Now, since bdrv_co_preadv_part() and bdrv_co_pwritev_part() have been updated, update all their wrappers. For all of them type of 'bytes' is widening, so callers are safe. We have update request_fn in blkverify.c simultaneously. Still it's just a pointer to one of bdrv_co_pwritev() or bdrv_co_preadv(), and type is widening for callers of the request_fn anyway. Signed-off-by: Vladimir Sementsov-Ogievskiy Message-Id: <20201211183934.169161-16-vsementsov@virtuozzo.com> Reviewed-by: Eric Blake [eblake: grammar tweak] Signed-off-by: Eric Blake --- include/block/block.h | 11 ++++++----- include/block/block_int.h | 4 ++-- 2 files changed, 8 insertions(+), 7 deletions(-) (limited to 'include') diff --git a/include/block/block.h b/include/block/block.h index 81fcaad5ac..5f28d0d33f 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -392,12 +392,13 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, void bdrv_reopen_commit(BDRVReopenState *reopen_state); void bdrv_reopen_abort(BDRVReopenState *reopen_state); int bdrv_pwrite_zeroes(BdrvChild *child, int64_t offset, - int bytes, BdrvRequestFlags flags); + int64_t bytes, BdrvRequestFlags flags); int bdrv_make_zero(BdrvChild *child, BdrvRequestFlags flags); -int bdrv_pread(BdrvChild *child, int64_t offset, void *buf, int bytes); -int bdrv_pwrite(BdrvChild *child, int64_t offset, const void *buf, int bytes); +int bdrv_pread(BdrvChild *child, int64_t offset, void *buf, int64_t bytes); +int bdrv_pwrite(BdrvChild *child, int64_t offset, const void *buf, + int64_t bytes); int bdrv_pwrite_sync(BdrvChild *child, int64_t offset, - const void *buf, int count); + const void *buf, int64_t bytes); /* * Efficiently zero a region of the disk image. Note that this is a regular * I/O request like read or write and should have a reasonable size. This @@ -405,7 +406,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); diff --git a/include/block/block_int.h b/include/block/block_int.h index f2ad8aa771..749d1fb9d0 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -1032,13 +1032,13 @@ 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, 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, int64_t bytes, -- cgit v1.2.3 From a5215b8fdf1f8b284b4e5ba4895a4b091b503444 Mon Sep 17 00:00:00 2001 From: Vladimir Sementsov-Ogievskiy Date: Fri, 11 Dec 2020 21:39:34 +0300 Subject: block/io: use int64_t bytes in copy_range 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 now copy_range parameters which are already 64bit to signed type. It's safe as we don't work with requests overflowing BDRV_MAX_LENGTH (which is less than INT64_MAX), and do check the requests in bdrv_co_copy_range_internal() (by bdrv_check_request32(), which calls bdrv_check_request()). Signed-off-by: Vladimir Sementsov-Ogievskiy Message-Id: <20201211183934.169161-17-vsementsov@virtuozzo.com> Reviewed-by: Eric Blake Signed-off-by: Eric Blake --- include/block/block.h | 6 +++--- include/block/block_int.h | 12 ++++++------ 2 files changed, 9 insertions(+), 9 deletions(-) (limited to 'include') diff --git a/include/block/block.h b/include/block/block.h index 5f28d0d33f..0a9f2c187c 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -845,8 +845,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 749d1fb9d0..22a2789d35 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -1357,14 +1357,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); -- cgit v1.2.3 From 5082fc82a6bc3fc06a04be47d39777c7cff61e5b Mon Sep 17 00:00:00 2001 From: Roman Kagan Date: Fri, 29 Jan 2021 10:38:59 +0300 Subject: nbd: make nbd_read* return -EIO on error NBD reconnect logic considers the error code from the functions that read NBD messages to tell if reconnect should be attempted or not: it is attempted on -EIO, otherwise the client transitions to NBD_CLIENT_QUIT state (see nbd_channel_error). This error code is propagated from the primitives like nbd_read. The problem, however, is that nbd_read itself turns every error into -1 rather than -EIO. As a result, if the NBD server happens to die while sending the message, the client in QEMU receives less data than it expects, considers it as a fatal error, and wouldn't attempt reestablishing the connection. Fix it by turning every negative return from qio_channel_read_all into -EIO returned from nbd_read. Apparently that was the original behavior, but got broken later. Also adjust nbd_readXX to follow. Fixes: e6798f06a6 ("nbd: generalize usage of nbd_read") Signed-off-by: Roman Kagan Reviewed-by: Vladimir Sementsov-Ogievskiy Message-Id: <20210129073859.683063-4-rvkagan@yandex-team.ru> Signed-off-by: Eric Blake --- include/block/nbd.h | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) (limited to 'include') diff --git a/include/block/nbd.h b/include/block/nbd.h index 4a52a43ef5..5f34d23bb0 100644 --- a/include/block/nbd.h +++ b/include/block/nbd.h @@ -364,7 +364,7 @@ static inline int nbd_read(QIOChannel *ioc, void *buffer, size_t size, if (desc) { error_prepend(errp, "Failed to read %s: ", desc); } - return -1; + return ret; } return 0; @@ -375,8 +375,9 @@ static inline int nbd_read##bits(QIOChannel *ioc, \ uint##bits##_t *val, \ const char *desc, Error **errp) \ { \ - if (nbd_read(ioc, val, sizeof(*val), desc, errp) < 0) { \ - return -1; \ + int ret = nbd_read(ioc, val, sizeof(*val), desc, errp); \ + if (ret < 0) { \ + return ret; \ } \ *val = be##bits##_to_cpu(*val); \ return 0; \ -- cgit v1.2.3