On 02/14/2018 08:06 AM, Stefan Hajnoczi wrote:
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
Thanks, that helps.
+ 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.
Okay, you've solved that one.
+++ 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.
And looks like we're safe, there, as well.
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?
Maybe, but that's a separate patch. What name would we bikeshed, maybe
aio_context_correct_thread() (we are the correct thread if we are the
iothread that owns ctx, or if we are the main thread and have properly
acquired ctx) or aio_context_use_okay() (we can only use the ctx if we
own it [native iothread] or have acquired it [main loop])
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.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
