On Tue, 04/05 10:39, Stefan Hajnoczi wrote: > > > block/mirror.c should call bdrv_co_drain() explicitly and bdrv_drain() > > > should assert(!qemu_in_coroutine()). > > > > > > The reason why existing bdrv_read() and friends detect coroutine context > > > at runtime is because it is meant for legacy code that runs in both > > > coroutine and non-coroutine contexts. > > > > blk_unref/bdrv_unref are called in coroutine (.bdrv_create implementations), > > and this doesn't just work with the assertion. Should I clean up this > > "legacy" > > code first, i.e. move bdrv_unref calls to BHs in the callers and > > assert(!qemu_in_coroutine()) there too? I didn't think this because it > > complicates the code somehow. > > This is a messy problem. > > In general I don't like introducing yields into non-coroutine_fn > functions because it can lead to bugs when the caller didn't expect a > yield point. > > For example, I myself wouldn't have expected bdrv_unref() to be a yield > point. So maybe coroutine code I've written would be vulnerable to > interference (I won't call it a race condition) from another coroutine > across the bdrv_unref() call. This could mean that another coroutine > now sees intermediate state that would never be visible without the new > yield point. > > I think attempting to invoke qemu_co_queue_run_restart() directly > instead of scheduling a BH and yielding does not improve the situation. > It's also a layering violation since qemu_co_queue_run_restart() is just > meant for the core coroutine code and isn't a public interface. > > Anyway, let's consider bdrv_drain() legacy code that can call if > (qemu_in_coroutine()) but please make bdrv_co_drain() public so > block/mirror.c can at least call it directly.
OK, will do. Fam