On Tue, Feb 13, 2018 at 10:01:06AM -0600, Eric Blake wrote: > Trying to understand here: > > > > +#define AIO_WAIT_WHILE(wait, ctx, cond) ({ \ > > + bool waited_ = false; \ > > + bool busy_ = true; \ > > + AioWait *wait_ = (wait); \ > > + AioContext *ctx_ = (ctx); \ > > + if (aio_context_in_iothread(ctx_)) { \ > > + while ((cond) || busy_) { \ > > + busy_ = aio_poll(ctx_, (cond)); \ > > + waited_ |= !!(cond) | busy_; \ > > + } \ > > If we are in an iothread already, we never dereference wait,
No, the name and documentation for aio_context_in_iothread() is misleading. It actually means "does this AioContext belong to the current thread?", which is more general than just the IOThread case. aio_context_in_iothread() returns true when: 1. We are the IOThread that owns ctx. <-- the case you thought of 2. We are the main loop and ctx == qemu_get_aio_context(). ^--- the sneaky case that BDRV_POLL_WHILE() has always relied on > > + } else { \ > > + assert(qemu_get_current_aio_context() == \ > > + qemu_get_aio_context()); \ > > + assert(!wait_->need_kick); \ > > but if we are in the main loop, wait must be non-NULL. The else statement only handles the case where the main loop is waiting for an IOThread AioContext. s/in the main loop/in the main loop and ctx is an IOThread AioContext/ > > > +++ b/include/block/block.h > > @@ -2,6 +2,7 @@ > > #define BLOCK_H > > #include "block/aio.h" > > +#include "block/aio-wait.h" > > #include "qapi-types.h" > > #include "qemu/iov.h" > > #include "qemu/coroutine.h" > > @@ -367,41 +368,14 @@ void bdrv_drain_all_begin(void); > > void bdrv_drain_all_end(void); > > void bdrv_drain_all(void); > > +/* Returns NULL when bs == NULL */ > > +AioWait *bdrv_get_aio_wait(BlockDriverState *bs); > > This can return NULL, so it is only ever safe to use in an iothread; because > if it is used in the main loop,... > > > + > > #define BDRV_POLL_WHILE(bs, cond) ({ \ > > > + AIO_WAIT_WHILE(bdrv_get_aio_wait(bs_), \ > > + bdrv_get_aio_context(bs_), \ > > + cond); }) > > ...we can pass NULL as the wait parameter, which will crash. It won't crash since if (aio_context_in_iothread(ctx_)) will take the true case when bs_ == NULL. > > +++ b/block/io.c > > > void bdrv_wakeup(BlockDriverState *bs) > > { > > - /* The barrier (or an atomic op) is in the caller. */ > > - if (atomic_read(&bs->wakeup)) { > > - aio_bh_schedule_oneshot(qemu_get_aio_context(), dummy_bh_cb, NULL); > > - } > > + aio_wait_kick(bdrv_get_aio_wait(bs)); > > this is another case where passing NULL... bdrv_wakeup() is only called when bs != NULL. I hope this explains things! The main issue that raised these questions was that aio_context_in_iothread() has a misleading name. Shall we rename it? I'm having a hard time picking a new name because it must not be confused with AioContext acquire/release, which doesn't influence the "native" AioContext that the current thread has an affinity with.
signature.asc
Description: PGP signature