[Qemu-devel] [PATCH v2 00/13] block: bdrv_set_aio_context() related fixes

[Qemu-devel] [PATCH v2 00/13] block: bdrv_set_aio_context() related fixes

[Kevin Wolf]

Background for this series is the following bug report, which is about a
crash with virtio-blk + iothread and request resubmission for werror/rerror:

https://bugzilla.redhat.com/show_bug.cgi?id=1671173

The reason is that bdrv_set_aio_context() didn't correctly quiesce
everything. Instead, it had a local hack to call aio_poll() for the
source AioContext, which covered some, but not all cases, and is wrong
because you can only call aio_poll() from the home thread.

So this series tries to make bdrv_drain() actually drain the known cases
(fixes virtio-blk and the NBD client) and use the regular drain
functions in bdrv_set_aio_context() instead of open-coding something
similar.

v2:
- New patch 5: Remove now redundant assignments [Paolo]
- Patch 8 (was 7): Instead of using aio_co_schedule() to switch the
  AioContext and modifying the coroutine code, enter the coroutine in a
  BH in the new AioContext and bdrv_dec_in_flight() in that BH [Paolo]
- Patch 12 (was 11): Cover new == old case in comment [Eric]

Kevin Wolf (13):
  block-backend: Make blk_inc/dec_in_flight public
  virtio-blk: Increase in_flight for request restart BH
  nbd: Restrict connection_co reentrance
  io: Make qio_channel_yield() interruptible
  io: Remove redundant read/write_coroutine assignments
  nbd: Move nbd_read_eof() to nbd/client.c
  nbd: Use low-level QIOChannel API in nbd_read_eof()
  nbd: Increase bs->in_flight during AioContext switch
  block: Don't poll in bdrv_set_aio_context()
  block: Fix AioContext switch for drained node
  test-bdrv-drain: AioContext switch in drained section
  block: Use normal drain for bdrv_set_aio_context()
  aio-posix: Assert that aio_poll() is always called in home thread

 block/nbd-client.h             |  1 +
 include/block/nbd.h            |  3 +-
 include/io/channel.h           |  9 ++++--
 include/sysemu/block-backend.h |  2 ++
 nbd/nbd-internal.h             | 19 -------------
 block.c                        | 26 ++++++++---------
 block/block-backend.c          |  4 +--
 block/nbd-client.c             | 36 +++++++++++++++++++++--
 hw/block/virtio-blk.c          |  4 +++
 io/channel.c                   | 24 ++++++++++------
 nbd/client.c                   | 52 ++++++++++++++++++++++++++++++++--
 tests/test-bdrv-drain.c        | 32 +++++++++++++++++++++
 util/aio-posix.c               |  3 +-
 13 files changed, 164 insertions(+), 51 deletions(-)

-- 
2.20.1

[Qemu-devel] [PATCH v2 01/13] block-backend: Make blk_inc/dec_in_flight public

[Kevin Wolf]

For some users of BlockBackends, just increasing the in_flight counter
is easier than implementing separate handlers in BlockDevOps. Make the
helper functions for this public.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 include/sysemu/block-backend.h | 2 ++
 block/block-backend.c          | 4 ++--
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index 832a4bf168..e2066eb06b 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -156,6 +156,8 @@ int blk_co_pdiscard(BlockBackend *blk, int64_t offset, int bytes);
 int blk_co_flush(BlockBackend *blk);
 int blk_flush(BlockBackend *blk);
 int blk_commit_all(void);
+void blk_inc_in_flight(BlockBackend *blk);
+void blk_dec_in_flight(BlockBackend *blk);
 void blk_drain(BlockBackend *blk);
 void blk_drain_all(void);
 void blk_set_on_error(BlockBackend *blk, BlockdevOnError on_read_error,
diff --git a/block/block-backend.c b/block/block-backend.c
index f6ea824308..0219555f89 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1262,12 +1262,12 @@ int blk_make_zero(BlockBackend *blk, BdrvRequestFlags flags)
     return bdrv_make_zero(blk->root, flags);
 }
 
-static void blk_inc_in_flight(BlockBackend *blk)
+void blk_inc_in_flight(BlockBackend *blk)
 {
     atomic_inc(&blk->in_flight);
 }
 
