[v1] block: fix coroutine_fn annotations

[PATCH 02/26] qcow2: remove incorrect coroutine_fn annotations

Posted by Paolo Bonzini 3 years, 10 months ago

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/qcow2-refcount.c | 4 ++--
 block/qcow2.h          | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index b91499410c..b6f90b2702 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -1206,7 +1206,7 @@ void qcow2_free_any_cluster(BlockDriverState *bs, uint64_t l2_entry,
     }
 }
 
-int coroutine_fn qcow2_write_caches(BlockDriverState *bs)
+int qcow2_write_caches(BlockDriverState *bs)
 {
     BDRVQcow2State *s = bs->opaque;
     int ret;
@@ -1226,7 +1226,7 @@ int coroutine_fn qcow2_write_caches(BlockDriverState *bs)
     return 0;
 }
 
-int coroutine_fn qcow2_flush_caches(BlockDriverState *bs)
+int qcow2_flush_caches(BlockDriverState *bs)
 {
     int ret = qcow2_write_caches(bs);
     if (ret < 0) {
diff --git a/block/qcow2.h b/block/qcow2.h
index ba436a8d0d..c8d9e8ea79 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -874,8 +874,8 @@ void qcow2_free_any_cluster(BlockDriverState *bs, uint64_t l2_entry,
 int qcow2_update_snapshot_refcount(BlockDriverState *bs,
     int64_t l1_table_offset, int l1_size, int addend);
 
-int coroutine_fn qcow2_flush_caches(BlockDriverState *bs);
-int coroutine_fn qcow2_write_caches(BlockDriverState *bs);
+int qcow2_flush_caches(BlockDriverState *bs);
+int qcow2_write_caches(BlockDriverState *bs);
 int qcow2_check_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
                           BdrvCheckMode fix);
 
-- 
2.35.1

Re: [PATCH 02/26] qcow2: remove incorrect coroutine_fn annotations

Posted by Eric Blake 3 years, 9 months ago

On Fri, Apr 15, 2022 at 03:18:36PM +0200, Paolo Bonzini wrote:
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---

Again, a sentence on why this is correct would be helpful.

>  block/qcow2-refcount.c | 4 ++--
>  block/qcow2.h          | 4 ++--
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
> index b91499410c..b6f90b2702 100644
> --- a/block/qcow2-refcount.c
> +++ b/block/qcow2-refcount.c
> @@ -1206,7 +1206,7 @@ void qcow2_free_any_cluster(BlockDriverState *bs, uint64_t l2_entry,
>      }
>  }
>  
> -int coroutine_fn qcow2_write_caches(BlockDriverState *bs)
> +int qcow2_write_caches(BlockDriverState *bs)
>  {
>      BDRVQcow2State *s = bs->opaque;
>      int ret;
> @@ -1226,7 +1226,7 @@ int coroutine_fn qcow2_write_caches(BlockDriverState *bs)
>      return 0;
>  }
>  
> -int coroutine_fn qcow2_flush_caches(BlockDriverState *bs)
> +int qcow2_flush_caches(BlockDriverState *bs)
>  {
>      int ret = qcow2_write_caches(bs);

Both of these eventually hit qcow2_cache_write, which is not marked
coroutine, so these should not be either.

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 02/26] qcow2: remove incorrect coroutine_fn annotations

Posted by Stefan Hajnoczi 3 years, 9 months ago

On Tue, Apr 19, 2022 at 01:07:19PM -0500, Eric Blake wrote:
> On Fri, Apr 15, 2022 at 03:18:36PM +0200, Paolo Bonzini wrote:
> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > ---
> 
> Again, a sentence on why this is correct would be helpful.
> 
> >  block/qcow2-refcount.c | 4 ++--
> >  block/qcow2.h          | 4 ++--
> >  2 files changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
> > index b91499410c..b6f90b2702 100644
> > --- a/block/qcow2-refcount.c
> > +++ b/block/qcow2-refcount.c
> > @@ -1206,7 +1206,7 @@ void qcow2_free_any_cluster(BlockDriverState *bs, uint64_t l2_entry,
> >      }
> >  }
> >  
> > -int coroutine_fn qcow2_write_caches(BlockDriverState *bs)
> > +int qcow2_write_caches(BlockDriverState *bs)
> >  {
> >      BDRVQcow2State *s = bs->opaque;
> >      int ret;
> > @@ -1226,7 +1226,7 @@ int coroutine_fn qcow2_write_caches(BlockDriverState *bs)
> >      return 0;
> >  }
> >  
> > -int coroutine_fn qcow2_flush_caches(BlockDriverState *bs)
> > +int qcow2_flush_caches(BlockDriverState *bs)
> >  {
> >      int ret = qcow2_write_caches(bs);
> 
> Both of these eventually hit qcow2_cache_write, which is not marked
> coroutine, so these should not be either.

coroutine_fn may call non-coroutine_fn, so this alone is not a reason
for removing it from qcow2_write_caches().

There must be a call chain where qcow2_write_caches() and
qcow2_flush_caches() are is invoked from outside coroutine_fn.

Stefan

Re: [PATCH 02/26] qcow2: remove incorrect coroutine_fn annotations

Posted by Paolo Bonzini 3 years, 9 months ago

On 4/21/22 12:24, Stefan Hajnoczi wrote:
>>> -int coroutine_fn qcow2_flush_caches(BlockDriverState *bs)
>>> +int qcow2_flush_caches(BlockDriverState *bs)
>>>   {
>>>       int ret = qcow2_write_caches(bs);
>>
>> Both of these eventually hit qcow2_cache_write, which is not marked
>> coroutine, so these should not be either.
> 
> coroutine_fn may call non-coroutine_fn, so this alone is not a reason
> for removing it from qcow2_write_caches().
> 
> There must be a call chain where qcow2_write_caches() and
> qcow2_flush_caches() are is invoked from outside coroutine_fn.

The main problematic caller is qcow2_inactivate(), which calls these 
functions via qcow2_mark_clean().  Another one is 
update_ext_header_and_dir(), called by 
qcow2_store_persistent_dirty_bitmaps(), called by qcow2_inactivate().

Converting inactivate to run in coroutine context would help.

Paolo

Re: [PATCH 02/26] qcow2: remove incorrect coroutine_fn annotations

Posted by Paolo Bonzini 3 years, 9 months ago

On 4/27/22 14:36, Paolo Bonzini wrote:
> On 4/21/22 12:24, Stefan Hajnoczi wrote:
>>>> -int coroutine_fn qcow2_flush_caches(BlockDriverState *bs)
>>>> +int qcow2_flush_caches(BlockDriverState *bs)
>>>>   {
>>>>       int ret = qcow2_write_caches(bs);
>>>
>>> Both of these eventually hit qcow2_cache_write, which is not marked
>>> coroutine, so these should not be either.
>>
>> coroutine_fn may call non-coroutine_fn, so this alone is not a reason
>> for removing it from qcow2_write_caches().
>>
>> There must be a call chain where qcow2_write_caches() and
>> qcow2_flush_caches() are is invoked from outside coroutine_fn.
> 
> The main problematic caller is qcow2_inactivate(), which calls these 
> functions via qcow2_mark_clean().  Another one is 
> update_ext_header_and_dir(), called by 
> qcow2_store_persistent_dirty_bitmaps(), called by qcow2_inactivate().
> 
> Converting inactivate to run in coroutine context would help.

Or maybe not so much because bdrv_close also calls the same paths.

Paolo