| Message ID | 20220922084924.201610-7-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | block: fix coroutine_fn annotations | expand |
Am 22.09.2022 um 10:49 hat Paolo Bonzini geschrieben: > Callers of coroutine_fn must be coroutine_fn themselves, or the call > must be within "if (qemu_in_coroutine())". Apply coroutine_fn to > functions where this holds. > > Reviewed-by: Alberto Faria <afaria@redhat.com> > Reviewed-by: Eric Blake <eblake@redhat.com> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> Hm... blkdebug_debug_event() is called from bdrv_debug_event(), which is not coroutine_fn. And I think that it's not coroutine_fn is correct: For example, with 'qemu-img snapshot -c' you get img_snapshot() -> bdrv_snapshot_create() -> qcow2_snapshot_create() -> qcow2_alloc_clusters() -> BLKDBG_EVENT. I'm sure many other calls in qcow2 can be reached from non-coroutine context as well. It almost looks like your code analysis didn't consider calls through function pointers? Kevin
Il mer 5 ott 2022, 06:32 Kevin Wolf <kwolf@redhat.com> ha scritto: > Hm... blkdebug_debug_event() is called from bdrv_debug_event(), which is > not coroutine_fn. And I think that it's not coroutine_fn is correct: > For example, with 'qemu-img snapshot -c' you get img_snapshot() -> > bdrv_snapshot_create() -> qcow2_snapshot_create() -> > qcow2_alloc_clusters() -> BLKDBG_EVENT. I'm sure many other calls in > qcow2 can be reached from non-coroutine context as well. > > It almost looks like your code analysis didn't consider calls through > function pointers? > No, that is not what happened. Rather it's that the analysis goes the other way round: because SUSPEND rules call qemu_coroutine_yield(), clang wants all the callers to be coroutine_fn too. It is technically incorrect that bdrv_debug_events sometimes are placed outside coroutine context, and QEMU would crash if those events were associated with a suspend rule. I guess I (or you) can just drop this patch? Paolo >
Am 05.10.2022 um 23:11 hat Paolo Bonzini geschrieben: > Il mer 5 ott 2022, 06:32 Kevin Wolf <kwolf@redhat.com> ha scritto: > > > Hm... blkdebug_debug_event() is called from bdrv_debug_event(), which is > > not coroutine_fn. And I think that it's not coroutine_fn is correct: > > For example, with 'qemu-img snapshot -c' you get img_snapshot() -> > > bdrv_snapshot_create() -> qcow2_snapshot_create() -> > > qcow2_alloc_clusters() -> BLKDBG_EVENT. I'm sure many other calls in > > qcow2 can be reached from non-coroutine context as well. > > > > It almost looks like your code analysis didn't consider calls through > > function pointers? > > > > No, that is not what happened. Rather it's that the analysis goes the > other way round: because SUSPEND rules call qemu_coroutine_yield(), > clang wants all the callers to be coroutine_fn too. Ah, ok, makes sense. So checking the callers is indeed something that requires manual review. > It is technically incorrect that bdrv_debug_events sometimes are > placed outside coroutine context, and QEMU would crash if those events > were associated with a suspend rule. Yes, looks like a bug. As long as it's only in blkdebug, we can probably live with it (the easiest fix would probably be generating coroutine wrappers for bdrv_debug_event(), too). > I guess I (or you) can just drop this patch? Yes, I can do that. Kevin
diff --git a/block/blkdebug.c b/block/blkdebug.c index bbf2948703..a93ba61487 100644 --- a/block/blkdebug.c +++ b/block/blkdebug.c @@ -587,8 +587,8 @@ out: return ret; } -static int rule_check(BlockDriverState *bs, uint64_t offset, uint64_t bytes, - BlkdebugIOType iotype) +static int coroutine_fn rule_check(BlockDriverState *bs, uint64_t offset, uint64_t bytes, + BlkdebugIOType iotype) { BDRVBlkdebugState *s = bs->opaque; BlkdebugRule *rule = NULL; @@ -672,7 +672,7 @@ blkdebug_co_pwritev(BlockDriverState *bs, int64_t offset, int64_t bytes, return bdrv_co_pwritev(bs->file, offset, bytes, qiov, flags); } -static int blkdebug_co_flush(BlockDriverState *bs) +static int coroutine_fn blkdebug_co_flush(BlockDriverState *bs) { int err = rule_check(bs, 0, 0, BLKDEBUG_IO_TYPE_FLUSH); @@ -791,7 +791,7 @@ static void blkdebug_close(BlockDriverState *bs) } /* Called with lock held. */ -static void suspend_request(BlockDriverState *bs, BlkdebugRule *rule) +static void coroutine_fn suspend_request(BlockDriverState *bs, BlkdebugRule *rule) { BDRVBlkdebugState *s = bs->opaque; BlkdebugSuspendedReq *r; @@ -810,8 +810,8 @@ static void suspend_request(BlockDriverState *bs, BlkdebugRule *rule) } /* Called with lock held. */ -static void process_rule(BlockDriverState *bs, struct BlkdebugRule *rule, - int *action_count, int *new_state) +static void coroutine_fn process_rule(BlockDriverState *bs, struct BlkdebugRule *rule, + int *action_count, int *new_state) { BDRVBlkdebugState *s = bs->opaque; @@ -840,7 +840,7 @@ static void process_rule(BlockDriverState *bs, struct BlkdebugRule *rule, } } -static void blkdebug_debug_event(BlockDriverState *bs, BlkdebugEvent event) +static void coroutine_fn blkdebug_debug_event(BlockDriverState *bs, BlkdebugEvent event) { BDRVBlkdebugState *s = bs->opaque; struct BlkdebugRule *rule, *next;