We are gradually moving away from sector-based interfaces, towards
byte-based. Now that all callers of blocking I/O have been converted
to use our preferred byte-based bdrv_p{read,write}(), we can
delete the unused bdrv_{read,write}().
Signed-off-by: Eric Blake <eblake@redhat.com>
---
include/block/block.h | 4 ----
block/io.c | 48 ++++++++----------------------------------------
2 files changed, 8 insertions(+), 44 deletions(-)
diff --git a/include/block/block.h b/include/block/block.h
index 8b191c23d97..a355c2c5319 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -265,10 +265,6 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state,
BlockReopenQueue *queue, Error **errp);
void bdrv_reopen_commit(BDRVReopenState *reopen_state);
void bdrv_reopen_abort(BDRVReopenState *reopen_state);
-int bdrv_read(BdrvChild *child, int64_t sector_num,
- uint8_t *buf, int nb_sectors);
-int bdrv_write(BdrvChild *child, int64_t sector_num,
- const uint8_t *buf, int nb_sectors);
int bdrv_pwrite_zeroes(BdrvChild *child, int64_t offset,
int bytes, BdrvRequestFlags flags);
int bdrv_make_zero(BdrvChild *child, BdrvRequestFlags flags);
diff --git a/block/io.c b/block/io.c
index 7155786eb47..f39d99ff210 100644
--- a/block/io.c
+++ b/block/io.c
@@ -715,45 +715,6 @@ static int bdrv_prwv_co(BdrvChild *child, int64_t offset,
return rwco.ret;
}
-/*
- * Process a synchronous request using coroutines
- */
-static int bdrv_rw_co(BdrvChild *child, int64_t sector_num, uint8_t *buf,
- int nb_sectors, bool is_write, BdrvRequestFlags flags)
-{
- QEMUIOVector qiov;
- struct iovec iov = {
- .iov_base = (void *)buf,
- .iov_len = nb_sectors * BDRV_SECTOR_SIZE,
- };
-
- if (nb_sectors < 0 || nb_sectors > BDRV_REQUEST_MAX_SECTORS) {
- return -EINVAL;
- }
-
- qemu_iovec_init_external(&qiov, &iov, 1);
- return bdrv_prwv_co(child, sector_num << BDRV_SECTOR_BITS,
- &qiov, is_write, flags);
-}
-
-/* return < 0 if error. See bdrv_write() for the return codes */
-int bdrv_read(BdrvChild *child, int64_t sector_num,
- uint8_t *buf, int nb_sectors)
-{
- return bdrv_rw_co(child, sector_num, buf, nb_sectors, false, 0);
-}
-
-/* Return < 0 if error. Important errors are:
- -EIO generic I/O error (may happen for all errors)
- -ENOMEDIUM No media inserted.
- -EINVAL Invalid sector number or nb_sectors
- -EACCES Trying to write a read-only device
-*/
-int bdrv_write(BdrvChild *child, int64_t sector_num,
- const uint8_t *buf, int nb_sectors)
-{
- return bdrv_rw_co(child, sector_num, (uint8_t *)buf, nb_sectors, true, 0);
-}
int bdrv_pwrite_zeroes(BdrvChild *child, int64_t offset,
int bytes, BdrvRequestFlags flags)
@@ -776,7 +737,7 @@ int bdrv_pwrite_zeroes(BdrvChild *child, int64_t offset,
* flags are passed through to bdrv_pwrite_zeroes (e.g. BDRV_REQ_MAY_UNMAP,
* BDRV_REQ_FUA).
*
- * Returns < 0 on error, 0 on success. For error codes see bdrv_write().
+ * Returns < 0 on error, 0 on success. For error codes see bdrv_pwrite().
*/
int bdrv_make_zero(BdrvChild *child, BdrvRequestFlags flags)
{
@@ -814,6 +775,7 @@ int bdrv_make_zero(BdrvChild *child, BdrvRequestFlags flags)
}
}
+/* return < 0 if error. See bdrv_pwrite() for the return codes */
int bdrv_preadv(BdrvChild *child, int64_t offset, QEMUIOVector *qiov)
{
int ret;
@@ -854,6 +816,12 @@ int bdrv_pwritev(BdrvChild *child, int64_t offset, QEMUIOVector *qiov)
return qiov->size;
}
+/* Return < 0 if error. Important errors are:
+ -EIO generic I/O error (may happen for all errors)
+ -ENOMEDIUM No media inserted.
+ -EINVAL Invalid sector number or nb_sectors
+ -EACCES Trying to write a read-only device
+*/
int bdrv_pwrite(BdrvChild *child, int64_t offset, const void *buf, int bytes)
{
QEMUIOVector qiov;
--
2.14.3
On Thu 26 Apr 2018 03:43:05 PM CEST, Eric Blake wrote:
> We are gradually moving away from sector-based interfaces, towards
> byte-based. Now that all callers of blocking I/O have been converted
> to use our preferred byte-based bdrv_p{read,write}(), we can
> delete the unused bdrv_{read,write}().
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
> -static int bdrv_rw_co(BdrvChild *child, int64_t sector_num, uint8_t *buf,
> - int nb_sectors, bool is_write, BdrvRequestFlags flags)
> -{
> - QEMUIOVector qiov;
> - struct iovec iov = {
> - .iov_base = (void *)buf,
> - .iov_len = nb_sectors * BDRV_SECTOR_SIZE,
> - };
> -
> - if (nb_sectors < 0 || nb_sectors > BDRV_REQUEST_MAX_SECTORS) {
> - return -EINVAL;
> - }
Do we have a check for BDRV_REQUEST_MAX_BYTES in the byte-based API?
Berto
On 04/27/2018 09:01 AM, Alberto Garcia wrote:
> On Thu 26 Apr 2018 03:43:05 PM CEST, Eric Blake wrote:
>> We are gradually moving away from sector-based interfaces, towards
>> byte-based. Now that all callers of blocking I/O have been converted
>> to use our preferred byte-based bdrv_p{read,write}(), we can
>> delete the unused bdrv_{read,write}().
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> Reviewed-by: Alberto Garcia <berto@igalia.com>
>
>> -static int bdrv_rw_co(BdrvChild *child, int64_t sector_num, uint8_t *buf,
>> - int nb_sectors, bool is_write, BdrvRequestFlags flags)
>> -{
>> - QEMUIOVector qiov;
>> - struct iovec iov = {
>> - .iov_base = (void *)buf,
>> - .iov_len = nb_sectors * BDRV_SECTOR_SIZE,
>> - };
>> -
>> - if (nb_sectors < 0 || nb_sectors > BDRV_REQUEST_MAX_SECTORS) {
>> - return -EINVAL;
>> - }
>
> Do we have a check for BDRV_REQUEST_MAX_BYTES in the byte-based API?
No, but we don't need one. First, note that bs->bl.max_transfer is
currently uint32_t, so right now, no driver can set it any larger than
4G; although they can certainly set it smaller (for example, NBD sets it
to 32M). Then note that bdrv_co_pwritev() and friends take care of
fragmenting any >4G request down to max_transfer. The old check was to
ensure that a sector count couldn't overflow larger than a 4G byte count
(since the sector API had a 32-bit intrinsic limit on byte count, even
though the parameter was in sectors); but the public byte-based API
doesn't have an intrinsic limit on byte count, and takes care of
fragmenting down to any intrinsic limit for the driver callbacks.
But, having said that, you've made me wonder if it's time to increase
max_transfer to [u]int64_t, then audit ALL drivers to ensure that they
either properly handle requests >=4G or else set max_transfer <4G
(ideally, the block layer will default max_transfer to the value of
BDRV_REQUEST_MAX_BYTES, even if the audit means we no longer need that
macro). Should be a separate series, though.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
On Fri 27 Apr 2018 05:43:33 PM CEST, Eric Blake wrote:
>>> -static int bdrv_rw_co(BdrvChild *child, int64_t sector_num, uint8_t *buf,
>>> - int nb_sectors, bool is_write, BdrvRequestFlags flags)
>>> -{
>>> - QEMUIOVector qiov;
>>> - struct iovec iov = {
>>> - .iov_base = (void *)buf,
>>> - .iov_len = nb_sectors * BDRV_SECTOR_SIZE,
>>> - };
>>> -
>>> - if (nb_sectors < 0 || nb_sectors > BDRV_REQUEST_MAX_SECTORS) {
>>> - return -EINVAL;
>>> - }
>>
>> Do we have a check for BDRV_REQUEST_MAX_BYTES in the byte-based API?
>
> No, but we don't need one. First, note that bs->bl.max_transfer is
> currently uint32_t, so right now, no driver can set it any larger than
> 4G
But note that the old limit was based on a signed integer.
BDRV_REQUEST_MAX_SECTORS is 2147483136 bytes (a bit less than 2GB).
For 32-bit integers, (INT_MAX - 1) & ~511 = 2147483136
Berto
[Reviving an old series]
On 5/3/18 8:36 AM, Alberto Garcia wrote:
> On Fri 27 Apr 2018 05:43:33 PM CEST, Eric Blake wrote:
>>>> -static int bdrv_rw_co(BdrvChild *child, int64_t sector_num, uint8_t *buf,
>>>> - int nb_sectors, bool is_write, BdrvRequestFlags flags)
>>>> -{
>>>> - QEMUIOVector qiov;
>>>> - struct iovec iov = {
>>>> - .iov_base = (void *)buf,
>>>> - .iov_len = nb_sectors * BDRV_SECTOR_SIZE,
>>>> - };
>>>> -
>>>> - if (nb_sectors < 0 || nb_sectors > BDRV_REQUEST_MAX_SECTORS) {
>>>> - return -EINVAL;
>>>> - }
>>>
>>> Do we have a check for BDRV_REQUEST_MAX_BYTES in the byte-based API?
>>
>> No, but we don't need one. First, note that bs->bl.max_transfer is
>> currently uint32_t, so right now, no driver can set it any larger than
>> 4G
>
> But note that the old limit was based on a signed integer.
>
> BDRV_REQUEST_MAX_SECTORS is 2147483136 bytes (a bit less than 2GB).
>
> For 32-bit integers, (INT_MAX - 1) & ~511 = 2147483136
The whole point of the byte-based callbacks is that they pass a 64-bit
length BUT also honor the driver's max_transfer. As long as drivers
default to (or explicitly set) a max_transfer of
BDRV_REQUEST_MAX_SECTORS or smaller, then the block layer has already
taken care of fragmenting things so that the callers no longer have to
worry about artificially fragmenting things themselves before handing
something to the block layer that might overflow. And you snipped the
rest of my mail, where I argued:
>
> But, having said that, you've made me wonder if it's time to increase
> max_transfer to [u]int64_t, then audit ALL drivers to ensure that they
> either properly handle requests >=4G or else set max_transfer <4G
> (ideally, the block layer will default max_transfer to the value of
> BDRV_REQUEST_MAX_BYTES, even if the audit means we no longer need that
> macro). Should be a separate series, though.
I will concede that you are right that because we previously used a
signed type, I should amend my argument to state that we should audit
ALL drivers to ensure that they either properly handle requests >= 2G
or else set max_transfer <2G, even though max_transfer is unsigned.
Maybe it's time that I attempt that audit, before posting a v2 of this
series for 4.0. (Fingers crossed that it will be a quick audit)
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
On Thu, Apr 26, 2018 at 08:43:05AM -0500, Eric Blake wrote:
> We are gradually moving away from sector-based interfaces, towards
> byte-based. Now that all callers of blocking I/O have been converted
> to use our preferred byte-based bdrv_p{read,write}(), we can
> delete the unused bdrv_{read,write}().
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
> include/block/block.h | 4 ----
> block/io.c | 48 ++++++++----------------------------------------
> 2 files changed, 8 insertions(+), 44 deletions(-)
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
© 2016 - 2025 Red Hat, Inc.