* [Qemu-devel] [PATCH] block: fix bdrv_ioctl called from coroutine
@ 2015-12-16 18:33 Paolo Bonzini
2015-12-17 0:59 ` Fam Zheng
2015-12-18 12:16 ` Kevin Wolf
0 siblings, 2 replies; 7+ messages in thread
From: Paolo Bonzini @ 2015-12-16 18:33 UTC (permalink / raw)
To: qemu-devel; +Cc: famz
When called from a coroutine, bdrv_ioctl must be asynchronous just like
e.g. bdrv_flush. The code was incorrectly making it synchronous, fix
it.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
Fam, any reason why you did it this way? I don't see
any coroutine caller, but it doesn't make much sense. :)
block/io.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/block/io.c b/block/io.c
index e00fb5d..841f5b5 100644
--- a/block/io.c
+++ b/block/io.c
@@ -2614,10 +2614,11 @@ int bdrv_ioctl(BlockDriverState *bs, unsigned long int req, void *buf)
bdrv_co_ioctl_entry(&data);
} else {
Coroutine *co = qemu_coroutine_create(bdrv_co_ioctl_entry);
+
qemu_coroutine_enter(co, &data);
- }
- while (data.ret == -EINPROGRESS) {
- aio_poll(bdrv_get_aio_context(bs), true);
+ while (data.ret == -EINPROGRESS) {
+ aio_poll(bdrv_get_aio_context(bs), true);
+ }
}
return data.ret;
}
--
2.5.0
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] block: fix bdrv_ioctl called from coroutine
2015-12-16 18:33 [Qemu-devel] [PATCH] block: fix bdrv_ioctl called from coroutine Paolo Bonzini
@ 2015-12-17 0:59 ` Fam Zheng
2015-12-17 8:44 ` Paolo Bonzini
2015-12-18 12:16 ` Kevin Wolf
1 sibling, 1 reply; 7+ messages in thread
From: Fam Zheng @ 2015-12-17 0:59 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel
On Wed, 12/16 19:33, Paolo Bonzini wrote:
> When called from a coroutine, bdrv_ioctl must be asynchronous just like
> e.g. bdrv_flush. The code was incorrectly making it synchronous, fix
> it.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> Fam, any reason why you did it this way? I don't see
> any coroutine caller, but it doesn't make much sense. :)
That is a surprising question! From a coroutine, it is bdrv_flush ->
bdrv_flush_co_entry -> bdrv_co_flush, which I think is always synchronous,
especially, noticing the code around calling bs->bdrv_aio_flush:
acb = bs->drv->bdrv_aio_flush(bs, bdrv_co_io_em_complete, &co);
if (acb == NULL) {
ret = -EIO;
} else {
qemu_coroutine_yield();
ret = co.ret;
}
Am I missing something?
Fam
>
> block/io.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/block/io.c b/block/io.c
> index e00fb5d..841f5b5 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -2614,10 +2614,11 @@ int bdrv_ioctl(BlockDriverState *bs, unsigned long int req, void *buf)
> bdrv_co_ioctl_entry(&data);
> } else {
> Coroutine *co = qemu_coroutine_create(bdrv_co_ioctl_entry);
> +
> qemu_coroutine_enter(co, &data);
> - }
> - while (data.ret == -EINPROGRESS) {
> - aio_poll(bdrv_get_aio_context(bs), true);
> + while (data.ret == -EINPROGRESS) {
> + aio_poll(bdrv_get_aio_context(bs), true);
> + }
> }
> return data.ret;
> }
> --
> 2.5.0
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] block: fix bdrv_ioctl called from coroutine
2015-12-17 0:59 ` Fam Zheng
@ 2015-12-17 8:44 ` Paolo Bonzini
2015-12-17 12:33 ` Fam Zheng
0 siblings, 1 reply; 7+ messages in thread
From: Paolo Bonzini @ 2015-12-17 8:44 UTC (permalink / raw)
To: Fam Zheng; +Cc: qemu-devel
On 17/12/2015 01:59, Fam Zheng wrote:
> On Wed, 12/16 19:33, Paolo Bonzini wrote:
>> When called from a coroutine, bdrv_ioctl must be asynchronous just like
>> e.g. bdrv_flush. The code was incorrectly making it synchronous, fix
>> it.
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>> Fam, any reason why you did it this way? I don't see
>> any coroutine caller, but it doesn't make much sense. :)
>
> That is a surprising question! From a coroutine, it is bdrv_flush ->
> bdrv_flush_co_entry -> bdrv_co_flush, which I think is always synchronous,
> especially, noticing the code around calling bs->bdrv_aio_flush:
>
> acb = bs->drv->bdrv_aio_flush(bs, bdrv_co_io_em_complete, &co);
> if (acb == NULL) {
> ret = -EIO;
> } else {
> qemu_coroutine_yield();
> ret = co.ret;
> }
>
> Am I missing something?
In the coroutine case, the yield is hidden in the drivers, and it may or
may not happen. For example, qcow2_co_flush_to_os starts with
qemu_co_mutex_lock(&s->lock);
which can yield.
Paolo
> Fam
>
>>
>> block/io.c | 7 ++++---
>> 1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/block/io.c b/block/io.c
>> index e00fb5d..841f5b5 100644
>> --- a/block/io.c
>> +++ b/block/io.c
>> @@ -2614,10 +2614,11 @@ int bdrv_ioctl(BlockDriverState *bs, unsigned long int req, void *buf)
>> bdrv_co_ioctl_entry(&data);
>> } else {
>> Coroutine *co = qemu_coroutine_create(bdrv_co_ioctl_entry);
>> +
>> qemu_coroutine_enter(co, &data);
>> - }
>> - while (data.ret == -EINPROGRESS) {
>> - aio_poll(bdrv_get_aio_context(bs), true);
>> + while (data.ret == -EINPROGRESS) {
>> + aio_poll(bdrv_get_aio_context(bs), true);
>> + }
>> }
>> return data.ret;
>> }
>> --
>> 2.5.0
>>
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] block: fix bdrv_ioctl called from coroutine
2015-12-17 8:44 ` Paolo Bonzini
@ 2015-12-17 12:33 ` Fam Zheng
2015-12-17 12:51 ` Paolo Bonzini
0 siblings, 1 reply; 7+ messages in thread
From: Fam Zheng @ 2015-12-17 12:33 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel
On Thu, 12/17 09:44, Paolo Bonzini wrote:
>
>
> On 17/12/2015 01:59, Fam Zheng wrote:
> > On Wed, 12/16 19:33, Paolo Bonzini wrote:
> >> When called from a coroutine, bdrv_ioctl must be asynchronous just like
> >> e.g. bdrv_flush. The code was incorrectly making it synchronous, fix
> >> it.
> >>
> >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> >> ---
> >> Fam, any reason why you did it this way? I don't see
> >> any coroutine caller, but it doesn't make much sense. :)
> >
> > That is a surprising question! From a coroutine, it is bdrv_flush ->
> > bdrv_flush_co_entry -> bdrv_co_flush, which I think is always synchronous,
> > especially, noticing the code around calling bs->bdrv_aio_flush:
> >
> > acb = bs->drv->bdrv_aio_flush(bs, bdrv_co_io_em_complete, &co);
> > if (acb == NULL) {
> > ret = -EIO;
> > } else {
> > qemu_coroutine_yield();
> > ret = co.ret;
> > }
> >
> > Am I missing something?
>
> In the coroutine case, the yield is hidden in the drivers, and it may or
> may not happen. For example, qcow2_co_flush_to_os starts with
>
> qemu_co_mutex_lock(&s->lock);
>
> which can yield.
bdrv_ioctl, on the contrary, is emulated with .bdrv_aio_ioctl, so it always
yields (unless -ENOTSUP), that's why I think aio_poll() is necessary in both
branches.
Fam
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] block: fix bdrv_ioctl called from coroutine
2015-12-17 12:33 ` Fam Zheng
@ 2015-12-17 12:51 ` Paolo Bonzini
2015-12-18 1:17 ` Fam Zheng
0 siblings, 1 reply; 7+ messages in thread
From: Paolo Bonzini @ 2015-12-17 12:51 UTC (permalink / raw)
To: Fam Zheng; +Cc: qemu-devel
On 17/12/2015 13:33, Fam Zheng wrote:
> > In the coroutine case, the yield is hidden in the drivers, and it may or
> > may not happen. For example, qcow2_co_flush_to_os starts with
> >
> > qemu_co_mutex_lock(&s->lock);
> >
> > which can yield.
>
> bdrv_ioctl, on the contrary, is emulated with .bdrv_aio_ioctl, so it always
> yields (unless -ENOTSUP), that's why I think aio_poll() is necessary in both
> branches.
But why should it yield, if called in coroutine context? bdrv_flush
doesn't yield if emulated with .bdrv_aio_flush *and* called in coroutine
context. bdrv_ioctl should do the same, I think.
Paolo
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] block: fix bdrv_ioctl called from coroutine
2015-12-17 12:51 ` Paolo Bonzini
@ 2015-12-18 1:17 ` Fam Zheng
0 siblings, 0 replies; 7+ messages in thread
From: Fam Zheng @ 2015-12-18 1:17 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel
On Thu, 12/17 13:51, Paolo Bonzini wrote:
>
>
> On 17/12/2015 13:33, Fam Zheng wrote:
> > > In the coroutine case, the yield is hidden in the drivers, and it may or
> > > may not happen. For example, qcow2_co_flush_to_os starts with
> > >
> > > qemu_co_mutex_lock(&s->lock);
> > >
> > > which can yield.
> >
> > bdrv_ioctl, on the contrary, is emulated with .bdrv_aio_ioctl, so it always
> > yields (unless -ENOTSUP), that's why I think aio_poll() is necessary in both
> > branches.
>
> But why should it yield, if called in coroutine context? bdrv_flush
> doesn't yield if emulated with .bdrv_aio_flush *and* called in coroutine
> context. bdrv_ioctl should do the same, I think.
>
Looking at bdrv_flush again I see what you mean. Thanks for explaining!
Reviewed-by: Fam Zheng <famz@redhat.com>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] block: fix bdrv_ioctl called from coroutine
2015-12-16 18:33 [Qemu-devel] [PATCH] block: fix bdrv_ioctl called from coroutine Paolo Bonzini
2015-12-17 0:59 ` Fam Zheng
@ 2015-12-18 12:16 ` Kevin Wolf
1 sibling, 0 replies; 7+ messages in thread
From: Kevin Wolf @ 2015-12-18 12:16 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: famz, qemu-devel, qemu-block
[ Cc: qemu-block ]
Am 16.12.2015 um 19:33 hat Paolo Bonzini geschrieben:
> When called from a coroutine, bdrv_ioctl must be asynchronous just like
> e.g. bdrv_flush. The code was incorrectly making it synchronous, fix
> it.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Thanks, applied to the block branch.
Kevin
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2015-12-18 12:16 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-12-16 18:33 [Qemu-devel] [PATCH] block: fix bdrv_ioctl called from coroutine Paolo Bonzini
2015-12-17 0:59 ` Fam Zheng
2015-12-17 8:44 ` Paolo Bonzini
2015-12-17 12:33 ` Fam Zheng
2015-12-17 12:51 ` Paolo Bonzini
2015-12-18 1:17 ` Fam Zheng
2015-12-18 12:16 ` Kevin Wolf
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.