Most of coroutine wrappers already follow this notation:
We have coroutine_fn bdrv_co_<something>(<normal argument list>), which
is the core functions, and wrapper, which does polling loope is called
bdrv_<something>(<same argument list>).
The only outsiders are bdrv_prwv_co and bdrv_common_block_status_above
wrappers. Let's refactor the to behave as the others, it simplifies
further conversion of coroutine wrappers.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
block/io.c | 61 +++++++++++++++++++++++++++++-------------------------
1 file changed, 33 insertions(+), 28 deletions(-)
diff --git a/block/io.c b/block/io.c
index 121ce17a49..bd00a70b47 100644
--- a/block/io.c
+++ b/block/io.c
@@ -900,28 +900,32 @@ typedef struct RwCo {
BdrvRequestFlags flags;
} RwCo;
+static int coroutine_fn bdrv_co_prwv(BdrvChild *child, int64_t offset,
+ QEMUIOVector *qiov, bool is_write,
+ BdrvRequestFlags flags)
+{
+ if (is_write) {
+ return bdrv_co_pwritev(child, offset, qiov->size, qiov, flags);
+ } else {
+ return bdrv_co_preadv(child, offset, qiov->size, qiov, flags);
+ }
+}
+
static void coroutine_fn bdrv_rw_co_entry(void *opaque)
{
RwCo *rwco = opaque;
- if (!rwco->is_write) {
- rwco->ret = bdrv_co_preadv(rwco->child, rwco->offset,
- rwco->qiov->size, rwco->qiov,
- rwco->flags);
- } else {
- rwco->ret = bdrv_co_pwritev(rwco->child, rwco->offset,
- rwco->qiov->size, rwco->qiov,
- rwco->flags);
- }
+ rwco->ret = bdrv_co_prwv(rwco->child, rwco->offset, rwco->qiov,
+ rwco->is_write, rwco->flags);
aio_wait_kick();
}
/*
* Process a vectored synchronous request using coroutines
*/
-static int bdrv_prwv_co(BdrvChild *child, int64_t offset,
- QEMUIOVector *qiov, bool is_write,
- BdrvRequestFlags flags)
+static int bdrv_prwv(BdrvChild *child, int64_t offset,
+ QEMUIOVector *qiov, bool is_write,
+ BdrvRequestFlags flags)
{
Coroutine *co;
RwCo rwco = {
@@ -949,8 +953,7 @@ int bdrv_pwrite_zeroes(BdrvChild *child, int64_t offset,
{
QEMUIOVector qiov = QEMU_IOVEC_INIT_BUF(qiov, NULL, bytes);
- return bdrv_prwv_co(child, offset, &qiov, true,
- BDRV_REQ_ZERO_WRITE | flags);
+ return bdrv_prwv(child, offset, &qiov, true, BDRV_REQ_ZERO_WRITE | flags);
}
/*
@@ -999,7 +1002,7 @@ int bdrv_preadv(BdrvChild *child, int64_t offset, QEMUIOVector *qiov)
{
int ret;
- ret = bdrv_prwv_co(child, offset, qiov, false, 0);
+ ret = bdrv_prwv(child, offset, qiov, false, 0);
if (ret < 0) {
return ret;
}
@@ -1023,7 +1026,7 @@ int bdrv_pwritev(BdrvChild *child, int64_t offset, QEMUIOVector *qiov)
{
int ret;
- ret = bdrv_prwv_co(child, offset, qiov, true, 0);
+ ret = bdrv_prwv(child, offset, qiov, true, 0);
if (ret < 0) {
return ret;
}
@@ -2443,14 +2446,15 @@ early_out:
return ret;
}
-static int coroutine_fn bdrv_co_block_status_above(BlockDriverState *bs,
- BlockDriverState *base,
- bool want_zero,
- int64_t offset,
- int64_t bytes,
- int64_t *pnum,
- int64_t *map,
- BlockDriverState **file)
+static int coroutine_fn
+bdrv_co_common_block_status_above(BlockDriverState *bs,
+ BlockDriverState *base,
+ bool want_zero,
+ int64_t offset,
+ int64_t bytes,
+ int64_t *pnum,
+ int64_t *map,
+ BlockDriverState **file)
{
BlockDriverState *p;
int ret = 0;
@@ -2488,10 +2492,11 @@ static void coroutine_fn bdrv_block_status_above_co_entry(void *opaque)
{
BdrvCoBlockStatusData *data = opaque;
- data->ret = bdrv_co_block_status_above(data->bs, data->base,
- data->want_zero,
- data->offset, data->bytes,
- data->pnum, data->map, data->file);
+ data->ret = bdrv_co_common_block_status_above(data->bs, data->base,
+ data->want_zero,
+ data->offset, data->bytes,
+ data->pnum, data->map,
+ data->file);
data->done = true;
aio_wait_kick();
}
--
2.21.0
On 5/22/20 11:19 AM, Vladimir Sementsov-Ogievskiy wrote:
> Most of coroutine wrappers already follow this notation:
s/of/of our/
s/notation/convention/
>
> We have coroutine_fn bdrv_co_<something>(<normal argument list>), which
> is the core functions, and wrapper, which does polling loope is called
> bdrv_<something>(<same argument list>).
We have 'coroutine_fn bdrv_co_<something>(<normal argument list>)' as
the core function, and a wrapper 'bdrv_<something>(<same argument
list>)' which does a polling loop.
>
> The only outsiders are bdrv_prwv_co and bdrv_common_block_status_above
s/are/are the/
> wrappers. Let's refactor the to behave as the others, it simplifies
s/the/them/
> further conversion of coroutine wrappers.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
> block/io.c | 61 +++++++++++++++++++++++++++++-------------------------
> 1 file changed, 33 insertions(+), 28 deletions(-)
>
> diff --git a/block/io.c b/block/io.c
> index 121ce17a49..bd00a70b47 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -900,28 +900,32 @@ typedef struct RwCo {
> BdrvRequestFlags flags;
> } RwCo;
>
> +static int coroutine_fn bdrv_co_prwv(BdrvChild *child, int64_t offset,
> + QEMUIOVector *qiov, bool is_write,
> + BdrvRequestFlags flags)
> +{
> + if (is_write) {
> + return bdrv_co_pwritev(child, offset, qiov->size, qiov, flags);
> + } else {
> + return bdrv_co_preadv(child, offset, qiov->size, qiov, flags);
> + }
> +}
> +
If we're trying to avoid needless indirection, wouldn't it be simpler to
quit trying to slam reads and writes through a single prwv function that
then has to split back out, and instead make two separate coroutine
wrappers, one for just reads, and the other for just writes, without
having to mess with a 'bool is_write' parameter?
> static void coroutine_fn bdrv_rw_co_entry(void *opaque)
> {
That is, should we have bdrv_co_preadv_entry and bdrv_co_pwritev_entry
instead of just one bdrv_rw_co_entry?
At any rate, the renames done here are mechanical enough that if we make
further changes, it could be a separate commit.
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
23.05.2020 00:33, Eric Blake wrote:
> On 5/22/20 11:19 AM, Vladimir Sementsov-Ogievskiy wrote:
>> Most of coroutine wrappers already follow this notation:
>
> s/of/of our/
> s/notation/convention/
>
>>
>> We have coroutine_fn bdrv_co_<something>(<normal argument list>), which
>> is the core functions, and wrapper, which does polling loope is called
>> bdrv_<something>(<same argument list>).
>
> We have 'coroutine_fn bdrv_co_<something>(<normal argument list>)' as the core function, and a wrapper 'bdrv_<something>(<same argument list>)' which does a polling loop.
>
>>
>> The only outsiders are bdrv_prwv_co and bdrv_common_block_status_above
>
> s/are/are the/
>
>> wrappers. Let's refactor the to behave as the others, it simplifies
>
> s/the/them/
>
>> further conversion of coroutine wrappers.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>> block/io.c | 61 +++++++++++++++++++++++++++++-------------------------
>> 1 file changed, 33 insertions(+), 28 deletions(-)
>>
>> diff --git a/block/io.c b/block/io.c
>> index 121ce17a49..bd00a70b47 100644
>> --- a/block/io.c
>> +++ b/block/io.c
>> @@ -900,28 +900,32 @@ typedef struct RwCo {
>> BdrvRequestFlags flags;
>> } RwCo;
>> +static int coroutine_fn bdrv_co_prwv(BdrvChild *child, int64_t offset,
>> + QEMUIOVector *qiov, bool is_write,
>> + BdrvRequestFlags flags)
>> +{
>> + if (is_write) {
>> + return bdrv_co_pwritev(child, offset, qiov->size, qiov, flags);
>> + } else {
>> + return bdrv_co_preadv(child, offset, qiov->size, qiov, flags);
>> + }
>> +}
>> +
>
> If we're trying to avoid needless indirection, wouldn't it be simpler to quit trying to slam reads and writes through a single prwv function that then has to split back out, and instead make two separate coroutine wrappers, one for just reads, and the other for just writes, without having to mess with a 'bool is_write' parameter?
Yes, and it's simpler after the transformation than before. I even wanted to do it but forget.. Will do as a follow-up, or with next version.
>
>> static void coroutine_fn bdrv_rw_co_entry(void *opaque)
>> {
>
> That is, should we have bdrv_co_preadv_entry and bdrv_co_pwritev_entry instead of just one bdrv_rw_co_entry?
>
> At any rate, the renames done here are mechanical enough that if we make further changes, it could be a separate commit.
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
>
--
Best regards,
Vladimir
© 2016 - 2025 Red Hat, Inc.