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

Reply via email to