-static void blk_dec_in_flight(BlockBackend *blk)
+void blk_dec_in_flight(BlockBackend *blk)
 {
     atomic_dec(&blk->in_flight);
     aio_wait_kick();
-- 
2.20.1

[Qemu-devel] [PATCH v2 02/13] virtio-blk: Increase in_flight for request restart BH

[Kevin Wolf]

virtio_blk_dma_restart_bh() submits new requests, so in order to make
sure that these requests are not started inside a drained section of the
attached BlockBackend, we need to make sure that draining the
BlockBackend waits for the BH to be executed.

This BH is still questionable because its scheduled in the main thread
instead of the configured iothread. Leave a FIXME comment for this.

But with this fix, enabling the data plane at least waits for these
requests (in bdrv_set_aio_context()) instead of changing the AioContext
under their feet and making them run in the wrong thread, causing
crashes and failures (e.g. due to missing locking).

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 hw/block/virtio-blk.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index cf7f47eaba..e11e6e45d0 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -675,6 +675,7 @@ static void virtio_blk_dma_restart_bh(void *opaque)
     if (mrb.num_reqs) {
         virtio_blk_submit_multireq(s->blk, &mrb);
     }
+    blk_dec_in_flight(s->conf.conf.blk);
     aio_context_release(blk_get_aio_context(s->conf.conf.blk));
 }
 
@@ -688,8 +689,11 @@ static void virtio_blk_dma_restart_cb(void *opaque, int running,
     }
 
     if (!s->bh) {
+        /* FIXME The data plane is not started yet, so these requests are
+         * processed in the main thread. */
         s->bh = aio_bh_new(blk_get_aio_context(s->conf.conf.blk),
                            virtio_blk_dma_restart_bh, s);
+        blk_inc_in_flight(s->conf.conf.blk);
         qemu_bh_schedule(s->bh);
     }
 }
-- 
2.20.1

[Qemu-devel] [PATCH v2 03/13] nbd: Restrict connection_co reentrance

[Kevin Wolf]

nbd_client_attach_aio_context() schedules connection_co in the new
AioContext and this way reenters it in any arbitrary place that has
yielded. We can restrict this a bit to the function call where the
coroutine actually sits waiting when it's idle.

This doesn't solve any bug yet, but it shows where in the code we need
to support this random reentrance and where we don't have to care.

Add FIXME comments for the existing bugs that the rest of this series
will fix.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 block/nbd-client.h |  1 +
 block/nbd-client.c | 22 ++++++++++++++++++++++
 2 files changed, 23 insertions(+)

diff --git a/block/nbd-client.h b/block/nbd-client.h
index d990207a5c..09e03013d2 100644
--- a/block/nbd-client.h
+++ b/block/nbd-client.h
@@ -35,6 +35,7 @@ typedef struct NBDClientSession {
 
     NBDClientRequest requests[MAX_NBD_REQUESTS];
     NBDReply reply;
+    BlockDriverState *bs;
     bool quit;
 } NBDClientSession;
 
