We are gradually moving away from sector-based interfaces, towards
byte-based. Make the change for the last few sector-based callbacks
in the null-co and null-aio drivers.
Note that since the null driver does nothing on writes, it trivially
supports the BDRV_REQ_FUA flag (all writes have already landed to
the same bit-bucket without needing an extra flush call). Furthermore,
bdrv_refresh_limits() defaults the block size to 512 for any driver
that does not support coroutines; while this is still correct for the
other aio-based drivers, the null driver does just as well with
byte-based requests, and being explicit means we can avoid cycles
wasted on read-modify-write.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
block/null.c | 66 ++++++++++++++++++++++++++++++++++--------------------------
1 file changed, 37 insertions(+), 29 deletions(-)
diff --git a/block/null.c b/block/null.c
index 806a8631e4d..2381090dfcf 100644
--- a/block/null.c
+++ b/block/null.c
@@ -93,6 +93,7 @@ static int null_file_open(BlockDriverState *bs, QDict *options, int flags,
}
s->read_zeroes = qemu_opt_get_bool(opts, NULL_OPT_ZEROES, false);
qemu_opts_del(opts);
+ bs->supported_write_flags = BDRV_REQ_FUA;
return ret;
}
@@ -116,22 +117,22 @@ static coroutine_fn int null_co_common(BlockDriverState *bs)
return 0;
}
-static coroutine_fn int null_co_readv(BlockDriverState *bs,
- int64_t sector_num, int nb_sectors,
- QEMUIOVector *qiov)
+static coroutine_fn int null_co_preadv(BlockDriverState *bs,
+ uint64_t offset, uint64_t bytes,
+ QEMUIOVector *qiov, int flags)
{
BDRVNullState *s = bs->opaque;
if (s->read_zeroes) {
- qemu_iovec_memset(qiov, 0, 0, nb_sectors * BDRV_SECTOR_SIZE);
+ qemu_iovec_memset(qiov, 0, 0, bytes);
}
return null_co_common(bs);
}
-static coroutine_fn int null_co_writev(BlockDriverState *bs,
- int64_t sector_num, int nb_sectors,
- QEMUIOVector *qiov)
+static coroutine_fn int null_co_pwritev(BlockDriverState *bs,
+ uint64_t offset, uint64_t bytes,
+ QEMUIOVector *qiov, int flags)
{
return null_co_common(bs);
}
@@ -186,26 +187,26 @@ static inline BlockAIOCB *null_aio_common(BlockDriverState *bs,
return &acb->common;
}
-static BlockAIOCB *null_aio_readv(BlockDriverState *bs,
- int64_t sector_num, QEMUIOVector *qiov,
- int nb_sectors,
- BlockCompletionFunc *cb,
- void *opaque)
-{
- BDRVNullState *s = bs->opaque;
-
- if (s->read_zeroes) {
- qemu_iovec_memset(qiov, 0, 0, nb_sectors * BDRV_SECTOR_SIZE);
- }
-
- return null_aio_common(bs, cb, opaque);
-}
-
-static BlockAIOCB *null_aio_writev(BlockDriverState *bs,
- int64_t sector_num, QEMUIOVector *qiov,
- int nb_sectors,
+static BlockAIOCB *null_aio_preadv(BlockDriverState *bs,
+ uint64_t offset, uint64_t bytes,
+ QEMUIOVector *qiov, int flags,
BlockCompletionFunc *cb,
void *opaque)
+{
+ BDRVNullState *s = bs->opaque;
+
+ if (s->read_zeroes) {
+ qemu_iovec_memset(qiov, 0, 0, bytes);
+ }
+
+ return null_aio_common(bs, cb, opaque);
+}
+
+static BlockAIOCB *null_aio_pwritev(BlockDriverState *bs,
+ uint64_t offset, uint64_t bytes,
+ QEMUIOVector *qiov, int flags,
+ BlockCompletionFunc *cb,
+ void *opaque)
{
return null_aio_common(bs, cb, opaque);
}
@@ -256,6 +257,11 @@ static void null_refresh_filename(BlockDriverState *bs, QDict *opts)
bs->full_open_options = opts;
}
+static void null_refresh_limits(BlockDriverState *bs, Error **errp)
+{
+ bs->bl.request_alignment = 1;
+}
+
static BlockDriver bdrv_null_co = {
.format_name = "null-co",
.protocol_name = "null-co",
@@ -266,14 +272,15 @@ static BlockDriver bdrv_null_co = {
.bdrv_close = null_close,
.bdrv_getlength = null_getlength,
- .bdrv_co_readv = null_co_readv,
- .bdrv_co_writev = null_co_writev,
+ .bdrv_co_preadv = null_co_preadv,
+ .bdrv_co_pwritev = null_co_pwritev,
.bdrv_co_flush_to_disk = null_co_flush,
.bdrv_reopen_prepare = null_reopen_prepare,
.bdrv_co_block_status = null_co_block_status,
.bdrv_refresh_filename = null_refresh_filename,
+ .bdrv_refresh_limits = null_refresh_limits,
};
static BlockDriver bdrv_null_aio = {
@@ -286,14 +293,15 @@ static BlockDriver bdrv_null_aio = {
.bdrv_close = null_close,
.bdrv_getlength = null_getlength,
- .bdrv_aio_readv = null_aio_readv,
- .bdrv_aio_writev = null_aio_writev,
+ .bdrv_aio_preadv = null_aio_preadv,
+ .bdrv_aio_pwritev = null_aio_pwritev,
.bdrv_aio_flush = null_aio_flush,
.bdrv_reopen_prepare = null_reopen_prepare,
.bdrv_co_block_status = null_co_block_status,
.bdrv_refresh_filename = null_refresh_filename,
+ .bdrv_refresh_limits = null_refresh_limits,
};
static void bdrv_null_init(void)
--
2.14.3
Am 15.02.2018 um 20:28 hat Eric Blake geschrieben:
> We are gradually moving away from sector-based interfaces, towards
> byte-based. Make the change for the last few sector-based callbacks
> in the null-co and null-aio drivers.
>
> Note that since the null driver does nothing on writes, it trivially
> supports the BDRV_REQ_FUA flag (all writes have already landed to
> the same bit-bucket without needing an extra flush call). Furthermore,
> bdrv_refresh_limits() defaults the block size to 512 for any driver
> that does not support coroutines; while this is still correct for the
> other aio-based drivers, the null driver does just as well with
> byte-based requests, and being explicit means we can avoid cycles
> wasted on read-modify-write.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> +static void null_refresh_limits(BlockDriverState *bs, Error **errp)
> +{
> + bs->bl.request_alignment = 1;
> +}
I would rather modify bdrv_refresh_limits() so that it defaults to 1 for
drivers supporting either .bdrv_co_preadv or .bdrv_aio_preadv.
Kevin
On 04/24/2018 10:52 AM, Kevin Wolf wrote:
> Am 15.02.2018 um 20:28 hat Eric Blake geschrieben:
>> We are gradually moving away from sector-based interfaces, towards
>> byte-based. Make the change for the last few sector-based callbacks
>> in the null-co and null-aio drivers.
>>
>> Note that since the null driver does nothing on writes, it trivially
>> supports the BDRV_REQ_FUA flag (all writes have already landed to
>> the same bit-bucket without needing an extra flush call). Furthermore,
>> bdrv_refresh_limits() defaults the block size to 512 for any driver
>> that does not support coroutines; while this is still correct for the
>> other aio-based drivers, the null driver does just as well with
>> byte-based requests, and being explicit means we can avoid cycles
>> wasted on read-modify-write.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>
>> +static void null_refresh_limits(BlockDriverState *bs, Error **errp)
>> +{
>> + bs->bl.request_alignment = 1;
>> +}
>
> I would rather modify bdrv_refresh_limits() so that it defaults to 1 for
> drivers supporting either .bdrv_co_preadv or .bdrv_aio_preadv.
Sure, I can do that (although then I may have to provide a
refresh_limits callback for each of the other drivers that I
specifically left at 512 alignment).
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
Am 24.04.2018 um 19:00 hat Eric Blake geschrieben:
> On 04/24/2018 10:52 AM, Kevin Wolf wrote:
> > Am 15.02.2018 um 20:28 hat Eric Blake geschrieben:
> >> We are gradually moving away from sector-based interfaces, towards
> >> byte-based. Make the change for the last few sector-based callbacks
> >> in the null-co and null-aio drivers.
> >>
> >> Note that since the null driver does nothing on writes, it trivially
> >> supports the BDRV_REQ_FUA flag (all writes have already landed to
> >> the same bit-bucket without needing an extra flush call). Furthermore,
> >> bdrv_refresh_limits() defaults the block size to 512 for any driver
> >> that does not support coroutines; while this is still correct for the
> >> other aio-based drivers, the null driver does just as well with
> >> byte-based requests, and being explicit means we can avoid cycles
> >> wasted on read-modify-write.
> >>
> >> Signed-off-by: Eric Blake <eblake@redhat.com>
> >
> >> +static void null_refresh_limits(BlockDriverState *bs, Error **errp)
> >> +{
> >> + bs->bl.request_alignment = 1;
> >> +}
> >
> > I would rather modify bdrv_refresh_limits() so that it defaults to 1 for
> > drivers supporting either .bdrv_co_preadv or .bdrv_aio_preadv.
>
> Sure, I can do that (although then I may have to provide a
> refresh_limits callback for each of the other drivers that I
> specifically left at 512 alignment).
Do we know that the other drivers need 512, or do we expect than they
can handle byte granularity, but we try to err on the safe side?
If it's the latter (which would be my expectation), then it's probably
better to set request_alignment = 512 with a comment in those drivers
and hope that someone will lift the restriction in the future.
Kevin
On 04/24/2018 12:19 PM, Kevin Wolf wrote:
>>>> +static void null_refresh_limits(BlockDriverState *bs, Error **errp)
>>>> +{
>>>> + bs->bl.request_alignment = 1;
>>>> +}
>>>
>>> I would rather modify bdrv_refresh_limits() so that it defaults to 1 for
>>> drivers supporting either .bdrv_co_preadv or .bdrv_aio_preadv.
>>
>> Sure, I can do that (although then I may have to provide a
>> refresh_limits callback for each of the other drivers that I
>> specifically left at 512 alignment).
>
> Do we know that the other drivers need 512, or do we expect than they
> can handle byte granularity, but we try to err on the safe side?
Done to err on the safe side and avoid the difficulty of a deep audit in
code I'm unfamiliar with; in fact, for vxhs, I wasn't even able to
compile test things, which means I certainly wasn't able to reproduce a
test to see if dropping the alignment would break anything under qemu-io.
>
> If it's the latter (which would be my expectation), then it's probably
> better to set request_alignment = 512 with a comment in those drivers
> and hope that someone will lift the restriction in the future.
Indeed, leaving a good comment for a future reader is worth the effort.
I'll send a v2 later today.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
© 2016 - 2025 Red Hat, Inc.