qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v2 1/4] block: extract AIO_WAIT_WHILE() from Blo


From: Eric Blake
Subject: Re: [Qemu-devel] [PATCH v2 1/4] block: extract AIO_WAIT_WHILE() from BlockDriverState
Date: Tue, 13 Feb 2018 10:01:06 -0600
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0

On 02/13/2018 08:20 AM, Stefan Hajnoczi wrote:
BlockDriverState has the BDRV_POLL_WHILE() macro to wait on event loop
activity while a condition evaluates to true.  This is used to implement
synchronous operations where it acts as a condvar between the IOThread
running the operation and the main loop waiting for the operation.  It
can also be called from the thread that owns the AioContext and in that
case it's just a nested event loop.

BlockBackend needs this behavior but doesn't always have a
BlockDriverState it can use.  This patch extracts BDRV_POLL_WHILE() into
the AioWait abstraction, which can be used with AioContext and isn't
tied to BlockDriverState anymore.

This feature could be built directly into AioContext but then all users
would kick the event loop even if they signal different conditions.
Imagine an AioContext with many BlockDriverStates, each time a request
completes any waiter would wake up and re-check their condition.  It's
nicer to keep a separate AioWait object for each condition instead.

Please see "block/aio-wait.h" for details on the API.

The name AIO_WAIT_WHILE() avoids the confusion between AIO_POLL_WHILE()
and AioContext polling.

Signed-off-by: Stefan Hajnoczi <address@hidden>
---

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,

+    } 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.

+++ 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.

+++ b/block.c
@@ -4716,6 +4716,11 @@ AioContext *bdrv_get_aio_context(BlockDriverState *bs)
      return bs->aio_context;
  }
+AioWait *bdrv_get_aio_wait(BlockDriverState *bs)
+{
+    return bs ? &bs->wait : NULL;
+}

So, do we need documentation to that fact?  Also,

+++ 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...

+++ b/util/aio-wait.c

+void aio_wait_kick(AioWait *wait)
+{
+    /* The barrier (or an atomic op) is in the caller.  */
+    if (atomic_read(&wait->need_kick)) {

...is bad. Does that mean bdrv_wakeup() can only be called when bs is non-NULL? Does that need documentation?

It may be that your patch is correct (as I'm not an expert on the rules in play here), but more comments may help. Or you may have a NULL dereference bug lurking. So at this point, I can't give R-b, even though the refactoring of the BDRV_POLL_WHILE() macro into a separate helper makes sense from the high level view.

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



reply via email to

[Prev in Thread] Current Thread [Next in Thread]