diff --git a/block/nbd-client.c b/block/nbd-client.c
index f0ad54ce21..5ce4aa9520 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -76,8 +76,24 @@ static coroutine_fn void nbd_connection_entry(void *opaque)
     Error *local_err = NULL;
 
     while (!s->quit) {
+        /*
+         * The NBD client can only really be considered idle when it has
+         * yielded from qio_channel_readv_all_eof(), waiting for data. This is
+         * the point where the additional scheduled coroutine entry happens
+         * after nbd_client_attach_aio_context().
+         *
+         * Therefore we keep an additional in_flight reference all the time and
+         * only drop it temporarily here.
+         *
+         * FIXME This is not safe because the QIOChannel could wake up the
+         * coroutine for a second time; it is not prepared for coroutine
+         * resumption from external code.
+         */
+        bdrv_dec_in_flight(s->bs);
         assert(s->reply.handle == 0);
         ret = nbd_receive_reply(s->ioc, &s->reply, &local_err);
+        bdrv_inc_in_flight(s->bs);
+
         if (local_err) {
             trace_nbd_read_reply_entry_fail(ret, error_get_pretty(local_err));
             error_free(local_err);
@@ -116,6 +132,8 @@ static coroutine_fn void nbd_connection_entry(void *opaque)
 
     s->quit = true;
     nbd_recv_coroutines_wake_all(s);
+    bdrv_dec_in_flight(s->bs);
+
     s->connection_co = NULL;
     aio_wait_kick();
 }
@@ -970,6 +988,8 @@ void nbd_client_attach_aio_context(BlockDriverState *bs,
 {
     NBDClientSession *client = nbd_get_client_session(bs);
     qio_channel_attach_aio_context(QIO_CHANNEL(client->ioc), new_context);
+
+    /* FIXME Really need a bdrv_inc_in_flight() here */
     aio_co_schedule(new_context, client->connection_co);
 }
 
@@ -1076,6 +1096,7 @@ static int nbd_client_connect(BlockDriverState *bs,
      * kick the reply mechanism.  */
     qio_channel_set_blocking(QIO_CHANNEL(sioc), false, NULL);
     client->connection_co = qemu_coroutine_create(nbd_connection_entry, client);
+    bdrv_inc_in_flight(bs);
     nbd_client_attach_aio_context(bs, bdrv_get_aio_context(bs));
 
     logout("Established connection with NBD server\n");
@@ -1108,6 +1129,7 @@ int nbd_client_init(BlockDriverState *bs,
 {
     NBDClientSession *client = nbd_get_client_session(bs);
 
+    client->bs = bs;
     qemu_co_mutex_init(&client->send_mutex);
     qemu_co_queue_init(&client->free_sema);
 
-- 
2.20.1

[Qemu-devel] [PATCH v2 04/13] io: Make qio_channel_yield() interruptible

[Kevin Wolf]

Similar to how qemu_co_sleep_ns() allows preemption from an external
coroutine entry, allow reentering qio_channel_yield() early.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 include/io/channel.h |  9 ++++++---
 io/channel.c         | 10 ++++++++++
 2 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/include/io/channel.h b/include/io/channel.h
index da2f138200..59460cb1ec 100644
--- a/include/io/channel.h
+++ b/include/io/channel.h
@@ -739,10 +739,13 @@ void qio_channel_detach_aio_context(QIOChannel *ioc);
  * addition, no two coroutine can be waiting on the same condition
  * and channel at the same time.
  *
- * This must only be called from coroutine context
+ * This must only be called from coroutine context. It is safe to
+ * reenter the coroutine externally while it is waiting; in this
+ * case the function will return even if @condition is not yet
+ * available.
  */
-void qio_channel_yield(QIOChannel *ioc,
-                       GIOCondition condition);
+void coroutine_fn qio_channel_yield(QIOChannel *ioc,
+                                    GIOCondition condition);
 
 /**
  * qio_channel_wait:
diff --git a/io/channel.c b/io/channel.c
index 8dd0684f5d..303376e08d 100644
--- a/io/channel.c
+++ b/io/channel.c
@@ -469,6 +469,16 @@ void coroutine_fn qio_channel_yield(QIOChannel *ioc,
     }
     qio_channel_set_aio_fd_handlers(ioc);
     qemu_coroutine_yield();
+
+    /* Allow interrupting the operation by reentering the coroutine other than
+     * through the aio_fd_handlers. */
+    if (condition == G_IO_IN && ioc->read_coroutine) {
+        ioc->read_coroutine = NULL;
+        qio_channel_set_aio_fd_handlers(ioc);
+    } else if (condition == G_IO_OUT && ioc->write_coroutine) {
+        ioc->write_coroutine = NULL;
+        qio_channel_set_aio_fd_handlers(ioc);
+    }
 }
 
 
-- 
2.20.1

[Qemu-devel] [PATCH v2 05/13] io: Remove redundant read/write_coroutine assignments

[Kevin Wolf]

qio_channel_yield() now updates ioc->read_write/coroutine and calls
qio_channel_set_aio_fd_handlers(), so the code in the handlers has
become redundant and can be removed.

This does not make a difference in intermediate states because
aio_co_wake() really enters the coroutine immediately here: These
handlers are never run in coroutine context, and we're in the right
AioContext because qio_channel_attach_aio_context() asserts that the
handlers are inactive.

To make these conditions more obvious, replace the aio_co_wake() with a
direct qemu_coroutine_enter() and assert the right AioContext.

Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 io/channel.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/io/channel.c b/io/channel.c
index 303376e08d..aa3edf6019 100644
--- a/io/channel.c
+++ b/io/channel.c
@@ -400,16 +400,14 @@ off_t qio_channel_io_seek(QIOChannel *ioc,
 }
 
 
-static void qio_channel_set_aio_fd_handlers(QIOChannel *ioc);
-
 static void qio_channel_restart_read(void *opaque)
 {
     QIOChannel *ioc = opaque;
     Coroutine *co = ioc->read_coroutine;
 
-    ioc->read_coroutine = NULL;
-    qio_channel_set_aio_fd_handlers(ioc);
-    aio_co_wake(co);
+    assert(qemu_get_current_aio_context() ==
+           qemu_coroutine_get_aio_context(co));
+    qemu_coroutine_enter(co);
 }
 
 static void qio_channel_restart_write(void *opaque)
@@ -417,9 +415,9 @@ static void qio_channel_restart_write(void *opaque)
     QIOChannel *ioc = opaque;
     Coroutine *co = ioc->write_coroutine;
 
-    ioc->write_coroutine = NULL;
-    qio_channel_set_aio_fd_handlers(ioc);
-    aio_co_wake(co);
+    assert(qemu_get_current_aio_context() ==
+           qemu_coroutine_get_aio_context(co));
+    qemu_coroutine_enter(co);
 }
 
 static void qio_channel_set_aio_fd_handlers(QIOChannel *ioc)
-- 
2.20.1

Re: [Qemu-devel] [PATCH v2 05/13] io: Remove redundant read/write_coroutine assignments

[Paolo Bonzini]

On 20/02/19 18:48, Kevin Wolf wrote:
> -static void qio_channel_set_aio_fd_handlers(QIOChannel *ioc);
> -
>  static void qio_channel_restart_read(void *opaque)
>  {
>      QIOChannel *ioc = opaque;
>      Coroutine *co = ioc->read_coroutine;
>  
> -    ioc->read_coroutine = NULL;
> -    qio_channel_set_aio_fd_handlers(ioc);
> -    aio_co_wake(co);
> +    assert(qemu_get_current_aio_context() ==
> +           qemu_coroutine_get_aio_context(co));
> +    qemu_coroutine_enter(co);
>  }
>  
>  static void qio_channel_restart_write(void *opaque)
> @@ -417,9 +415,9 @@ static void qio_channel_restart_write(void *opaque)
>      QIOChannel *ioc = opaque;
>      Coroutine *co = ioc->write_coroutine;
>  
> -    ioc->write_coroutine = NULL;
> -    qio_channel_set_aio_fd_handlers(ioc);
> -    aio_co_wake(co);
> +    assert(qemu_get_current_aio_context() ==
> +           qemu_coroutine_get_aio_context(co));
> +    qemu_coroutine_enter(co);
>  }
>  
>  static void qio_channel_set_aio_fd_handlers(QIOChannel *ioc)
> 

aio_co_wake was also acquiring/releasing the AioContext, so
that needs to stay for now.

Paolo

Re: [Qemu-devel] [PATCH v2 05/13] io: Remove redundant read/write_coroutine assignments

[Kevin Wolf]

Am 20.02.2019 um 19:01 hat Paolo Bonzini geschrieben:
> On 20/02/19 18:48, Kevin Wolf wrote:
> > -static void qio_channel_set_aio_fd_handlers(QIOChannel *ioc);
> > -
> >  static void qio_channel_restart_read(void *opaque)
> >  {
> >      QIOChannel *ioc = opaque;
> >      Coroutine *co = ioc->read_coroutine;
> >  
> > -    ioc->read_coroutine = NULL;
> > -    qio_channel_set_aio_fd_handlers(ioc);
> > -    aio_co_wake(co);
> > +    assert(qemu_get_current_aio_context() ==
> > +           qemu_coroutine_get_aio_context(co));
> > +    qemu_coroutine_enter(co);
> >  }
> >  
> >  static void qio_channel_restart_write(void *opaque)
> > @@ -417,9 +415,9 @@ static void qio_channel_restart_write(void *opaque)
> >      QIOChannel *ioc = opaque;
> >      Coroutine *co = ioc->write_coroutine;
> >  
> > -    ioc->write_coroutine = NULL;
> > -    qio_channel_set_aio_fd_handlers(ioc);
> > -    aio_co_wake(co);
> > +    assert(qemu_get_current_aio_context() ==
> > +           qemu_coroutine_get_aio_context(co));
> > +    qemu_coroutine_enter(co);
> >  }
> >  
> >  static void qio_channel_set_aio_fd_handlers(QIOChannel *ioc)
> > 
> 
> aio_co_wake was also acquiring/releasing the AioContext, so
> that needs to stay for now.

True. Maybe I should leave the aio_co_wake call alone() and just add the
assertion to avoid complicating the code rather than simplifying it...

Kevin

Re: [Qemu-devel] [PATCH v2 05/13] io: Remove redundant read/write_coroutine assignments

[Paolo Bonzini]

On 20/02/19 19:07, Kevin Wolf wrote:
> Am 20.02.2019 um 19:01 hat Paolo Bonzini geschrieben:
>> aio_co_wake was also acquiring/releasing the AioContext, so
>> that needs to stay for now.
> 
> True. Maybe I should leave the aio_co_wake call alone() and just add the
> assertion to avoid complicating the code rather than simplifying it...

Yes, I agree.

Paolo

ps: where is alone() defined? :)

Paolo

Re: [Qemu-devel] [PATCH v2 05/13] io: Remove redundant read/write_coroutine assignments

[Kevin Wolf]

Am 20.02.2019 um 19:22 hat Paolo Bonzini geschrieben:
> On 20/02/19 19:07, Kevin Wolf wrote:
> > Am 20.02.2019 um 19:01 hat Paolo Bonzini geschrieben:
> >> aio_co_wake was also acquiring/releasing the AioContext, so
> >> that needs to stay for now.
> > 
> > True. Maybe I should leave the aio_co_wake call alone() and just add the
> > assertion to avoid complicating the code rather than simplifying it...
> 
> Yes, I agree.

Okay, I made this change locally. I'm not sending a v3 for this, but
will just directly merge it if no other comments come up.

> ps: where is alone() defined? :)

#define aio_co_wake aio_co_wake()
#define alone() alone

Here. :-)

Kevin

[Qemu-devel] [PATCH v2 06/13] nbd: Move nbd_read_eof() to nbd/client.c

[Kevin Wolf]

The only caller of nbd_read_eof() is nbd_receive_reply(), so it doesn't
have to live in the header file, but can move next to its caller.

Also add the missing coroutine_fn to the function and its caller.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 include/block/nbd.h |  3 ++-
 nbd/nbd-internal.h  | 19 -------------------
 nbd/client.c        | 22 +++++++++++++++++++++-
 3 files changed, 23 insertions(+), 21 deletions(-)

