* [Qemu-devel] [PATCH v2 1/4] block: extract AIO_WAIT_WHILE() from BlockDriverState
2018-02-13 14:20 [Qemu-devel] [PATCH v2 0/4] block: fix blk_aio_*() segfault when blk->root == NULL Stefan Hajnoczi
@ 2018-02-13 14:20 ` Stefan Hajnoczi
2018-02-13 16:01 ` Eric Blake
2018-02-13 14:21 ` [Qemu-devel] [PATCH v2 2/4] block: add BlockBackend->in_flight counter Stefan Hajnoczi
` (3 subsequent siblings)
4 siblings, 1 reply; 14+ messages in thread
From: Stefan Hajnoczi @ 2018-02-13 14:20 UTC (permalink / raw)
To: qemu-devel; +Cc: John Snow, mark.kanda, Kevin Wolf, Stefan Hajnoczi
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 <stefanha@redhat.com>
---
util/Makefile.objs | 2 +-
include/block/aio-wait.h | 116 ++++++++++++++++++++++++++++++++++++++++++++++
include/block/block.h | 40 +++-------------
include/block/block_int.h | 7 ++-
block.c | 5 ++
block/io.c | 10 +---
util/aio-wait.c | 40 ++++++++++++++++
7 files changed, 174 insertions(+), 46 deletions(-)
create mode 100644 include/block/aio-wait.h
create mode 100644 util/aio-wait.c
diff --git a/util/Makefile.objs b/util/Makefile.objs
index 3fb611631f..ae90b9963d 100644
--- a/util/Makefile.objs
+++ b/util/Makefile.objs
@@ -1,7 +1,7 @@
util-obj-y = osdep.o cutils.o unicode.o qemu-timer-common.o
util-obj-y += bufferiszero.o
util-obj-y += lockcnt.o
-util-obj-y += aiocb.o async.o thread-pool.o qemu-timer.o
+util-obj-y += aiocb.o async.o aio-wait.o thread-pool.o qemu-timer.o
util-obj-y += main-loop.o iohandler.o
util-obj-$(CONFIG_POSIX) += aio-posix.o
util-obj-$(CONFIG_POSIX) += compatfd.o
diff --git a/include/block/aio-wait.h b/include/block/aio-wait.h
new file mode 100644
index 0000000000..a08e5807dc
--- /dev/null
+++ b/include/block/aio-wait.h
@@ -0,0 +1,116 @@
+/*
+ * AioContext wait support
+ *
+ * Copyright (C) 2018 Red Hat, Inc.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#ifndef QEMU_AIO_WAIT_H
+#define QEMU_AIO_WAIT_H
+
+#include "block/aio.h"
+
+/**
+ * AioWait:
+ *
+ * An object that facilitates synchronous waiting on a condition. The main
+ * loop can wait on an operation running in an IOThread as follows:
+ *
+ * AioWait *wait = ...;
+ * AioContext *ctx = ...;
+ * MyWork work = { .done = false };
+ * schedule_my_work_in_iothread(ctx, &work);
+ * AIO_WAIT_WHILE(wait, ctx, !work.done);
+ *
+ * The IOThread must call aio_wait_kick() to notify the main loop when
+ * work.done changes:
+ *
+ * static void do_work(...)
+ * {
+ * ...
+ * work.done = true;
+ * aio_wait_kick(wait);
+ * }
+ */
+typedef struct {
+ /* Is the main loop waiting for a kick? Accessed with atomic ops. */
+ bool need_kick;
+} AioWait;
+
+/**
+ * AIO_WAIT_WHILE:
+ * @wait: the aio wait object
+ * @ctx: the aio context
+ * @cond: wait while this conditional expression is true
+ *
+ * Wait while a condition is true. Use this to implement synchronous
+ * operations that require event loop activity.
+ *
+ * The caller must be sure that something calls aio_wait_kick() when the value
+ * of @cond might have changed.
+ *
+ * The caller's thread must be the IOThread that owns @ctx or the main loop
+ * thread (with @ctx acquired exactly once). This function cannot be used to
+ * wait on conditions between two IOThreads since that could lead to deadlock,
+ * go via the main loop instead.
+ */
+#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_; \
+ } \
+ } else { \
+ assert(qemu_get_current_aio_context() == \
+ qemu_get_aio_context()); \
+ assert(!wait_->need_kick); \
+ /* Set wait_->need_kick before evaluating cond. */ \
+ atomic_mb_set(&wait_->need_kick, true); \
+ while (busy_) { \
+ if ((cond)) { \
+ waited_ = busy_ = true; \
+ aio_context_release(ctx_); \
+ aio_poll(qemu_get_aio_context(), true); \
+ aio_context_acquire(ctx_); \
+ } else { \
+ busy_ = aio_poll(ctx_, false); \
+ waited_ |= busy_; \
+ } \
+ } \
+ atomic_set(&wait_->need_kick, false); \
+ } \
+ waited_; })
+
+/**
+ * aio_wait_kick:
+ * @wait: the aio wait object that should re-evaluate its condition
+ *
+ * Wake up the main thread if it is waiting on AIO_WAIT_WHILE(). During
+ * synchronous operations performed in an IOThread, the main thread lets the
+ * IOThread's event loop run, waiting for the operation to complete. A
+ * aio_wait_kick() call will wake up the main thread.
+ */
+void aio_wait_kick(AioWait *wait);
+
+#endif /* QEMU_AIO_WAIT */
diff --git a/include/block/block.h b/include/block/block.h
index 19b3ab9cb5..c4beda33a8 100644
--- a/include/block/block.h
+++ 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);
+
#define BDRV_POLL_WHILE(bs, cond) ({ \
- bool waited_ = false; \
- bool busy_ = true; \
BlockDriverState *bs_ = (bs); \
- AioContext *ctx_ = bdrv_get_aio_context(bs_); \
- if (aio_context_in_iothread(ctx_)) { \
- while ((cond) || busy_) { \
- busy_ = aio_poll(ctx_, (cond)); \
- waited_ |= !!(cond) | busy_; \
- } \
- } else { \
- assert(qemu_get_current_aio_context() == \
- qemu_get_aio_context()); \
- /* Ask bdrv_dec_in_flight to wake up the main \
- * QEMU AioContext. Extra I/O threads never take \
- * other I/O threads' AioContexts (see for example \
- * block_job_defer_to_main_loop for how to do it). \
- */ \
- assert(!bs_->wakeup); \
- /* Set bs->wakeup before evaluating cond. */ \
- atomic_mb_set(&bs_->wakeup, true); \
- while (busy_) { \
- if ((cond)) { \
- waited_ = busy_ = true; \
- aio_context_release(ctx_); \
- aio_poll(qemu_get_aio_context(), true); \
- aio_context_acquire(ctx_); \
- } else { \
- busy_ = aio_poll(ctx_, false); \
- waited_ |= busy_; \
- } \
- } \
- atomic_set(&bs_->wakeup, false); \
- } \
- waited_; })
+ AIO_WAIT_WHILE(bdrv_get_aio_wait(bs_), \
+ bdrv_get_aio_context(bs_), \
+ cond); })
int bdrv_pdiscard(BlockDriverState *bs, int64_t offset, int bytes);
int bdrv_co_pdiscard(BlockDriverState *bs, int64_t offset, int bytes);
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 5ea63f8fa8..c1f6ee961a 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -26,6 +26,7 @@
#include "block/accounting.h"
#include "block/block.h"
+#include "block/aio-wait.h"
#include "qemu/queue.h"
#include "qemu/coroutine.h"
#include "qemu/stats64.h"
@@ -709,10 +710,8 @@ struct BlockDriverState {
unsigned int in_flight;
unsigned int serialising_in_flight;
- /* Internal to BDRV_POLL_WHILE and bdrv_wakeup. Accessed with atomic
- * ops.
- */
- bool wakeup;
+ /* Kicked to signal main loop when a request completes. */
+ AioWait wait;
/* counter for nested bdrv_io_plug.
* Accessed with atomic ops.
diff --git a/block.c b/block.c
index 814e5a02da..9e4da81213 100644
--- a/block.c
+++ 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;
+}
+
void bdrv_coroutine_enter(BlockDriverState *bs, Coroutine *co)
{
aio_co_enter(bdrv_get_aio_context(bs), co);
diff --git a/block/io.c b/block/io.c
index 89d0745e95..69170ee234 100644
--- a/block/io.c
+++ b/block/io.c
@@ -25,6 +25,7 @@
#include "qemu/osdep.h"
#include "trace.h"
#include "sysemu/block-backend.h"
+#include "block/aio-wait.h"
#include "block/blockjob.h"
#include "block/blockjob_int.h"
#include "block/block_int.h"
@@ -587,16 +588,9 @@ void bdrv_inc_in_flight(BlockDriverState *bs)
atomic_inc(&bs->in_flight);
}
-static void dummy_bh_cb(void *opaque)
-{
-}
-
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));
}
void bdrv_dec_in_flight(BlockDriverState *bs)
diff --git a/util/aio-wait.c b/util/aio-wait.c
new file mode 100644
index 0000000000..a487cdb852
--- /dev/null
+++ b/util/aio-wait.c
@@ -0,0 +1,40 @@
+/*
+ * AioContext wait support
+ *
+ * Copyright (C) 2018 Red Hat, Inc.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/main-loop.h"
+#include "block/aio-wait.h"
+
+static void dummy_bh_cb(void *opaque)
+{
+ /* The point is to make AIO_WAIT_WHILE()'s aio_poll() return */
+}
+
+void aio_wait_kick(AioWait *wait)
+{
+ /* The barrier (or an atomic op) is in the caller. */
+ if (atomic_read(&wait->need_kick)) {
+ aio_bh_schedule_oneshot(qemu_get_aio_context(), dummy_bh_cb, NULL);
+ }
+}
--
2.14.3
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [Qemu-devel] [PATCH v2 1/4] block: extract AIO_WAIT_WHILE() from BlockDriverState
2018-02-13 14:20 ` [Qemu-devel] [PATCH v2 1/4] block: extract AIO_WAIT_WHILE() from BlockDriverState Stefan Hajnoczi
@ 2018-02-13 16:01 ` Eric Blake
2018-02-14 14:06 ` Stefan Hajnoczi
2018-02-14 22:33 ` Eric Blake
0 siblings, 2 replies; 14+ messages in thread
From: Eric Blake @ 2018-02-13 16:01 UTC (permalink / raw)
To: Stefan Hajnoczi, qemu-devel; +Cc: Kevin Wolf, John Snow
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 <stefanha@redhat.com>
> ---
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
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [Qemu-devel] [PATCH v2 1/4] block: extract AIO_WAIT_WHILE() from BlockDriverState
2018-02-13 16:01 ` Eric Blake
@ 2018-02-14 14:06 ` Stefan Hajnoczi
2018-02-14 22:31 ` Eric Blake
2018-02-14 22:33 ` Eric Blake
1 sibling, 1 reply; 14+ messages in thread
From: Stefan Hajnoczi @ 2018-02-14 14:06 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel, Kevin Wolf, John Snow
[-- Attachment #1: Type: text/plain, Size: 3619 bytes --]
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.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [Qemu-devel] [PATCH v2 1/4] block: extract AIO_WAIT_WHILE() from BlockDriverState
2018-02-14 14:06 ` Stefan Hajnoczi
@ 2018-02-14 22:31 ` Eric Blake
2018-02-15 9:27 ` Stefan Hajnoczi
0 siblings, 1 reply; 14+ messages in thread
From: Eric Blake @ 2018-02-14 22:31 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: qemu-devel, Kevin Wolf, John Snow
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
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [Qemu-devel] [PATCH v2 1/4] block: extract AIO_WAIT_WHILE() from BlockDriverState
2018-02-14 22:31 ` Eric Blake
@ 2018-02-15 9:27 ` Stefan Hajnoczi
2018-02-15 10:12 ` Kevin Wolf
0 siblings, 1 reply; 14+ messages in thread
From: Stefan Hajnoczi @ 2018-02-15 9:27 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel, Kevin Wolf, John Snow
[-- Attachment #1: Type: text/plain, Size: 1155 bytes --]
On Wed, Feb 14, 2018 at 04:31:45PM -0600, Eric Blake wrote:
> On 02/14/2018 08:06 AM, Stefan Hajnoczi wrote:
> > On Tue, Feb 13, 2018 at 10:01:06AM -0600, Eric Blake wrote:
> > 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)
Having acquired the AioContext does not make this function return true.
The semantics are:
1. Current thread is the IOThread that runs the AioContext
2. Current thread is the main loop and the AioContext is the global
AioContext.
The function tests whether the current thread is the "native" or "home"
thread for this AioContext. Perhaps we could also call it the "poller"
thread because only that thread is allowed to call aio_poll(ctx, true).
if (aio_context_in_native_thread(ctx)) {
...
} else {
...
}
What do you think?
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [Qemu-devel] [PATCH v2 1/4] block: extract AIO_WAIT_WHILE() from BlockDriverState
2018-02-15 9:27 ` Stefan Hajnoczi
@ 2018-02-15 10:12 ` Kevin Wolf
2018-02-15 14:15 ` Eric Blake
0 siblings, 1 reply; 14+ messages in thread
From: Kevin Wolf @ 2018-02-15 10:12 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: Eric Blake, qemu-devel, John Snow
[-- Attachment #1: Type: text/plain, Size: 1537 bytes --]
Am 15.02.2018 um 10:27 hat Stefan Hajnoczi geschrieben:
> On Wed, Feb 14, 2018 at 04:31:45PM -0600, Eric Blake wrote:
> > On 02/14/2018 08:06 AM, Stefan Hajnoczi wrote:
> > > On Tue, Feb 13, 2018 at 10:01:06AM -0600, Eric Blake wrote:
> > > 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)
>
> Having acquired the AioContext does not make this function return true.
> The semantics are:
> 1. Current thread is the IOThread that runs the AioContext
> 2. Current thread is the main loop and the AioContext is the global
> AioContext.
>
> The function tests whether the current thread is the "native" or "home"
> thread for this AioContext. Perhaps we could also call it the "poller"
> thread because only that thread is allowed to call aio_poll(ctx, true).
>
> if (aio_context_in_native_thread(ctx)) {
> ...
> } else {
> ...
> }
>
> What do you think?
"home" or "native" both work for me. Or if we want to keep the name
short, maybe just changing the order and s/iothread/thread/ would be
enough: bool in_aio_context_thread(AioContext *ctx) - do you think that
would still be prone to misunderstandings?
Kevin
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [Qemu-devel] [PATCH v2 1/4] block: extract AIO_WAIT_WHILE() from BlockDriverState
2018-02-15 10:12 ` Kevin Wolf
@ 2018-02-15 14:15 ` Eric Blake
0 siblings, 0 replies; 14+ messages in thread
From: Eric Blake @ 2018-02-15 14:15 UTC (permalink / raw)
To: Kevin Wolf, Stefan Hajnoczi; +Cc: qemu-devel, John Snow
On 02/15/2018 04:12 AM, Kevin Wolf wrote:
>> Having acquired the AioContext does not make this function return true.
>> The semantics are:
>> 1. Current thread is the IOThread that runs the AioContext
>> 2. Current thread is the main loop and the AioContext is the global
>> AioContext.
>>
>> The function tests whether the current thread is the "native" or "home"
>> thread for this AioContext. Perhaps we could also call it the "poller"
>> thread because only that thread is allowed to call aio_poll(ctx, true).
>>
>> if (aio_context_in_native_thread(ctx)) {
>> ...
>> } else {
>> ...
>> }
>>
>> What do you think?
>
> "home" or "native" both work for me. Or if we want to keep the name
> short, maybe just changing the order and s/iothread/thread/ would be
> enough: bool in_aio_context_thread(AioContext *ctx) - do you think that
> would still be prone to misunderstandings?
in_aio_context_home_thread() sounds slightly better than
in_aio_context_thread(), but swapping the verb makes a nice difference
where either name is better than what we currently have.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/4] block: extract AIO_WAIT_WHILE() from BlockDriverState
2018-02-13 16:01 ` Eric Blake
2018-02-14 14:06 ` Stefan Hajnoczi
@ 2018-02-14 22:33 ` Eric Blake
1 sibling, 0 replies; 14+ messages in thread
From: Eric Blake @ 2018-02-14 22:33 UTC (permalink / raw)
To: Stefan Hajnoczi, qemu-devel; +Cc: Kevin Wolf, John Snow
On 02/13/2018 10:01 AM, Eric Blake wrote:
> 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.
>>
>
> 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.
Okay, based on your responses, I can now give
Reviewed-by: Eric Blake <eblake@redhat.com>
although it may still help to do followups with better documentation
and/or a rename of the confusing functions.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 14+ messages in thread
* [Qemu-devel] [PATCH v2 2/4] block: add BlockBackend->in_flight counter
2018-02-13 14:20 [Qemu-devel] [PATCH v2 0/4] block: fix blk_aio_*() segfault when blk->root == NULL Stefan Hajnoczi
2018-02-13 14:20 ` [Qemu-devel] [PATCH v2 1/4] block: extract AIO_WAIT_WHILE() from BlockDriverState Stefan Hajnoczi
@ 2018-02-13 14:21 ` Stefan Hajnoczi
2018-02-13 16:03 ` Eric Blake
2018-02-13 14:21 ` [Qemu-devel] [PATCH v2 3/4] block: test blk_aio_flush() with blk->root == NULL Stefan Hajnoczi
` (2 subsequent siblings)
4 siblings, 1 reply; 14+ messages in thread
From: Stefan Hajnoczi @ 2018-02-13 14:21 UTC (permalink / raw)
To: qemu-devel; +Cc: John Snow, mark.kanda, Kevin Wolf, Stefan Hajnoczi
BlockBackend currently relies on BlockDriverState->in_flight to track
requests for blk_drain(). There is a corner case where
BlockDriverState->in_flight cannot be used though: blk->root can be NULL
when there is no medium. This results in a segfault when the NULL
pointer is dereferenced.
Introduce a BlockBackend->in_flight counter for aio requests so it works
even when blk->root == NULL.
Based on a patch by Kevin Wolf <kwolf@redhat.com>.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
block.c | 2 +-
block/block-backend.c | 60 +++++++++++++++++++++++++++++++++++++++++++++------
2 files changed, 54 insertions(+), 8 deletions(-)
diff --git a/block.c b/block.c
index 9e4da81213..a83037c2a5 100644
--- a/block.c
+++ b/block.c
@@ -4713,7 +4713,7 @@ out:
AioContext *bdrv_get_aio_context(BlockDriverState *bs)
{
- return bs->aio_context;
+ return bs ? bs->aio_context : qemu_get_aio_context();
}
AioWait *bdrv_get_aio_wait(BlockDriverState *bs)
diff --git a/block/block-backend.c b/block/block-backend.c
index 0266ac990b..a775a3dd2f 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -73,6 +73,14 @@ struct BlockBackend {
int quiesce_counter;
VMChangeStateEntry *vmsh;
bool force_allow_inactivate;
+
+ /* Number of in-flight aio requests. BlockDriverState also counts
+ * in-flight requests but aio requests can exist even when blk->root is
+ * NULL, so we cannot rely on its counter for that case.
+ * Accessed with atomic ops.
+ */
+ unsigned int in_flight;
+ AioWait wait;
};
typedef struct BlockBackendAIOCB {
@@ -1225,11 +1233,22 @@ int blk_make_zero(BlockBackend *blk, BdrvRequestFlags flags)
return bdrv_make_zero(blk->root, flags);
}
+static void blk_inc_in_flight(BlockBackend *blk)
+{
+ atomic_inc(&blk->in_flight);
+}
+
+static void blk_dec_in_flight(BlockBackend *blk)
+{
+ atomic_dec(&blk->in_flight);
+ aio_wait_kick(&blk->wait);
+}
+
static void error_callback_bh(void *opaque)
{
struct BlockBackendAIOCB *acb = opaque;
- bdrv_dec_in_flight(acb->common.bs);
+ blk_dec_in_flight(acb->blk);
acb->common.cb(acb->common.opaque, acb->ret);
qemu_aio_unref(acb);
}
@@ -1240,7 +1259,7 @@ BlockAIOCB *blk_abort_aio_request(BlockBackend *blk,
{
struct BlockBackendAIOCB *acb;
- bdrv_inc_in_flight(blk_bs(blk));
+ blk_inc_in_flight(blk);
acb = blk_aio_get(&block_backend_aiocb_info, blk, cb, opaque);
acb->blk = blk;
acb->ret = ret;
@@ -1263,7 +1282,7 @@ static const AIOCBInfo blk_aio_em_aiocb_info = {
static void blk_aio_complete(BlkAioEmAIOCB *acb)
{
if (acb->has_returned) {
- bdrv_dec_in_flight(acb->common.bs);
+ blk_dec_in_flight(acb->rwco.blk);
acb->common.cb(acb->common.opaque, acb->rwco.ret);
qemu_aio_unref(acb);
}
@@ -1284,7 +1303,7 @@ static BlockAIOCB *blk_aio_prwv(BlockBackend *blk, int64_t offset, int bytes,
BlkAioEmAIOCB *acb;
Coroutine *co;
- bdrv_inc_in_flight(blk_bs(blk));
+ blk_inc_in_flight(blk);
acb = blk_aio_get(&blk_aio_em_aiocb_info, blk, cb, opaque);
acb->rwco = (BlkRwCo) {
.blk = blk,
@@ -1521,14 +1540,41 @@ int blk_flush(BlockBackend *blk)
void blk_drain(BlockBackend *blk)
{
- if (blk_bs(blk)) {
- bdrv_drain(blk_bs(blk));
+ BlockDriverState *bs = blk_bs(blk);
+
+ if (bs) {
+ bdrv_drained_begin(bs);
+ }
+
+ /* We may have -ENOMEDIUM completions in flight */
+ AIO_WAIT_WHILE(&blk->wait,
+ blk_get_aio_context(blk),
+ atomic_mb_read(&blk->in_flight) > 0);
+
+ if (bs) {
+ bdrv_drained_end(bs);
}
}
void blk_drain_all(void)
{
- bdrv_drain_all();
+ BlockBackend *blk = NULL;
+
+ bdrv_drain_all_begin();
+
+ while ((blk = blk_all_next(blk)) != NULL) {
+ AioContext *ctx = blk_get_aio_context(blk);
+
+ aio_context_acquire(ctx);
+
+ /* We may have -ENOMEDIUM completions in flight */
+ AIO_WAIT_WHILE(&blk->wait, ctx,
+ atomic_mb_read(&blk->in_flight) > 0);
+
+ aio_context_release(ctx);
+ }
+
+ bdrv_drain_all_end();
}
void blk_set_on_error(BlockBackend *blk, BlockdevOnError on_read_error,
--
2.14.3
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [Qemu-devel] [PATCH v2 2/4] block: add BlockBackend->in_flight counter
2018-02-13 14:21 ` [Qemu-devel] [PATCH v2 2/4] block: add BlockBackend->in_flight counter Stefan Hajnoczi
@ 2018-02-13 16:03 ` Eric Blake
0 siblings, 0 replies; 14+ messages in thread
From: Eric Blake @ 2018-02-13 16:03 UTC (permalink / raw)
To: Stefan Hajnoczi, qemu-devel; +Cc: Kevin Wolf, John Snow
On 02/13/2018 08:21 AM, Stefan Hajnoczi wrote:
> BlockBackend currently relies on BlockDriverState->in_flight to track
> requests for blk_drain(). There is a corner case where
> BlockDriverState->in_flight cannot be used though: blk->root can be NULL
> when there is no medium. This results in a segfault when the NULL
> pointer is dereferenced.
>
> Introduce a BlockBackend->in_flight counter for aio requests so it works
> even when blk->root == NULL.
>
> Based on a patch by Kevin Wolf <kwolf@redhat.com>.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
> block.c | 2 +-
> block/block-backend.c | 60 +++++++++++++++++++++++++++++++++++++++++++++------
> 2 files changed, 54 insertions(+), 8 deletions(-)
>
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 14+ messages in thread
* [Qemu-devel] [PATCH v2 3/4] block: test blk_aio_flush() with blk->root == NULL
2018-02-13 14:20 [Qemu-devel] [PATCH v2 0/4] block: fix blk_aio_*() segfault when blk->root == NULL Stefan Hajnoczi
2018-02-13 14:20 ` [Qemu-devel] [PATCH v2 1/4] block: extract AIO_WAIT_WHILE() from BlockDriverState Stefan Hajnoczi
2018-02-13 14:21 ` [Qemu-devel] [PATCH v2 2/4] block: add BlockBackend->in_flight counter Stefan Hajnoczi
@ 2018-02-13 14:21 ` Stefan Hajnoczi
2018-02-13 14:21 ` [Qemu-devel] [PATCH v2 4/4] Revert "IDE: Do not flush empty CDROM drives" Stefan Hajnoczi
2018-02-15 9:28 ` [Qemu-devel] [PATCH v2 0/4] block: fix blk_aio_*() segfault when blk->root == NULL Stefan Hajnoczi
4 siblings, 0 replies; 14+ messages in thread
From: Stefan Hajnoczi @ 2018-02-13 14:21 UTC (permalink / raw)
To: qemu-devel; +Cc: John Snow, mark.kanda, Kevin Wolf, Stefan Hajnoczi
From: Kevin Wolf <kwolf@redhat.com>
This patch adds test cases for the scenario where blk_aio_flush() is
called on a BlockBackend with no root. Calling drain afterwards should
complete the requests with -ENOMEDIUM.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
tests/Makefile.include | 2 ++
tests/test-block-backend.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 84 insertions(+)
create mode 100644 tests/test-block-backend.c
diff --git a/tests/Makefile.include b/tests/Makefile.include
index f41da235ae..1a457c29bc 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -83,6 +83,7 @@ gcov-files-test-hbitmap-y = blockjob.c
check-unit-y += tests/test-bdrv-drain$(EXESUF)
check-unit-y += tests/test-blockjob$(EXESUF)
check-unit-y += tests/test-blockjob-txn$(EXESUF)
+check-unit-y += tests/test-block-backend$(EXESUF)
check-unit-y += tests/test-x86-cpuid$(EXESUF)
# all code tested by test-x86-cpuid is inside topology.h
gcov-files-test-x86-cpuid-y =
@@ -609,6 +610,7 @@ tests/test-throttle$(EXESUF): tests/test-throttle.o $(test-block-obj-y)
tests/test-bdrv-drain$(EXESUF): tests/test-bdrv-drain.o $(test-block-obj-y) $(test-util-obj-y)
tests/test-blockjob$(EXESUF): tests/test-blockjob.o $(test-block-obj-y) $(test-util-obj-y)
tests/test-blockjob-txn$(EXESUF): tests/test-blockjob-txn.o $(test-block-obj-y) $(test-util-obj-y)
+tests/test-block-backend$(EXESUF): tests/test-block-backend.o $(test-block-obj-y) $(test-util-obj-y)
tests/test-thread-pool$(EXESUF): tests/test-thread-pool.o $(test-block-obj-y)
tests/test-iov$(EXESUF): tests/test-iov.o $(test-util-obj-y)
tests/test-hbitmap$(EXESUF): tests/test-hbitmap.o $(test-util-obj-y) $(test-crypto-obj-y)
diff --git a/tests/test-block-backend.c b/tests/test-block-backend.c
new file mode 100644
index 0000000000..fd59f02bd0
--- /dev/null
+++ b/tests/test-block-backend.c
@@ -0,0 +1,82 @@
+/*
+ * BlockBackend tests
+ *
+ * Copyright (c) 2017 Kevin Wolf <kwolf@redhat.com>
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "qemu/osdep.h"
+#include "block/block.h"
+#include "sysemu/block-backend.h"
+#include "qapi/error.h"
+
+static void test_drain_aio_error_flush_cb(void *opaque, int ret)
+{
+ bool *completed = opaque;
+
+ g_assert(ret == -ENOMEDIUM);
+ *completed = true;
+}
+
+static void test_drain_aio_error(void)
+{
+ BlockBackend *blk = blk_new(BLK_PERM_ALL, BLK_PERM_ALL);
+ BlockAIOCB *acb;
+ bool completed = false;
+
+ acb = blk_aio_flush(blk, test_drain_aio_error_flush_cb, &completed);
+ g_assert(acb != NULL);
+ g_assert(completed == false);
+
+ blk_drain(blk);
+ g_assert(completed == true);
+
+ blk_unref(blk);
+}
+
+static void test_drain_all_aio_error(void)
+{
+ BlockBackend *blk = blk_new(BLK_PERM_ALL, BLK_PERM_ALL);
+ BlockAIOCB *acb;
+ bool completed = false;
+
+ acb = blk_aio_flush(blk, test_drain_aio_error_flush_cb, &completed);
+ g_assert(acb != NULL);
+ g_assert(completed == false);
+
+ blk_drain_all();
+ g_assert(completed == true);
+
+ blk_unref(blk);
+}
+
+int main(int argc, char **argv)
+{
+ bdrv_init();
+ qemu_init_main_loop(&error_abort);
+
+ g_test_init(&argc, &argv, NULL);
+
+ g_test_add_func("/block-backend/drain_aio_error", test_drain_aio_error);
+ g_test_add_func("/block-backend/drain_all_aio_error",
+ test_drain_all_aio_error);
+
+ return g_test_run();
+}
--
2.14.3
^ permalink raw reply related [flat|nested] 14+ messages in thread* [Qemu-devel] [PATCH v2 4/4] Revert "IDE: Do not flush empty CDROM drives"
2018-02-13 14:20 [Qemu-devel] [PATCH v2 0/4] block: fix blk_aio_*() segfault when blk->root == NULL Stefan Hajnoczi
` (2 preceding siblings ...)
2018-02-13 14:21 ` [Qemu-devel] [PATCH v2 3/4] block: test blk_aio_flush() with blk->root == NULL Stefan Hajnoczi
@ 2018-02-13 14:21 ` Stefan Hajnoczi
2018-02-15 9:28 ` [Qemu-devel] [PATCH v2 0/4] block: fix blk_aio_*() segfault when blk->root == NULL Stefan Hajnoczi
4 siblings, 0 replies; 14+ messages in thread
From: Stefan Hajnoczi @ 2018-02-13 14:21 UTC (permalink / raw)
To: qemu-devel; +Cc: John Snow, mark.kanda, Kevin Wolf, Stefan Hajnoczi
This reverts commit 4da97120d51a4383aa96d741a2b837f8c4bbcd0b.
blk_aio_flush() now handles the blk->root == NULL case, so we no longer
need this workaround.
Cc: John Snow <jsnow@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
hw/ide/core.c | 10 +---------
1 file changed, 1 insertion(+), 9 deletions(-)
diff --git a/hw/ide/core.c b/hw/ide/core.c
index 257b429381..139c843514 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -1087,15 +1087,7 @@ static void ide_flush_cache(IDEState *s)
s->status |= BUSY_STAT;
ide_set_retry(s);
block_acct_start(blk_get_stats(s->blk), &s->acct, 0, BLOCK_ACCT_FLUSH);
-
- if (blk_bs(s->blk)) {
- s->pio_aiocb = blk_aio_flush(s->blk, ide_flush_cb, s);
- } else {
- /* XXX blk_aio_flush() crashes when blk_bs(blk) is NULL, remove this
- * temporary workaround when blk_aio_*() functions handle NULL blk_bs.
- */
- ide_flush_cb(s, 0);
- }
+ s->pio_aiocb = blk_aio_flush(s->blk, ide_flush_cb, s);
}
static void ide_cfata_metadata_inquiry(IDEState *s)
--
2.14.3
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [Qemu-devel] [PATCH v2 0/4] block: fix blk_aio_*() segfault when blk->root == NULL
2018-02-13 14:20 [Qemu-devel] [PATCH v2 0/4] block: fix blk_aio_*() segfault when blk->root == NULL Stefan Hajnoczi
` (3 preceding siblings ...)
2018-02-13 14:21 ` [Qemu-devel] [PATCH v2 4/4] Revert "IDE: Do not flush empty CDROM drives" Stefan Hajnoczi
@ 2018-02-15 9:28 ` Stefan Hajnoczi
4 siblings, 0 replies; 14+ messages in thread
From: Stefan Hajnoczi @ 2018-02-15 9:28 UTC (permalink / raw)
To: qemu-devel; +Cc: John Snow, mark.kanda, Kevin Wolf
[-- Attachment #1: Type: text/plain, Size: 1702 bytes --]
On Tue, Feb 13, 2018 at 02:20:58PM +0000, Stefan Hajnoczi wrote:
> v2:
> * Introduce AIO_WAIT_WHILE() since aio_poll(ctx, true) is not allowed [Paolo]
>
> Using bdrv_inc_in_flight(blk_bs(blk)) doesn't work since BlockBackend->root may
> be NULL.
>
> This patch series solves the issue by adding an BlockBackend->in_flight counter
> so requests can be tracked even when there is no BlockDriverState.
>
> This should fix the IDE and virtio-blk segfaults that have been encountered
> when there is no BlockDriverState.
>
> The patch is based on work by Kevin Wolf.
>
> Kevin Wolf (1):
> block: test blk_aio_flush() with blk->root == NULL
>
> Stefan Hajnoczi (3):
> block: extract AIO_WAIT_WHILE() from BlockDriverState
> block: add BlockBackend->in_flight counter
> Revert "IDE: Do not flush empty CDROM drives"
>
> tests/Makefile.include | 2 +
> util/Makefile.objs | 2 +-
> include/block/aio-wait.h | 116 +++++++++++++++++++++++++++++++++++++++++++++
> include/block/block.h | 40 +++-------------
> include/block/block_int.h | 7 ++-
> block.c | 7 ++-
> block/block-backend.c | 60 ++++++++++++++++++++---
> block/io.c | 10 +---
> hw/ide/core.c | 10 +---
> tests/test-block-backend.c | 82 ++++++++++++++++++++++++++++++++
> util/aio-wait.c | 40 ++++++++++++++++
> 11 files changed, 313 insertions(+), 63 deletions(-)
> create mode 100644 include/block/aio-wait.h
> create mode 100644 tests/test-block-backend.c
> create mode 100644 util/aio-wait.c
Eric has posted R-b for all patches.
Kevin or Paolo: Are you happy with this series?
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread