[Qemu-devel] [PATCH] block: fix bdrv_ioctl called from coroutine

[Qemu-devel] [PATCH] block: fix bdrv_ioctl called from coroutine

[Paolo Bonzini]

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

Re: [Qemu-devel] [PATCH] block: fix bdrv_ioctl called from coroutine

[Fam Zheng]

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
> 

Re: [Qemu-devel] [PATCH] block: fix bdrv_ioctl called from coroutine

[Paolo Bonzini]

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
>>
> 
> 

Re: [Qemu-devel] [PATCH] block: fix bdrv_ioctl called from coroutine

[Fam Zheng]

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

Re: [Qemu-devel] [PATCH] block: fix bdrv_ioctl called from coroutine

[Paolo Bonzini]

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

Re: [Qemu-devel] [PATCH] block: fix bdrv_ioctl called from coroutine

[Fam Zheng]

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>

Re: [Qemu-devel] [PATCH] block: fix bdrv_ioctl called from coroutine

[Kevin Wolf]

[ 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