We have a few bdrv_*() functions that can either spawn a new coroutine
and wait for it with BDRV_POLL_WHILE() or use a fastpath if they are
alreeady running in a coroutine. All of them duplicate basically the
same code.
Factor the common code into a new function bdrv_run_co().
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
block/io.c | 104 +++++++++++++++--------------------------------------
1 file changed, 28 insertions(+), 76 deletions(-)
diff --git a/block/io.c b/block/io.c
index 7d30e61edc..c1badaadc9 100644
--- a/block/io.c
+++ b/block/io.c
@@ -891,6 +891,22 @@ static int bdrv_check_byte_request(BlockDriverState *bs, int64_t offset,
return 0;
}
+static int bdrv_run_co(BlockDriverState *bs, CoroutineEntry *entry,
+ void *opaque, int *ret)
+{
+ if (qemu_in_coroutine()) {
+ /* Fast-path if already in coroutine context */
+ entry(opaque);
+ } else {
+ Coroutine *co = qemu_coroutine_create(entry, opaque);
+ *ret = NOT_DONE;
+ bdrv_coroutine_enter(bs, co);
+ BDRV_POLL_WHILE(bs, *ret == NOT_DONE);
+ }
+
+ return *ret;
+}
+
typedef struct RwCo {
BdrvChild *child;
int64_t offset;
@@ -923,25 +939,15 @@ static int bdrv_prwv_co(BdrvChild *child, int64_t offset,
QEMUIOVector *qiov, bool is_write,
BdrvRequestFlags flags)
{
- Coroutine *co;
RwCo rwco = {
.child = child,
.offset = offset,
.qiov = qiov,
.is_write = is_write,
- .ret = NOT_DONE,
.flags = flags,
};
- if (qemu_in_coroutine()) {
- /* Fast-path if already in coroutine context */
- bdrv_rw_co_entry(&rwco);
- } else {
- co = qemu_coroutine_create(bdrv_rw_co_entry, &rwco);
- bdrv_coroutine_enter(child->bs, co);
- BDRV_POLL_WHILE(child->bs, rwco.ret == NOT_DONE);
- }
- return rwco.ret;
+ return bdrv_run_co(child->bs, bdrv_rw_co_entry, &rwco, &rwco.ret);
}
int bdrv_pwrite_zeroes(BdrvChild *child, int64_t offset,
@@ -2230,7 +2236,6 @@ typedef struct BdrvCoBlockStatusData {
int64_t *map;
BlockDriverState **file;
int ret;
- bool done;
} BdrvCoBlockStatusData;
int coroutine_fn bdrv_co_block_status_from_file(BlockDriverState *bs,
@@ -2492,7 +2497,6 @@ static void coroutine_fn bdrv_block_status_above_co_entry(void *opaque)
data->want_zero,
data->offset, data->bytes,
data->pnum, data->map, data->file);
- data->done = true;
aio_wait_kick();
}
@@ -2508,7 +2512,6 @@ static int bdrv_common_block_status_above(BlockDriverState *bs,
int64_t *map,
BlockDriverState **file)
{
- Coroutine *co;
BdrvCoBlockStatusData data = {
.bs = bs,
.base = base,
@@ -2518,18 +2521,9 @@ static int bdrv_common_block_status_above(BlockDriverState *bs,
.pnum = pnum,
.map = map,
.file = file,
- .done = false,
};
- if (qemu_in_coroutine()) {
- /* Fast-path if already in coroutine context */
- bdrv_block_status_above_co_entry(&data);
- } else {
- co = qemu_coroutine_create(bdrv_block_status_above_co_entry, &data);
- bdrv_coroutine_enter(bs, co);
- BDRV_POLL_WHILE(bs, !data.done);
- }
- return data.ret;
+ return bdrv_run_co(bs, bdrv_block_status_above_co_entry, &data, &data.ret);
}
int bdrv_block_status_above(BlockDriverState *bs, BlockDriverState *base,
@@ -2669,22 +2663,13 @@ static inline int
bdrv_rw_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos,
bool is_read)
{
- if (qemu_in_coroutine()) {
- return bdrv_co_rw_vmstate(bs, qiov, pos, is_read);
- } else {
- BdrvVmstateCo data = {
- .bs = bs,
- .qiov = qiov,
- .pos = pos,
- .is_read = is_read,
- .ret = -EINPROGRESS,
- };
- Coroutine *co = qemu_coroutine_create(bdrv_co_rw_vmstate_entry, &data);
-
- bdrv_coroutine_enter(bs, co);
- BDRV_POLL_WHILE(bs, data.ret == -EINPROGRESS);
- return data.ret;
- }
+ BdrvVmstateCo data = {
+ .bs = bs,
+ .qiov = qiov,
+ .pos = pos,
+ .is_read = is_read,
+ };
+ return bdrv_run_co(bs, bdrv_co_rw_vmstate_entry, &data, &data.ret);
}
int bdrv_save_vmstate(BlockDriverState *bs, const uint8_t *buf,
@@ -2890,22 +2875,11 @@ early_exit:
int bdrv_flush(BlockDriverState *bs)
{
- Coroutine *co;
FlushCo flush_co = {
.bs = bs,
- .ret = NOT_DONE,
};
- if (qemu_in_coroutine()) {
- /* Fast-path if already in coroutine context */
- bdrv_flush_co_entry(&flush_co);
- } else {
- co = qemu_coroutine_create(bdrv_flush_co_entry, &flush_co);
- bdrv_coroutine_enter(bs, co);
- BDRV_POLL_WHILE(bs, flush_co.ret == NOT_DONE);
- }
-
- return flush_co.ret;
+ return bdrv_run_co(bs, bdrv_flush_co_entry, &flush_co, &flush_co.ret);
}
typedef struct DiscardCo {
@@ -3038,24 +3012,13 @@ out:
int bdrv_pdiscard(BdrvChild *child, int64_t offset, int64_t bytes)
{
- Coroutine *co;
DiscardCo rwco = {
.child = child,
.offset = offset,
.bytes = bytes,
- .ret = NOT_DONE,
};
- if (qemu_in_coroutine()) {
- /* Fast-path if already in coroutine context */
- bdrv_pdiscard_co_entry(&rwco);
- } else {
- co = qemu_coroutine_create(bdrv_pdiscard_co_entry, &rwco);
- bdrv_coroutine_enter(child->bs, co);
- BDRV_POLL_WHILE(child->bs, rwco.ret == NOT_DONE);
- }
-
- return rwco.ret;
+ return bdrv_run_co(child->bs, bdrv_pdiscard_co_entry, &rwco, &rwco.ret);
}
int bdrv_co_ioctl(BlockDriverState *bs, int req, void *buf)
@@ -3477,7 +3440,6 @@ static void coroutine_fn bdrv_truncate_co_entry(void *opaque)
int bdrv_truncate(BdrvChild *child, int64_t offset, bool exact,
PreallocMode prealloc, BdrvRequestFlags flags, Error **errp)
{
- Coroutine *co;
TruncateCo tco = {
.child = child,
.offset = offset,
@@ -3485,17 +3447,7 @@ int bdrv_truncate(BdrvChild *child, int64_t offset, bool exact,
.prealloc = prealloc,
.flags = flags,
.errp = errp,
- .ret = NOT_DONE,
};
- if (qemu_in_coroutine()) {
- /* Fast-path if already in coroutine context */
- bdrv_truncate_co_entry(&tco);
- } else {
- co = qemu_coroutine_create(bdrv_truncate_co_entry, &tco);
- bdrv_coroutine_enter(child->bs, co);
- BDRV_POLL_WHILE(child->bs, tco.ret == NOT_DONE);
- }
-
- return tco.ret;
+ return bdrv_run_co(child->bs, bdrv_truncate_co_entry, &tco, &tco.ret);
}
--
2.25.3
On 5/12/20 9:43 AM, Kevin Wolf wrote:
> We have a few bdrv_*() functions that can either spawn a new coroutine
> and wait for it with BDRV_POLL_WHILE() or use a fastpath if they are
> alreeady running in a coroutine. All of them duplicate basically the
already
> same code.
>
> Factor the common code into a new function bdrv_run_co().
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> block/io.c | 104 +++++++++++++++--------------------------------------
> 1 file changed, 28 insertions(+), 76 deletions(-)
>
> diff --git a/block/io.c b/block/io.c
> index 7d30e61edc..c1badaadc9 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -891,6 +891,22 @@ static int bdrv_check_byte_request(BlockDriverState *bs, int64_t offset,
> return 0;
> }
>
> +static int bdrv_run_co(BlockDriverState *bs, CoroutineEntry *entry,
> + void *opaque, int *ret)
> +{
> + if (qemu_in_coroutine()) {
> + /* Fast-path if already in coroutine context */
> + entry(opaque);
> + } else {
> + Coroutine *co = qemu_coroutine_create(entry, opaque);
> + *ret = NOT_DONE;
> + bdrv_coroutine_enter(bs, co);
> + BDRV_POLL_WHILE(bs, *ret == NOT_DONE);
For my reference, NOT_DONE is defined as INT_MAX, which does not seem to
be used as a return value in other situations.
> @@ -923,25 +939,15 @@ static int bdrv_prwv_co(BdrvChild *child, int64_t offset,
> QEMUIOVector *qiov, bool is_write,
> BdrvRequestFlags flags)
> {
> - Coroutine *co;
> RwCo rwco = {
> .child = child,
> .offset = offset,
> .qiov = qiov,
> .is_write = is_write,
> - .ret = NOT_DONE,
> .flags = flags,
> };
>
> - if (qemu_in_coroutine()) {
> - /* Fast-path if already in coroutine context */
> - bdrv_rw_co_entry(&rwco);
> - } else {
> - co = qemu_coroutine_create(bdrv_rw_co_entry, &rwco);
> - bdrv_coroutine_enter(child->bs, co);
> - BDRV_POLL_WHILE(child->bs, rwco.ret == NOT_DONE);
> - }
> - return rwco.ret;
> + return bdrv_run_co(child->bs, bdrv_rw_co_entry, &rwco, &rwco.ret);
So code that previously looped on NOT_DONE is obviously safe, while...
> }
>
> int bdrv_pwrite_zeroes(BdrvChild *child, int64_t offset,
> @@ -2230,7 +2236,6 @@ typedef struct BdrvCoBlockStatusData {
> int64_t *map;
> BlockDriverState **file;
> int ret;
> - bool done;
> } BdrvCoBlockStatusData;
>
> int coroutine_fn bdrv_co_block_status_from_file(BlockDriverState *bs,
> @@ -2492,7 +2497,6 @@ static void coroutine_fn bdrv_block_status_above_co_entry(void *opaque)
> data->want_zero,
> data->offset, data->bytes,
> data->pnum, data->map, data->file);
> - data->done = true;
> aio_wait_kick();
...code that looped on something else now has to be checked that
data->ret is still being set to something useful. Fortunately that is
true here.
> @@ -2669,22 +2663,13 @@ static inline int
> bdrv_rw_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos,
> bool is_read)
> {
> - if (qemu_in_coroutine()) {
> - return bdrv_co_rw_vmstate(bs, qiov, pos, is_read);
> - } else {
> - BdrvVmstateCo data = {
> - .bs = bs,
> - .qiov = qiov,
> - .pos = pos,
> - .is_read = is_read,
> - .ret = -EINPROGRESS,
> - };
> - Coroutine *co = qemu_coroutine_create(bdrv_co_rw_vmstate_entry, &data);
> -
> - bdrv_coroutine_enter(bs, co);
> - BDRV_POLL_WHILE(bs, data.ret == -EINPROGRESS);
> - return data.ret;
It's a little harder to see whether -EINPROGRESS might ever be returned
by a driver, but again this looks safe.
Here, it's a little less obvious whether any driver might return
-EINPROGRESS, but it looks like if they did that,
Reviewed-by: Eric Blake <eblake@redhat.com>
Conflicts with Vladimir's patches which try to add more coroutine
wrappers (but those need a rebase anyway):
https://lists.gnu.org/archive/html/qemu-devel/2020-04/msg04559.html
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
On 5/12/20 5:37 PM, Eric Blake wrote:
> On 5/12/20 9:43 AM, Kevin Wolf wrote:
>> We have a few bdrv_*() functions that can either spawn a new coroutine
>> and wait for it with BDRV_POLL_WHILE() or use a fastpath if they are
>> alreeady running in a coroutine. All of them duplicate basically the
>
> already
>
>> same code.
>>
>> Factor the common code into a new function bdrv_run_co().
>>
>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>> ---
>> block/io.c | 104 +++++++++++++++--------------------------------------
>> 1 file changed, 28 insertions(+), 76 deletions(-)
>>
>> diff --git a/block/io.c b/block/io.c
>> index 7d30e61edc..c1badaadc9 100644
>> --- a/block/io.c
>> +++ b/block/io.c
>> @@ -891,6 +891,22 @@ static int
>> bdrv_check_byte_request(BlockDriverState *bs, int64_t offset,
>> return 0;
>> }
>> +static int bdrv_run_co(BlockDriverState *bs, CoroutineEntry *entry,
>> + void *opaque, int *ret)
>> +{
>> + if (qemu_in_coroutine()) {
>> + /* Fast-path if already in coroutine context */
>> + entry(opaque);
>> + } else {
>> + Coroutine *co = qemu_coroutine_create(entry, opaque);
>> + *ret = NOT_DONE;
>> + bdrv_coroutine_enter(bs, co);
>> + BDRV_POLL_WHILE(bs, *ret == NOT_DONE);
>
> For my reference, NOT_DONE is defined as INT_MAX, which does not seem to
> be used as a return value in other situations.
>
>> @@ -923,25 +939,15 @@ static int bdrv_prwv_co(BdrvChild *child,
>> int64_t offset,
>> QEMUIOVector *qiov, bool is_write,
>> BdrvRequestFlags flags)
>> {
>> - Coroutine *co;
>> RwCo rwco = {
>> .child = child,
>> .offset = offset,
>> .qiov = qiov,
>> .is_write = is_write,
>> - .ret = NOT_DONE,
>> .flags = flags,
>> };
>> - if (qemu_in_coroutine()) {
>> - /* Fast-path if already in coroutine context */
>> - bdrv_rw_co_entry(&rwco);
>> - } else {
>> - co = qemu_coroutine_create(bdrv_rw_co_entry, &rwco);
>> - bdrv_coroutine_enter(child->bs, co);
>> - BDRV_POLL_WHILE(child->bs, rwco.ret == NOT_DONE);
>> - }
>> - return rwco.ret;
>> + return bdrv_run_co(child->bs, bdrv_rw_co_entry, &rwco, &rwco.ret);
>
> So code that previously looped on NOT_DONE is obviously safe, while...
>
>> }
>> int bdrv_pwrite_zeroes(BdrvChild *child, int64_t offset,
>> @@ -2230,7 +2236,6 @@ typedef struct BdrvCoBlockStatusData {
>> int64_t *map;
>> BlockDriverState **file;
>> int ret;
>> - bool done;
>> } BdrvCoBlockStatusData;
>> int coroutine_fn bdrv_co_block_status_from_file(BlockDriverState *bs,
>> @@ -2492,7 +2497,6 @@ static void coroutine_fn
>> bdrv_block_status_above_co_entry(void *opaque)
>> data->want_zero,
>> data->offset, data->bytes,
>> data->pnum, data->map,
>> data->file);
>> - data->done = true;
>> aio_wait_kick();
>
> ...code that looped on something else now has to be checked that
> data->ret is still being set to something useful. Fortunately that is
> true here.
>
>> @@ -2669,22 +2663,13 @@ static inline int
>> bdrv_rw_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos,
>> bool is_read)
>> {
>> - if (qemu_in_coroutine()) {
>> - return bdrv_co_rw_vmstate(bs, qiov, pos, is_read);
>> - } else {
>> - BdrvVmstateCo data = {
>> - .bs = bs,
>> - .qiov = qiov,
>> - .pos = pos,
>> - .is_read = is_read,
>> - .ret = -EINPROGRESS,
>> - };
>> - Coroutine *co =
>> qemu_coroutine_create(bdrv_co_rw_vmstate_entry, &data);
>> -
>> - bdrv_coroutine_enter(bs, co);
>> - BDRV_POLL_WHILE(bs, data.ret == -EINPROGRESS);
>> - return data.ret;
>
> It's a little harder to see whether -EINPROGRESS might ever be returned
> by a driver, but again this looks safe.
Maybe add a comment regarding -EINPROGRESS before calling bdrv_run_co()
in bdrv_rw_vmstate()?
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>
> Here, it's a little less obvious whether any driver might return
> -EINPROGRESS, but it looks like if they did that,
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
>
> Conflicts with Vladimir's patches which try to add more coroutine
> wrappers (but those need a rebase anyway):
> https://lists.gnu.org/archive/html/qemu-devel/2020-04/msg04559.html
>
20.05.2020 12:09, Philippe Mathieu-Daudé wrote:
> On 5/12/20 5:37 PM, Eric Blake wrote:
>> On 5/12/20 9:43 AM, Kevin Wolf wrote:
>>> We have a few bdrv_*() functions that can either spawn a new coroutine
>>> and wait for it with BDRV_POLL_WHILE() or use a fastpath if they are
>>> alreeady running in a coroutine. All of them duplicate basically the
>>
>> already
>>
>>> same code.
>>>
>>> Factor the common code into a new function bdrv_run_co().
>>>
>>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>>> ---
>>> block/io.c | 104 +++++++++++++++--------------------------------------
>>> 1 file changed, 28 insertions(+), 76 deletions(-)
>>>
>>> diff --git a/block/io.c b/block/io.c
>>> index 7d30e61edc..c1badaadc9 100644
>>> --- a/block/io.c
>>> +++ b/block/io.c
>>> @@ -891,6 +891,22 @@ static int bdrv_check_byte_request(BlockDriverState *bs, int64_t offset,
>>> return 0;
>>> }
>>> +static int bdrv_run_co(BlockDriverState *bs, CoroutineEntry *entry,
>>> + void *opaque, int *ret)
>>> +{
>>> + if (qemu_in_coroutine()) {
>>> + /* Fast-path if already in coroutine context */
>>> + entry(opaque);
>>> + } else {
>>> + Coroutine *co = qemu_coroutine_create(entry, opaque);
>>> + *ret = NOT_DONE;
>>> + bdrv_coroutine_enter(bs, co);
>>> + BDRV_POLL_WHILE(bs, *ret == NOT_DONE);
>>
>> For my reference, NOT_DONE is defined as INT_MAX, which does not seem to be used as a return value in other situations.
>>
>>> @@ -923,25 +939,15 @@ static int bdrv_prwv_co(BdrvChild *child, int64_t offset,
>>> QEMUIOVector *qiov, bool is_write,
>>> BdrvRequestFlags flags)
>>> {
>>> - Coroutine *co;
>>> RwCo rwco = {
>>> .child = child,
>>> .offset = offset,
>>> .qiov = qiov,
>>> .is_write = is_write,
>>> - .ret = NOT_DONE,
>>> .flags = flags,
>>> };
>>> - if (qemu_in_coroutine()) {
>>> - /* Fast-path if already in coroutine context */
>>> - bdrv_rw_co_entry(&rwco);
>>> - } else {
>>> - co = qemu_coroutine_create(bdrv_rw_co_entry, &rwco);
>>> - bdrv_coroutine_enter(child->bs, co);
>>> - BDRV_POLL_WHILE(child->bs, rwco.ret == NOT_DONE);
>>> - }
>>> - return rwco.ret;
>>> + return bdrv_run_co(child->bs, bdrv_rw_co_entry, &rwco, &rwco.ret);
>>
>> So code that previously looped on NOT_DONE is obviously safe, while...
>>
>>> }
>>> int bdrv_pwrite_zeroes(BdrvChild *child, int64_t offset,
>>> @@ -2230,7 +2236,6 @@ typedef struct BdrvCoBlockStatusData {
>>> int64_t *map;
>>> BlockDriverState **file;
>>> int ret;
>>> - bool done;
>>> } BdrvCoBlockStatusData;
>>> int coroutine_fn bdrv_co_block_status_from_file(BlockDriverState *bs,
>>> @@ -2492,7 +2497,6 @@ static void coroutine_fn bdrv_block_status_above_co_entry(void *opaque)
>>> data->want_zero,
>>> data->offset, data->bytes,
>>> data->pnum, data->map, data->file);
>>> - data->done = true;
>>> aio_wait_kick();
>>
>> ...code that looped on something else now has to be checked that data->ret is still being set to something useful. Fortunately that is true here.
>>
>>> @@ -2669,22 +2663,13 @@ static inline int
>>> bdrv_rw_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos,
>>> bool is_read)
>>> {
>>> - if (qemu_in_coroutine()) {
>>> - return bdrv_co_rw_vmstate(bs, qiov, pos, is_read);
>>> - } else {
>>> - BdrvVmstateCo data = {
>>> - .bs = bs,
>>> - .qiov = qiov,
>>> - .pos = pos,
>>> - .is_read = is_read,
>>> - .ret = -EINPROGRESS,
>>> - };
>>> - Coroutine *co = qemu_coroutine_create(bdrv_co_rw_vmstate_entry, &data);
>>> -
>>> - bdrv_coroutine_enter(bs, co);
>>> - BDRV_POLL_WHILE(bs, data.ret == -EINPROGRESS);
>>> - return data.ret;
>>
>> It's a little harder to see whether -EINPROGRESS might ever be returned by a driver, but again this looks safe.
>
> Maybe add a comment regarding -EINPROGRESS before calling bdrv_run_co() in bdrv_rw_vmstate()?
>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>
Hi!
Actually, I've sent a v2 of this patch:
"[PATCH v2] block: Factor out bdrv_run_co()"
https://lists.gnu.org/archive/html/qemu-devel/2020-05/msg05437.html
--
Best regards,
Vladimir
© 2016 - 2026 Red Hat, Inc.