Re: [PATCH 02/26] qcow2: remove incorrect coroutine_fn annotations
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
Re: [PATCH 02/26] qcow2: remove incorrect coroutine_fn annotations
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
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 > > --- > > 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 signature.asc Description: PGP signature
Re: [PATCH 02/26] qcow2: remove incorrect coroutine_fn annotations
On Fri, Apr 15, 2022 at 03:18:36PM +0200, Paolo Bonzini wrote: > Signed-off-by: Paolo Bonzini > --- 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 -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
[PATCH 02/26] qcow2: remove incorrect coroutine_fn annotations
Signed-off-by: Paolo Bonzini --- 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