diff --git a/include/block/nbd.h b/include/block/nbd.h
index 96cfb1d7d5..cad975e00c 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -300,7 +300,8 @@ int nbd_receive_export_list(QIOChannel *ioc, QCryptoTLSCreds *tlscreds,
 int nbd_init(int fd, QIOChannelSocket *sioc, NBDExportInfo *info,
              Error **errp);
 int nbd_send_request(QIOChannel *ioc, NBDRequest *request);
-int nbd_receive_reply(QIOChannel *ioc, NBDReply *reply, Error **errp);
+int coroutine_fn nbd_receive_reply(QIOChannel *ioc, NBDReply *reply,
+                                   Error **errp);
 int nbd_client(int fd);
 int nbd_disconnect(int fd);
 int nbd_errno_to_system_errno(int err);
diff --git a/nbd/nbd-internal.h b/nbd/nbd-internal.h
index 82aa221227..049f83df77 100644
--- a/nbd/nbd-internal.h
+++ b/nbd/nbd-internal.h
@@ -64,25 +64,6 @@
 #define NBD_SET_TIMEOUT             _IO(0xab, 9)
 #define NBD_SET_FLAGS               _IO(0xab, 10)
 
-/* nbd_read_eof
- * Tries to read @size bytes from @ioc.
- * Returns 1 on success
- *         0 on eof, when no data was read (errp is not set)
- *         negative errno on failure (errp is set)
- */
-static inline int nbd_read_eof(QIOChannel *ioc, void *buffer, size_t size,
-                               Error **errp)
-{
-    int ret;
-
-    assert(size);
-    ret = qio_channel_read_all_eof(ioc, buffer, size, errp);
-    if (ret < 0) {
-        ret = -EIO;
-    }
-    return ret;
-}
-
 /* nbd_write
  * Writes @size bytes to @ioc. Returns 0 on success.
  */
diff --git a/nbd/client.c b/nbd/client.c
index 10a52ad7d0..28d174c0f3 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -1387,12 +1387,32 @@ static int nbd_receive_structured_reply_chunk(QIOChannel *ioc,
     return 0;
 }
 
+/* nbd_read_eof
+ * Tries to read @size bytes from @ioc.
+ * Returns 1 on success
+ *         0 on eof, when no data was read (errp is not set)
+ *         negative errno on failure (errp is set)
+ */
+static inline int coroutine_fn
+nbd_read_eof(QIOChannel *ioc, void *buffer, size_t size, Error **errp)
+{
+    int ret;
+
+    assert(size);
+    ret = qio_channel_read_all_eof(ioc, buffer, size, errp);
+    if (ret < 0) {
+        ret = -EIO;
+    }
+    return ret;
+}
+
 /* nbd_receive_reply
  * Returns 1 on success
  *         0 on eof, when no data was read (errp is not set)
  *         negative errno on failure (errp is set)
  */
-int nbd_receive_reply(QIOChannel *ioc, NBDReply *reply, Error **errp)
+int coroutine_fn nbd_receive_reply(QIOChannel *ioc, NBDReply *reply,
+                                   Error **errp)
 {
     int ret;
     const char *type;
-- 
2.20.1

[Qemu-devel] [PATCH v2 07/13] nbd: Use low-level QIOChannel API in nbd_read_eof()

[Kevin Wolf]

Instead of using the convenience wrapper qio_channel_read_all_eof(), use
the lower level QIOChannel API. This means duplicating some code, but
we'll need this because this coroutine yield is special: We want it to
be interruptible so that nbd_client_attach_aio_context() can correctly
reenter the coroutine.

This moves the bdrv_dec/inc_in_flight() pair into nbd_read_eof(), so
that connection_co will always sit in this exact qio_channel_yield()
call when bdrv_drain() returns.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 include/block/nbd.h |  4 ++--
 block/nbd-client.c  |  8 +-------
 nbd/client.c        | 46 ++++++++++++++++++++++++++++++++++++---------
 3 files changed, 40 insertions(+), 18 deletions(-)

diff --git a/include/block/nbd.h b/include/block/nbd.h
index cad975e00c..c6ef1ef42e 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -300,8 +300,8 @@ int nbd_receive_export_list(QIOChannel *ioc, QCryptoTLSCreds *tlscreds,
 int nbd_init(int fd, QIOChannelSocket *sioc, NBDExportInfo *info,
              Error **errp);
 int nbd_send_request(QIOChannel *ioc, NBDRequest *request);
-int coroutine_fn nbd_receive_reply(QIOChannel *ioc, NBDReply *reply,
-                                   Error **errp);
+int coroutine_fn nbd_receive_reply(BlockDriverState *bs, QIOChannel *ioc,
+                                   NBDReply *reply, Error **errp);
 int nbd_client(int fd);
 int nbd_disconnect(int fd);
 int nbd_errno_to_system_errno(int err);
diff --git a/block/nbd-client.c b/block/nbd-client.c
index 5ce4aa9520..60f38f0320 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -84,15 +84,9 @@ static coroutine_fn void nbd_connection_entry(void *opaque)
          *
          * Therefore we keep an additional in_flight reference all the time and
          * only drop it temporarily here.
-         *
-         * FIXME This is not safe because the QIOChannel could wake up the
-         * coroutine for a second time; it is not prepared for coroutine
-         * resumption from external code.
          */
-        bdrv_dec_in_flight(s->bs);
         assert(s->reply.handle == 0);
-        ret = nbd_receive_reply(s->ioc, &s->reply, &local_err);
-        bdrv_inc_in_flight(s->bs);
+        ret = nbd_receive_reply(s->bs, s->ioc, &s->reply, &local_err);
 
         if (local_err) {
             trace_nbd_read_reply_entry_fail(ret, error_get_pretty(local_err));
diff --git a/nbd/client.c b/nbd/client.c
index 28d174c0f3..de7da48246 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -1394,30 +1394,58 @@ static int nbd_receive_structured_reply_chunk(QIOChannel *ioc,
  *         negative errno on failure (errp is set)
  */
 static inline int coroutine_fn
-nbd_read_eof(QIOChannel *ioc, void *buffer, size_t size, Error **errp)
+nbd_read_eof(BlockDriverState *bs, QIOChannel *ioc, void *buffer, size_t size,
+             Error **errp)
 {
-    int ret;
+    bool partial = false;
 
     assert(size);
-    ret = qio_channel_read_all_eof(ioc, buffer, size, errp);
-    if (ret < 0) {
-        ret = -EIO;
+    while (size > 0) {
+        struct iovec iov = { .iov_base = buffer, .iov_len = size };
+        ssize_t len;
+
+        len = qio_channel_readv(ioc, &iov, 1, errp);
+        if (len == QIO_CHANNEL_ERR_BLOCK) {
+            bdrv_dec_in_flight(bs);
+            qio_channel_yield(ioc, G_IO_IN);
+            bdrv_inc_in_flight(bs);
+            continue;
+        } else if (len < 0) {
+            return -EIO;
+        } else if (len == 0) {
+            if (partial) {
+                error_setg(errp,
+                           "Unexpected end-of-file before all bytes were read");
+                return -EIO;
+            } else {
+                return 0;
+            }
+        }
+
+        partial = true;
+        size -= len;
+        buffer = (uint8_t*) buffer + len;
     }
-    return ret;
+    return 1;
 }
 
 /* nbd_receive_reply
+ *
+ * Decreases bs->in_flight while waiting for a new reply. This yield is where
+ * we wait indefinitely and the coroutine must be able to be safely reentered
+ * for nbd_client_attach_aio_context().
+ *
  * Returns 1 on success
  *         0 on eof, when no data was read (errp is not set)
  *         negative errno on failure (errp is set)
  */
-int coroutine_fn nbd_receive_reply(QIOChannel *ioc, NBDReply *reply,
-                                   Error **errp)
+int coroutine_fn nbd_receive_reply(BlockDriverState *bs, QIOChannel *ioc,
+                                   NBDReply *reply, Error **errp)
 {
     int ret;
     const char *type;
 
-    ret = nbd_read_eof(ioc, &reply->magic, sizeof(reply->magic), errp);
+    ret = nbd_read_eof(bs, ioc, &reply->magic, sizeof(reply->magic), errp);
     if (ret <= 0) {
         return ret;
     }
-- 
2.20.1

[Qemu-devel] [PATCH v2 08/13] nbd: Increase bs->in_flight during AioContext switch

[Kevin Wolf]

bdrv_drain() must not leave connection_co scheduled, so bs->in_flight
needs to be increased while the coroutine is waiting to be scheduled
in the new AioContext after nbd_client_attach_aio_context().

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/nbd-client.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/block/nbd-client.c b/block/nbd-client.c
index 60f38f0320..bfbaf7ebe9 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -977,14 +977,30 @@ void nbd_client_detach_aio_context(BlockDriverState *bs)
     qio_channel_detach_aio_context(QIO_CHANNEL(client->ioc));
 }
 
+static void nbd_client_attach_aio_context_bh(void *opaque)
+{
+    BlockDriverState *bs = opaque;
+    NBDClientSession *client = nbd_get_client_session(bs);
+
+    /* The node is still drained, so we know the coroutine has yielded in
+     * nbd_read_eof(), the only place where bs->in_flight can reach 0, or it is
+     * entered for the first time. Both places are safe for entering the
+     * coroutine.*/
+    qemu_aio_coroutine_enter(bs->aio_context, client->connection_co);
+    bdrv_dec_in_flight(bs);
+}
+
 void nbd_client_attach_aio_context(BlockDriverState *bs,
                                    AioContext *new_context)
 {
     NBDClientSession *client = nbd_get_client_session(bs);
     qio_channel_attach_aio_context(QIO_CHANNEL(client->ioc), new_context);
 
-    /* FIXME Really need a bdrv_inc_in_flight() here */
-    aio_co_schedule(new_context, client->connection_co);
+    bdrv_inc_in_flight(bs);
+
+    /* Need to wait here for the BH to run because the BH must run while the
+     * node is still drained. */
+    aio_wait_bh_oneshot(new_context, nbd_client_attach_aio_context_bh, bs);
 }
 
 void nbd_client_close(BlockDriverState *bs)
-- 
2.20.1

[Qemu-devel] [PATCH v2 09/13] block: Don't poll in bdrv_set_aio_context()

[Kevin Wolf]

The explicit aio_poll() call in bdrv_set_aio_context() was added in
commit c2b6428d388 as a workaround for bdrv_drain() failing to achieve
to actually quiesce everything (specifically the NBD client code to
switch AioContext).

Now that the NBD client has been fixed to complete this operation during
bdrv_drain(), we don't need the workaround any more.

It was wrong anyway: aio_poll() must always be run in the home thread of
the AioContext.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 block.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/block.c b/block.c
index 0c12632661..17bc1d3dca 100644
--- a/block.c
+++ b/block.c
@@ -5273,10 +5273,6 @@ void bdrv_set_aio_context(BlockDriverState *bs, AioContext *new_context)
     bdrv_parent_drained_begin(bs, NULL, false);
     bdrv_drain(bs); /* ensure there are no in-flight requests */
 
-    while (aio_poll(ctx, false)) {
-        /* wait for all bottom halves to execute */
-    }
-
     bdrv_detach_aio_context(bs);
 
     /* This function executes in the old AioContext so acquire the new one in
-- 
2.20.1

[Qemu-devel] [PATCH v2 10/13] block: Fix AioContext switch for drained node

[Kevin Wolf]

When a drained node changes its AioContext, we need to move its
aio_disable_external() to the new context, too.

Without this fix, drain_end will try to reenable the new context, which
has never been disabled, so an assertion failure is triggered.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 block.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/block.c b/block.c
index 17bc1d3dca..aefb5701f5 100644
--- a/block.c
+++ b/block.c
@@ -5227,6 +5227,9 @@ void bdrv_detach_aio_context(BlockDriverState *bs)
         bdrv_detach_aio_context(child->bs);
     }
 
+    if (bs->quiesce_counter) {
+        aio_enable_external(bs->aio_context);
+    }
     bs->aio_context = NULL;
 }
 
@@ -5240,6 +5243,10 @@ void bdrv_attach_aio_context(BlockDriverState *bs,
         return;
     }
 
+    if (bs->quiesce_counter) {
+        aio_disable_external(new_context);
+    }
+
     bs->aio_context = new_context;
 
     QLIST_FOREACH(child, &bs->children, next) {
-- 
2.20.1

[Qemu-devel] [PATCH v2 11/13] test-bdrv-drain: AioContext switch in drained section

[Kevin Wolf]

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 tests/test-bdrv-drain.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/tests/test-bdrv-drain.c b/tests/test-bdrv-drain.c
index ee1740ff06..1b1f6c17a5 100644
--- a/tests/test-bdrv-drain.c
+++ b/tests/test-bdrv-drain.c
@@ -1522,6 +1522,36 @@ static void test_append_to_drained(void)
     blk_unref(blk);
 }
 
+static void test_set_aio_context(void)
+{
+    BlockDriverState *bs;
+    IOThread *a = iothread_new();
+    IOThread *b = iothread_new();
+    AioContext *ctx_a = iothread_get_aio_context(a);
+    AioContext *ctx_b = iothread_get_aio_context(b);
+
+    bs = bdrv_new_open_driver(&bdrv_test, "test-node", BDRV_O_RDWR,
+                              &error_abort);
+
+    bdrv_drained_begin(bs);
+    bdrv_set_aio_context(bs, ctx_a);
+
+    aio_context_acquire(ctx_a);
+    bdrv_drained_end(bs);
+
+    bdrv_drained_begin(bs);
+    bdrv_set_aio_context(bs, ctx_b);
+    aio_context_release(ctx_a);
+    aio_context_acquire(ctx_b);
+    bdrv_set_aio_context(bs, qemu_get_aio_context());
+    aio_context_release(ctx_b);
+    bdrv_drained_end(bs);
+
+    bdrv_unref(bs);
+    iothread_join(a);
+    iothread_join(b);
+}
+
 int main(int argc, char **argv)
 {
     int ret;
@@ -1603,6 +1633,8 @@ int main(int argc, char **argv)
 
     g_test_add_func("/bdrv-drain/attach/drain", test_append_to_drained);
 
+    g_test_add_func("/bdrv-drain/set_aio_context", test_set_aio_context);
+
     ret = g_test_run();
     qemu_event_destroy(&done_event);
     return ret;
-- 
2.20.1

[Qemu-devel] [PATCH v2 12/13] block: Use normal drain for bdrv_set_aio_context()

[Kevin Wolf]

Now that bdrv_set_aio_context() works inside drained sections, it can
also use the real drain function instead of open coding something
similar.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/block.c b/block.c
index aefb5701f5..375a216f76 100644
--- a/block.c
+++ b/block.c
@@ -5268,18 +5268,16 @@ void bdrv_attach_aio_context(BlockDriverState *bs,
     bs->walking_aio_notifiers = false;
 }
 
+/* The caller must own the AioContext lock for the old AioContext of bs, but it
+ * must not own the AioContext lock for new_context (unless new_context is
+ * the same as the current context of bs). */
 void bdrv_set_aio_context(BlockDriverState *bs, AioContext *new_context)
 {
-    AioContext *ctx = bdrv_get_aio_context(bs);
-
-    if (ctx == new_context) {
+    if (bdrv_get_aio_context(bs) == new_context) {
         return;
     }
 
-    aio_disable_external(ctx);
-    bdrv_parent_drained_begin(bs, NULL, false);
-    bdrv_drain(bs); /* ensure there are no in-flight requests */
-
+    bdrv_drained_begin(bs);
     bdrv_detach_aio_context(bs);
 
     /* This function executes in the old AioContext so acquire the new one in
@@ -5287,8 +5285,7 @@ void bdrv_set_aio_context(BlockDriverState *bs, AioContext *new_context)
      */
     aio_context_acquire(new_context);
     bdrv_attach_aio_context(bs, new_context);
-    bdrv_parent_drained_end(bs, NULL, false);
-    aio_enable_external(ctx);
+    bdrv_drained_end(bs);
     aio_context_release(new_context);
 }
 
-- 
2.20.1

[Qemu-devel] [PATCH v2 13/13] aio-posix: Assert that aio_poll() is always called in home thread

[Kevin Wolf]

aio_poll() has an existing assertion that the function is only called
from the AioContext's home thread if blocking is allowed.

This is not enough, some handlers make assumptions about the thread they
run in. Extend the assertion to non-blocking calls, too.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 util/aio-posix.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/util/aio-posix.c b/util/aio-posix.c
index 8640dfde9f..6fbfa7924f 100644
--- a/util/aio-posix.c
+++ b/util/aio-posix.c
@@ -613,6 +613,8 @@ bool aio_poll(AioContext *ctx, bool blocking)
     int64_t timeout;
     int64_t start = 0;
 
+    assert(in_aio_context_home_thread(ctx));
+
     /* aio_notify can avoid the expensive event_notifier_set if
      * everything (file descriptors, bottom halves, timers) will
      * be re-evaluated before the next blocking poll().  This is
@@ -621,7 +623,6 @@ bool aio_poll(AioContext *ctx, bool blocking)
      * so disable the optimization now.
      */
     if (blocking) {
-        assert(in_aio_context_home_thread(ctx));
         atomic_add(&ctx->notify_me, 2);
     }
 
-- 
2.20.1

Re: [Qemu-devel] [PATCH v2 00/13] block: bdrv_set_aio_context() related fixes

[Kevin Wolf]

Am 20.02.2019 um 18:48 hat Kevin Wolf geschrieben:
> Background for this series is the following bug report, which is about a
> crash with virtio-blk + iothread and request resubmission for werror/rerror:
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1671173
> 
> The reason is that bdrv_set_aio_context() didn't correctly quiesce
> everything. Instead, it had a local hack to call aio_poll() for the
> source AioContext, which covered some, but not all cases, and is wrong
> because you can only call aio_poll() from the home thread.
> 
> So this series tries to make bdrv_drain() actually drain the known cases
> (fixes virtio-blk and the NBD client) and use the regular drain
> functions in bdrv_set_aio_context() instead of open-coding something
> similar.

Applied to the block branch.

Kevin