| Message ID | 20220415131900.793161-3-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | block: fix coroutine_fn annotations | expand |
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>
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
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
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
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);
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- block/qcow2-refcount.c | 4 ++-- block/qcow2.h | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-)