* [Qemu-devel] [PULL 01/35] test-bdrv-drain: bdrv_drain() works with cross-AioContext events
2018-06-18 16:44 [Qemu-devel] [PULL 00/35] Block layer patches Kevin Wolf
@ 2018-06-18 16:44 ` Kevin Wolf
2018-06-18 16:44 ` [Qemu-devel] [PULL 02/35] block: Use bdrv_do_drain_begin/end in bdrv_drain_all() Kevin Wolf
` (35 subsequent siblings)
36 siblings, 0 replies; 47+ messages in thread
From: Kevin Wolf @ 2018-06-18 16:44 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel
As long as nobody keeps the other I/O thread from working, there is no
reason why bdrv_drain() wouldn't work with cross-AioContext events. The
key is that the root request we're waiting for is in the AioContext
we're polling (which it always is for bdrv_drain()) so that aio_poll()
is woken up in the end.
Add a test case that shows that it works. Remove the comment in
bdrv_drain() that claims otherwise.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
block/io.c | 4 --
tests/test-bdrv-drain.c | 187 +++++++++++++++++++++++++++++++++++++++++++++++-
2 files changed, 186 insertions(+), 5 deletions(-)
diff --git a/block/io.c b/block/io.c
index b7beaeeb9f..5f286bcedb 100644
--- a/block/io.c
+++ b/block/io.c
@@ -370,10 +370,6 @@ void bdrv_unapply_subtree_drain(BdrvChild *child, BlockDriverState *old_parent)
*
* Note that unlike bdrv_drain_all(), the caller must hold the BlockDriverState
* AioContext.
- *
- * Only this BlockDriverState's AioContext is run, so in-flight requests must
- * not depend on events in other AioContexts. In that case, use
- * bdrv_drain_all() instead.
*/
void coroutine_fn bdrv_co_drain(BlockDriverState *bs)
{
diff --git a/tests/test-bdrv-drain.c b/tests/test-bdrv-drain.c
index a11c4cfbf2..fb68539d17 100644
--- a/tests/test-bdrv-drain.c
+++ b/tests/test-bdrv-drain.c
@@ -27,9 +27,13 @@
#include "block/blockjob_int.h"
#include "sysemu/block-backend.h"
#include "qapi/error.h"
+#include "iothread.h"
+
+static QemuEvent done_event;
typedef struct BDRVTestState {
int drain_count;
+ AioContext *bh_indirection_ctx;
} BDRVTestState;
static void coroutine_fn bdrv_test_co_drain_begin(BlockDriverState *bs)
@@ -50,16 +54,29 @@ static void bdrv_test_close(BlockDriverState *bs)
g_assert_cmpint(s->drain_count, >, 0);
}
+static void co_reenter_bh(void *opaque)
+{
+ aio_co_wake(opaque);
+}
+
static int coroutine_fn bdrv_test_co_preadv(BlockDriverState *bs,
uint64_t offset, uint64_t bytes,
QEMUIOVector *qiov, int flags)
{
+ BDRVTestState *s = bs->opaque;
+
/* We want this request to stay until the polling loop in drain waits for
* it to complete. We need to sleep a while as bdrv_drain_invoke() comes
* first and polls its result, too, but it shouldn't accidentally complete
* this request yet. */
qemu_co_sleep_ns(QEMU_CLOCK_REALTIME, 100000);
+ if (s->bh_indirection_ctx) {
+ aio_bh_schedule_oneshot(s->bh_indirection_ctx, co_reenter_bh,
+ qemu_coroutine_self());
+ qemu_coroutine_yield();
+ }
+
return 0;
}
@@ -490,6 +507,164 @@ static void test_graph_change(void)
blk_unref(blk_b);
}
+struct test_iothread_data {
+ BlockDriverState *bs;
+ enum drain_type drain_type;
+ int *aio_ret;
+};
+
+static void test_iothread_drain_entry(void *opaque)
+{
+ struct test_iothread_data *data = opaque;
+
+ aio_context_acquire(bdrv_get_aio_context(data->bs));
+ do_drain_begin(data->drain_type, data->bs);
+ g_assert_cmpint(*data->aio_ret, ==, 0);
+ do_drain_end(data->drain_type, data->bs);
+ aio_context_release(bdrv_get_aio_context(data->bs));
+
+ qemu_event_set(&done_event);
+}
+
+static void test_iothread_aio_cb(void *opaque, int ret)
+{
+ int *aio_ret = opaque;
+ *aio_ret = ret;
+ qemu_event_set(&done_event);
+}
+
+/*
+ * Starts an AIO request on a BDS that runs in the AioContext of iothread 1.
+ * The request involves a BH on iothread 2 before it can complete.
+ *
+ * @drain_thread = 0 means that do_drain_begin/end are called from the main
+ * thread, @drain_thread = 1 means that they are called from iothread 1. Drain
+ * for this BDS cannot be called from iothread 2 because only the main thread
+ * may do cross-AioContext polling.
+ */
+static void test_iothread_common(enum drain_type drain_type, int drain_thread)
+{
+ BlockBackend *blk;
+ BlockDriverState *bs;
+ BDRVTestState *s;
+ BlockAIOCB *acb;
+ int aio_ret;
+ struct test_iothread_data data;
+
+ IOThread *a = iothread_new();
+ IOThread *b = iothread_new();
+ AioContext *ctx_a = iothread_get_aio_context(a);
+ AioContext *ctx_b = iothread_get_aio_context(b);
+
+ QEMUIOVector qiov;
+ struct iovec iov = {
+ .iov_base = NULL,
+ .iov_len = 0,
+ };
+ qemu_iovec_init_external(&qiov, &iov, 1);
+
+ /* bdrv_drain_all() may only be called from the main loop thread */
+ if (drain_type == BDRV_DRAIN_ALL && drain_thread != 0) {
+ goto out;
+ }
+
+ blk = blk_new(BLK_PERM_ALL, BLK_PERM_ALL);
+ bs = bdrv_new_open_driver(&bdrv_test, "test-node", BDRV_O_RDWR,
+ &error_abort);
+ s = bs->opaque;
+ blk_insert_bs(blk, bs, &error_abort);
+
+ blk_set_aio_context(blk, ctx_a);
+ aio_context_acquire(ctx_a);
+
+ s->bh_indirection_ctx = ctx_b;
+
+ aio_ret = -EINPROGRESS;
+ if (drain_thread == 0) {
+ acb = blk_aio_preadv(blk, 0, &qiov, 0, test_iothread_aio_cb, &aio_ret);
+ } else {
+ acb = blk_aio_preadv(blk, 0, &qiov, 0, aio_ret_cb, &aio_ret);
+ }
+ g_assert(acb != NULL);
+ g_assert_cmpint(aio_ret, ==, -EINPROGRESS);
+
+ aio_context_release(ctx_a);
+
+ data = (struct test_iothread_data) {
+ .bs = bs,
+ .drain_type = drain_type,
+ .aio_ret = &aio_ret,
+ };
+
+ switch (drain_thread) {
+ case 0:
+ if (drain_type != BDRV_DRAIN_ALL) {
+ aio_context_acquire(ctx_a);
+ }
+
+ /* The request is running on the IOThread a. Draining its block device
+ * will make sure that it has completed as far as the BDS is concerned,
+ * but the drain in this thread can continue immediately after
+ * bdrv_dec_in_flight() and aio_ret might be assigned only slightly
+ * later. */
+ qemu_event_reset(&done_event);
+ do_drain_begin(drain_type, bs);
+ g_assert_cmpint(bs->in_flight, ==, 0);
+
+ if (drain_type != BDRV_DRAIN_ALL) {
+ aio_context_release(ctx_a);
+ }
+ qemu_event_wait(&done_event);
+ if (drain_type != BDRV_DRAIN_ALL) {
+ aio_context_acquire(ctx_a);
+ }
+
+ g_assert_cmpint(aio_ret, ==, 0);
+ do_drain_end(drain_type, bs);
+
+ if (drain_type != BDRV_DRAIN_ALL) {
+ aio_context_release(ctx_a);
+ }
+ break;
+ case 1:
+ qemu_event_reset(&done_event);
+ aio_bh_schedule_oneshot(ctx_a, test_iothread_drain_entry, &data);
+ qemu_event_wait(&done_event);
+ break;
+ default:
+ g_assert_not_reached();
+ }
+
+ aio_context_acquire(ctx_a);
+ blk_set_aio_context(blk, qemu_get_aio_context());
+ aio_context_release(ctx_a);
+
+ bdrv_unref(bs);
+ blk_unref(blk);
+
+out:
+ iothread_join(a);
+ iothread_join(b);
+}
+
+static void test_iothread_drain_all(void)
+{
+ test_iothread_common(BDRV_DRAIN_ALL, 0);
+ test_iothread_common(BDRV_DRAIN_ALL, 1);
+}
+
+static void test_iothread_drain(void)
+{
+ test_iothread_common(BDRV_DRAIN, 0);
+ test_iothread_common(BDRV_DRAIN, 1);
+}
+
+static void test_iothread_drain_subtree(void)
+{
+ test_iothread_common(BDRV_SUBTREE_DRAIN, 0);
+ test_iothread_common(BDRV_SUBTREE_DRAIN, 1);
+}
+
typedef struct TestBlockJob {
BlockJob common;
@@ -618,10 +793,13 @@ static void test_blockjob_drain_subtree(void)
int main(int argc, char **argv)
{
+ int ret;
+
bdrv_init();
qemu_init_main_loop(&error_abort);
g_test_init(&argc, &argv, NULL);
+ qemu_event_init(&done_event, false);
g_test_add_func("/bdrv-drain/driver-cb/drain_all", test_drv_cb_drain_all);
g_test_add_func("/bdrv-drain/driver-cb/drain", test_drv_cb_drain);
@@ -648,10 +826,17 @@ int main(int argc, char **argv)
g_test_add_func("/bdrv-drain/multiparent", test_multiparent);
g_test_add_func("/bdrv-drain/graph-change", test_graph_change);
+ g_test_add_func("/bdrv-drain/iothread/drain_all", test_iothread_drain_all);
+ g_test_add_func("/bdrv-drain/iothread/drain", test_iothread_drain);
+ g_test_add_func("/bdrv-drain/iothread/drain_subtree",
+ test_iothread_drain_subtree);
+
g_test_add_func("/bdrv-drain/blockjob/drain_all", test_blockjob_drain_all);
g_test_add_func("/bdrv-drain/blockjob/drain", test_blockjob_drain);
g_test_add_func("/bdrv-drain/blockjob/drain_subtree",
test_blockjob_drain_subtree);
- return g_test_run();
+ ret = g_test_run();
+ qemu_event_destroy(&done_event);
+ return ret;
}
--
2.13.6
^ permalink raw reply related [flat|nested] 47+ messages in thread* [Qemu-devel] [PULL 02/35] block: Use bdrv_do_drain_begin/end in bdrv_drain_all()
2018-06-18 16:44 [Qemu-devel] [PULL 00/35] Block layer patches Kevin Wolf
2018-06-18 16:44 ` [Qemu-devel] [PULL 01/35] test-bdrv-drain: bdrv_drain() works with cross-AioContext events Kevin Wolf
@ 2018-06-18 16:44 ` Kevin Wolf
2018-06-18 16:44 ` [Qemu-devel] [PULL 03/35] block: Remove 'recursive' parameter from bdrv_drain_invoke() Kevin Wolf
` (34 subsequent siblings)
36 siblings, 0 replies; 47+ messages in thread
From: Kevin Wolf @ 2018-06-18 16:44 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel
bdrv_do_drain_begin/end() implement already everything that
bdrv_drain_all_begin/end() need and currently still do manually: Disable
external events, call parent drain callbacks, call block driver
callbacks.
It also does two more things:
The first is incrementing bs->quiesce_counter. bdrv_drain_all() already
stood out in the test case by behaving different from the other drain
variants. Adding this is not only safe, but in fact a bug fix.
The second is calling bdrv_drain_recurse(). We already do that later in
the same function in a loop, so basically doing an early first iteration
doesn't hurt.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
---
block/io.c | 10 ++--------
tests/test-bdrv-drain.c | 14 ++++----------
2 files changed, 6 insertions(+), 18 deletions(-)
diff --git a/block/io.c b/block/io.c
index 5f286bcedb..b355009f2c 100644
--- a/block/io.c
+++ b/block/io.c
@@ -413,11 +413,8 @@ void bdrv_drain_all_begin(void)
for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
AioContext *aio_context = bdrv_get_aio_context(bs);
- /* Stop things in parent-to-child order */
aio_context_acquire(aio_context);
- aio_disable_external(aio_context);
- bdrv_parent_drained_begin(bs, NULL);
- bdrv_drain_invoke(bs, true, true);
+ bdrv_do_drained_begin(bs, true, NULL);
aio_context_release(aio_context);
if (!g_slist_find(aio_ctxs, aio_context)) {
@@ -458,11 +455,8 @@ void bdrv_drain_all_end(void)
for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
AioContext *aio_context = bdrv_get_aio_context(bs);
- /* Re-enable things in child-to-parent order */
aio_context_acquire(aio_context);
- bdrv_drain_invoke(bs, false, true);
- bdrv_parent_drained_end(bs, NULL);
- aio_enable_external(aio_context);
+ bdrv_do_drained_end(bs, true, NULL);
aio_context_release(aio_context);
}
}
diff --git a/tests/test-bdrv-drain.c b/tests/test-bdrv-drain.c
index fb68539d17..c7f107142c 100644
--- a/tests/test-bdrv-drain.c
+++ b/tests/test-bdrv-drain.c
@@ -276,8 +276,7 @@ static void test_quiesce_common(enum drain_type drain_type, bool recursive)
static void test_quiesce_drain_all(void)
{
- // XXX drain_all doesn't quiesce
- //test_quiesce_common(BDRV_DRAIN_ALL, true);
+ test_quiesce_common(BDRV_DRAIN_ALL, true);
}
static void test_quiesce_drain(void)
@@ -319,12 +318,7 @@ static void test_nested(void)
for (outer = 0; outer < DRAIN_TYPE_MAX; outer++) {
for (inner = 0; inner < DRAIN_TYPE_MAX; inner++) {
- /* XXX bdrv_drain_all() doesn't increase the quiesce_counter */
- int bs_quiesce = (outer != BDRV_DRAIN_ALL) +
- (inner != BDRV_DRAIN_ALL);
- int backing_quiesce = (outer == BDRV_SUBTREE_DRAIN) +
- (inner == BDRV_SUBTREE_DRAIN);
- int backing_cb_cnt = (outer != BDRV_DRAIN) +
+ int backing_quiesce = (outer != BDRV_DRAIN) +
(inner != BDRV_DRAIN);
g_assert_cmpint(bs->quiesce_counter, ==, 0);
@@ -335,10 +329,10 @@ static void test_nested(void)
do_drain_begin(outer, bs);
do_drain_begin(inner, bs);
- g_assert_cmpint(bs->quiesce_counter, ==, bs_quiesce);
+ g_assert_cmpint(bs->quiesce_counter, ==, 2);
g_assert_cmpint(backing->quiesce_counter, ==, backing_quiesce);
g_assert_cmpint(s->drain_count, ==, 2);
- g_assert_cmpint(backing_s->drain_count, ==, backing_cb_cnt);
+ g_assert_cmpint(backing_s->drain_count, ==, backing_quiesce);
do_drain_end(inner, bs);
do_drain_end(outer, bs);
--
2.13.6
^ permalink raw reply related [flat|nested] 47+ messages in thread* [Qemu-devel] [PULL 03/35] block: Remove 'recursive' parameter from bdrv_drain_invoke()
2018-06-18 16:44 [Qemu-devel] [PULL 00/35] Block layer patches Kevin Wolf
2018-06-18 16:44 ` [Qemu-devel] [PULL 01/35] test-bdrv-drain: bdrv_drain() works with cross-AioContext events Kevin Wolf
2018-06-18 16:44 ` [Qemu-devel] [PULL 02/35] block: Use bdrv_do_drain_begin/end in bdrv_drain_all() Kevin Wolf
@ 2018-06-18 16:44 ` Kevin Wolf
2018-06-18 16:44 ` [Qemu-devel] [PULL 04/35] block: Don't manually poll in bdrv_drain_all() Kevin Wolf
` (33 subsequent siblings)
36 siblings, 0 replies; 47+ messages in thread
From: Kevin Wolf @ 2018-06-18 16:44 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel
All callers pass false for the 'recursive' parameter now. Remove it.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
---
block/io.c | 13 +++----------
1 file changed, 3 insertions(+), 10 deletions(-)
diff --git a/block/io.c b/block/io.c
index b355009f2c..b75d68886a 100644
--- a/block/io.c
+++ b/block/io.c
@@ -168,9 +168,8 @@ static void coroutine_fn bdrv_drain_invoke_entry(void *opaque)
}
/* Recursively call BlockDriver.bdrv_co_drain_begin/end callbacks */
-static void bdrv_drain_invoke(BlockDriverState *bs, bool begin, bool recursive)
+static void bdrv_drain_invoke(BlockDriverState *bs, bool begin)
{
- BdrvChild *child, *tmp;
BdrvCoDrainData data = { .bs = bs, .done = false, .begin = begin};
if (!bs->drv || (begin && !bs->drv->bdrv_co_drain_begin) ||
@@ -181,12 +180,6 @@ static void bdrv_drain_invoke(BlockDriverState *bs, bool begin, bool recursive)
data.co = qemu_coroutine_create(bdrv_drain_invoke_entry, &data);
bdrv_coroutine_enter(bs, data.co);
BDRV_POLL_WHILE(bs, !data.done);
-
- if (recursive) {
- QLIST_FOREACH_SAFE(child, &bs->children, next, tmp) {
- bdrv_drain_invoke(child->bs, begin, true);
- }
- }
}
static bool bdrv_drain_recurse(BlockDriverState *bs)
@@ -287,7 +280,7 @@ void bdrv_do_drained_begin(BlockDriverState *bs, bool recursive,
}
bdrv_parent_drained_begin(bs, parent);
- bdrv_drain_invoke(bs, true, false);
+ bdrv_drain_invoke(bs, true);
bdrv_drain_recurse(bs);
if (recursive) {
@@ -322,7 +315,7 @@ void bdrv_do_drained_end(BlockDriverState *bs, bool recursive,
old_quiesce_counter = atomic_fetch_dec(&bs->quiesce_counter);
/* Re-enable things in child-to-parent order */
- bdrv_drain_invoke(bs, false, false);
+ bdrv_drain_invoke(bs, false);
bdrv_parent_drained_end(bs, parent);
if (old_quiesce_counter == 1) {
aio_enable_external(bdrv_get_aio_context(bs));
--
2.13.6
^ permalink raw reply related [flat|nested] 47+ messages in thread* [Qemu-devel] [PULL 04/35] block: Don't manually poll in bdrv_drain_all()
2018-06-18 16:44 [Qemu-devel] [PULL 00/35] Block layer patches Kevin Wolf
` (2 preceding siblings ...)
2018-06-18 16:44 ` [Qemu-devel] [PULL 03/35] block: Remove 'recursive' parameter from bdrv_drain_invoke() Kevin Wolf
@ 2018-06-18 16:44 ` Kevin Wolf
2018-06-18 16:44 ` [Qemu-devel] [PULL 05/35] tests/test-bdrv-drain: bdrv_drain_all() works in coroutines now Kevin Wolf
` (32 subsequent siblings)
36 siblings, 0 replies; 47+ messages in thread
From: Kevin Wolf @ 2018-06-18 16:44 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel
All involved nodes are already idle, we called bdrv_do_drain_begin() on
them.
The comment in the code suggested that this was not correct because the
completion of a request on one node could spawn a new request on a
different node (which might have been drained before, so we wouldn't
drain the new request). In reality, new requests to different nodes
aren't spawned out of nothing, but only in the context of a parent
request, and they aren't submitted to random nodes, but only to child
nodes. As long as we still poll for the completion of the parent request
(which we do), draining each root node separately is good enough.
Remove the additional polling code from bdrv_drain_all_begin() and
replace it with an assertion that all nodes are already idle after we
drained them separately.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
---
block/io.c | 41 ++++++++++++-----------------------------
1 file changed, 12 insertions(+), 29 deletions(-)
diff --git a/block/io.c b/block/io.c
index b75d68886a..983307cf03 100644
--- a/block/io.c
+++ b/block/io.c
@@ -377,6 +377,16 @@ void bdrv_drain(BlockDriverState *bs)
bdrv_drained_end(bs);
}
+static void bdrv_drain_assert_idle(BlockDriverState *bs)
+{
+ BdrvChild *child, *next;
+
+ assert(atomic_read(&bs->in_flight) == 0);
+ QLIST_FOREACH_SAFE(child, &bs->children, next, next) {
+ bdrv_drain_assert_idle(child->bs);
+ }
+}
+
/*
* Wait for pending requests to complete across all BlockDriverStates
*
@@ -391,11 +401,8 @@ void bdrv_drain(BlockDriverState *bs)
*/
void bdrv_drain_all_begin(void)
{
- /* Always run first iteration so any pending completion BHs run */
- bool waited = true;
BlockDriverState *bs;
BdrvNextIterator it;
- GSList *aio_ctxs = NULL, *ctx;
/* BDRV_POLL_WHILE() for a node can only be called from its own I/O thread
* or the main loop AioContext. We potentially use BDRV_POLL_WHILE() on
@@ -409,35 +416,11 @@ void bdrv_drain_all_begin(void)
aio_context_acquire(aio_context);
bdrv_do_drained_begin(bs, true, NULL);
aio_context_release(aio_context);
-
- if (!g_slist_find(aio_ctxs, aio_context)) {
- aio_ctxs = g_slist_prepend(aio_ctxs, aio_context);
- }
}
- /* Note that completion of an asynchronous I/O operation can trigger any
- * number of other I/O operations on other devices---for example a
- * coroutine can submit an I/O request to another device in response to
- * request completion. Therefore we must keep looping until there was no
- * more activity rather than simply draining each device independently.
- */
- while (waited) {
- waited = false;
-
- for (ctx = aio_ctxs; ctx != NULL; ctx = ctx->next) {
- AioContext *aio_context = ctx->data;
-
- aio_context_acquire(aio_context);
- for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
- if (aio_context == bdrv_get_aio_context(bs)) {
- waited |= bdrv_drain_recurse(bs);
- }
- }
- aio_context_release(aio_context);
- }
+ for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
+ bdrv_drain_assert_idle(bs);
}
-
- g_slist_free(aio_ctxs);
}
void bdrv_drain_all_end(void)
--
2.13.6
^ permalink raw reply related [flat|nested] 47+ messages in thread* [Qemu-devel] [PULL 05/35] tests/test-bdrv-drain: bdrv_drain_all() works in coroutines now
2018-06-18 16:44 [Qemu-devel] [PULL 00/35] Block layer patches Kevin Wolf
` (3 preceding siblings ...)
2018-06-18 16:44 ` [Qemu-devel] [PULL 04/35] block: Don't manually poll in bdrv_drain_all() Kevin Wolf
@ 2018-06-18 16:44 ` Kevin Wolf
2018-06-18 16:44 ` [Qemu-devel] [PULL 06/35] block: Avoid unnecessary aio_poll() in AIO_WAIT_WHILE() Kevin Wolf
` (31 subsequent siblings)
36 siblings, 0 replies; 47+ messages in thread
From: Kevin Wolf @ 2018-06-18 16:44 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel
Since we use bdrv_do_drained_begin/end() for bdrv_drain_all_begin/end(),
coroutine context is automatically left with a BH, preventing the
deadlocks that made bdrv_drain_all*() unsafe in coroutine context. Now
that we even removed the old polling code as dead code, it's obvious
that it's compatible now.
Enable the coroutine test cases for bdrv_drain_all().
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
---
tests/test-bdrv-drain.c | 16 ++++++++++++++--
1 file changed, 14 insertions(+), 2 deletions(-)
diff --git a/tests/test-bdrv-drain.c b/tests/test-bdrv-drain.c
index c7f107142c..cc03bc171b 100644
--- a/tests/test-bdrv-drain.c
+++ b/tests/test-bdrv-drain.c
@@ -233,6 +233,11 @@ static void test_drv_cb_drain_subtree(void)
test_drv_cb_common(BDRV_SUBTREE_DRAIN, true);
}
+static void test_drv_cb_co_drain_all(void)
+{
+ call_in_coroutine(test_drv_cb_drain_all);
+}
+
static void test_drv_cb_co_drain(void)
{
call_in_coroutine(test_drv_cb_drain);
@@ -289,6 +294,11 @@ static void test_quiesce_drain_subtree(void)
test_quiesce_common(BDRV_SUBTREE_DRAIN, true);
}
+static void test_quiesce_co_drain_all(void)
+{
+ call_in_coroutine(test_quiesce_drain_all);
+}
+
static void test_quiesce_co_drain(void)
{
call_in_coroutine(test_quiesce_drain);
@@ -800,7 +810,8 @@ int main(int argc, char **argv)
g_test_add_func("/bdrv-drain/driver-cb/drain_subtree",
test_drv_cb_drain_subtree);
- // XXX bdrv_drain_all() doesn't work in coroutine context
+ g_test_add_func("/bdrv-drain/driver-cb/co/drain_all",
+ test_drv_cb_co_drain_all);
g_test_add_func("/bdrv-drain/driver-cb/co/drain", test_drv_cb_co_drain);
g_test_add_func("/bdrv-drain/driver-cb/co/drain_subtree",
test_drv_cb_co_drain_subtree);
@@ -811,7 +822,8 @@ int main(int argc, char **argv)
g_test_add_func("/bdrv-drain/quiesce/drain_subtree",
test_quiesce_drain_subtree);
- // XXX bdrv_drain_all() doesn't work in coroutine context
+ g_test_add_func("/bdrv-drain/quiesce/co/drain_all",
+ test_quiesce_co_drain_all);
g_test_add_func("/bdrv-drain/quiesce/co/drain", test_quiesce_co_drain);
g_test_add_func("/bdrv-drain/quiesce/co/drain_subtree",
test_quiesce_co_drain_subtree);
--
2.13.6
^ permalink raw reply related [flat|nested] 47+ messages in thread* [Qemu-devel] [PULL 06/35] block: Avoid unnecessary aio_poll() in AIO_WAIT_WHILE()
2018-06-18 16:44 [Qemu-devel] [PULL 00/35] Block layer patches Kevin Wolf
` (4 preceding siblings ...)
2018-06-18 16:44 ` [Qemu-devel] [PULL 05/35] tests/test-bdrv-drain: bdrv_drain_all() works in coroutines now Kevin Wolf
@ 2018-06-18 16:44 ` Kevin Wolf
2018-06-18 16:44 ` [Qemu-devel] [PULL 07/35] block: Really pause block jobs on drain Kevin Wolf
` (30 subsequent siblings)
36 siblings, 0 replies; 47+ messages in thread
From: Kevin Wolf @ 2018-06-18 16:44 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel
Commit 91af091f923 added an additional aio_poll() to BDRV_POLL_WHILE()
in order to make sure that all pending BHs are executed on drain. This
was the wrong place to make the fix, as it is useless overhead for all
other users of the macro and unnecessarily complicates the mechanism.
This patch effectively reverts said commit (the context has changed a
bit and the code has moved to AIO_WAIT_WHILE()) and instead polls in the
loop condition for drain.
The effect is probably hard to measure in any real-world use case
because actual I/O will dominate, but if I run only the initialisation
part of 'qemu-img convert' where it calls bdrv_block_status() for the
whole image to find out how much data there is copy, this phase actually
needs only roughly half the time after this patch.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
---
include/block/aio-wait.h | 22 ++++++++--------------
block/io.c | 11 ++++++++++-
2 files changed, 18 insertions(+), 15 deletions(-)
diff --git a/include/block/aio-wait.h b/include/block/aio-wait.h
index 8c90a2e66e..783d3678dd 100644
--- a/include/block/aio-wait.h
+++ b/include/block/aio-wait.h
@@ -73,29 +73,23 @@ typedef struct {
*/
#define AIO_WAIT_WHILE(wait, ctx, cond) ({ \
bool waited_ = false; \
- bool busy_ = true; \
AioWait *wait_ = (wait); \
AioContext *ctx_ = (ctx); \
if (in_aio_context_home_thread(ctx_)) { \
- while ((cond) || busy_) { \
- busy_ = aio_poll(ctx_, (cond)); \
- waited_ |= !!(cond) | busy_; \
+ while ((cond)) { \
+ aio_poll(ctx_, true); \
+ waited_ = true; \
} \
} else { \
assert(qemu_get_current_aio_context() == \
qemu_get_aio_context()); \
/* Increment wait_->num_waiters before evaluating cond. */ \
atomic_inc(&wait_->num_waiters); \
- 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_; \
- } \
+ while ((cond)) { \
+ aio_context_release(ctx_); \
+ aio_poll(qemu_get_aio_context(), true); \
+ aio_context_acquire(ctx_); \
+ waited_ = true; \
} \
atomic_dec(&wait_->num_waiters); \
} \
diff --git a/block/io.c b/block/io.c
index 983307cf03..bc7a2d78b8 100644
--- a/block/io.c
+++ b/block/io.c
@@ -182,13 +182,22 @@ static void bdrv_drain_invoke(BlockDriverState *bs, bool begin)
BDRV_POLL_WHILE(bs, !data.done);
}
+/* Returns true if BDRV_POLL_WHILE() should go into a blocking aio_poll() */
+static bool bdrv_drain_poll(BlockDriverState *bs)
+{
+ /* Execute pending BHs first and check everything else only after the BHs
+ * have executed. */
+ while (aio_poll(bs->aio_context, false));
+ return atomic_read(&bs->in_flight);
+}
+
static bool bdrv_drain_recurse(BlockDriverState *bs)
{
BdrvChild *child, *tmp;
bool waited;
/* Wait for drained requests to finish */
- waited = BDRV_POLL_WHILE(bs, atomic_read(&bs->in_flight) > 0);
+ waited = BDRV_POLL_WHILE(bs, bdrv_drain_poll(bs));
QLIST_FOREACH_SAFE(child, &bs->children, next, tmp) {
BlockDriverState *bs = child->bs;
--
2.13.6
^ permalink raw reply related [flat|nested] 47+ messages in thread* [Qemu-devel] [PULL 07/35] block: Really pause block jobs on drain
2018-06-18 16:44 [Qemu-devel] [PULL 00/35] Block layer patches Kevin Wolf
` (5 preceding siblings ...)
2018-06-18 16:44 ` [Qemu-devel] [PULL 06/35] block: Avoid unnecessary aio_poll() in AIO_WAIT_WHILE() Kevin Wolf
@ 2018-06-18 16:44 ` Kevin Wolf
2018-06-18 16:44 ` [Qemu-devel] [PULL 08/35] block: Remove bdrv_drain_recurse() Kevin Wolf
` (29 subsequent siblings)
36 siblings, 0 replies; 47+ messages in thread
From: Kevin Wolf @ 2018-06-18 16:44 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel
We already requested that block jobs be paused in .bdrv_drained_begin,
but no guarantee was made that the job was actually inactive at the
point where bdrv_drained_begin() returned.
This introduces a new callback BdrvChildRole.bdrv_drained_poll() and
uses it to make bdrv_drain_poll() consider block jobs using the node to
be drained.
For the test case to work as expected, we have to switch from
block_job_sleep_ns() to qemu_co_sleep_ns() so that the test job is even
considered active and must be waited for when draining the node.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
include/block/block.h | 8 ++++++++
include/block/block_int.h | 7 +++++++
include/block/blockjob_int.h | 8 ++++++++
block.c | 9 +++++++++
block/io.c | 40 ++++++++++++++++++++++++++++++++++------
block/mirror.c | 8 ++++++++
blockjob.c | 23 +++++++++++++++++++++++
tests/test-bdrv-drain.c | 18 ++++++++++--------
8 files changed, 107 insertions(+), 14 deletions(-)
diff --git a/include/block/block.h b/include/block/block.h
index e677080c4e..cebbb39c6c 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -568,6 +568,14 @@ void bdrv_parent_drained_begin(BlockDriverState *bs, BdrvChild *ignore);
void bdrv_parent_drained_end(BlockDriverState *bs, BdrvChild *ignore);
/**
+ * bdrv_drain_poll:
+ *
+ * Poll for pending requests in @bs and its parents (except for
+ * @ignore_parent). This is part of bdrv_drained_begin.
+ */
+bool bdrv_drain_poll(BlockDriverState *bs, BdrvChild *ignore_parent);
+
+/**
* bdrv_drained_begin:
*
* Begin a quiesced section for exclusive access to the BDS, by disabling
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 327e478a73..1b811db8ec 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -605,6 +605,13 @@ struct BdrvChildRole {
void (*drained_begin)(BdrvChild *child);
void (*drained_end)(BdrvChild *child);
+ /*
+ * Returns whether the parent has pending requests for the child. This
+ * callback is polled after .drained_begin() has been called until all
+ * activity on the child has stopped.
+ */
+ bool (*drained_poll)(BdrvChild *child);
+
/* Notifies the parent that the child has been activated/inactivated (e.g.
* when migration is completing) and it can start/stop requesting
* permissions and doing I/O on it. */
diff --git a/include/block/blockjob_int.h b/include/block/blockjob_int.h
index 5cd50c6639..e4a318dd15 100644
--- a/include/block/blockjob_int.h
+++ b/include/block/blockjob_int.h
@@ -39,6 +39,14 @@ struct BlockJobDriver {
JobDriver job_driver;
/*
+ * Returns whether the job has pending requests for the child or will
+ * submit new requests before the next pause point. This callback is polled
+ * in the context of draining a job node after requesting that the job be
+ * paused, until all activity on the child has stopped.
+ */
+ bool (*drained_poll)(BlockJob *job);
+
+ /*
* If the callback is not NULL, it will be invoked before the job is
* resumed in a new AioContext. This is the place to move any resources
* besides job->blk to the new AioContext.
diff --git a/block.c b/block.c
index afe30caac3..8cf9cd8855 100644
--- a/block.c
+++ b/block.c
@@ -821,6 +821,12 @@ static void bdrv_child_cb_drained_begin(BdrvChild *child)
bdrv_drained_begin(bs);
}
+static bool bdrv_child_cb_drained_poll(BdrvChild *child)
+{
+ BlockDriverState *bs = child->opaque;
+ return bdrv_drain_poll(bs, NULL);
+}
+
static void bdrv_child_cb_drained_end(BdrvChild *child)
{
BlockDriverState *bs = child->opaque;
@@ -905,6 +911,7 @@ const BdrvChildRole child_file = {
.get_parent_desc = bdrv_child_get_parent_desc,
.inherit_options = bdrv_inherited_options,
.drained_begin = bdrv_child_cb_drained_begin,
+ .drained_poll = bdrv_child_cb_drained_poll,
.drained_end = bdrv_child_cb_drained_end,
.attach = bdrv_child_cb_attach,
.detach = bdrv_child_cb_detach,
@@ -929,6 +936,7 @@ const BdrvChildRole child_format = {
.get_parent_desc = bdrv_child_get_parent_desc,
.inherit_options = bdrv_inherited_fmt_options,
.drained_begin = bdrv_child_cb_drained_begin,
+ .drained_poll = bdrv_child_cb_drained_poll,
.drained_end = bdrv_child_cb_drained_end,
.attach = bdrv_child_cb_attach,
.detach = bdrv_child_cb_detach,
@@ -1048,6 +1056,7 @@ const BdrvChildRole child_backing = {
.detach = bdrv_backing_detach,
.inherit_options = bdrv_backing_options,
.drained_begin = bdrv_child_cb_drained_begin,
+ .drained_poll = bdrv_child_cb_drained_poll,
.drained_end = bdrv_child_cb_drained_end,
.inactivate = bdrv_child_cb_inactivate,
.update_filename = bdrv_backing_update_filename,
diff --git a/block/io.c b/block/io.c
index bc7a2d78b8..5820e73bb2 100644
--- a/block/io.c
+++ b/block/io.c
@@ -69,6 +69,23 @@ void bdrv_parent_drained_end(BlockDriverState *bs, BdrvChild *ignore)
}
}
+static bool bdrv_parent_drained_poll(BlockDriverState *bs, BdrvChild *ignore)
+{
+ BdrvChild *c, *next;
+ bool busy = false;
+
+ QLIST_FOREACH_SAFE(c, &bs->parents, next_parent, next) {
+ if (c == ignore) {
+ continue;
+ }
+ if (c->role->drained_poll) {
+ busy |= c->role->drained_poll(c);
+ }
+ }
+
+ return busy;
+}
+
static void bdrv_merge_limits(BlockLimits *dst, const BlockLimits *src)
{
dst->opt_transfer = MAX(dst->opt_transfer, src->opt_transfer);
@@ -183,21 +200,32 @@ static void bdrv_drain_invoke(BlockDriverState *bs, bool begin)
}
/* Returns true if BDRV_POLL_WHILE() should go into a blocking aio_poll() */
-static bool bdrv_drain_poll(BlockDriverState *bs)
+bool bdrv_drain_poll(BlockDriverState *bs, BdrvChild *ignore_parent)
+{
+ if (bdrv_parent_drained_poll(bs, ignore_parent)) {
+ return true;
+ }
+
+ return atomic_read(&bs->in_flight);
+}
+
+static bool bdrv_drain_poll_top_level(BlockDriverState *bs,
+ BdrvChild *ignore_parent)
{
/* Execute pending BHs first and check everything else only after the BHs
* have executed. */
while (aio_poll(bs->aio_context, false));
- return atomic_read(&bs->in_flight);
+
+ return bdrv_drain_poll(bs, ignore_parent);
}
-static bool bdrv_drain_recurse(BlockDriverState *bs)
+static bool bdrv_drain_recurse(BlockDriverState *bs, BdrvChild *parent)
{
BdrvChild *child, *tmp;
bool waited;
/* Wait for drained requests to finish */
- waited = BDRV_POLL_WHILE(bs, bdrv_drain_poll(bs));
+ waited = BDRV_POLL_WHILE(bs, bdrv_drain_poll_top_level(bs, parent));
QLIST_FOREACH_SAFE(child, &bs->children, next, tmp) {
BlockDriverState *bs = child->bs;
@@ -214,7 +242,7 @@ static bool bdrv_drain_recurse(BlockDriverState *bs)
*/
bdrv_ref(bs);
}
- waited |= bdrv_drain_recurse(bs);
+ waited |= bdrv_drain_recurse(bs, child);
if (in_main_loop) {
bdrv_unref(bs);
}
@@ -290,7 +318,7 @@ void bdrv_do_drained_begin(BlockDriverState *bs, bool recursive,
bdrv_parent_drained_begin(bs, parent);
bdrv_drain_invoke(bs, true);
- bdrv_drain_recurse(bs);
+ bdrv_drain_recurse(bs, parent);
if (recursive) {
bs->recursive_quiesce_counter++;
diff --git a/block/mirror.c b/block/mirror.c
index 435268bbbf..c2146c1ab3 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -964,6 +964,12 @@ static void mirror_pause(Job *job)
mirror_wait_for_all_io(s);
}
+static bool mirror_drained_poll(BlockJob *job)
+{
+ MirrorBlockJob *s = container_of(job, MirrorBlockJob, common);
+ return !!s->in_flight;
+}
+
static void mirror_attached_aio_context(BlockJob *job, AioContext *new_context)
{
MirrorBlockJob *s = container_of(job, MirrorBlockJob, common);
@@ -997,6 +1003,7 @@ static const BlockJobDriver mirror_job_driver = {
.pause = mirror_pause,
.complete = mirror_complete,
},
+ .drained_poll = mirror_drained_poll,
.attached_aio_context = mirror_attached_aio_context,
.drain = mirror_drain,
};
@@ -1012,6 +1019,7 @@ static const BlockJobDriver commit_active_job_driver = {
.pause = mirror_pause,
.complete = mirror_complete,
},
+ .drained_poll = mirror_drained_poll,
.attached_aio_context = mirror_attached_aio_context,
.drain = mirror_drain,
};
diff --git a/blockjob.c b/blockjob.c
index 0306533a2e..be5903aa96 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -155,6 +155,28 @@ static void child_job_drained_begin(BdrvChild *c)
job_pause(&job->job);
}
+static bool child_job_drained_poll(BdrvChild *c)
+{
+ BlockJob *bjob = c->opaque;
+ Job *job = &bjob->job;
+ const BlockJobDriver *drv = block_job_driver(bjob);
+
+ /* An inactive or completed job doesn't have any pending requests. Jobs
+ * with !job->busy are either already paused or have a pause point after
+ * being reentered, so no job driver code will run before they pause. */
+ if (!job->busy || job_is_completed(job) || job->deferred_to_main_loop) {
+ return false;
+ }
+
+ /* Otherwise, assume that it isn't fully stopped yet, but allow the job to
+ * override this assumption. */
+ if (drv->drained_poll) {
+ return drv->drained_poll(bjob);
+ } else {
+ return true;
+ }
+}
+
static void child_job_drained_end(BdrvChild *c)
{
BlockJob *job = c->opaque;
@@ -164,6 +186,7 @@ static void child_job_drained_end(BdrvChild *c)
static const BdrvChildRole child_job = {
.get_parent_desc = child_job_get_parent_desc,
.drained_begin = child_job_drained_begin,
+ .drained_poll = child_job_drained_poll,
.drained_end = child_job_drained_end,
.stay_at_node = true,
};
diff --git a/tests/test-bdrv-drain.c b/tests/test-bdrv-drain.c
index cc03bc171b..22d31c953e 100644
--- a/tests/test-bdrv-drain.c
+++ b/tests/test-bdrv-drain.c
@@ -686,7 +686,11 @@ static void coroutine_fn test_job_start(void *opaque)
job_transition_to_ready(&s->common.job);
while (!s->should_complete) {
- job_sleep_ns(&s->common.job, 100000);
+ /* Avoid block_job_sleep_ns() because it marks the job as !busy. We
+ * want to emulate some actual activity (probably some I/O) here so
+ * that drain has to wait for this acitivity to stop. */
+ qemu_co_sleep_ns(QEMU_CLOCK_REALTIME, 100000);
+ job_pause_point(&s->common.job);
}
job_defer_to_main_loop(&s->common.job, test_job_completed, NULL);
@@ -733,7 +737,7 @@ static void test_blockjob_common(enum drain_type drain_type)
g_assert_cmpint(job->job.pause_count, ==, 0);
g_assert_false(job->job.paused);
- g_assert_false(job->job.busy); /* We're in job_sleep_ns() */
+ g_assert_true(job->job.busy); /* We're in job_sleep_ns() */
do_drain_begin(drain_type, src);
@@ -743,15 +747,14 @@ static void test_blockjob_common(enum drain_type drain_type)
} else {
g_assert_cmpint(job->job.pause_count, ==, 1);
}
- /* XXX We don't wait until the job is actually paused. Is this okay? */
- /* g_assert_true(job->job.paused); */
+ g_assert_true(job->job.paused);
g_assert_false(job->job.busy); /* The job is paused */
do_drain_end(drain_type, src);
g_assert_cmpint(job->job.pause_count, ==, 0);
g_assert_false(job->job.paused);
- g_assert_false(job->job.busy); /* We're in job_sleep_ns() */
+ g_assert_true(job->job.busy); /* We're in qemu_co_sleep_ns() */
do_drain_begin(drain_type, target);
@@ -761,15 +764,14 @@ static void test_blockjob_common(enum drain_type drain_type)
} else {
g_assert_cmpint(job->job.pause_count, ==, 1);
}
- /* XXX We don't wait until the job is actually paused. Is this okay? */
- /* g_assert_true(job->job.paused); */
+ g_assert_true(job->job.paused);
g_assert_false(job->job.busy); /* The job is paused */
do_drain_end(drain_type, target);
g_assert_cmpint(job->job.pause_count, ==, 0);
g_assert_false(job->job.paused);
- g_assert_false(job->job.busy); /* We're in job_sleep_ns() */
+ g_assert_true(job->job.busy); /* We're in job_sleep_ns() */
ret = job_complete_sync(&job->job, &error_abort);
g_assert_cmpint(ret, ==, 0);
--
2.13.6
^ permalink raw reply related [flat|nested] 47+ messages in thread* [Qemu-devel] [PULL 08/35] block: Remove bdrv_drain_recurse()
2018-06-18 16:44 [Qemu-devel] [PULL 00/35] Block layer patches Kevin Wolf
` (6 preceding siblings ...)
2018-06-18 16:44 ` [Qemu-devel] [PULL 07/35] block: Really pause block jobs on drain Kevin Wolf
@ 2018-06-18 16:44 ` Kevin Wolf
2018-06-18 16:44 ` [Qemu-devel] [PULL 09/35] test-bdrv-drain: Add test for node deletion Kevin Wolf
` (28 subsequent siblings)
36 siblings, 0 replies; 47+ messages in thread
From: Kevin Wolf @ 2018-06-18 16:44 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel
For bdrv_drain(), recursively waiting for child node requests is
pointless because we didn't quiesce their parents, so new requests could
come in anyway. Letting the function work only on a single node makes it
more consistent.
For subtree drains and drain_all, we already have the recursion in
bdrv_do_drained_begin(), so the extra recursion doesn't add anything
either.
Remove the useless code.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
---
block/io.c | 36 +++---------------------------------
1 file changed, 3 insertions(+), 33 deletions(-)
diff --git a/block/io.c b/block/io.c
index 5820e73bb2..5f6d5eed52 100644
--- a/block/io.c
+++ b/block/io.c
@@ -219,38 +219,6 @@ static bool bdrv_drain_poll_top_level(BlockDriverState *bs,
return bdrv_drain_poll(bs, ignore_parent);
}
-static bool bdrv_drain_recurse(BlockDriverState *bs, BdrvChild *parent)
-{
- BdrvChild *child, *tmp;
- bool waited;
-
- /* Wait for drained requests to finish */
- waited = BDRV_POLL_WHILE(bs, bdrv_drain_poll_top_level(bs, parent));
-
- QLIST_FOREACH_SAFE(child, &bs->children, next, tmp) {
- BlockDriverState *bs = child->bs;
- bool in_main_loop =
- qemu_get_current_aio_context() == qemu_get_aio_context();
- assert(bs->refcnt > 0);
- if (in_main_loop) {
- /* In case the recursive bdrv_drain_recurse processes a
- * block_job_defer_to_main_loop BH and modifies the graph,
- * let's hold a reference to bs until we are done.
- *
- * IOThread doesn't have such a BH, and it is not safe to call
- * bdrv_unref without BQL, so skip doing it there.
- */
- bdrv_ref(bs);
- }
- waited |= bdrv_drain_recurse(bs, child);
- if (in_main_loop) {
- bdrv_unref(bs);
- }
- }
-
- return waited;
-}
-
static void bdrv_do_drained_begin(BlockDriverState *bs, bool recursive,
BdrvChild *parent);
static void bdrv_do_drained_end(BlockDriverState *bs, bool recursive,
@@ -318,7 +286,9 @@ void bdrv_do_drained_begin(BlockDriverState *bs, bool recursive,
bdrv_parent_drained_begin(bs, parent);
bdrv_drain_invoke(bs, true);
- bdrv_drain_recurse(bs, parent);
+
+ /* Wait for drained requests to finish */
+ BDRV_POLL_WHILE(bs, bdrv_drain_poll_top_level(bs, parent));
if (recursive) {
bs->recursive_quiesce_counter++;
--
2.13.6
^ permalink raw reply related [flat|nested] 47+ messages in thread* [Qemu-devel] [PULL 09/35] test-bdrv-drain: Add test for node deletion
2018-06-18 16:44 [Qemu-devel] [PULL 00/35] Block layer patches Kevin Wolf
` (7 preceding siblings ...)
2018-06-18 16:44 ` [Qemu-devel] [PULL 08/35] block: Remove bdrv_drain_recurse() Kevin Wolf
@ 2018-06-18 16:44 ` Kevin Wolf
2018-06-18 16:44 ` [Qemu-devel] [PULL 10/35] block: Drain recursively with a single BDRV_POLL_WHILE() Kevin Wolf
` (27 subsequent siblings)
36 siblings, 0 replies; 47+ messages in thread
From: Kevin Wolf @ 2018-06-18 16:44 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel
From: Max Reitz <mreitz@redhat.com>
This patch adds two bdrv-drain tests for what happens if some BDS goes
away during the drainage.
The basic idea is that you have a parent BDS with some child nodes.
Then, you drain one of the children. Because of that, the party who
actually owns the parent decides to (A) delete it, or (B) detach all its
children from it -- both while the child is still being drained.
A real-world case where this can happen is the mirror block job, which
may exit if you drain one of its children.
Signed-off-by: Max Reitz <mreitz@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
tests/test-bdrv-drain.c | 169 ++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 169 insertions(+)
diff --git a/tests/test-bdrv-drain.c b/tests/test-bdrv-drain.c
index 22d31c953e..e8a5515b08 100644
--- a/tests/test-bdrv-drain.c
+++ b/tests/test-bdrv-drain.c
@@ -797,6 +797,172 @@ static void test_blockjob_drain_subtree(void)
test_blockjob_common(BDRV_SUBTREE_DRAIN);
}
+
+typedef struct BDRVTestTopState {
+ BdrvChild *wait_child;
+} BDRVTestTopState;
+
+static void bdrv_test_top_close(BlockDriverState *bs)
+{
+ BdrvChild *c, *next_c;
+ QLIST_FOREACH_SAFE(c, &bs->children, next, next_c) {
+ bdrv_unref_child(bs, c);
+ }
+}
+
+static int coroutine_fn bdrv_test_top_co_preadv(BlockDriverState *bs,
+ uint64_t offset, uint64_t bytes,
+ QEMUIOVector *qiov, int flags)
+{
+ BDRVTestTopState *tts = bs->opaque;
+ return bdrv_co_preadv(tts->wait_child, offset, bytes, qiov, flags);
+}
+
+static BlockDriver bdrv_test_top_driver = {
+ .format_name = "test_top_driver",
+ .instance_size = sizeof(BDRVTestTopState),
+
+ .bdrv_close = bdrv_test_top_close,
+ .bdrv_co_preadv = bdrv_test_top_co_preadv,
+
+ .bdrv_child_perm = bdrv_format_default_perms,
+};
+
+typedef struct TestCoDeleteByDrainData {
+ BlockBackend *blk;
+ bool detach_instead_of_delete;
+ bool done;
+} TestCoDeleteByDrainData;
+
+static void coroutine_fn test_co_delete_by_drain(void *opaque)
+{
+ TestCoDeleteByDrainData *dbdd = opaque;
+ BlockBackend *blk = dbdd->blk;
+ BlockDriverState *bs = blk_bs(blk);
+ BDRVTestTopState *tts = bs->opaque;
+ void *buffer = g_malloc(65536);
+ QEMUIOVector qiov;
+ struct iovec iov = {
+ .iov_base = buffer,
+ .iov_len = 65536,
+ };
+
+ qemu_iovec_init_external(&qiov, &iov, 1);
+
+ /* Pretend some internal write operation from parent to child.
+ * Important: We have to read from the child, not from the parent!
+ * Draining works by first propagating it all up the tree to the
+ * root and then waiting for drainage from root to the leaves
+ * (protocol nodes). If we have a request waiting on the root,
+ * everything will be drained before we go back down the tree, but
+ * we do not want that. We want to be in the middle of draining
+ * when this following requests returns. */
+ bdrv_co_preadv(tts->wait_child, 0, 65536, &qiov, 0);
+
+ g_assert_cmpint(bs->refcnt, ==, 1);
+
+ if (!dbdd->detach_instead_of_delete) {
+ blk_unref(blk);
+ } else {
+ BdrvChild *c, *next_c;
+ QLIST_FOREACH_SAFE(c, &bs->children, next, next_c) {
+ bdrv_unref_child(bs, c);
+ }
+ }
+
+ dbdd->done = true;
+}
+
+/**
+ * Test what happens when some BDS has some children, you drain one of
+ * them and this results in the BDS being deleted.
+ *
+ * If @detach_instead_of_delete is set, the BDS is not going to be
+ * deleted but will only detach all of its children.
+ */
+static void do_test_delete_by_drain(bool detach_instead_of_delete)
+{
+ BlockBackend *blk;
+ BlockDriverState *bs, *child_bs, *null_bs;
+ BDRVTestTopState *tts;
+ TestCoDeleteByDrainData dbdd;
+ Coroutine *co;
+
+ bs = bdrv_new_open_driver(&bdrv_test_top_driver, "top", BDRV_O_RDWR,
+ &error_abort);
+ bs->total_sectors = 65536 >> BDRV_SECTOR_BITS;
+ tts = bs->opaque;
+
+ null_bs = bdrv_open("null-co://", NULL, NULL, BDRV_O_RDWR | BDRV_O_PROTOCOL,
+ &error_abort);
+ bdrv_attach_child(bs, null_bs, "null-child", &child_file, &error_abort);
+
+ /* This child will be the one to pass to requests through to, and
+ * it will stall until a drain occurs */
+ child_bs = bdrv_new_open_driver(&bdrv_test, "child", BDRV_O_RDWR,
+ &error_abort);
+ child_bs->total_sectors = 65536 >> BDRV_SECTOR_BITS;
+ /* Takes our reference to child_bs */
+ tts->wait_child = bdrv_attach_child(bs, child_bs, "wait-child", &child_file,
+ &error_abort);
+
+ /* This child is just there to be deleted
+ * (for detach_instead_of_delete == true) */
+ null_bs = bdrv_open("null-co://", NULL, NULL, BDRV_O_RDWR | BDRV_O_PROTOCOL,
+ &error_abort);
+ bdrv_attach_child(bs, null_bs, "null-child", &child_file, &error_abort);
+
+ blk = blk_new(BLK_PERM_ALL, BLK_PERM_ALL);
+ blk_insert_bs(blk, bs, &error_abort);
+
+ /* Referenced by blk now */
+ bdrv_unref(bs);
+
+ g_assert_cmpint(bs->refcnt, ==, 1);
+ g_assert_cmpint(child_bs->refcnt, ==, 1);
+ g_assert_cmpint(null_bs->refcnt, ==, 1);
+
+
+ dbdd = (TestCoDeleteByDrainData){
+ .blk = blk,
+ .detach_instead_of_delete = detach_instead_of_delete,
+ .done = false,
+ };
+ co = qemu_coroutine_create(test_co_delete_by_drain, &dbdd);
+ qemu_coroutine_enter(co);
+
+ /* Drain the child while the read operation is still pending.
+ * This should result in the operation finishing and
+ * test_co_delete_by_drain() resuming. Thus, @bs will be deleted
+ * and the coroutine will exit while this drain operation is still
+ * in progress. */
+ bdrv_ref(child_bs);
+ bdrv_drain(child_bs);
+ bdrv_unref(child_bs);
+
+ while (!dbdd.done) {
+ aio_poll(qemu_get_aio_context(), true);
+ }
+
+ if (detach_instead_of_delete) {
+ /* Here, the reference has not passed over to the coroutine,
+ * so we have to delete the BB ourselves */
+ blk_unref(blk);
+ }
+}
+
+
+static void test_delete_by_drain(void)
+{
+ do_test_delete_by_drain(false);
+}
+
+static void test_detach_by_drain(void)
+{
+ do_test_delete_by_drain(true);
+}
+
+
int main(int argc, char **argv)
{
int ret;
@@ -844,6 +1010,9 @@ int main(int argc, char **argv)
g_test_add_func("/bdrv-drain/blockjob/drain_subtree",
test_blockjob_drain_subtree);
+ g_test_add_func("/bdrv-drain/deletion", test_delete_by_drain);
+ g_test_add_func("/bdrv-drain/detach", test_detach_by_drain);
+
ret = g_test_run();
qemu_event_destroy(&done_event);
return ret;
--
2.13.6
^ permalink raw reply related [flat|nested] 47+ messages in thread* [Qemu-devel] [PULL 10/35] block: Drain recursively with a single BDRV_POLL_WHILE()
2018-06-18 16:44 [Qemu-devel] [PULL 00/35] Block layer patches Kevin Wolf
` (8 preceding siblings ...)
2018-06-18 16:44 ` [Qemu-devel] [PULL 09/35] test-bdrv-drain: Add test for node deletion Kevin Wolf
@ 2018-06-18 16:44 ` Kevin Wolf
2018-06-18 16:44 ` [Qemu-devel] [PULL 11/35] test-bdrv-drain: Test node deletion in subtree recursion Kevin Wolf
` (26 subsequent siblings)
36 siblings, 0 replies; 47+ messages in thread
From: Kevin Wolf @ 2018-06-18 16:44 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel
Anything can happen inside BDRV_POLL_WHILE(), including graph
changes that may interfere with its callers (e.g. child list iteration
in recursive callers of bdrv_do_drained_begin).
Switch to a single BDRV_POLL_WHILE() call for the whole subtree at the
end of bdrv_do_drained_begin() to avoid such effects. The recursion
happens now inside the loop condition. As the graph can only change
between bdrv_drain_poll() calls, but not inside of it, doing the
recursion here is safe.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
include/block/block.h | 9 +++++---
block.c | 2 +-
block/io.c | 63 ++++++++++++++++++++++++++++++++++++---------------
3 files changed, 52 insertions(+), 22 deletions(-)
diff --git a/include/block/block.h b/include/block/block.h
index cebbb39c6c..254ed2e4c9 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -570,10 +570,13 @@ void bdrv_parent_drained_end(BlockDriverState *bs, BdrvChild *ignore);
/**
* bdrv_drain_poll:
*
- * Poll for pending requests in @bs and its parents (except for
- * @ignore_parent). This is part of bdrv_drained_begin.
+ * Poll for pending requests in @bs, its parents (except for @ignore_parent),
+ * and if @recursive is true its children as well.
+ *
+ * This is part of bdrv_drained_begin.
*/
-bool bdrv_drain_poll(BlockDriverState *bs, BdrvChild *ignore_parent);
+bool bdrv_drain_poll(BlockDriverState *bs, bool recursive,
+ BdrvChild *ignore_parent);
/**
* bdrv_drained_begin:
diff --git a/block.c b/block.c
index 8cf9cd8855..80abd3c2ae 100644
--- a/block.c
+++ b/block.c
@@ -824,7 +824,7 @@ static void bdrv_child_cb_drained_begin(BdrvChild *child)
static bool bdrv_child_cb_drained_poll(BdrvChild *child)
{
BlockDriverState *bs = child->opaque;
- return bdrv_drain_poll(bs, NULL);
+ return bdrv_drain_poll(bs, false, NULL);
}
static void bdrv_child_cb_drained_end(BdrvChild *child)
diff --git a/block/io.c b/block/io.c
index 5f6d5eed52..a413841bfc 100644
--- a/block/io.c
+++ b/block/io.c
@@ -165,6 +165,7 @@ typedef struct {
bool done;
bool begin;
bool recursive;
+ bool poll;
BdrvChild *parent;
} BdrvCoDrainData;
@@ -200,27 +201,42 @@ static void bdrv_drain_invoke(BlockDriverState *bs, bool begin)
}
/* Returns true if BDRV_POLL_WHILE() should go into a blocking aio_poll() */
-bool bdrv_drain_poll(BlockDriverState *bs, BdrvChild *ignore_parent)
+bool bdrv_drain_poll(BlockDriverState *bs, bool recursive,
+ BdrvChild *ignore_parent)
{
+ BdrvChild *child, *next;
+
if (bdrv_parent_drained_poll(bs, ignore_parent)) {
return true;
}
- return atomic_read(&bs->in_flight);
+ if (atomic_read(&bs->in_flight)) {
+ return true;
+ }
+
+ if (recursive) {
+ QLIST_FOREACH_SAFE(child, &bs->children, next, next) {
+ if (bdrv_drain_poll(child->bs, recursive, child)) {
+ return true;
+ }
+ }
+ }
+
+ return false;
}
-static bool bdrv_drain_poll_top_level(BlockDriverState *bs,
+static bool bdrv_drain_poll_top_level(BlockDriverState *bs, bool recursive,
BdrvChild *ignore_parent)
{
/* Execute pending BHs first and check everything else only after the BHs
* have executed. */
while (aio_poll(bs->aio_context, false));
- return bdrv_drain_poll(bs, ignore_parent);
+ return bdrv_drain_poll(bs, recursive, ignore_parent);
}
static void bdrv_do_drained_begin(BlockDriverState *bs, bool recursive,
- BdrvChild *parent);
+ BdrvChild *parent, bool poll);
static void bdrv_do_drained_end(BlockDriverState *bs, bool recursive,
BdrvChild *parent);
@@ -232,7 +248,7 @@ static void bdrv_co_drain_bh_cb(void *opaque)
bdrv_dec_in_flight(bs);
if (data->begin) {
- bdrv_do_drained_begin(bs, data->recursive, data->parent);
+ bdrv_do_drained_begin(bs, data->recursive, data->parent, data->poll);
} else {
bdrv_do_drained_end(bs, data->recursive, data->parent);
}
@@ -243,7 +259,7 @@ static void bdrv_co_drain_bh_cb(void *opaque)
static void coroutine_fn bdrv_co_yield_to_drain(BlockDriverState *bs,
bool begin, bool recursive,
- BdrvChild *parent)
+ BdrvChild *parent, bool poll)
{
BdrvCoDrainData data;
@@ -258,6 +274,7 @@ static void coroutine_fn bdrv_co_yield_to_drain(BlockDriverState *bs,
.begin = begin,
.recursive = recursive,
.parent = parent,
+ .poll = poll,
};
bdrv_inc_in_flight(bs);
aio_bh_schedule_oneshot(bdrv_get_aio_context(bs),
@@ -270,12 +287,12 @@ static void coroutine_fn bdrv_co_yield_to_drain(BlockDriverState *bs,
}
void bdrv_do_drained_begin(BlockDriverState *bs, bool recursive,
- BdrvChild *parent)
+ BdrvChild *parent, bool poll)
{
BdrvChild *child, *next;
if (qemu_in_coroutine()) {
- bdrv_co_yield_to_drain(bs, true, recursive, parent);
+ bdrv_co_yield_to_drain(bs, true, recursive, parent, poll);
return;
}
@@ -287,25 +304,35 @@ void bdrv_do_drained_begin(BlockDriverState *bs, bool recursive,
bdrv_parent_drained_begin(bs, parent);
bdrv_drain_invoke(bs, true);
- /* Wait for drained requests to finish */
- BDRV_POLL_WHILE(bs, bdrv_drain_poll_top_level(bs, parent));
-
if (recursive) {
bs->recursive_quiesce_counter++;
QLIST_FOREACH_SAFE(child, &bs->children, next, next) {
- bdrv_do_drained_begin(child->bs, true, child);
+ bdrv_do_drained_begin(child->bs, true, child, false);
}
}
+
+ /*
+ * Wait for drained requests to finish.
+ *
+ * Calling BDRV_POLL_WHILE() only once for the top-level node is okay: The
+ * call is needed so things in this AioContext can make progress even
+ * though we don't return to the main AioContext loop - this automatically
+ * includes other nodes in the same AioContext and therefore all child
+ * nodes.
+ */
+ if (poll) {
+ BDRV_POLL_WHILE(bs, bdrv_drain_poll_top_level(bs, recursive, parent));
+ }
}
void bdrv_drained_begin(BlockDriverState *bs)
{
- bdrv_do_drained_begin(bs, false, NULL);
+ bdrv_do_drained_begin(bs, false, NULL, true);
}
void bdrv_subtree_drained_begin(BlockDriverState *bs)
{
- bdrv_do_drained_begin(bs, true, NULL);
+ bdrv_do_drained_begin(bs, true, NULL, true);
}
void bdrv_do_drained_end(BlockDriverState *bs, bool recursive,
@@ -315,7 +342,7 @@ void bdrv_do_drained_end(BlockDriverState *bs, bool recursive,
int old_quiesce_counter;
if (qemu_in_coroutine()) {
- bdrv_co_yield_to_drain(bs, false, recursive, parent);
+ bdrv_co_yield_to_drain(bs, false, recursive, parent, false);
return;
}
assert(bs->quiesce_counter > 0);
@@ -351,7 +378,7 @@ void bdrv_apply_subtree_drain(BdrvChild *child, BlockDriverState *new_parent)
int i;
for (i = 0; i < new_parent->recursive_quiesce_counter; i++) {
- bdrv_do_drained_begin(child->bs, true, child);
+ bdrv_do_drained_begin(child->bs, true, child, true);
}
}
@@ -421,7 +448,7 @@ void bdrv_drain_all_begin(void)
AioContext *aio_context = bdrv_get_aio_context(bs);
aio_context_acquire(aio_context);
- bdrv_do_drained_begin(bs, true, NULL);
+ bdrv_do_drained_begin(bs, true, NULL, true);
aio_context_release(aio_context);
}
--
2.13.6
^ permalink raw reply related [flat|nested] 47+ messages in thread* [Qemu-devel] [PULL 11/35] test-bdrv-drain: Test node deletion in subtree recursion
2018-06-18 16:44 [Qemu-devel] [PULL 00/35] Block layer patches Kevin Wolf
` (9 preceding siblings ...)
2018-06-18 16:44 ` [Qemu-devel] [PULL 10/35] block: Drain recursively with a single BDRV_POLL_WHILE() Kevin Wolf
@ 2018-06-18 16:44 ` Kevin Wolf
2018-06-18 16:44 ` [Qemu-devel] [PULL 12/35] block: Don't poll in parent drain callbacks Kevin Wolf
` (25 subsequent siblings)
36 siblings, 0 replies; 47+ messages in thread
From: Kevin Wolf @ 2018-06-18 16:44 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel
If bdrv_do_drained_begin() polls during its subtree recursion, the graph
can change and mess up the bs->children iteration. Test that this
doesn't happen.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
tests/test-bdrv-drain.c | 38 +++++++++++++++++++++++++++++---------
1 file changed, 29 insertions(+), 9 deletions(-)
diff --git a/tests/test-bdrv-drain.c b/tests/test-bdrv-drain.c
index e8a5515b08..0c8f632f2d 100644
--- a/tests/test-bdrv-drain.c
+++ b/tests/test-bdrv-drain.c
@@ -880,7 +880,8 @@ static void coroutine_fn test_co_delete_by_drain(void *opaque)
* If @detach_instead_of_delete is set, the BDS is not going to be
* deleted but will only detach all of its children.
*/
-static void do_test_delete_by_drain(bool detach_instead_of_delete)
+static void do_test_delete_by_drain(bool detach_instead_of_delete,
+ enum drain_type drain_type)
{
BlockBackend *blk;
BlockDriverState *bs, *child_bs, *null_bs;
@@ -936,9 +937,23 @@ static void do_test_delete_by_drain(bool detach_instead_of_delete)
* test_co_delete_by_drain() resuming. Thus, @bs will be deleted
* and the coroutine will exit while this drain operation is still
* in progress. */
- bdrv_ref(child_bs);
- bdrv_drain(child_bs);
- bdrv_unref(child_bs);
+ switch (drain_type) {
+ case BDRV_DRAIN:
+ bdrv_ref(child_bs);
+ bdrv_drain(child_bs);
+ bdrv_unref(child_bs);
+ break;
+ case BDRV_SUBTREE_DRAIN:
+ /* Would have to ref/unref bs here for !detach_instead_of_delete, but
+ * then the whole test becomes pointless because the graph changes
+ * don't occur during the drain any more. */
+ assert(detach_instead_of_delete);
+ bdrv_subtree_drained_begin(bs);
+ bdrv_subtree_drained_end(bs);
+ break;
+ default:
+ g_assert_not_reached();
+ }
while (!dbdd.done) {
aio_poll(qemu_get_aio_context(), true);
@@ -951,15 +966,19 @@ static void do_test_delete_by_drain(bool detach_instead_of_delete)
}
}
-
static void test_delete_by_drain(void)
{
- do_test_delete_by_drain(false);
+ do_test_delete_by_drain(false, BDRV_DRAIN);
}
static void test_detach_by_drain(void)
{
- do_test_delete_by_drain(true);
+ do_test_delete_by_drain(true, BDRV_DRAIN);
+}
+
+static void test_detach_by_drain_subtree(void)
+{
+ do_test_delete_by_drain(true, BDRV_SUBTREE_DRAIN);
}
@@ -1010,8 +1029,9 @@ int main(int argc, char **argv)
g_test_add_func("/bdrv-drain/blockjob/drain_subtree",
test_blockjob_drain_subtree);
- g_test_add_func("/bdrv-drain/deletion", test_delete_by_drain);
- g_test_add_func("/bdrv-drain/detach", test_detach_by_drain);
+ g_test_add_func("/bdrv-drain/deletion/drain", test_delete_by_drain);
+ g_test_add_func("/bdrv-drain/detach/drain", test_detach_by_drain);
+ g_test_add_func("/bdrv-drain/detach/drain_subtree", test_detach_by_drain_subtree);
ret = g_test_run();
qemu_event_destroy(&done_event);
--
2.13.6
^ permalink raw reply related [flat|nested] 47+ messages in thread* [Qemu-devel] [PULL 12/35] block: Don't poll in parent drain callbacks
2018-06-18 16:44 [Qemu-devel] [PULL 00/35] Block layer patches Kevin Wolf
` (10 preceding siblings ...)
2018-06-18 16:44 ` [Qemu-devel] [PULL 11/35] test-bdrv-drain: Test node deletion in subtree recursion Kevin Wolf
@ 2018-06-18 16:44 ` Kevin Wolf
2018-06-18 16:44 ` [Qemu-devel] [PULL 13/35] test-bdrv-drain: Graph change through parent callback Kevin Wolf
` (24 subsequent siblings)
36 siblings, 0 replies; 47+ messages in thread
From: Kevin Wolf @ 2018-06-18 16:44 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel
bdrv_do_drained_begin() is only safe if we have a single
BDRV_POLL_WHILE() after quiescing all affected nodes. We cannot allow
that parent callbacks introduce a nested polling loop that could cause
graph changes while we're traversing the graph.
Split off bdrv_do_drained_begin_quiesce(), which only quiesces a single
node without waiting for its requests to complete. These requests will
be waited for in the BDRV_POLL_WHILE() call down the call chain.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
include/block/block.h | 9 +++++++++
block.c | 2 +-
block/io.c | 24 ++++++++++++++++--------
3 files changed, 26 insertions(+), 9 deletions(-)
diff --git a/include/block/block.h b/include/block/block.h
index 254ed2e4c9..067d24cc4a 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -591,6 +591,15 @@ bool bdrv_drain_poll(BlockDriverState *bs, bool recursive,
void bdrv_drained_begin(BlockDriverState *bs);
/**
+ * bdrv_do_drained_begin_quiesce:
+ *
+ * Quiesces a BDS like bdrv_drained_begin(), but does not wait for already
+ * running requests to complete.
+ */
+void bdrv_do_drained_begin_quiesce(BlockDriverState *bs,
+ BdrvChild *parent);
+
+/**
* Like bdrv_drained_begin, but recursively begins a quiesced section for
* exclusive access to all child nodes as well.
*/
diff --git a/block.c b/block.c
index 80abd3c2ae..50f8e3dc3b 100644
--- a/block.c
+++ b/block.c
@@ -818,7 +818,7 @@ static char *bdrv_child_get_parent_desc(BdrvChild *c)
static void bdrv_child_cb_drained_begin(BdrvChild *child)
{
BlockDriverState *bs = child->opaque;
- bdrv_drained_begin(bs);
+ bdrv_do_drained_begin_quiesce(bs, NULL);
}
static bool bdrv_child_cb_drained_poll(BdrvChild *child)
diff --git a/block/io.c b/block/io.c
index a413841bfc..ffb273708a 100644
--- a/block/io.c
+++ b/block/io.c
@@ -286,15 +286,10 @@ static void coroutine_fn bdrv_co_yield_to_drain(BlockDriverState *bs,
assert(data.done);
}
-void bdrv_do_drained_begin(BlockDriverState *bs, bool recursive,
- BdrvChild *parent, bool poll)
+void bdrv_do_drained_begin_quiesce(BlockDriverState *bs,
+ BdrvChild *parent)
{
- BdrvChild *child, *next;
-
- if (qemu_in_coroutine()) {
- bdrv_co_yield_to_drain(bs, true, recursive, parent, poll);
- return;
- }
+ assert(!qemu_in_coroutine());
/* Stop things in parent-to-child order */
if (atomic_fetch_inc(&bs->quiesce_counter) == 0) {
@@ -303,6 +298,19 @@ void bdrv_do_drained_begin(BlockDriverState *bs, bool recursive,
bdrv_parent_drained_begin(bs, parent);
bdrv_drain_invoke(bs, true);
+}
+
+static void bdrv_do_drained_begin(BlockDriverState *bs, bool recursive,
+ BdrvChild *parent, bool poll)
+{
+ BdrvChild *child, *next;
+
+ if (qemu_in_coroutine()) {
+ bdrv_co_yield_to_drain(bs, true, recursive, parent, poll);
+ return;
+ }
+
+ bdrv_do_drained_begin_quiesce(bs, parent);
if (recursive) {
bs->recursive_quiesce_counter++;
--
2.13.6
^ permalink raw reply related [flat|nested] 47+ messages in thread* [Qemu-devel] [PULL 13/35] test-bdrv-drain: Graph change through parent callback
2018-06-18 16:44 [Qemu-devel] [PULL 00/35] Block layer patches Kevin Wolf
` (11 preceding siblings ...)
2018-06-18 16:44 ` [Qemu-devel] [PULL 12/35] block: Don't poll in parent drain callbacks Kevin Wolf
@ 2018-06-18 16:44 ` Kevin Wolf
2018-06-18 16:44 ` [Qemu-devel] [PULL 14/35] block: Defer .bdrv_drain_begin callback to polling phase Kevin Wolf
` (23 subsequent siblings)
36 siblings, 0 replies; 47+ messages in thread
From: Kevin Wolf @ 2018-06-18 16:44 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
tests/test-bdrv-drain.c | 130 ++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 130 insertions(+)
diff --git a/tests/test-bdrv-drain.c b/tests/test-bdrv-drain.c
index 0c8f632f2d..abb287e597 100644
--- a/tests/test-bdrv-drain.c
+++ b/tests/test-bdrv-drain.c
@@ -982,6 +982,135 @@ static void test_detach_by_drain_subtree(void)
}
+struct detach_by_parent_data {
+ BlockDriverState *parent_b;
+ BdrvChild *child_b;
+ BlockDriverState *c;
+ BdrvChild *child_c;
+};
+
+static void detach_by_parent_aio_cb(void *opaque, int ret)
+{
+ struct detach_by_parent_data *data = opaque;
+
+ g_assert_cmpint(ret, ==, 0);
+ bdrv_unref_child(data->parent_b, data->child_b);
+
+ bdrv_ref(data->c);
+ data->child_c = bdrv_attach_child(data->parent_b, data->c, "PB-C",
+ &child_file, &error_abort);
+}
+
+/*
+ * Initial graph:
+ *
+ * PA PB
+ * \ / \
+ * A B C
+ *
+ * PA has a pending write request whose callback changes the child nodes of PB:
+ * It removes B and adds C instead. The subtree of PB is drained, which will
+ * indirectly drain the write request, too.
+ */
+static void test_detach_by_parent_cb(void)
+{
+ BlockBackend *blk;
+ BlockDriverState *parent_a, *parent_b, *a, *b, *c;
+ BdrvChild *child_a, *child_b;
+ BlockAIOCB *acb;
+ struct detach_by_parent_data data;
+
+ QEMUIOVector qiov;
+ struct iovec iov = {
+ .iov_base = NULL,
+ .iov_len = 0,
+ };
+ qemu_iovec_init_external(&qiov, &iov, 1);
+
+ /* Create all involved nodes */
+ parent_a = bdrv_new_open_driver(&bdrv_test, "parent-a", BDRV_O_RDWR,
+ &error_abort);
+ parent_b = bdrv_new_open_driver(&bdrv_test, "parent-b", 0,
+ &error_abort);
+
+ a = bdrv_new_open_driver(&bdrv_test, "a", BDRV_O_RDWR, &error_abort);
+ b = bdrv_new_open_driver(&bdrv_test, "b", BDRV_O_RDWR, &error_abort);
+ c = bdrv_new_open_driver(&bdrv_test, "c", BDRV_O_RDWR, &error_abort);
+
+ /* blk is a BB for parent-a */
+ blk = blk_new(BLK_PERM_ALL, BLK_PERM_ALL);
+ blk_insert_bs(blk, parent_a, &error_abort);
+ bdrv_unref(parent_a);
+
+ /* Set child relationships */
+ bdrv_ref(b);
+ bdrv_ref(a);
+ child_b = bdrv_attach_child(parent_b, b, "PB-B", &child_file, &error_abort);
+ child_a = bdrv_attach_child(parent_b, a, "PB-A", &child_backing, &error_abort);
+
+ bdrv_ref(a);
+ bdrv_attach_child(parent_a, a, "PA-A", &child_file, &error_abort);
+
+ g_assert_cmpint(parent_a->refcnt, ==, 1);
+ g_assert_cmpint(parent_b->refcnt, ==, 1);
+ g_assert_cmpint(a->refcnt, ==, 3);
+ g_assert_cmpint(b->refcnt, ==, 2);
+ g_assert_cmpint(c->refcnt, ==, 1);
+
+ g_assert(QLIST_FIRST(&parent_b->children) == child_a);
+ g_assert(QLIST_NEXT(child_a, next) == child_b);
+ g_assert(QLIST_NEXT(child_b, next) == NULL);
+
+ /* Start the evil write request */
+ data = (struct detach_by_parent_data) {
+ .parent_b = parent_b,
+ .child_b = child_b,
+ .c = c,
+ };
+ acb = blk_aio_preadv(blk, 0, &qiov, 0, detach_by_parent_aio_cb, &data);
+ g_assert(acb != NULL);
+
+ /* Drain and check the expected result */
+ bdrv_subtree_drained_begin(parent_b);
+
+ g_assert(data.child_c != NULL);
+
+ g_assert_cmpint(parent_a->refcnt, ==, 1);
+ g_assert_cmpint(parent_b->refcnt, ==, 1);
+ g_assert_cmpint(a->refcnt, ==, 3);
+ g_assert_cmpint(b->refcnt, ==, 1);
+ g_assert_cmpint(c->refcnt, ==, 2);
+
+ g_assert(QLIST_FIRST(&parent_b->children) == data.child_c);
+ g_assert(QLIST_NEXT(data.child_c, next) == child_a);
+ g_assert(QLIST_NEXT(child_a, next) == NULL);
+
+ g_assert_cmpint(parent_a->quiesce_counter, ==, 1);
+ g_assert_cmpint(parent_b->quiesce_counter, ==, 1);
+ g_assert_cmpint(a->quiesce_counter, ==, 1);
+ g_assert_cmpint(b->quiesce_counter, ==, 0);
+ g_assert_cmpint(c->quiesce_counter, ==, 1);
+
+ bdrv_subtree_drained_end(parent_b);
+
+ bdrv_unref(parent_b);
+ blk_unref(blk);
+
+ /* XXX Once bdrv_close() unref's children instead of just detaching them,
+ * this won't be necessary any more. */
+ bdrv_unref(a);
+ bdrv_unref(a);
+ bdrv_unref(c);
+
+ g_assert_cmpint(a->refcnt, ==, 1);
+ g_assert_cmpint(b->refcnt, ==, 1);
+ g_assert_cmpint(c->refcnt, ==, 1);
+ bdrv_unref(a);
+ bdrv_unref(b);
+ bdrv_unref(c);
+}
+
+
int main(int argc, char **argv)
{
int ret;
@@ -1032,6 +1161,7 @@ int main(int argc, char **argv)
g_test_add_func("/bdrv-drain/deletion/drain", test_delete_by_drain);
g_test_add_func("/bdrv-drain/detach/drain", test_detach_by_drain);
g_test_add_func("/bdrv-drain/detach/drain_subtree", test_detach_by_drain_subtree);
+ g_test_add_func("/bdrv-drain/detach/parent_cb", test_detach_by_parent_cb);
ret = g_test_run();
qemu_event_destroy(&done_event);
--
2.13.6
^ permalink raw reply related [flat|nested] 47+ messages in thread* [Qemu-devel] [PULL 14/35] block: Defer .bdrv_drain_begin callback to polling phase
2018-06-18 16:44 [Qemu-devel] [PULL 00/35] Block layer patches Kevin Wolf
` (12 preceding siblings ...)
2018-06-18 16:44 ` [Qemu-devel] [PULL 13/35] test-bdrv-drain: Graph change through parent callback Kevin Wolf
@ 2018-06-18 16:44 ` Kevin Wolf
2018-06-18 16:44 ` [Qemu-devel] [PULL 15/35] test-bdrv-drain: Test that bdrv_drain_invoke() doesn't poll Kevin Wolf
` (22 subsequent siblings)
36 siblings, 0 replies; 47+ messages in thread
From: Kevin Wolf @ 2018-06-18 16:44 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel
We cannot allow aio_poll() in bdrv_drain_invoke(begin=true) until we're
done with propagating the drain through the graph and are doing the
single final BDRV_POLL_WHILE().
Just schedule the coroutine with the callback and increase bs->in_flight
to make sure that the polling phase will wait for it.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
block/io.c | 28 +++++++++++++++++++++++-----
1 file changed, 23 insertions(+), 5 deletions(-)
diff --git a/block/io.c b/block/io.c
index ffb273708a..e08d53c8a5 100644
--- a/block/io.c
+++ b/block/io.c
@@ -182,22 +182,40 @@ static void coroutine_fn bdrv_drain_invoke_entry(void *opaque)
/* Set data->done before reading bs->wakeup. */
atomic_mb_set(&data->done, true);
- bdrv_wakeup(bs);
+ bdrv_dec_in_flight(bs);
+
+ if (data->begin) {
+ g_free(data);
+ }
}
/* Recursively call BlockDriver.bdrv_co_drain_begin/end callbacks */
static void bdrv_drain_invoke(BlockDriverState *bs, bool begin)
{
- BdrvCoDrainData data = { .bs = bs, .done = false, .begin = begin};
+ BdrvCoDrainData *data;
if (!bs->drv || (begin && !bs->drv->bdrv_co_drain_begin) ||
(!begin && !bs->drv->bdrv_co_drain_end)) {
return;
}
- data.co = qemu_coroutine_create(bdrv_drain_invoke_entry, &data);
- bdrv_coroutine_enter(bs, data.co);
- BDRV_POLL_WHILE(bs, !data.done);
+ data = g_new(BdrvCoDrainData, 1);
+ *data = (BdrvCoDrainData) {
+ .bs = bs,
+ .done = false,
+ .begin = begin
+ };
+
+ /* Make sure the driver callback completes during the polling phase for
+ * drain_begin. */
+ bdrv_inc_in_flight(bs);
+ data->co = qemu_coroutine_create(bdrv_drain_invoke_entry, data);
+ aio_co_schedule(bdrv_get_aio_context(bs), data->co);
+
+ if (!begin) {
+ BDRV_POLL_WHILE(bs, !data->done);
+ g_free(data);
+ }
}
/* Returns true if BDRV_POLL_WHILE() should go into a blocking aio_poll() */
--
2.13.6
^ permalink raw reply related [flat|nested] 47+ messages in thread* [Qemu-devel] [PULL 15/35] test-bdrv-drain: Test that bdrv_drain_invoke() doesn't poll
2018-06-18 16:44 [Qemu-devel] [PULL 00/35] Block layer patches Kevin Wolf
` (13 preceding siblings ...)
2018-06-18 16:44 ` [Qemu-devel] [PULL 14/35] block: Defer .bdrv_drain_begin callback to polling phase Kevin Wolf
@ 2018-06-18 16:44 ` Kevin Wolf
2018-06-18 16:44 ` [Qemu-devel] [PULL 16/35] block: Allow AIO_WAIT_WHILE with NULL ctx Kevin Wolf
` (21 subsequent siblings)
36 siblings, 0 replies; 47+ messages in thread
From: Kevin Wolf @ 2018-06-18 16:44 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel
This adds a test case that goes wrong if bdrv_drain_invoke() calls
aio_poll().
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
tests/test-bdrv-drain.c | 102 +++++++++++++++++++++++++++++++++++++++++-------
1 file changed, 88 insertions(+), 14 deletions(-)
diff --git a/tests/test-bdrv-drain.c b/tests/test-bdrv-drain.c
index abb287e597..5cc179a778 100644
--- a/tests/test-bdrv-drain.c
+++ b/tests/test-bdrv-drain.c
@@ -34,12 +34,16 @@ static QemuEvent done_event;
typedef struct BDRVTestState {
int drain_count;
AioContext *bh_indirection_ctx;
+ bool sleep_in_drain_begin;
} BDRVTestState;
static void coroutine_fn bdrv_test_co_drain_begin(BlockDriverState *bs)
{
BDRVTestState *s = bs->opaque;
s->drain_count++;
+ if (s->sleep_in_drain_begin) {
+ qemu_co_sleep_ns(QEMU_CLOCK_REALTIME, 100000);
+ }
}
static void coroutine_fn bdrv_test_co_drain_end(BlockDriverState *bs)
@@ -80,6 +84,22 @@ static int coroutine_fn bdrv_test_co_preadv(BlockDriverState *bs,
return 0;
}
+static void bdrv_test_child_perm(BlockDriverState *bs, BdrvChild *c,
+ const BdrvChildRole *role,
+ BlockReopenQueue *reopen_queue,
+ uint64_t perm, uint64_t shared,
+ uint64_t *nperm, uint64_t *nshared)
+{
+ /* bdrv_format_default_perms() accepts only these two, so disguise
+ * detach_by_driver_cb_role as one of them. */
+ if (role != &child_file && role != &child_backing) {
+ role = &child_file;
+ }
+
+ bdrv_format_default_perms(bs, c, role, reopen_queue, perm, shared,
+ nperm, nshared);
+}
+
static BlockDriver bdrv_test = {
.format_name = "test",
.instance_size = sizeof(BDRVTestState),
@@ -90,7 +110,7 @@ static BlockDriver bdrv_test = {
.bdrv_co_drain_begin = bdrv_test_co_drain_begin,
.bdrv_co_drain_end = bdrv_test_co_drain_end,
- .bdrv_child_perm = bdrv_format_default_perms,
+ .bdrv_child_perm = bdrv_test_child_perm,
};
static void aio_ret_cb(void *opaque, int ret)
@@ -987,13 +1007,14 @@ struct detach_by_parent_data {
BdrvChild *child_b;
BlockDriverState *c;
BdrvChild *child_c;
+ bool by_parent_cb;
};
+static struct detach_by_parent_data detach_by_parent_data;
-static void detach_by_parent_aio_cb(void *opaque, int ret)
+static void detach_indirect_bh(void *opaque)
{
struct detach_by_parent_data *data = opaque;
- g_assert_cmpint(ret, ==, 0);
bdrv_unref_child(data->parent_b, data->child_b);
bdrv_ref(data->c);
@@ -1001,6 +1022,25 @@ static void detach_by_parent_aio_cb(void *opaque, int ret)
&child_file, &error_abort);
}
+static void detach_by_parent_aio_cb(void *opaque, int ret)
+{
+ struct detach_by_parent_data *data = &detach_by_parent_data;
+
+ g_assert_cmpint(ret, ==, 0);
+ if (data->by_parent_cb) {
+ detach_indirect_bh(data);
+ }
+}
+
+static void detach_by_driver_cb_drained_begin(BdrvChild *child)
+{
+ aio_bh_schedule_oneshot(qemu_get_current_aio_context(),
+ detach_indirect_bh, &detach_by_parent_data);
+ child_file.drained_begin(child);
+}
+
+static BdrvChildRole detach_by_driver_cb_role;
+
/*
* Initial graph:
*
@@ -1008,17 +1048,25 @@ static void detach_by_parent_aio_cb(void *opaque, int ret)
* \ / \
* A B C
*
- * PA has a pending write request whose callback changes the child nodes of PB:
- * It removes B and adds C instead. The subtree of PB is drained, which will
- * indirectly drain the write request, too.
+ * by_parent_cb == true: Test that parent callbacks don't poll
+ *
+ * PA has a pending write request whose callback changes the child nodes of
+ * PB: It removes B and adds C instead. The subtree of PB is drained, which
+ * will indirectly drain the write request, too.
+ *
+ * by_parent_cb == false: Test that bdrv_drain_invoke() doesn't poll
+ *
+ * PA's BdrvChildRole has a .drained_begin callback that schedules a BH
+ * that does the same graph change. If bdrv_drain_invoke() calls it, the
+ * state is messed up, but if it is only polled in the single
+ * BDRV_POLL_WHILE() at the end of the drain, this should work fine.
*/
-static void test_detach_by_parent_cb(void)
+static void test_detach_indirect(bool by_parent_cb)
{
BlockBackend *blk;
BlockDriverState *parent_a, *parent_b, *a, *b, *c;
BdrvChild *child_a, *child_b;
BlockAIOCB *acb;
- struct detach_by_parent_data data;
QEMUIOVector qiov;
struct iovec iov = {
@@ -1027,6 +1075,12 @@ static void test_detach_by_parent_cb(void)
};
qemu_iovec_init_external(&qiov, &iov, 1);
+ if (!by_parent_cb) {
+ detach_by_driver_cb_role = child_file;
+ detach_by_driver_cb_role.drained_begin =
+ detach_by_driver_cb_drained_begin;
+ }
+
/* Create all involved nodes */
parent_a = bdrv_new_open_driver(&bdrv_test, "parent-a", BDRV_O_RDWR,
&error_abort);
@@ -1042,6 +1096,13 @@ static void test_detach_by_parent_cb(void)
blk_insert_bs(blk, parent_a, &error_abort);
bdrv_unref(parent_a);
+ /* If we want to get bdrv_drain_invoke() to call aio_poll(), the driver
+ * callback must not return immediately. */
+ if (!by_parent_cb) {
+ BDRVTestState *s = parent_a->opaque;
+ s->sleep_in_drain_begin = true;
+ }
+
/* Set child relationships */
bdrv_ref(b);
bdrv_ref(a);
@@ -1049,7 +1110,9 @@ static void test_detach_by_parent_cb(void)
child_a = bdrv_attach_child(parent_b, a, "PB-A", &child_backing, &error_abort);
bdrv_ref(a);
- bdrv_attach_child(parent_a, a, "PA-A", &child_file, &error_abort);
+ bdrv_attach_child(parent_a, a, "PA-A",
+ by_parent_cb ? &child_file : &detach_by_driver_cb_role,
+ &error_abort);
g_assert_cmpint(parent_a->refcnt, ==, 1);
g_assert_cmpint(parent_b->refcnt, ==, 1);
@@ -1062,18 +1125,19 @@ static void test_detach_by_parent_cb(void)
g_assert(QLIST_NEXT(child_b, next) == NULL);
/* Start the evil write request */
- data = (struct detach_by_parent_data) {
+ detach_by_parent_data = (struct detach_by_parent_data) {
.parent_b = parent_b,
.child_b = child_b,
.c = c,
+ .by_parent_cb = by_parent_cb,
};
- acb = blk_aio_preadv(blk, 0, &qiov, 0, detach_by_parent_aio_cb, &data);
+ acb = blk_aio_preadv(blk, 0, &qiov, 0, detach_by_parent_aio_cb, NULL);
g_assert(acb != NULL);
/* Drain and check the expected result */
bdrv_subtree_drained_begin(parent_b);
- g_assert(data.child_c != NULL);
+ g_assert(detach_by_parent_data.child_c != NULL);
g_assert_cmpint(parent_a->refcnt, ==, 1);
g_assert_cmpint(parent_b->refcnt, ==, 1);
@@ -1081,8 +1145,8 @@ static void test_detach_by_parent_cb(void)
g_assert_cmpint(b->refcnt, ==, 1);
g_assert_cmpint(c->refcnt, ==, 2);
- g_assert(QLIST_FIRST(&parent_b->children) == data.child_c);
- g_assert(QLIST_NEXT(data.child_c, next) == child_a);
+ g_assert(QLIST_FIRST(&parent_b->children) == detach_by_parent_data.child_c);
+ g_assert(QLIST_NEXT(detach_by_parent_data.child_c, next) == child_a);
g_assert(QLIST_NEXT(child_a, next) == NULL);
g_assert_cmpint(parent_a->quiesce_counter, ==, 1);
@@ -1110,6 +1174,15 @@ static void test_detach_by_parent_cb(void)
bdrv_unref(c);
}
+static void test_detach_by_parent_cb(void)
+{
+ test_detach_indirect(true);
+}
+
+static void test_detach_by_driver_cb(void)
+{
+ test_detach_indirect(false);
+}
int main(int argc, char **argv)
{
@@ -1162,6 +1235,7 @@ int main(int argc, char **argv)
g_test_add_func("/bdrv-drain/detach/drain", test_detach_by_drain);
g_test_add_func("/bdrv-drain/detach/drain_subtree", test_detach_by_drain_subtree);
g_test_add_func("/bdrv-drain/detach/parent_cb", test_detach_by_parent_cb);
+ g_test_add_func("/bdrv-drain/detach/driver_cb", test_detach_by_driver_cb);
ret = g_test_run();
qemu_event_destroy(&done_event);
--
2.13.6
^ permalink raw reply related [flat|nested] 47+ messages in thread* [Qemu-devel] [PULL 16/35] block: Allow AIO_WAIT_WHILE with NULL ctx
2018-06-18 16:44 [Qemu-devel] [PULL 00/35] Block layer patches Kevin Wolf
` (14 preceding siblings ...)
2018-06-18 16:44 ` [Qemu-devel] [PULL 15/35] test-bdrv-drain: Test that bdrv_drain_invoke() doesn't poll Kevin Wolf
@ 2018-06-18 16:44 ` Kevin Wolf
2018-06-18 16:44 ` [Qemu-devel] [PULL 17/35] block: Move bdrv_drain_all_begin() out of coroutine context Kevin Wolf
` (20 subsequent siblings)
36 siblings, 0 replies; 47+ messages in thread
From: Kevin Wolf @ 2018-06-18 16:44 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel
bdrv_drain_all() wants to have a single polling loop for draining the
in-flight requests of all nodes. This means that the AIO_WAIT_WHILE()
condition relies on activity in multiple AioContexts, which is polled
from the mainloop context. We must therefore call AIO_WAIT_WHILE() from
the mainloop thread and use the AioWait notification mechanism.
Just randomly picking the AioContext of any non-mainloop thread would
work, but instead of bothering to find such a context in the caller, we
can just as well accept NULL for ctx.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
include/block/aio-wait.h | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/include/block/aio-wait.h b/include/block/aio-wait.h
index 783d3678dd..c85a62f798 100644
--- a/include/block/aio-wait.h
+++ b/include/block/aio-wait.h
@@ -57,7 +57,8 @@ typedef struct {
/**
* AIO_WAIT_WHILE:
* @wait: the aio wait object
- * @ctx: the aio context
+ * @ctx: the aio context, or NULL if multiple aio contexts (for which the
+ * caller does not hold a lock) are involved in the polling condition.
* @cond: wait while this conditional expression is true
*
* Wait while a condition is true. Use this to implement synchronous
@@ -75,7 +76,7 @@ typedef struct {
bool waited_ = false; \
AioWait *wait_ = (wait); \
AioContext *ctx_ = (ctx); \
- if (in_aio_context_home_thread(ctx_)) { \
+ if (ctx_ && in_aio_context_home_thread(ctx_)) { \
while ((cond)) { \
aio_poll(ctx_, true); \
waited_ = true; \
@@ -86,9 +87,13 @@ typedef struct {
/* Increment wait_->num_waiters before evaluating cond. */ \
atomic_inc(&wait_->num_waiters); \
while ((cond)) { \
- aio_context_release(ctx_); \
+ if (ctx_) { \
+ aio_context_release(ctx_); \
+ } \
aio_poll(qemu_get_aio_context(), true); \
- aio_context_acquire(ctx_); \
+ if (ctx_) { \
+ aio_context_acquire(ctx_); \
+ } \
waited_ = true; \
} \
atomic_dec(&wait_->num_waiters); \
--
2.13.6
^ permalink raw reply related [flat|nested] 47+ messages in thread* [Qemu-devel] [PULL 17/35] block: Move bdrv_drain_all_begin() out of coroutine context
2018-06-18 16:44 [Qemu-devel] [PULL 00/35] Block layer patches Kevin Wolf
` (15 preceding siblings ...)
2018-06-18 16:44 ` [Qemu-devel] [PULL 16/35] block: Allow AIO_WAIT_WHILE with NULL ctx Kevin Wolf
@ 2018-06-18 16:44 ` Kevin Wolf
2018-06-18 16:44 ` [Qemu-devel] [PULL 18/35] block: ignore_bds_parents parameter for drain functions Kevin Wolf
` (19 subsequent siblings)
36 siblings, 0 replies; 47+ messages in thread
From: Kevin Wolf @ 2018-06-18 16:44 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel
Before we can introduce a single polling loop for all nodes in
bdrv_drain_all_begin(), we must make sure to run it outside of coroutine
context like we already do for bdrv_do_drained_begin().
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
block/io.c | 22 +++++++++++++++++-----
1 file changed, 17 insertions(+), 5 deletions(-)
diff --git a/block/io.c b/block/io.c
index e08d53c8a5..1a9974c136 100644
--- a/block/io.c
+++ b/block/io.c
@@ -264,11 +264,16 @@ static void bdrv_co_drain_bh_cb(void *opaque)
Coroutine *co = data->co;
BlockDriverState *bs = data->bs;
- bdrv_dec_in_flight(bs);
- if (data->begin) {
- bdrv_do_drained_begin(bs, data->recursive, data->parent, data->poll);
+ if (bs) {
+ bdrv_dec_in_flight(bs);
+ if (data->begin) {
+ bdrv_do_drained_begin(bs, data->recursive, data->parent, data->poll);
+ } else {
+ bdrv_do_drained_end(bs, data->recursive, data->parent);
+ }
} else {
- bdrv_do_drained_end(bs, data->recursive, data->parent);
+ assert(data->begin);
+ bdrv_drain_all_begin();
}
data->done = true;
@@ -294,7 +299,9 @@ static void coroutine_fn bdrv_co_yield_to_drain(BlockDriverState *bs,
.parent = parent,
.poll = poll,
};
- bdrv_inc_in_flight(bs);
+ if (bs) {
+ bdrv_inc_in_flight(bs);
+ }
aio_bh_schedule_oneshot(bdrv_get_aio_context(bs),
bdrv_co_drain_bh_cb, &data);
@@ -464,6 +471,11 @@ void bdrv_drain_all_begin(void)
BlockDriverState *bs;
BdrvNextIterator it;
+ if (qemu_in_coroutine()) {
+ bdrv_co_yield_to_drain(NULL, true, false, NULL, true);
+ return;
+ }
+
/* BDRV_POLL_WHILE() for a node can only be called from its own I/O thread
* or the main loop AioContext. We potentially use BDRV_POLL_WHILE() on
* nodes in several different AioContexts, so make sure we're in the main
--
2.13.6
^ permalink raw reply related [flat|nested] 47+ messages in thread* [Qemu-devel] [PULL 18/35] block: ignore_bds_parents parameter for drain functions
2018-06-18 16:44 [Qemu-devel] [PULL 00/35] Block layer patches Kevin Wolf
` (16 preceding siblings ...)
2018-06-18 16:44 ` [Qemu-devel] [PULL 17/35] block: Move bdrv_drain_all_begin() out of coroutine context Kevin Wolf
@ 2018-06-18 16:44 ` Kevin Wolf
2018-06-18 16:44 ` [Qemu-devel] [PULL 19/35] block: Allow graph changes in bdrv_drain_all_begin/end sections Kevin Wolf
` (18 subsequent siblings)
36 siblings, 0 replies; 47+ messages in thread
From: Kevin Wolf @ 2018-06-18 16:44 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel
In the future, bdrv_drained_all_begin/end() will drain all invidiual
nodes separately rather than whole subtrees. This means that we don't
want to propagate the drain to all parents any more: If the parent is a
BDS, it will already be drained separately. Recursing to all parents is
unnecessary work and would make it an O(n²) operation.
Prepare the drain function for the changed drain_all by adding an
ignore_bds_parents parameter to the internal implementation that
prevents the propagation of the drain to BDS parents. We still (have to)
propagate it to non-BDS parents like BlockBackends or Jobs because those
are not drained separately.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
include/block/block.h | 16 ++++++---
include/block/block_int.h | 6 ++++
block.c | 11 +++---
block/io.c | 88 ++++++++++++++++++++++++++++-------------------
block/vvfat.c | 1 +
5 files changed, 78 insertions(+), 44 deletions(-)
diff --git a/include/block/block.h b/include/block/block.h
index 067d24cc4a..836746e4e1 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -557,7 +557,8 @@ void bdrv_io_unplug(BlockDriverState *bs);
* Begin a quiesced section of all users of @bs. This is part of
* bdrv_drained_begin.
*/
-void bdrv_parent_drained_begin(BlockDriverState *bs, BdrvChild *ignore);
+void bdrv_parent_drained_begin(BlockDriverState *bs, BdrvChild *ignore,
+ bool ignore_bds_parents);
/**
* bdrv_parent_drained_end:
@@ -565,18 +566,23 @@ void bdrv_parent_drained_begin(BlockDriverState *bs, BdrvChild *ignore);
* End a quiesced section of all users of @bs. This is part of
* bdrv_drained_end.
*/
-void bdrv_parent_drained_end(BlockDriverState *bs, BdrvChild *ignore);
+void bdrv_parent_drained_end(BlockDriverState *bs, BdrvChild *ignore,
+ bool ignore_bds_parents);
/**
* bdrv_drain_poll:
*
* Poll for pending requests in @bs, its parents (except for @ignore_parent),
- * and if @recursive is true its children as well.
+ * and if @recursive is true its children as well (used for subtree drain).
+ *
+ * If @ignore_bds_parents is true, parents that are BlockDriverStates must
+ * ignore the drain request because they will be drained separately (used for
+ * drain_all).
*
* This is part of bdrv_drained_begin.
*/
bool bdrv_drain_poll(BlockDriverState *bs, bool recursive,
- BdrvChild *ignore_parent);
+ BdrvChild *ignore_parent, bool ignore_bds_parents);
/**
* bdrv_drained_begin:
@@ -597,7 +603,7 @@ void bdrv_drained_begin(BlockDriverState *bs);
* running requests to complete.
*/
void bdrv_do_drained_begin_quiesce(BlockDriverState *bs,
- BdrvChild *parent);
+ BdrvChild *parent, bool ignore_bds_parents);
/**
* Like bdrv_drained_begin, but recursively begins a quiesced section for
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 1b811db8ec..1abfc26d76 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -577,6 +577,12 @@ struct BdrvChildRole {
* points to. */
bool stay_at_node;
+ /* If true, the parent is a BlockDriverState and bdrv_next_all_states()
+ * will return it. This information is used for drain_all, where every node
+ * will be drained separately, so the drain only needs to be propagated to
+ * non-BDS parents. */
+ bool parent_is_bds;
+
void (*inherit_options)(int *child_flags, QDict *child_options,
int parent_flags, QDict *parent_options);
diff --git a/block.c b/block.c
index 50f8e3dc3b..c8586f41ba 100644
--- a/block.c
+++ b/block.c
@@ -818,13 +818,13 @@ static char *bdrv_child_get_parent_desc(BdrvChild *c)
static void bdrv_child_cb_drained_begin(BdrvChild *child)
{
BlockDriverState *bs = child->opaque;
- bdrv_do_drained_begin_quiesce(bs, NULL);
+ bdrv_do_drained_begin_quiesce(bs, NULL, false);
}
static bool bdrv_child_cb_drained_poll(BdrvChild *child)
{
BlockDriverState *bs = child->opaque;
- return bdrv_drain_poll(bs, false, NULL);
+ return bdrv_drain_poll(bs, false, NULL, false);
}
static void bdrv_child_cb_drained_end(BdrvChild *child)
@@ -908,6 +908,7 @@ static void bdrv_inherited_options(int *child_flags, QDict *child_options,
}
const BdrvChildRole child_file = {
+ .parent_is_bds = true,
.get_parent_desc = bdrv_child_get_parent_desc,
.inherit_options = bdrv_inherited_options,
.drained_begin = bdrv_child_cb_drained_begin,
@@ -933,6 +934,7 @@ static void bdrv_inherited_fmt_options(int *child_flags, QDict *child_options,
}
const BdrvChildRole child_format = {
+ .parent_is_bds = true,
.get_parent_desc = bdrv_child_get_parent_desc,
.inherit_options = bdrv_inherited_fmt_options,
.drained_begin = bdrv_child_cb_drained_begin,
@@ -1051,6 +1053,7 @@ static int bdrv_backing_update_filename(BdrvChild *c, BlockDriverState *base,
}
const BdrvChildRole child_backing = {
+ .parent_is_bds = true,
.get_parent_desc = bdrv_child_get_parent_desc,
.attach = bdrv_backing_attach,
.detach = bdrv_backing_detach,
@@ -4957,7 +4960,7 @@ void bdrv_set_aio_context(BlockDriverState *bs, AioContext *new_context)
AioContext *ctx = bdrv_get_aio_context(bs);
aio_disable_external(ctx);
- bdrv_parent_drained_begin(bs, NULL);
+ bdrv_parent_drained_begin(bs, NULL, false);
bdrv_drain(bs); /* ensure there are no in-flight requests */
while (aio_poll(ctx, false)) {
@@ -4971,7 +4974,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);
+ bdrv_parent_drained_end(bs, NULL, false);
aio_enable_external(ctx);
aio_context_release(new_context);
}
diff --git a/block/io.c b/block/io.c
index 1a9974c136..1834a14aa6 100644
--- a/block/io.c
+++ b/block/io.c
@@ -41,12 +41,13 @@
static int coroutine_fn bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,
int64_t offset, int bytes, BdrvRequestFlags flags);
-void bdrv_parent_drained_begin(BlockDriverState *bs, BdrvChild *ignore)
+void bdrv_parent_drained_begin(BlockDriverState *bs, BdrvChild *ignore,
+ bool ignore_bds_parents)
{
BdrvChild *c, *next;
QLIST_FOREACH_SAFE(c, &bs->parents, next_parent, next) {
- if (c == ignore) {
+ if (c == ignore || (ignore_bds_parents && c->role->parent_is_bds)) {
continue;
}
if (c->role->drained_begin) {
@@ -55,12 +56,13 @@ void bdrv_parent_drained_begin(BlockDriverState *bs, BdrvChild *ignore)
}
}
-void bdrv_parent_drained_end(BlockDriverState *bs, BdrvChild *ignore)
+void bdrv_parent_drained_end(BlockDriverState *bs, BdrvChild *ignore,
+ bool ignore_bds_parents)
{
BdrvChild *c, *next;
QLIST_FOREACH_SAFE(c, &bs->parents, next_parent, next) {
- if (c == ignore) {
+ if (c == ignore || (ignore_bds_parents && c->role->parent_is_bds)) {
continue;
}
if (c->role->drained_end) {
@@ -69,13 +71,14 @@ void bdrv_parent_drained_end(BlockDriverState *bs, BdrvChild *ignore)
}
}
-static bool bdrv_parent_drained_poll(BlockDriverState *bs, BdrvChild *ignore)
+static bool bdrv_parent_drained_poll(BlockDriverState *bs, BdrvChild *ignore,
+ bool ignore_bds_parents)
{
BdrvChild *c, *next;
bool busy = false;
QLIST_FOREACH_SAFE(c, &bs->parents, next_parent, next) {
- if (c == ignore) {
+ if (c == ignore || (ignore_bds_parents && c->role->parent_is_bds)) {
continue;
}
if (c->role->drained_poll) {
@@ -167,6 +170,7 @@ typedef struct {
bool recursive;
bool poll;
BdrvChild *parent;
+ bool ignore_bds_parents;
} BdrvCoDrainData;
static void coroutine_fn bdrv_drain_invoke_entry(void *opaque)
@@ -220,11 +224,11 @@ static void bdrv_drain_invoke(BlockDriverState *bs, bool begin)
/* Returns true if BDRV_POLL_WHILE() should go into a blocking aio_poll() */
bool bdrv_drain_poll(BlockDriverState *bs, bool recursive,
- BdrvChild *ignore_parent)
+ BdrvChild *ignore_parent, bool ignore_bds_parents)
{
BdrvChild *child, *next;
- if (bdrv_parent_drained_poll(bs, ignore_parent)) {
+ if (bdrv_parent_drained_poll(bs, ignore_parent, ignore_bds_parents)) {
return true;
}
@@ -233,8 +237,9 @@ bool bdrv_drain_poll(BlockDriverState *bs, bool recursive,
}
if (recursive) {
+ assert(!ignore_bds_parents);
QLIST_FOREACH_SAFE(child, &bs->children, next, next) {
- if (bdrv_drain_poll(child->bs, recursive, child)) {
+ if (bdrv_drain_poll(child->bs, recursive, child, false)) {
return true;
}
}
@@ -250,13 +255,14 @@ static bool bdrv_drain_poll_top_level(BlockDriverState *bs, bool recursive,
* have executed. */
while (aio_poll(bs->aio_context, false));
- return bdrv_drain_poll(bs, recursive, ignore_parent);
+ return bdrv_drain_poll(bs, recursive, ignore_parent, false);
}
static void bdrv_do_drained_begin(BlockDriverState *bs, bool recursive,
- BdrvChild *parent, bool poll);
+ BdrvChild *parent, bool ignore_bds_parents,
+ bool poll);
static void bdrv_do_drained_end(BlockDriverState *bs, bool recursive,
- BdrvChild *parent);
+ BdrvChild *parent, bool ignore_bds_parents);
static void bdrv_co_drain_bh_cb(void *opaque)
{
@@ -267,9 +273,11 @@ static void bdrv_co_drain_bh_cb(void *opaque)
if (bs) {
bdrv_dec_in_flight(bs);
if (data->begin) {
- bdrv_do_drained_begin(bs, data->recursive, data->parent, data->poll);
+ bdrv_do_drained_begin(bs, data->recursive, data->parent,
+ data->ignore_bds_parents, data->poll);
} else {
- bdrv_do_drained_end(bs, data->recursive, data->parent);
+ bdrv_do_drained_end(bs, data->recursive, data->parent,
+ data->ignore_bds_parents);
}
} else {
assert(data->begin);
@@ -282,7 +290,9 @@ static void bdrv_co_drain_bh_cb(void *opaque)
static void coroutine_fn bdrv_co_yield_to_drain(BlockDriverState *bs,
bool begin, bool recursive,
- BdrvChild *parent, bool poll)
+ BdrvChild *parent,
+ bool ignore_bds_parents,
+ bool poll)
{
BdrvCoDrainData data;
@@ -297,6 +307,7 @@ static void coroutine_fn bdrv_co_yield_to_drain(BlockDriverState *bs,
.begin = begin,
.recursive = recursive,
.parent = parent,
+ .ignore_bds_parents = ignore_bds_parents,
.poll = poll,
};
if (bs) {
@@ -312,7 +323,7 @@ static void coroutine_fn bdrv_co_yield_to_drain(BlockDriverState *bs,
}
void bdrv_do_drained_begin_quiesce(BlockDriverState *bs,
- BdrvChild *parent)
+ BdrvChild *parent, bool ignore_bds_parents)
{
assert(!qemu_in_coroutine());
@@ -321,26 +332,30 @@ void bdrv_do_drained_begin_quiesce(BlockDriverState *bs,
aio_disable_external(bdrv_get_aio_context(bs));
}
- bdrv_parent_drained_begin(bs, parent);
+ bdrv_parent_drained_begin(bs, parent, ignore_bds_parents);
bdrv_drain_invoke(bs, true);
}
static void bdrv_do_drained_begin(BlockDriverState *bs, bool recursive,
- BdrvChild *parent, bool poll)
+ BdrvChild *parent, bool ignore_bds_parents,
+ bool poll)
{
BdrvChild *child, *next;
if (qemu_in_coroutine()) {
- bdrv_co_yield_to_drain(bs, true, recursive, parent, poll);
+ bdrv_co_yield_to_drain(bs, true, recursive, parent, ignore_bds_parents,
+ poll);
return;
}
- bdrv_do_drained_begin_quiesce(bs, parent);
+ bdrv_do_drained_begin_quiesce(bs, parent, ignore_bds_parents);
if (recursive) {
+ assert(!ignore_bds_parents);
bs->recursive_quiesce_counter++;
QLIST_FOREACH_SAFE(child, &bs->children, next, next) {
- bdrv_do_drained_begin(child->bs, true, child, false);
+ bdrv_do_drained_begin(child->bs, true, child, ignore_bds_parents,
+ false);
}
}
@@ -354,28 +369,30 @@ static void bdrv_do_drained_begin(BlockDriverState *bs, bool recursive,
* nodes.
*/
if (poll) {
+ assert(!ignore_bds_parents);
BDRV_POLL_WHILE(bs, bdrv_drain_poll_top_level(bs, recursive, parent));
}
}
void bdrv_drained_begin(BlockDriverState *bs)
{
- bdrv_do_drained_begin(bs, false, NULL, true);
+ bdrv_do_drained_begin(bs, false, NULL, false, true);
}
void bdrv_subtree_drained_begin(BlockDriverState *bs)
{
- bdrv_do_drained_begin(bs, true, NULL, true);
+ bdrv_do_drained_begin(bs, true, NULL, false, true);
}
-void bdrv_do_drained_end(BlockDriverState *bs, bool recursive,
- BdrvChild *parent)
+static void bdrv_do_drained_end(BlockDriverState *bs, bool recursive,
+ BdrvChild *parent, bool ignore_bds_parents)
{
BdrvChild *child, *next;
int old_quiesce_counter;
if (qemu_in_coroutine()) {
- bdrv_co_yield_to_drain(bs, false, recursive, parent, false);
+ bdrv_co_yield_to_drain(bs, false, recursive, parent, ignore_bds_parents,
+ false);
return;
}
assert(bs->quiesce_counter > 0);
@@ -383,27 +400,28 @@ void bdrv_do_drained_end(BlockDriverState *bs, bool recursive,
/* Re-enable things in child-to-parent order */
bdrv_drain_invoke(bs, false);
- bdrv_parent_drained_end(bs, parent);
+ bdrv_parent_drained_end(bs, parent, ignore_bds_parents);
if (old_quiesce_counter == 1) {
aio_enable_external(bdrv_get_aio_context(bs));
}
if (recursive) {
+ assert(!ignore_bds_parents);
bs->recursive_quiesce_counter--;
QLIST_FOREACH_SAFE(child, &bs->children, next, next) {
- bdrv_do_drained_end(child->bs, true, child);
+ bdrv_do_drained_end(child->bs, true, child, ignore_bds_parents);
}
}
}
void bdrv_drained_end(BlockDriverState *bs)
{
- bdrv_do_drained_end(bs, false, NULL);
+ bdrv_do_drained_end(bs, false, NULL, false);
}
void bdrv_subtree_drained_end(BlockDriverState *bs)
{
- bdrv_do_drained_end(bs, true, NULL);
+ bdrv_do_drained_end(bs, true, NULL, false);
}
void bdrv_apply_subtree_drain(BdrvChild *child, BlockDriverState *new_parent)
@@ -411,7 +429,7 @@ void bdrv_apply_subtree_drain(BdrvChild *child, BlockDriverState *new_parent)
int i;
for (i = 0; i < new_parent->recursive_quiesce_counter; i++) {
- bdrv_do_drained_begin(child->bs, true, child, true);
+ bdrv_do_drained_begin(child->bs, true, child, false, true);
}
}
@@ -420,7 +438,7 @@ void bdrv_unapply_subtree_drain(BdrvChild *child, BlockDriverState *old_parent)
int i;
for (i = 0; i < old_parent->recursive_quiesce_counter; i++) {
- bdrv_do_drained_end(child->bs, true, child);
+ bdrv_do_drained_end(child->bs, true, child, false);
}
}
@@ -472,7 +490,7 @@ void bdrv_drain_all_begin(void)
BdrvNextIterator it;
if (qemu_in_coroutine()) {
- bdrv_co_yield_to_drain(NULL, true, false, NULL, true);
+ bdrv_co_yield_to_drain(NULL, true, false, NULL, false, true);
return;
}
@@ -486,7 +504,7 @@ void bdrv_drain_all_begin(void)
AioContext *aio_context = bdrv_get_aio_context(bs);
aio_context_acquire(aio_context);
- bdrv_do_drained_begin(bs, true, NULL, true);
+ bdrv_do_drained_begin(bs, true, NULL, false, true);
aio_context_release(aio_context);
}
@@ -504,7 +522,7 @@ void bdrv_drain_all_end(void)
AioContext *aio_context = bdrv_get_aio_context(bs);
aio_context_acquire(aio_context);
- bdrv_do_drained_end(bs, true, NULL);
+ bdrv_do_drained_end(bs, true, NULL, false);
aio_context_release(aio_context);
}
}
diff --git a/block/vvfat.c b/block/vvfat.c
index 4595f335b8..c7d2ed2d96 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -3134,6 +3134,7 @@ static void vvfat_qcow_options(int *child_flags, QDict *child_options,
}
static const BdrvChildRole child_vvfat_qcow = {
+ .parent_is_bds = true,
.inherit_options = vvfat_qcow_options,
};
--
2.13.6
^ permalink raw reply related [flat|nested] 47+ messages in thread* [Qemu-devel] [PULL 19/35] block: Allow graph changes in bdrv_drain_all_begin/end sections
2018-06-18 16:44 [Qemu-devel] [PULL 00/35] Block layer patches Kevin Wolf
` (17 preceding siblings ...)
2018-06-18 16:44 ` [Qemu-devel] [PULL 18/35] block: ignore_bds_parents parameter for drain functions Kevin Wolf
@ 2018-06-18 16:44 ` Kevin Wolf
2018-06-18 16:44 ` [Qemu-devel] [PULL 20/35] test-bdrv-drain: Test graph changes in drain_all section Kevin Wolf
` (17 subsequent siblings)
36 siblings, 0 replies; 47+ messages in thread
From: Kevin Wolf @ 2018-06-18 16:44 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel
bdrv_drain_all_*() used bdrv_next() to iterate over all root nodes and
did a subtree drain for each of them. This works fine as long as the
graph is static, but sadly, reality looks different.
If the graph changes so that root nodes are added or removed, we would
have to compensate for this. bdrv_next() returns each root node only
once even if it's the root node for multiple BlockBackends or for a
monitor-owned block driver tree, which would only complicate things.
The much easier and more obviously correct way is to fundamentally
change the way the functions work: Iterate over all BlockDriverStates,
no matter who owns them, and drain them individually. Compensation is
only necessary when a new BDS is created inside a drain_all section.
Removal of a BDS doesn't require any action because it's gone afterwards
anyway.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
include/block/block.h | 1 +
include/block/block_int.h | 1 +
block.c | 34 ++++++++++++++++++++++++---
block/io.c | 60 ++++++++++++++++++++++++++++++++++++-----------
4 files changed, 79 insertions(+), 17 deletions(-)
diff --git a/include/block/block.h b/include/block/block.h
index 836746e4e1..b1d6fdb97a 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -421,6 +421,7 @@ BlockDriverState *bdrv_lookup_bs(const char *device,
Error **errp);
bool bdrv_chain_contains(BlockDriverState *top, BlockDriverState *base);
BlockDriverState *bdrv_next_node(BlockDriverState *bs);
+BlockDriverState *bdrv_next_all_states(BlockDriverState *bs);
typedef struct BdrvNextIterator {
enum {
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 1abfc26d76..7cd7eed83b 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -854,6 +854,7 @@ int coroutine_fn bdrv_co_pwritev(BdrvChild *child,
int64_t offset, unsigned int bytes, QEMUIOVector *qiov,
BdrvRequestFlags flags);
+extern unsigned int bdrv_drain_all_count;
void bdrv_apply_subtree_drain(BdrvChild *child, BlockDriverState *new_parent);
void bdrv_unapply_subtree_drain(BdrvChild *child, BlockDriverState *old_parent);
diff --git a/block.c b/block.c
index c8586f41ba..6c128007fd 100644
--- a/block.c
+++ b/block.c
@@ -333,6 +333,10 @@ BlockDriverState *bdrv_new(void)
qemu_co_queue_init(&bs->flush_queue);
+ for (i = 0; i < bdrv_drain_all_count; i++) {
+ bdrv_drained_begin(bs);
+ }
+
QTAILQ_INSERT_TAIL(&all_bdrv_states, bs, bs_list);
return bs;
@@ -1164,7 +1168,7 @@ static int bdrv_open_driver(BlockDriverState *bs, BlockDriver *drv,
int open_flags, Error **errp)
{
Error *local_err = NULL;
- int ret;
+ int i, ret;
bdrv_assign_node_name(bs, node_name, &local_err);
if (local_err) {
@@ -1212,6 +1216,12 @@ static int bdrv_open_driver(BlockDriverState *bs, BlockDriver *drv,
assert(bdrv_min_mem_align(bs) != 0);
assert(is_power_of_2(bs->bl.request_alignment));
+ for (i = 0; i < bs->quiesce_counter; i++) {
+ if (drv->bdrv_co_drain_begin) {
+ drv->bdrv_co_drain_begin(bs);
+ }
+ }
+
return 0;
open_failed:
bs->drv = NULL;
@@ -2033,7 +2043,12 @@ static void bdrv_replace_child_noperm(BdrvChild *child,
child->role->detach(child);
}
if (old_bs->quiesce_counter && child->role->drained_end) {
- for (i = 0; i < old_bs->quiesce_counter; i++) {
+ int num = old_bs->quiesce_counter;
+ if (child->role->parent_is_bds) {
+ num -= bdrv_drain_all_count;
+ }
+ assert(num >= 0);
+ for (i = 0; i < num; i++) {
child->role->drained_end(child);
}
}
@@ -2045,7 +2060,12 @@ static void bdrv_replace_child_noperm(BdrvChild *child,
if (new_bs) {
QLIST_INSERT_HEAD(&new_bs->parents, child, next_parent);
if (new_bs->quiesce_counter && child->role->drained_begin) {
- for (i = 0; i < new_bs->quiesce_counter; i++) {
+ int num = new_bs->quiesce_counter;
+ if (child->role->parent_is_bds) {
+ num -= bdrv_drain_all_count;
+ }
+ assert(num >= 0);
+ for (i = 0; i < num; i++) {
child->role->drained_begin(child);
}
}
@@ -4049,6 +4069,14 @@ BlockDriverState *bdrv_next_node(BlockDriverState *bs)
return QTAILQ_NEXT(bs, node_list);
}
+BlockDriverState *bdrv_next_all_states(BlockDriverState *bs)
+{
+ if (!bs) {
+ return QTAILQ_FIRST(&all_bdrv_states);
+ }
+ return QTAILQ_NEXT(bs, bs_list);
+}
+
const char *bdrv_get_node_name(const BlockDriverState *bs)
{
return bs->node_name;
diff --git a/block/io.c b/block/io.c
index 1834a14aa6..ef4fedd364 100644
--- a/block/io.c
+++ b/block/io.c
@@ -38,6 +38,8 @@
/* Maximum bounce buffer for copy-on-read and write zeroes, in bytes */
#define MAX_BOUNCE_BUFFER (32768 << BDRV_SECTOR_BITS)
+static AioWait drain_all_aio_wait;
+
static int coroutine_fn bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,
int64_t offset, int bytes, BdrvRequestFlags flags);
@@ -472,6 +474,29 @@ static void bdrv_drain_assert_idle(BlockDriverState *bs)
}
}
+unsigned int bdrv_drain_all_count = 0;
+
+static bool bdrv_drain_all_poll(void)
+{
+ BlockDriverState *bs = NULL;
+ bool result = false;
+
+ /* Execute pending BHs first (may modify the graph) and check everything
+ * else only after the BHs have executed. */
+ while (aio_poll(qemu_get_aio_context(), false));
+
+ /* bdrv_drain_poll() can't make changes to the graph and we are holding the
+ * main AioContext lock, so iterating bdrv_next_all_states() is safe. */
+ while ((bs = bdrv_next_all_states(bs))) {
+ AioContext *aio_context = bdrv_get_aio_context(bs);
+ aio_context_acquire(aio_context);
+ result |= bdrv_drain_poll(bs, false, NULL, true);
+ aio_context_release(aio_context);
+ }
+
+ return result;
+}
+
/*
* Wait for pending requests to complete across all BlockDriverStates
*
@@ -486,45 +511,51 @@ static void bdrv_drain_assert_idle(BlockDriverState *bs)
*/
void bdrv_drain_all_begin(void)
{
- BlockDriverState *bs;
- BdrvNextIterator it;
+ BlockDriverState *bs = NULL;
if (qemu_in_coroutine()) {
- bdrv_co_yield_to_drain(NULL, true, false, NULL, false, true);
+ bdrv_co_yield_to_drain(NULL, true, false, NULL, true, true);
return;
}
- /* BDRV_POLL_WHILE() for a node can only be called from its own I/O thread
- * or the main loop AioContext. We potentially use BDRV_POLL_WHILE() on
- * nodes in several different AioContexts, so make sure we're in the main
- * context. */
+ /* AIO_WAIT_WHILE() with a NULL context can only be called from the main
+ * loop AioContext, so make sure we're in the main context. */
assert(qemu_get_current_aio_context() == qemu_get_aio_context());
+ assert(bdrv_drain_all_count < INT_MAX);
+ bdrv_drain_all_count++;
- for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
+ /* Quiesce all nodes, without polling in-flight requests yet. The graph
+ * cannot change during this loop. */
+ while ((bs = bdrv_next_all_states(bs))) {
AioContext *aio_context = bdrv_get_aio_context(bs);
aio_context_acquire(aio_context);
- bdrv_do_drained_begin(bs, true, NULL, false, true);
+ bdrv_do_drained_begin(bs, false, NULL, true, false);
aio_context_release(aio_context);
}
- for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
+ /* Now poll the in-flight requests */
+ AIO_WAIT_WHILE(&drain_all_aio_wait, NULL, bdrv_drain_all_poll());
+
+ while ((bs = bdrv_next_all_states(bs))) {
bdrv_drain_assert_idle(bs);
}
}
void bdrv_drain_all_end(void)
{
- BlockDriverState *bs;
- BdrvNextIterator it;
+ BlockDriverState *bs = NULL;
- for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
+ while ((bs = bdrv_next_all_states(bs))) {
AioContext *aio_context = bdrv_get_aio_context(bs);
aio_context_acquire(aio_context);
- bdrv_do_drained_end(bs, true, NULL, false);
+ bdrv_do_drained_end(bs, false, NULL, true);
aio_context_release(aio_context);
}
+
+ assert(bdrv_drain_all_count > 0);
+ bdrv_drain_all_count--;
}
void bdrv_drain_all(void)
@@ -647,6 +678,7 @@ void bdrv_inc_in_flight(BlockDriverState *bs)
void bdrv_wakeup(BlockDriverState *bs)
{
aio_wait_kick(bdrv_get_aio_wait(bs));
+ aio_wait_kick(&drain_all_aio_wait);
}
void bdrv_dec_in_flight(BlockDriverState *bs)
--
2.13.6
^ permalink raw reply related [flat|nested] 47+ messages in thread* [Qemu-devel] [PULL 20/35] test-bdrv-drain: Test graph changes in drain_all section
2018-06-18 16:44 [Qemu-devel] [PULL 00/35] Block layer patches Kevin Wolf
` (18 preceding siblings ...)
2018-06-18 16:44 ` [Qemu-devel] [PULL 19/35] block: Allow graph changes in bdrv_drain_all_begin/end sections Kevin Wolf
@ 2018-06-18 16:44 ` Kevin Wolf
2018-06-18 16:44 ` [Qemu-devel] [PULL 21/35] block: fix QEMU crash with scsi-hd and drive_del Kevin Wolf
` (16 subsequent siblings)
36 siblings, 0 replies; 47+ messages in thread
From: Kevin Wolf @ 2018-06-18 16:44 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel
This tests both adding and remove a node between bdrv_drain_all_begin()
and bdrv_drain_all_end(), and enabled the existing detach test for
drain_all.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
tests/test-bdrv-drain.c | 75 +++++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 73 insertions(+), 2 deletions(-)
diff --git a/tests/test-bdrv-drain.c b/tests/test-bdrv-drain.c
index 5cc179a778..291a050f86 100644
--- a/tests/test-bdrv-drain.c
+++ b/tests/test-bdrv-drain.c
@@ -452,7 +452,7 @@ static void test_multiparent(void)
blk_unref(blk_b);
}
-static void test_graph_change(void)
+static void test_graph_change_drain_subtree(void)
{
BlockBackend *blk_a, *blk_b;
BlockDriverState *bs_a, *bs_b, *backing;
@@ -531,6 +531,63 @@ static void test_graph_change(void)
blk_unref(blk_b);
}
+static void test_graph_change_drain_all(void)
+{
+ BlockBackend *blk_a, *blk_b;
+ BlockDriverState *bs_a, *bs_b;
+ BDRVTestState *a_s, *b_s;
+
+ /* Create node A with a BlockBackend */
+ blk_a = blk_new(BLK_PERM_ALL, BLK_PERM_ALL);
+ bs_a = bdrv_new_open_driver(&bdrv_test, "test-node-a", BDRV_O_RDWR,
+ &error_abort);
+ a_s = bs_a->opaque;
+ blk_insert_bs(blk_a, bs_a, &error_abort);
+
+ g_assert_cmpint(bs_a->quiesce_counter, ==, 0);
+ g_assert_cmpint(a_s->drain_count, ==, 0);
+
+ /* Call bdrv_drain_all_begin() */
+ bdrv_drain_all_begin();
+
+ g_assert_cmpint(bs_a->quiesce_counter, ==, 1);
+ g_assert_cmpint(a_s->drain_count, ==, 1);
+
+ /* Create node B with a BlockBackend */
+ blk_b = blk_new(BLK_PERM_ALL, BLK_PERM_ALL);
+ bs_b = bdrv_new_open_driver(&bdrv_test, "test-node-b", BDRV_O_RDWR,
+ &error_abort);
+ b_s = bs_b->opaque;
+ blk_insert_bs(blk_b, bs_b, &error_abort);
+
+ g_assert_cmpint(bs_a->quiesce_counter, ==, 1);
+ g_assert_cmpint(bs_b->quiesce_counter, ==, 1);
+ g_assert_cmpint(a_s->drain_count, ==, 1);
+ g_assert_cmpint(b_s->drain_count, ==, 1);
+
+ /* Unref and finally delete node A */
+ blk_unref(blk_a);
+
+ g_assert_cmpint(bs_a->quiesce_counter, ==, 1);
+ g_assert_cmpint(bs_b->quiesce_counter, ==, 1);
+ g_assert_cmpint(a_s->drain_count, ==, 1);
+ g_assert_cmpint(b_s->drain_count, ==, 1);
+
+ bdrv_unref(bs_a);
+
+ g_assert_cmpint(bs_b->quiesce_counter, ==, 1);
+ g_assert_cmpint(b_s->drain_count, ==, 1);
+
+ /* End the drained section */
+ bdrv_drain_all_end();
+
+ g_assert_cmpint(bs_b->quiesce_counter, ==, 0);
+ g_assert_cmpint(b_s->drain_count, ==, 0);
+
+ bdrv_unref(bs_b);
+ blk_unref(blk_b);
+}
+
struct test_iothread_data {
BlockDriverState *bs;
enum drain_type drain_type;
@@ -971,6 +1028,10 @@ static void do_test_delete_by_drain(bool detach_instead_of_delete,
bdrv_subtree_drained_begin(bs);
bdrv_subtree_drained_end(bs);
break;
+ case BDRV_DRAIN_ALL:
+ bdrv_drain_all_begin();
+ bdrv_drain_all_end();
+ break;
default:
g_assert_not_reached();
}
@@ -991,6 +1052,11 @@ static void test_delete_by_drain(void)
do_test_delete_by_drain(false, BDRV_DRAIN);
}
+static void test_detach_by_drain_all(void)
+{
+ do_test_delete_by_drain(true, BDRV_DRAIN_ALL);
+}
+
static void test_detach_by_drain(void)
{
do_test_delete_by_drain(true, BDRV_DRAIN);
@@ -1219,7 +1285,11 @@ int main(int argc, char **argv)
g_test_add_func("/bdrv-drain/nested", test_nested);
g_test_add_func("/bdrv-drain/multiparent", test_multiparent);
- g_test_add_func("/bdrv-drain/graph-change", test_graph_change);
+
+ g_test_add_func("/bdrv-drain/graph-change/drain_subtree",
+ test_graph_change_drain_subtree);
+ g_test_add_func("/bdrv-drain/graph-change/drain_all",
+ test_graph_change_drain_all);
g_test_add_func("/bdrv-drain/iothread/drain_all", test_iothread_drain_all);
g_test_add_func("/bdrv-drain/iothread/drain", test_iothread_drain);
@@ -1232,6 +1302,7 @@ int main(int argc, char **argv)
test_blockjob_drain_subtree);
g_test_add_func("/bdrv-drain/deletion/drain", test_delete_by_drain);
+ g_test_add_func("/bdrv-drain/detach/drain_all", test_detach_by_drain_all);
g_test_add_func("/bdrv-drain/detach/drain", test_detach_by_drain);
g_test_add_func("/bdrv-drain/detach/drain_subtree", test_detach_by_drain_subtree);
g_test_add_func("/bdrv-drain/detach/parent_cb", test_detach_by_parent_cb);
--
2.13.6
^ permalink raw reply related [flat|nested] 47+ messages in thread* [Qemu-devel] [PULL 21/35] block: fix QEMU crash with scsi-hd and drive_del
2018-06-18 16:44 [Qemu-devel] [PULL 00/35] Block layer patches Kevin Wolf
` (19 preceding siblings ...)
2018-06-18 16:44 ` [Qemu-devel] [PULL 20/35] test-bdrv-drain: Test graph changes in drain_all section Kevin Wolf
@ 2018-06-18 16:44 ` Kevin Wolf
2018-08-06 22:04 ` Eric Blake
2018-06-18 16:44 ` [Qemu-devel] [PULL 22/35] block/mirror: Pull out mirror_perform() Kevin Wolf
` (15 subsequent siblings)
36 siblings, 1 reply; 47+ messages in thread
From: Kevin Wolf @ 2018-06-18 16:44 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel
From: Greg Kurz <groug@kaod.org>
Removing a drive with drive_del while it is being used to run an I/O
intensive workload can cause QEMU to crash.
An AIO flush can yield at some point:
blk_aio_flush_entry()
blk_co_flush(blk)
bdrv_co_flush(blk->root->bs)
...
qemu_coroutine_yield()
and let the HMP command to run, free blk->root and give control
back to the AIO flush:
hmp_drive_del()
blk_remove_bs()
bdrv_root_unref_child(blk->root)
child_bs = blk->root->bs
bdrv_detach_child(blk->root)
bdrv_replace_child(blk->root, NULL)
blk->root->bs = NULL
g_free(blk->root) <============== blk->root becomes stale
bdrv_unref(child_bs)
bdrv_delete(child_bs)
bdrv_close()
bdrv_drained_begin()
bdrv_do_drained_begin()
bdrv_drain_recurse()
aio_poll()
...
qemu_coroutine_switch()
and the AIO flush completion ends up dereferencing blk->root:
blk_aio_complete()
scsi_aio_complete()
blk_get_aio_context(blk)
bs = blk_bs(blk)
ie, bs = blk->root ? blk->root->bs : NULL
^^^^^
stale
The problem is that we should avoid making block driver graph
changes while we have in-flight requests. Let's drain all I/O
for this BB before calling bdrv_root_unref_child().
Signed-off-by: Greg Kurz <groug@kaod.org>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
block/block-backend.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/block/block-backend.c b/block/block-backend.c
index 2d1a3463e8..6b75bca317 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -767,6 +767,11 @@ void blk_remove_bs(BlockBackend *blk)
blk_update_root_state(blk);
+ /* bdrv_root_unref_child() will cause blk->root to become stale and may
+ * switch to a completion coroutine later on. Let's drain all I/O here
+ * to avoid that and a potential QEMU crash.
+ */
+ blk_drain(blk);
bdrv_root_unref_child(blk->root);
blk->root = NULL;
}
--
2.13.6
^ permalink raw reply related [flat|nested] 47+ messages in thread* Re: [Qemu-devel] [PULL 21/35] block: fix QEMU crash with scsi-hd and drive_del
2018-06-18 16:44 ` [Qemu-devel] [PULL 21/35] block: fix QEMU crash with scsi-hd and drive_del Kevin Wolf
@ 2018-08-06 22:04 ` Eric Blake
2018-08-07 19:57 ` Eric Blake
0 siblings, 1 reply; 47+ messages in thread
From: Eric Blake @ 2018-08-06 22:04 UTC (permalink / raw)
To: Kevin Wolf, qemu-block; +Cc: qemu-devel, Max Reitz
On 06/18/2018 11:44 AM, Kevin Wolf wrote:
> From: Greg Kurz <groug@kaod.org>
>
> Removing a drive with drive_del while it is being used to run an I/O
> intensive workload can cause QEMU to crash.
>
...
> The problem is that we should avoid making block driver graph
> changes while we have in-flight requests. Let's drain all I/O
> for this BB before calling bdrv_root_unref_child().
>
> Signed-off-by: Greg Kurz <groug@kaod.org>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> block/block-backend.c | 5 +++++
> 1 file changed, 5 insertions(+)
Interestingly enough, git bisect is reliably pointing to this commit as
the culprit in the changed output of iotests './check -nbd 83':
083 8s ... - output mismatch (see 083.out.bad)
--- /home/eblake/qemu/tests/qemu-iotests/083.out 2018-02-26
11:40:31.605792220 -0600
+++ /home/eblake/qemu/tests/qemu-iotests/083.out.bad 2018-08-06
16:57:03.411861660 -0500
@@ -41,7 +41,6 @@
=== Check disconnect after neg2 ===
-Connection closed
read failed: Input/output error
=== Check disconnect 8 neg2 ===
@@ -54,7 +53,6 @@
=== Check disconnect before request ===
-Connection closed
read failed: Input/output error
=== Check disconnect after request ===
@@ -116,7 +114,6 @@
=== Check disconnect after neg-classic ===
-Connection closed
read failed: Input/output error
=== Check disconnect before neg1 ===
Failures: 083
Failed 1 of 1 tests
>
> diff --git a/block/block-backend.c b/block/block-backend.c
> index 2d1a3463e8..6b75bca317 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -767,6 +767,11 @@ void blk_remove_bs(BlockBackend *blk)
>
> blk_update_root_state(blk);
>
> + /* bdrv_root_unref_child() will cause blk->root to become stale and may
> + * switch to a completion coroutine later on. Let's drain all I/O here
> + * to avoid that and a potential QEMU crash.
> + */
> + blk_drain(blk);
> bdrv_root_unref_child(blk->root);
> blk->root = NULL;
> }
>
Test 83 sets up a client that intentionally disconnects at critical
points in the NBD protocol exchange, to ensure that the server reacts
sanely. I suspect that somewhere in the NBD code, the server detects
the disconnect and was somehow calling into blk_remove_bs() (although I
could not quickly find the backtrace); and that prior to this patch, the
'Connection closed' message resulted from other NBD coroutines getting a
shot at the (now-closed) connection, while after this patch, the
additional blk_drain() somehow tweaks things in a way that prevents the
other NBD coroutines from printing a message. If so, then the change in
83 reference output is probably intentional, and we should update it.
But I'm having a hard time convincing myself that this is the case,
particularly since I'm not even sure how to easily debug the assumptions
I made above.
Since I'm very weak on the whole notion of what blk_drain() vs.
blk_remove_bs() is really supposed to be doing, and could easily be
persuaded that the change in output is a regression instead of a fix.
Thoughts?
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [Qemu-devel] [PULL 21/35] block: fix QEMU crash with scsi-hd and drive_del
2018-08-06 22:04 ` Eric Blake
@ 2018-08-07 19:57 ` Eric Blake
2018-08-08 9:33 ` Vladimir Sementsov-Ogievskiy
2018-08-08 11:40 ` Vladimir Sementsov-Ogievskiy
0 siblings, 2 replies; 47+ messages in thread
From: Eric Blake @ 2018-08-07 19:57 UTC (permalink / raw)
To: Kevin Wolf, qemu-block
Cc: qemu-devel, Max Reitz, Vladimir Sementsov-Ogievskiy
On 08/06/2018 05:04 PM, Eric Blake wrote:
> On 06/18/2018 11:44 AM, Kevin Wolf wrote:
>> From: Greg Kurz <groug@kaod.org>
>>
>> Removing a drive with drive_del while it is being used to run an I/O
>> intensive workload can cause QEMU to crash.
>>
> ...
>
> Test 83 sets up a client that intentionally disconnects at critical
> points in the NBD protocol exchange, to ensure that the server reacts
> sanely.
Rather, nbd-fault-injector.py is a server that disconnects at critical
points, and the test is of client reaction.
> I suspect that somewhere in the NBD code, the server detects
> the disconnect and was somehow calling into blk_remove_bs() (although I
> could not quickly find the backtrace); and that prior to this patch, the
> 'Connection closed' message resulted from other NBD coroutines getting a
> shot at the (now-closed) connection, while after this patch, the
> additional blk_drain() somehow tweaks things in a way that prevents the
> other NBD coroutines from printing a message. If so, then the change in
> 83 reference output is probably intentional, and we should update it.
It seems like this condition is racy, and that the race is more likely
to be lost prior to this patch than after. It's a question of whether
the client has time to start a request to the server prior to the server
hanging up, as the message is generated during
nbd_co_do_receive_one_chunk. Here's a demonstration of the fact that
things are racy:
$ git revert f45280cbf
$ make
$ cd tests/qemu-iotests
$ cat fault.txt
[inject-error "a"]
event=neg2
when=after
$ python nbd-fault-injector.py localhost:10809 ./fault.txt &
Listening on 127.0.0.1:10809
$ ../../qemu-io -f raw nbd://localhost:10809 -c 'r 0 512'
Closing connection on rule match inject-error "a"
Connection closed
read failed: Input/output error
$ python nbd-fault-injector.py localhost:10809 ./fault.txt &
Listening on 127.0.0.1:10809
$ ../../qemu-io -f raw nbd://localhost:10809
Closing connection on rule match inject-error "a"
qemu-io> r 0 512
read failed: Input/output error
qemu-io> q
So, depending on whether the read command is kicked off quickly (via -c)
or slowly (via typing into qemu-io) determines whether the message appears.
What's more, in commit f140e300, we specifically called out in the
commit message that maybe it was better to trace when we detect
connection closed rather than log it to stdout, and in all cases in that
commit, the additional 'Connection closed' messages do not add any
information to the error message already displayed by the rest of the code.
I don't know how much the proposed NBD reconnect code will change things
in 3.1. Meanwhile, we've missed any chance for 3.0 to fix test 83.
>
> But I'm having a hard time convincing myself that this is the case,
> particularly since I'm not even sure how to easily debug the assumptions
> I made above.
>
> Since I'm very weak on the whole notion of what blk_drain() vs.
> blk_remove_bs() is really supposed to be doing, and could easily be
> persuaded that the change in output is a regression instead of a fix.
At this point, I don't think we have a regression, just merely a bad
iotests reference output. The extra blk_drain() merely adds more time
before the NBD code can send out its first request, and thus makes it
more likely that the fault injector has closed the connection before the
read request is issued rather than after (the message only appears when
read beats the race), but the NBD code shouldn't be printing the error
message in the first place, and 083 needs to be tweaked to remove the
noisy lines added in f140e300 (not just the three lines that are
reliably different due to this patch, but all other such lines due to
strategic server drops at other points in the NBD protocol).
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [Qemu-devel] [PULL 21/35] block: fix QEMU crash with scsi-hd and drive_del
2018-08-07 19:57 ` Eric Blake
@ 2018-08-08 9:33 ` Vladimir Sementsov-Ogievskiy
2018-08-08 14:32 ` Vladimir Sementsov-Ogievskiy
2018-08-08 11:40 ` Vladimir Sementsov-Ogievskiy
1 sibling, 1 reply; 47+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2018-08-08 9:33 UTC (permalink / raw)
To: Eric Blake, Kevin Wolf, qemu-block; +Cc: qemu-devel, Max Reitz
07.08.2018 22:57, Eric Blake wrote:
> On 08/06/2018 05:04 PM, Eric Blake wrote:
>> On 06/18/2018 11:44 AM, Kevin Wolf wrote:
>>> From: Greg Kurz <groug@kaod.org>
>>>
>>> Removing a drive with drive_del while it is being used to run an I/O
>>> intensive workload can cause QEMU to crash.
>>>
>> ...
>
>>
>> Test 83 sets up a client that intentionally disconnects at critical
>> points in the NBD protocol exchange, to ensure that the server reacts
>> sanely.
>
> Rather, nbd-fault-injector.py is a server that disconnects at critical
> points, and the test is of client reaction.
>
>> I suspect that somewhere in the NBD code, the server detects the
>> disconnect and was somehow calling into blk_remove_bs() (although I
>> could not quickly find the backtrace); and that prior to this patch,
>> the 'Connection closed' message resulted from other NBD coroutines
>> getting a shot at the (now-closed) connection, while after this
>> patch, the additional blk_drain() somehow tweaks things in a way that
>> prevents the other NBD coroutines from printing a message. If so,
>> then the change in 83 reference output is probably intentional, and
>> we should update it.
>
> It seems like this condition is racy, and that the race is more likely
> to be lost prior to this patch than after. It's a question of whether
> the client has time to start a request to the server prior to the
> server hanging up, as the message is generated during
> nbd_co_do_receive_one_chunk. Here's a demonstration of the fact that
> things are racy:
>
> $ git revert f45280cbf
> $ make
> $ cd tests/qemu-iotests
> $ cat fault.txt
> [inject-error "a"]
> event=neg2
> when=after
> $ python nbd-fault-injector.py localhost:10809 ./fault.txt &
> Listening on 127.0.0.1:10809
> $ ../../qemu-io -f raw nbd://localhost:10809 -c 'r 0 512'
> Closing connection on rule match inject-error "a"
> Connection closed
> read failed: Input/output error
> $ python nbd-fault-injector.py localhost:10809 ./fault.txt &
> Listening on 127.0.0.1:10809
> $ ../../qemu-io -f raw nbd://localhost:10809
> Closing connection on rule match inject-error "a"
> qemu-io> r 0 512
> read failed: Input/output error
> qemu-io> q
>
> So, depending on whether the read command is kicked off quickly (via
> -c) or slowly (via typing into qemu-io) determines whether the message
> appears.
>
> What's more, in commit f140e300, we specifically called out in the
> commit message that maybe it was better to trace when we detect
> connection closed rather than log it to stdout, and in all cases in
> that commit, the additional 'Connection closed' messages do not add
> any information to the error message already displayed by the rest of
> the code.
>
> I don't know how much the proposed NBD reconnect code will change
> things in 3.1. Meanwhile, we've missed any chance for 3.0 to fix test
> 83.
>
>>
>> But I'm having a hard time convincing myself that this is the case,
>> particularly since I'm not even sure how to easily debug the
>> assumptions I made above.
>>
>> Since I'm very weak on the whole notion of what blk_drain() vs.
>> blk_remove_bs() is really supposed to be doing, and could easily be
>> persuaded that the change in output is a regression instead of a fix.
>
> At this point, I don't think we have a regression, just merely a bad
> iotests reference output. The extra blk_drain() merely adds more time
> before the NBD code can send out its first request, and thus makes it
> more likely that the fault injector has closed the connection before
> the read request is issued rather than after (the message only appears
> when read beats the race), but the NBD code shouldn't be printing the
> error message in the first place, and 083 needs to be tweaked to
> remove the noisy lines added in f140e300 (not just the three lines
> that are reliably different due to this patch, but all other such
> lines due to strategic server drops at other points in the NBD protocol).
>
Ok, agree, I'll do it in reconnect series.
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [Qemu-devel] [PULL 21/35] block: fix QEMU crash with scsi-hd and drive_del
2018-08-08 9:33 ` Vladimir Sementsov-Ogievskiy
@ 2018-08-08 14:32 ` Vladimir Sementsov-Ogievskiy
2018-08-08 14:53 ` Eric Blake
0 siblings, 1 reply; 47+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2018-08-08 14:32 UTC (permalink / raw)
To: Eric Blake, Kevin Wolf, qemu-block; +Cc: qemu-devel, Max Reitz
08.08.2018 12:33, Vladimir Sementsov-Ogievskiy wrote:
> 07.08.2018 22:57, Eric Blake wrote:
>> On 08/06/2018 05:04 PM, Eric Blake wrote:
>>> On 06/18/2018 11:44 AM, Kevin Wolf wrote:
>>>> From: Greg Kurz <groug@kaod.org>
>>>>
>>>> Removing a drive with drive_del while it is being used to run an I/O
>>>> intensive workload can cause QEMU to crash.
>>>>
>>> ...
>>
>>>
>>> Test 83 sets up a client that intentionally disconnects at critical
>>> points in the NBD protocol exchange, to ensure that the server
>>> reacts sanely.
>>
>> Rather, nbd-fault-injector.py is a server that disconnects at
>> critical points, and the test is of client reaction.
>>
>>> I suspect that somewhere in the NBD code, the server detects the
>>> disconnect and was somehow calling into blk_remove_bs() (although I
>>> could not quickly find the backtrace); and that prior to this patch,
>>> the 'Connection closed' message resulted from other NBD coroutines
>>> getting a shot at the (now-closed) connection, while after this
>>> patch, the additional blk_drain() somehow tweaks things in a way
>>> that prevents the other NBD coroutines from printing a message. If
>>> so, then the change in 83 reference output is probably intentional,
>>> and we should update it.
>>
>> It seems like this condition is racy, and that the race is more
>> likely to be lost prior to this patch than after. It's a question of
>> whether the client has time to start a request to the server prior to
>> the server hanging up, as the message is generated during
>> nbd_co_do_receive_one_chunk. Here's a demonstration of the fact that
>> things are racy:
>>
>> $ git revert f45280cbf
>> $ make
>> $ cd tests/qemu-iotests
>> $ cat fault.txt
>> [inject-error "a"]
>> event=neg2
>> when=after
>> $ python nbd-fault-injector.py localhost:10809 ./fault.txt &
>> Listening on 127.0.0.1:10809
>> $ ../../qemu-io -f raw nbd://localhost:10809 -c 'r 0 512'
>> Closing connection on rule match inject-error "a"
>> Connection closed
>> read failed: Input/output error
>> $ python nbd-fault-injector.py localhost:10809 ./fault.txt &
>> Listening on 127.0.0.1:10809
>> $ ../../qemu-io -f raw nbd://localhost:10809
>> Closing connection on rule match inject-error "a"
>> qemu-io> r 0 512
>> read failed: Input/output error
>> qemu-io> q
>>
>> So, depending on whether the read command is kicked off quickly (via
>> -c) or slowly (via typing into qemu-io) determines whether the
>> message appears.
>>
>> What's more, in commit f140e300, we specifically called out in the
>> commit message that maybe it was better to trace when we detect
>> connection closed rather than log it to stdout, and in all cases in
>> that commit, the additional 'Connection closed' messages do not add
>> any information to the error message already displayed by the rest of
>> the code.
>>
>> I don't know how much the proposed NBD reconnect code will change
>> things in 3.1. Meanwhile, we've missed any chance for 3.0 to fix
>> test 83.
>>
>>>
>>> But I'm having a hard time convincing myself that this is the case,
>>> particularly since I'm not even sure how to easily debug the
>>> assumptions I made above.
>>>
>>> Since I'm very weak on the whole notion of what blk_drain() vs.
>>> blk_remove_bs() is really supposed to be doing, and could easily be
>>> persuaded that the change in output is a regression instead of a fix.
>>
>> At this point, I don't think we have a regression, just merely a bad
>> iotests reference output. The extra blk_drain() merely adds more time
>> before the NBD code can send out its first request, and thus makes it
>> more likely that the fault injector has closed the connection before
>> the read request is issued rather than after (the message only
>> appears when read beats the race), but the NBD code shouldn't be
>> printing the error message in the first place, and 083 needs to be
>> tweaked to remove the noisy lines added in f140e300 (not just the
>> three lines that are reliably different due to this patch, but all
>> other such lines due to strategic server drops at other points in the
>> NBD protocol).
>>
>
> Ok, agree, I'll do it in reconnect series.
>
hmm, do what?
I was going to change these error messages to be traces, but now I'm not
sure that it's a good idea. We have generic errp returned from the
function, and why to drop it from logs? Fixing iotest is not a good
reason, better is to adjust iotest itself a bit (just commit changed
output) and forget about it. Is iotest racy itself, did you see
different output running 83 iotest, not testing by hand?
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [Qemu-devel] [PULL 21/35] block: fix QEMU crash with scsi-hd and drive_del
2018-08-08 14:32 ` Vladimir Sementsov-Ogievskiy
@ 2018-08-08 14:53 ` Eric Blake
0 siblings, 0 replies; 47+ messages in thread
From: Eric Blake @ 2018-08-08 14:53 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, Kevin Wolf, qemu-block
Cc: qemu-devel, Max Reitz
On 08/08/2018 09:32 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> What's more, in commit f140e300, we specifically called out in the
>>> commit message that maybe it was better to trace when we detect
>>> connection closed rather than log it to stdout, and in all cases in
>>> that commit, the additional 'Connection closed' messages do not add
>>> any information to the error message already displayed by the rest of
>>> the code.
>>>
>> Ok, agree, I'll do it in reconnect series.
>>
>
>
> hmm, do what?
>
> I was going to change these error messages to be traces, but now I'm not
> sure that it's a good idea.
Traces are fine. They won't show up in iotests, but will show up when
debugging a failed connection.
> We have generic errp returned from the
> function, and why to drop it from logs?
Because it is redundant with the very next line already in the log. Any
error encountered when trying to write to a disconnected server is
redundant with an already-reported error due to detecting EOF on reading
from the server.
> Fixing iotest is not a good
> reason, better is to adjust iotest itself a bit (just commit changed
> output) and forget about it. Is iotest racy itself, did you see
> different output running 83 iotest, not testing by hand?
The condition for the output of the 'Connection closed' message is racy
- it depends entirely on the timing of whether the client was able to
send() a read request to the server prior to the server disconnecting
immediately after negotiation ended. If the client loses the race and
detects the server hangup prior to writing anything, you get one path;
if the client wins the race and successfully writes the request and only
later learns that the server has disconnected when trying to read the
response to that request, you get the other path. The window for the
race changed (and the iotests did not seem to ever expose it short of
this particular change to the block layer to do an extra drain), but I
could still imagine scenarios where iotests will trigger the opposite
path of the race from what is expected, depending on load, since I don't
see any synchronization points between the two processes where the
server is hanging up after negotiation without reading the client's
request, but where the client may or may not have had time to get its
request sent to the server's queue.
So, just because I have not seen the iotest fail directly because of a
race, I think that this commit causing failures in the iotest is
evidence that the test is not robust with those extra 'Connection
closed' messages being output. Switching the output to be a trace
instead should be just fine; overall, the client's attempt to read when
the server hangs up will be an EIO failure whether or not the client was
able to send() its request and merely fails to get a reply (server
disconnect was slow), or whether the client was not even able to send()
its request (server disconnect was fast).
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [Qemu-devel] [PULL 21/35] block: fix QEMU crash with scsi-hd and drive_del
2018-08-07 19:57 ` Eric Blake
2018-08-08 9:33 ` Vladimir Sementsov-Ogievskiy
@ 2018-08-08 11:40 ` Vladimir Sementsov-Ogievskiy
2018-08-08 12:53 ` Eric Blake
1 sibling, 1 reply; 47+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2018-08-08 11:40 UTC (permalink / raw)
To: Eric Blake, Kevin Wolf, qemu-block; +Cc: qemu-devel, Max Reitz
07.08.2018 22:57, Eric Blake wrote:
> On 08/06/2018 05:04 PM, Eric Blake wrote:
>> On 06/18/2018 11:44 AM, Kevin Wolf wrote:
>>> From: Greg Kurz <groug@kaod.org>
>>>
>>> Removing a drive with drive_del while it is being used to run an I/O
>>> intensive workload can cause QEMU to crash.
>>>
>> ...
>
>>
>> Test 83 sets up a client that intentionally disconnects at critical
>> points in the NBD protocol exchange, to ensure that the server reacts
>> sanely.
>
> Rather, nbd-fault-injector.py is a server that disconnects at critical
> points, and the test is of client reaction.
>
>> I suspect that somewhere in the NBD code, the server detects the
>> disconnect and was somehow calling into blk_remove_bs() (although I
>> could not quickly find the backtrace); and that prior to this patch,
>> the 'Connection closed' message resulted from other NBD coroutines
>> getting a shot at the (now-closed) connection, while after this
>> patch, the additional blk_drain() somehow tweaks things in a way that
>> prevents the other NBD coroutines from printing a message. If so,
>> then the change in 83 reference output is probably intentional, and
>> we should update it.
>
> It seems like this condition is racy, and that the race is more likely
> to be lost prior to this patch than after. It's a question of whether
> the client has time to start a request to the server prior to the
> server hanging up, as the message is generated during
> nbd_co_do_receive_one_chunk. Here's a demonstration of the fact that
> things are racy:
>
> $ git revert f45280cbf
> $ make
> $ cd tests/qemu-iotests
> $ cat fault.txt
> [inject-error "a"]
> event=neg2
> when=after
> $ python nbd-fault-injector.py localhost:10809 ./fault.txt &
> Listening on 127.0.0.1:10809
> $ ../../qemu-io -f raw nbd://localhost:10809 -c 'r 0 512'
> Closing connection on rule match inject-error "a"
> Connection closed
> read failed: Input/output error
> $ python nbd-fault-injector.py localhost:10809 ./fault.txt &
> Listening on 127.0.0.1:10809
> $ ../../qemu-io -f raw nbd://localhost:10809
> Closing connection on rule match inject-error "a"
> qemu-io> r 0 512
> read failed: Input/output error
> qemu-io> q
>
> So, depending on whether the read command is kicked off quickly (via
> -c) or slowly (via typing into qemu-io) determines whether the message
> appears.
>
> What's more, in commit f140e300, we specifically called out in the
> commit message that maybe it was better to trace when we detect
> connection closed rather than log it to stdout, and in all cases in
> that commit, the additional 'Connection closed' messages do not add
> any information to the error message already displayed by the rest of
> the code.
>
> I don't know how much the proposed NBD reconnect code will change
> things in 3.1. Meanwhile, we've missed any chance for 3.0 to fix test
> 83.
hmm, isn't 3.0 release planned on August 14th?
>
>>
>> But I'm having a hard time convincing myself that this is the case,
>> particularly since I'm not even sure how to easily debug the
>> assumptions I made above.
>>
>> Since I'm very weak on the whole notion of what blk_drain() vs.
>> blk_remove_bs() is really supposed to be doing, and could easily be
>> persuaded that the change in output is a regression instead of a fix.
>
> At this point, I don't think we have a regression, just merely a bad
> iotests reference output. The extra blk_drain() merely adds more time
> before the NBD code can send out its first request, and thus makes it
> more likely that the fault injector has closed the connection before
> the read request is issued rather than after (the message only appears
> when read beats the race), but the NBD code shouldn't be printing the
> error message in the first place, and 083 needs to be tweaked to
> remove the noisy lines added in f140e300 (not just the three lines
> that are reliably different due to this patch, but all other such
> lines due to strategic server drops at other points in the NBD protocol).
>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [Qemu-devel] [PULL 21/35] block: fix QEMU crash with scsi-hd and drive_del
2018-08-08 11:40 ` Vladimir Sementsov-Ogievskiy
@ 2018-08-08 12:53 ` Eric Blake
0 siblings, 0 replies; 47+ messages in thread
From: Eric Blake @ 2018-08-08 12:53 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, Kevin Wolf, qemu-block
Cc: qemu-devel, Max Reitz
On 08/08/2018 06:40 AM, Vladimir Sementsov-Ogievskiy wrote:
>> I don't know how much the proposed NBD reconnect code will change
>> things in 3.1. Meanwhile, we've missed any chance for 3.0 to fix test
>> 83.
>
> hmm, isn't 3.0 release planned on August 14th?
Yes, but -rc4 has already been cut, and there will not be an -rc5 unless
there is a MAJOR regression identified in qemu proper (a failing iotest
is not major).
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 47+ messages in thread
* [Qemu-devel] [PULL 22/35] block/mirror: Pull out mirror_perform()
2018-06-18 16:44 [Qemu-devel] [PULL 00/35] Block layer patches Kevin Wolf
` (20 preceding siblings ...)
2018-06-18 16:44 ` [Qemu-devel] [PULL 21/35] block: fix QEMU crash with scsi-hd and drive_del Kevin Wolf
@ 2018-06-18 16:44 ` Kevin Wolf
2018-06-18 16:44 ` [Qemu-devel] [PULL 23/35] block/mirror: Convert to coroutines Kevin Wolf
` (14 subsequent siblings)
36 siblings, 0 replies; 47+ messages in thread
From: Kevin Wolf @ 2018-06-18 16:44 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel
From: Max Reitz <mreitz@redhat.com>
When converting mirror's I/O to coroutines, we are going to need a point
where these coroutines are created. mirror_perform() is going to be
that point.
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Jeff Cody <jcody@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
Message-id: 20180613181823.13618-2-mreitz@redhat.com
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
block/mirror.c | 51 +++++++++++++++++++++++++++++----------------------
1 file changed, 29 insertions(+), 22 deletions(-)
diff --git a/block/mirror.c b/block/mirror.c
index c2146c1ab3..043bb7beaf 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -80,6 +80,12 @@ typedef struct MirrorOp {
uint64_t bytes;
} MirrorOp;
+typedef enum MirrorMethod {
+ MIRROR_METHOD_COPY,
+ MIRROR_METHOD_ZERO,
+ MIRROR_METHOD_DISCARD,
+} MirrorMethod;
+
static BlockErrorAction mirror_error_action(MirrorBlockJob *s, bool read,
int error)
{
@@ -319,6 +325,22 @@ static void mirror_do_zero_or_discard(MirrorBlockJob *s,
}
}
+static unsigned mirror_perform(MirrorBlockJob *s, int64_t offset,
+ unsigned bytes, MirrorMethod mirror_method)
+{
+ switch (mirror_method) {
+ case MIRROR_METHOD_COPY:
+ return mirror_do_read(s, offset, bytes);
+ case MIRROR_METHOD_ZERO:
+ case MIRROR_METHOD_DISCARD:
+ mirror_do_zero_or_discard(s, offset, bytes,
+ mirror_method == MIRROR_METHOD_DISCARD);
+ return bytes;
+ default:
+ abort();
+ }
+}
+
static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
{
BlockDriverState *source = s->source;
@@ -385,11 +407,7 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
int ret;
int64_t io_bytes;
int64_t io_bytes_acct;
- enum MirrorMethod {
- MIRROR_METHOD_COPY,
- MIRROR_METHOD_ZERO,
- MIRROR_METHOD_DISCARD
- } mirror_method = MIRROR_METHOD_COPY;
+ MirrorMethod mirror_method = MIRROR_METHOD_COPY;
assert(!(offset % s->granularity));
ret = bdrv_block_status_above(source, NULL, offset,
@@ -427,22 +445,11 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
}
io_bytes = mirror_clip_bytes(s, offset, io_bytes);
- switch (mirror_method) {
- case MIRROR_METHOD_COPY:
- io_bytes = io_bytes_acct = mirror_do_read(s, offset, io_bytes);
- break;
- case MIRROR_METHOD_ZERO:
- case MIRROR_METHOD_DISCARD:
- mirror_do_zero_or_discard(s, offset, io_bytes,
- mirror_method == MIRROR_METHOD_DISCARD);
- if (write_zeroes_ok) {
- io_bytes_acct = 0;
- } else {
- io_bytes_acct = io_bytes;
- }
- break;
- default:
- abort();
+ io_bytes = mirror_perform(s, offset, io_bytes, mirror_method);
+ if (mirror_method != MIRROR_METHOD_COPY && write_zeroes_ok) {
+ io_bytes_acct = 0;
+ } else {
+ io_bytes_acct = io_bytes;
}
assert(io_bytes);
offset += io_bytes;
@@ -635,7 +642,7 @@ static int coroutine_fn mirror_dirty_init(MirrorBlockJob *s)
continue;
}
- mirror_do_zero_or_discard(s, offset, bytes, false);
+ mirror_perform(s, offset, bytes, MIRROR_METHOD_ZERO);
offset += bytes;
}
--
2.13.6
^ permalink raw reply related [flat|nested] 47+ messages in thread* [Qemu-devel] [PULL 23/35] block/mirror: Convert to coroutines
2018-06-18 16:44 [Qemu-devel] [PULL 00/35] Block layer patches Kevin Wolf
` (21 preceding siblings ...)
2018-06-18 16:44 ` [Qemu-devel] [PULL 22/35] block/mirror: Pull out mirror_perform() Kevin Wolf
@ 2018-06-18 16:44 ` Kevin Wolf
2018-06-18 16:44 ` [Qemu-devel] [PULL 24/35] block/mirror: Use CoQueue to wait on in-flight ops Kevin Wolf
` (13 subsequent siblings)
36 siblings, 0 replies; 47+ messages in thread
From: Kevin Wolf @ 2018-06-18 16:44 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel
From: Max Reitz <mreitz@redhat.com>
In order to talk to the source BDS (and maybe in the future to the
target BDS as well) directly, we need to convert our existing AIO
requests into coroutine I/O requests.
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
Message-id: 20180613181823.13618-3-mreitz@redhat.com
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
block/mirror.c | 152 ++++++++++++++++++++++++++++++++++-----------------------
1 file changed, 90 insertions(+), 62 deletions(-)
diff --git a/block/mirror.c b/block/mirror.c
index 043bb7beaf..dcc19ae288 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -78,6 +78,10 @@ typedef struct MirrorOp {
QEMUIOVector qiov;
int64_t offset;
uint64_t bytes;
+
+ /* The pointee is set by mirror_co_read(), mirror_co_zero(), and
+ * mirror_co_discard() before yielding for the first time */
+ int64_t *bytes_handled;
} MirrorOp;
typedef enum MirrorMethod {
@@ -99,7 +103,7 @@ static BlockErrorAction mirror_error_action(MirrorBlockJob *s, bool read,
}
}
-static void mirror_iteration_done(MirrorOp *op, int ret)
+static void coroutine_fn mirror_iteration_done(MirrorOp *op, int ret)
{
MirrorBlockJob *s = op->s;
struct iovec *iov;
@@ -136,9 +140,8 @@ static void mirror_iteration_done(MirrorOp *op, int ret)
}
}
-static void mirror_write_complete(void *opaque, int ret)
+static void coroutine_fn mirror_write_complete(MirrorOp *op, int ret)
{
- MirrorOp *op = opaque;
MirrorBlockJob *s = op->s;
aio_context_acquire(blk_get_aio_context(s->common.blk));
@@ -155,9 +158,8 @@ static void mirror_write_complete(void *opaque, int ret)
aio_context_release(blk_get_aio_context(s->common.blk));
}
-static void mirror_read_complete(void *opaque, int ret)
+static void coroutine_fn mirror_read_complete(MirrorOp *op, int ret)
{
- MirrorOp *op = opaque;
MirrorBlockJob *s = op->s;
aio_context_acquire(blk_get_aio_context(s->common.blk));
@@ -172,8 +174,9 @@ static void mirror_read_complete(void *opaque, int ret)
mirror_iteration_done(op, ret);
} else {
- blk_aio_pwritev(s->target, op->offset, &op->qiov,
- 0, mirror_write_complete, op);
+ ret = blk_co_pwritev(s->target, op->offset,
+ op->qiov.size, &op->qiov, 0);
+ mirror_write_complete(op, ret);
}
aio_context_release(blk_get_aio_context(s->common.blk));
}
@@ -230,60 +233,57 @@ static inline void mirror_wait_for_io(MirrorBlockJob *s)
s->waiting_for_io = false;
}
-/* Submit async read while handling COW.
- * Returns: The number of bytes copied after and including offset,
- * excluding any bytes copied prior to offset due to alignment.
- * This will be @bytes if no alignment is necessary, or
- * (new_end - offset) if tail is rounded up or down due to
- * alignment or buffer limit.
+/* Perform a mirror copy operation.
+ *
+ * *op->bytes_handled is set to the number of bytes copied after and
+ * including offset, excluding any bytes copied prior to offset due
+ * to alignment. This will be op->bytes if no alignment is necessary,
+ * or (new_end - op->offset) if the tail is rounded up or down due to
+ * alignment or buffer limit.
*/
-static uint64_t mirror_do_read(MirrorBlockJob *s, int64_t offset,
- uint64_t bytes)
+static void coroutine_fn mirror_co_read(void *opaque)
{
+ MirrorOp *op = opaque;
+ MirrorBlockJob *s = op->s;
BlockBackend *source = s->common.blk;
int nb_chunks;
uint64_t ret;
- MirrorOp *op;
uint64_t max_bytes;
max_bytes = s->granularity * s->max_iov;
/* We can only handle as much as buf_size at a time. */
- bytes = MIN(s->buf_size, MIN(max_bytes, bytes));
- assert(bytes);
- assert(bytes < BDRV_REQUEST_MAX_BYTES);
- ret = bytes;
+ op->bytes = MIN(s->buf_size, MIN(max_bytes, op->bytes));
+ assert(op->bytes);
+ assert(op->bytes < BDRV_REQUEST_MAX_BYTES);
+ *op->bytes_handled = op->bytes;
if (s->cow_bitmap) {
- ret += mirror_cow_align(s, &offset, &bytes);
+ *op->bytes_handled += mirror_cow_align(s, &op->offset, &op->bytes);
}
- assert(bytes <= s->buf_size);
+ /* Cannot exceed BDRV_REQUEST_MAX_BYTES + INT_MAX */
+ assert(*op->bytes_handled <= UINT_MAX);
+ assert(op->bytes <= s->buf_size);
/* The offset is granularity-aligned because:
* 1) Caller passes in aligned values;
* 2) mirror_cow_align is used only when target cluster is larger. */
- assert(QEMU_IS_ALIGNED(offset, s->granularity));
+ assert(QEMU_IS_ALIGNED(op->offset, s->granularity));
/* The range is sector-aligned, since bdrv_getlength() rounds up. */
- assert(QEMU_IS_ALIGNED(bytes, BDRV_SECTOR_SIZE));
- nb_chunks = DIV_ROUND_UP(bytes, s->granularity);
+ assert(QEMU_IS_ALIGNED(op->bytes, BDRV_SECTOR_SIZE));
+ nb_chunks = DIV_ROUND_UP(op->bytes, s->granularity);
while (s->buf_free_count < nb_chunks) {
- trace_mirror_yield_in_flight(s, offset, s->in_flight);
+ trace_mirror_yield_in_flight(s, op->offset, s->in_flight);
mirror_wait_for_io(s);
}
- /* Allocate a MirrorOp that is used as an AIO callback. */
- op = g_new(MirrorOp, 1);
- op->s = s;
- op->offset = offset;
- op->bytes = bytes;
-
/* Now make a QEMUIOVector taking enough granularity-sized chunks
* from s->buf_free.
*/
qemu_iovec_init(&op->qiov, nb_chunks);
while (nb_chunks-- > 0) {
MirrorBuffer *buf = QSIMPLEQ_FIRST(&s->buf_free);
- size_t remaining = bytes - op->qiov.size;
+ size_t remaining = op->bytes - op->qiov.size;
QSIMPLEQ_REMOVE_HEAD(&s->buf_free, next);
s->buf_free_count--;
@@ -292,53 +292,81 @@ static uint64_t mirror_do_read(MirrorBlockJob *s, int64_t offset,
/* Copy the dirty cluster. */
s->in_flight++;
- s->bytes_in_flight += bytes;
- trace_mirror_one_iteration(s, offset, bytes);
+ s->bytes_in_flight += op->bytes;
+ trace_mirror_one_iteration(s, op->offset, op->bytes);
- blk_aio_preadv(source, offset, &op->qiov, 0, mirror_read_complete, op);
- return ret;
+ ret = blk_co_preadv(source, op->offset, op->bytes, &op->qiov, 0);
+ mirror_read_complete(op, ret);
}
-static void mirror_do_zero_or_discard(MirrorBlockJob *s,
- int64_t offset,
- uint64_t bytes,
- bool is_discard)
+static void coroutine_fn mirror_co_zero(void *opaque)
{
- MirrorOp *op;
+ MirrorOp *op = opaque;
+ int ret;
- /* Allocate a MirrorOp that is used as an AIO callback. The qiov is zeroed
- * so the freeing in mirror_iteration_done is nop. */
- op = g_new0(MirrorOp, 1);
- op->s = s;
- op->offset = offset;
- op->bytes = bytes;
+ op->s->in_flight++;
+ op->s->bytes_in_flight += op->bytes;
+ *op->bytes_handled = op->bytes;
- s->in_flight++;
- s->bytes_in_flight += bytes;
- if (is_discard) {
- blk_aio_pdiscard(s->target, offset,
- op->bytes, mirror_write_complete, op);
- } else {
- blk_aio_pwrite_zeroes(s->target, offset,
- op->bytes, s->unmap ? BDRV_REQ_MAY_UNMAP : 0,
- mirror_write_complete, op);
- }
+ ret = blk_co_pwrite_zeroes(op->s->target, op->offset, op->bytes,
+ op->s->unmap ? BDRV_REQ_MAY_UNMAP : 0);
+ mirror_write_complete(op, ret);
+}
+
+static void coroutine_fn mirror_co_discard(void *opaque)
+{
+ MirrorOp *op = opaque;
+ int ret;
+
+ op->s->in_flight++;
+ op->s->bytes_in_flight += op->bytes;
+ *op->bytes_handled = op->bytes;
+
+ ret = blk_co_pdiscard(op->s->target, op->offset, op->bytes);
+ mirror_write_complete(op, ret);
}
static unsigned mirror_perform(MirrorBlockJob *s, int64_t offset,
unsigned bytes, MirrorMethod mirror_method)
{
+ MirrorOp *op;
+ Coroutine *co;
+ int64_t bytes_handled = -1;
+
+ op = g_new(MirrorOp, 1);
+ *op = (MirrorOp){
+ .s = s,
+ .offset = offset,
+ .bytes = bytes,
+ .bytes_handled = &bytes_handled,
+ };
+
switch (mirror_method) {
case MIRROR_METHOD_COPY:
- return mirror_do_read(s, offset, bytes);
+ co = qemu_coroutine_create(mirror_co_read, op);
+ break;
case MIRROR_METHOD_ZERO:
+ co = qemu_coroutine_create(mirror_co_zero, op);
+ break;
case MIRROR_METHOD_DISCARD:
- mirror_do_zero_or_discard(s, offset, bytes,
- mirror_method == MIRROR_METHOD_DISCARD);
- return bytes;
+ co = qemu_coroutine_create(mirror_co_discard, op);
+ break;
default:
abort();
}
+
+ qemu_coroutine_enter(co);
+ /* At this point, ownership of op has been moved to the coroutine
+ * and the object may already be freed */
+
+ /* Assert that this value has been set */
+ assert(bytes_handled >= 0);
+
+ /* Same assertion as in mirror_co_read() (and for mirror_co_read()
+ * and mirror_co_discard(), bytes_handled == op->bytes, which
+ * is the @bytes parameter given to this function) */
+ assert(bytes_handled <= UINT_MAX);
+ return bytes_handled;
}
static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
--
2.13.6
^ permalink raw reply related [flat|nested] 47+ messages in thread* [Qemu-devel] [PULL 24/35] block/mirror: Use CoQueue to wait on in-flight ops
2018-06-18 16:44 [Qemu-devel] [PULL 00/35] Block layer patches Kevin Wolf
` (22 preceding siblings ...)
2018-06-18 16:44 ` [Qemu-devel] [PULL 23/35] block/mirror: Convert to coroutines Kevin Wolf
@ 2018-06-18 16:44 ` Kevin Wolf
2018-06-18 16:44 ` [Qemu-devel] [PULL 25/35] block/mirror: Wait for in-flight op conflicts Kevin Wolf
` (12 subsequent siblings)
36 siblings, 0 replies; 47+ messages in thread
From: Kevin Wolf @ 2018-06-18 16:44 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel
From: Max Reitz <mreitz@redhat.com>
Attach a CoQueue to each in-flight operation so if we need to wait for
any we can use it to wait instead of just blindly yielding and hoping
for some operation to wake us.
A later patch will use this infrastructure to allow requests accessing
the same area of the virtual disk to specifically wait for each other.
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
Message-id: 20180613181823.13618-4-mreitz@redhat.com
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
block/mirror.c | 34 +++++++++++++++++++++++-----------
1 file changed, 23 insertions(+), 11 deletions(-)
diff --git a/block/mirror.c b/block/mirror.c
index dcc19ae288..e2348b818a 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -13,6 +13,7 @@
#include "qemu/osdep.h"
#include "qemu/cutils.h"
+#include "qemu/coroutine.h"
#include "trace.h"
#include "block/blockjob_int.h"
#include "block/block_int.h"
@@ -33,6 +34,8 @@ typedef struct MirrorBuffer {
QSIMPLEQ_ENTRY(MirrorBuffer) next;
} MirrorBuffer;
+typedef struct MirrorOp MirrorOp;
+
typedef struct MirrorBlockJob {
BlockJob common;
BlockBackend *target;
@@ -65,15 +68,15 @@ typedef struct MirrorBlockJob {
unsigned long *in_flight_bitmap;
int in_flight;
int64_t bytes_in_flight;
+ QTAILQ_HEAD(MirrorOpList, MirrorOp) ops_in_flight;
int ret;
bool unmap;
- bool waiting_for_io;
int target_cluster_size;
int max_iov;
bool initial_zeroing_ongoing;
} MirrorBlockJob;
-typedef struct MirrorOp {
+struct MirrorOp {
MirrorBlockJob *s;
QEMUIOVector qiov;
int64_t offset;
@@ -82,7 +85,11 @@ typedef struct MirrorOp {
/* The pointee is set by mirror_co_read(), mirror_co_zero(), and
* mirror_co_discard() before yielding for the first time */
int64_t *bytes_handled;
-} MirrorOp;
+
+ CoQueue waiting_requests;
+
+ QTAILQ_ENTRY(MirrorOp) next;
+};
typedef enum MirrorMethod {
MIRROR_METHOD_COPY,
@@ -123,7 +130,9 @@ static void coroutine_fn mirror_iteration_done(MirrorOp *op, int ret)
chunk_num = op->offset / s->granularity;
nb_chunks = DIV_ROUND_UP(op->bytes, s->granularity);
+
bitmap_clear(s->in_flight_bitmap, chunk_num, nb_chunks);
+ QTAILQ_REMOVE(&s->ops_in_flight, op, next);
if (ret >= 0) {
if (s->cow_bitmap) {
bitmap_set(s->cow_bitmap, chunk_num, nb_chunks);
@@ -133,11 +142,9 @@ static void coroutine_fn mirror_iteration_done(MirrorOp *op, int ret)
}
}
qemu_iovec_destroy(&op->qiov);
- g_free(op);
- if (s->waiting_for_io) {
- qemu_coroutine_enter(s->common.job.co);
- }
+ qemu_co_queue_restart_all(&op->waiting_requests);
+ g_free(op);
}
static void coroutine_fn mirror_write_complete(MirrorOp *op, int ret)
@@ -227,10 +234,11 @@ static int mirror_cow_align(MirrorBlockJob *s, int64_t *offset,
static inline void mirror_wait_for_io(MirrorBlockJob *s)
{
- assert(!s->waiting_for_io);
- s->waiting_for_io = true;
- qemu_coroutine_yield();
- s->waiting_for_io = false;
+ MirrorOp *op;
+
+ op = QTAILQ_FIRST(&s->ops_in_flight);
+ assert(op);
+ qemu_co_queue_wait(&op->waiting_requests, NULL);
}
/* Perform a mirror copy operation.
@@ -340,6 +348,7 @@ static unsigned mirror_perform(MirrorBlockJob *s, int64_t offset,
.bytes = bytes,
.bytes_handled = &bytes_handled,
};
+ qemu_co_queue_init(&op->waiting_requests);
switch (mirror_method) {
case MIRROR_METHOD_COPY:
@@ -355,6 +364,7 @@ static unsigned mirror_perform(MirrorBlockJob *s, int64_t offset,
abort();
}
+ QTAILQ_INSERT_TAIL(&s->ops_in_flight, op, next);
qemu_coroutine_enter(co);
/* At this point, ownership of op has been moved to the coroutine
* and the object may already be freed */
@@ -1290,6 +1300,8 @@ static void mirror_start_job(const char *job_id, BlockDriverState *bs,
}
}
+ QTAILQ_INIT(&s->ops_in_flight);
+
trace_mirror_start(bs, s, opaque);
job_start(&s->common.job);
return;
--
2.13.6
^ permalink raw reply related [flat|nested] 47+ messages in thread* [Qemu-devel] [PULL 25/35] block/mirror: Wait for in-flight op conflicts
2018-06-18 16:44 [Qemu-devel] [PULL 00/35] Block layer patches Kevin Wolf
` (23 preceding siblings ...)
2018-06-18 16:44 ` [Qemu-devel] [PULL 24/35] block/mirror: Use CoQueue to wait on in-flight ops Kevin Wolf
@ 2018-06-18 16:44 ` Kevin Wolf
2018-06-18 16:44 ` [Qemu-devel] [PULL 26/35] block/mirror: Use source as a BdrvChild Kevin Wolf
` (11 subsequent siblings)
36 siblings, 0 replies; 47+ messages in thread
From: Kevin Wolf @ 2018-06-18 16:44 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel
From: Max Reitz <mreitz@redhat.com>
This patch makes the mirror code differentiate between simply waiting
for any operation to complete (mirror_wait_for_free_in_flight_slot())
and specifically waiting for all operations touching a certain range of
the virtual disk to complete (mirror_wait_on_conflicts()).
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
Message-id: 20180613181823.13618-5-mreitz@redhat.com
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
block/mirror.c | 102 +++++++++++++++++++++++++++++++++++++++++++++++----------
1 file changed, 84 insertions(+), 18 deletions(-)
diff --git a/block/mirror.c b/block/mirror.c
index e2348b818a..5df6515731 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -14,6 +14,7 @@
#include "qemu/osdep.h"
#include "qemu/cutils.h"
#include "qemu/coroutine.h"
+#include "qemu/range.h"
#include "trace.h"
#include "block/blockjob_int.h"
#include "block/block_int.h"
@@ -86,6 +87,7 @@ struct MirrorOp {
* mirror_co_discard() before yielding for the first time */
int64_t *bytes_handled;
+ bool is_pseudo_op;
CoQueue waiting_requests;
QTAILQ_ENTRY(MirrorOp) next;
@@ -110,6 +112,41 @@ static BlockErrorAction mirror_error_action(MirrorBlockJob *s, bool read,
}
}
+static void coroutine_fn mirror_wait_on_conflicts(MirrorOp *self,
+ MirrorBlockJob *s,
+ uint64_t offset,
+ uint64_t bytes)
+{
+ uint64_t self_start_chunk = offset / s->granularity;
+ uint64_t self_end_chunk = DIV_ROUND_UP(offset + bytes, s->granularity);
+ uint64_t self_nb_chunks = self_end_chunk - self_start_chunk;
+
+ while (find_next_bit(s->in_flight_bitmap, self_end_chunk,
+ self_start_chunk) < self_end_chunk &&
+ s->ret >= 0)
+ {
+ MirrorOp *op;
+
+ QTAILQ_FOREACH(op, &s->ops_in_flight, next) {
+ uint64_t op_start_chunk = op->offset / s->granularity;
+ uint64_t op_nb_chunks = DIV_ROUND_UP(op->offset + op->bytes,
+ s->granularity) -
+ op_start_chunk;
+
+ if (op == self) {
+ continue;
+ }
+
+ if (ranges_overlap(self_start_chunk, self_nb_chunks,
+ op_start_chunk, op_nb_chunks))
+ {
+ qemu_co_queue_wait(&op->waiting_requests, NULL);
+ break;
+ }
+ }
+ }
+}
+
static void coroutine_fn mirror_iteration_done(MirrorOp *op, int ret)
{
MirrorBlockJob *s = op->s;
@@ -232,13 +269,22 @@ static int mirror_cow_align(MirrorBlockJob *s, int64_t *offset,
return ret;
}
-static inline void mirror_wait_for_io(MirrorBlockJob *s)
+static inline void mirror_wait_for_free_in_flight_slot(MirrorBlockJob *s)
{
MirrorOp *op;
- op = QTAILQ_FIRST(&s->ops_in_flight);
- assert(op);
- qemu_co_queue_wait(&op->waiting_requests, NULL);
+ QTAILQ_FOREACH(op, &s->ops_in_flight, next) {
+ /* Do not wait on pseudo ops, because it may in turn wait on
+ * some other operation to start, which may in fact be the
+ * caller of this function. Since there is only one pseudo op
+ * at any given time, we will always find some real operation
+ * to wait on. */
+ if (!op->is_pseudo_op) {
+ qemu_co_queue_wait(&op->waiting_requests, NULL);
+ return;
+ }
+ }
+ abort();
}
/* Perform a mirror copy operation.
@@ -282,7 +328,7 @@ static void coroutine_fn mirror_co_read(void *opaque)
while (s->buf_free_count < nb_chunks) {
trace_mirror_yield_in_flight(s, op->offset, s->in_flight);
- mirror_wait_for_io(s);
+ mirror_wait_for_free_in_flight_slot(s);
}
/* Now make a QEMUIOVector taking enough granularity-sized chunks
@@ -382,8 +428,9 @@ static unsigned mirror_perform(MirrorBlockJob *s, int64_t offset,
static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
{
BlockDriverState *source = s->source;
- int64_t offset, first_chunk;
- uint64_t delay_ns = 0;
+ MirrorOp *pseudo_op;
+ int64_t offset;
+ uint64_t delay_ns = 0, ret = 0;
/* At least the first dirty chunk is mirrored in one iteration. */
int nb_chunks = 1;
bool write_zeroes_ok = bdrv_can_write_zeroes_with_unmap(blk_bs(s->target));
@@ -399,11 +446,7 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
}
bdrv_dirty_bitmap_unlock(s->dirty_bitmap);
- first_chunk = offset / s->granularity;
- while (test_bit(first_chunk, s->in_flight_bitmap)) {
- trace_mirror_yield_in_flight(s, offset, s->in_flight);
- mirror_wait_for_io(s);
- }
+ mirror_wait_on_conflicts(NULL, s, offset, 1);
job_pause_point(&s->common.job);
@@ -440,6 +483,21 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
nb_chunks * s->granularity);
bdrv_dirty_bitmap_unlock(s->dirty_bitmap);
+ /* Before claiming an area in the in-flight bitmap, we have to
+ * create a MirrorOp for it so that conflicting requests can wait
+ * for it. mirror_perform() will create the real MirrorOps later,
+ * for now we just create a pseudo operation that will wake up all
+ * conflicting requests once all real operations have been
+ * launched. */
+ pseudo_op = g_new(MirrorOp, 1);
+ *pseudo_op = (MirrorOp){
+ .offset = offset,
+ .bytes = nb_chunks * s->granularity,
+ .is_pseudo_op = true,
+ };
+ qemu_co_queue_init(&pseudo_op->waiting_requests);
+ QTAILQ_INSERT_TAIL(&s->ops_in_flight, pseudo_op, next);
+
bitmap_set(s->in_flight_bitmap, offset / s->granularity, nb_chunks);
while (nb_chunks > 0 && offset < s->bdev_length) {
int ret;
@@ -475,11 +533,12 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
while (s->in_flight >= MAX_IN_FLIGHT) {
trace_mirror_yield_in_flight(s, offset, s->in_flight);
- mirror_wait_for_io(s);
+ mirror_wait_for_free_in_flight_slot(s);
}
if (s->ret < 0) {
- return 0;
+ ret = 0;
+ goto fail;
}
io_bytes = mirror_clip_bytes(s, offset, io_bytes);
@@ -494,7 +553,14 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
nb_chunks -= DIV_ROUND_UP(io_bytes, s->granularity);
delay_ns = block_job_ratelimit_get_delay(&s->common, io_bytes_acct);
}
- return delay_ns;
+
+ ret = delay_ns;
+fail:
+ QTAILQ_REMOVE(&s->ops_in_flight, pseudo_op, next);
+ qemu_co_queue_restart_all(&pseudo_op->waiting_requests);
+ g_free(pseudo_op);
+
+ return ret;
}
static void mirror_free_init(MirrorBlockJob *s)
@@ -521,7 +587,7 @@ static void mirror_free_init(MirrorBlockJob *s)
static void mirror_wait_for_all_io(MirrorBlockJob *s)
{
while (s->in_flight > 0) {
- mirror_wait_for_io(s);
+ mirror_wait_for_free_in_flight_slot(s);
}
}
@@ -676,7 +742,7 @@ static int coroutine_fn mirror_dirty_init(MirrorBlockJob *s)
if (s->in_flight >= MAX_IN_FLIGHT) {
trace_mirror_yield(s, UINT64_MAX, s->buf_free_count,
s->in_flight);
- mirror_wait_for_io(s);
+ mirror_wait_for_free_in_flight_slot(s);
continue;
}
@@ -849,7 +915,7 @@ static void coroutine_fn mirror_run(void *opaque)
if (s->in_flight >= MAX_IN_FLIGHT || s->buf_free_count == 0 ||
(cnt == 0 && s->in_flight > 0)) {
trace_mirror_yield(s, cnt, s->buf_free_count, s->in_flight);
- mirror_wait_for_io(s);
+ mirror_wait_for_free_in_flight_slot(s);
continue;
} else if (cnt != 0) {
delay_ns = mirror_iteration(s);
--
2.13.6
^ permalink raw reply related [flat|nested] 47+ messages in thread* [Qemu-devel] [PULL 26/35] block/mirror: Use source as a BdrvChild
2018-06-18 16:44 [Qemu-devel] [PULL 00/35] Block layer patches Kevin Wolf
` (24 preceding siblings ...)
2018-06-18 16:44 ` [Qemu-devel] [PULL 25/35] block/mirror: Wait for in-flight op conflicts Kevin Wolf
@ 2018-06-18 16:44 ` Kevin Wolf
2018-06-18 16:44 ` [Qemu-devel] [PULL 27/35] block: Generalize should_update_child() rule Kevin Wolf
` (10 subsequent siblings)
36 siblings, 0 replies; 47+ messages in thread
From: Kevin Wolf @ 2018-06-18 16:44 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel
From: Max Reitz <mreitz@redhat.com>
With this, the mirror_top_bs is no longer just a technically required
node in the BDS graph but actually represents the block job operation.
Also, drop MirrorBlockJob.source, as we can reach it through
mirror_top_bs->backing.
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
Message-id: 20180613181823.13618-6-mreitz@redhat.com
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
block/mirror.c | 14 ++++++--------
1 file changed, 6 insertions(+), 8 deletions(-)
diff --git a/block/mirror.c b/block/mirror.c
index 5df6515731..abf231c478 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -41,7 +41,6 @@ typedef struct MirrorBlockJob {
BlockJob common;
BlockBackend *target;
BlockDriverState *mirror_top_bs;
- BlockDriverState *source;
BlockDriverState *base;
/* The name of the graph node to replace */
@@ -299,7 +298,6 @@ static void coroutine_fn mirror_co_read(void *opaque)
{
MirrorOp *op = opaque;
MirrorBlockJob *s = op->s;
- BlockBackend *source = s->common.blk;
int nb_chunks;
uint64_t ret;
uint64_t max_bytes;
@@ -349,7 +347,8 @@ static void coroutine_fn mirror_co_read(void *opaque)
s->bytes_in_flight += op->bytes;
trace_mirror_one_iteration(s, op->offset, op->bytes);
- ret = blk_co_preadv(source, op->offset, op->bytes, &op->qiov, 0);
+ ret = bdrv_co_preadv(s->mirror_top_bs->backing, op->offset, op->bytes,
+ &op->qiov, 0);
mirror_read_complete(op, ret);
}
@@ -427,7 +426,7 @@ static unsigned mirror_perform(MirrorBlockJob *s, int64_t offset,
static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
{
- BlockDriverState *source = s->source;
+ BlockDriverState *source = s->mirror_top_bs->backing->bs;
MirrorOp *pseudo_op;
int64_t offset;
uint64_t delay_ns = 0, ret = 0;
@@ -601,7 +600,7 @@ static void mirror_exit(Job *job, void *opaque)
BlockJob *bjob = &s->common;
MirrorExitData *data = opaque;
AioContext *replace_aio_context = NULL;
- BlockDriverState *src = s->source;
+ BlockDriverState *src = s->mirror_top_bs->backing->bs;
BlockDriverState *target_bs = blk_bs(s->target);
BlockDriverState *mirror_top_bs = s->mirror_top_bs;
Error *local_err = NULL;
@@ -716,7 +715,7 @@ static int coroutine_fn mirror_dirty_init(MirrorBlockJob *s)
{
int64_t offset;
BlockDriverState *base = s->base;
- BlockDriverState *bs = s->source;
+ BlockDriverState *bs = s->mirror_top_bs->backing->bs;
BlockDriverState *target_bs = blk_bs(s->target);
int ret;
int64_t count;
@@ -798,7 +797,7 @@ static void coroutine_fn mirror_run(void *opaque)
{
MirrorBlockJob *s = opaque;
MirrorExitData *data;
- BlockDriverState *bs = s->source;
+ BlockDriverState *bs = s->mirror_top_bs->backing->bs;
BlockDriverState *target_bs = blk_bs(s->target);
bool need_drain = true;
int64_t length;
@@ -1293,7 +1292,6 @@ static void mirror_start_job(const char *job_id, BlockDriverState *bs,
/* The block job now has a reference to this node */
bdrv_unref(mirror_top_bs);
- s->source = bs;
s->mirror_top_bs = mirror_top_bs;
/* No resize for the target either; while the mirror is still running, a
--
2.13.6
^ permalink raw reply related [flat|nested] 47+ messages in thread* [Qemu-devel] [PULL 27/35] block: Generalize should_update_child() rule
2018-06-18 16:44 [Qemu-devel] [PULL 00/35] Block layer patches Kevin Wolf
` (25 preceding siblings ...)
2018-06-18 16:44 ` [Qemu-devel] [PULL 26/35] block/mirror: Use source as a BdrvChild Kevin Wolf
@ 2018-06-18 16:44 ` Kevin Wolf
2018-06-18 16:44 ` [Qemu-devel] [PULL 28/35] hbitmap: Add @advance param to hbitmap_iter_next() Kevin Wolf
` (9 subsequent siblings)
36 siblings, 0 replies; 47+ messages in thread
From: Kevin Wolf @ 2018-06-18 16:44 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel
From: Max Reitz <mreitz@redhat.com>
Currently, bdrv_replace_node() refuses to create loops from one BDS to
itself if the BDS to be replaced is the backing node of the BDS to
replace it: Say there is a node A and a node B. Replacing B by A means
making all references to B point to A. If B is a child of A (i.e. A has
a reference to B), that would mean we would have to make this reference
point to A itself -- so we'd create a loop.
bdrv_replace_node() (through should_update_child()) refuses to do so if
B is the backing node of A. There is no reason why we should create
loops if B is not the backing node of A, though. The BDS graph should
never contain loops, so we should always refuse to create them.
If B is a child of A and B is to be replaced by A, we should simply
leave B in place there because it is the most sensible choice.
A more specific argument would be: Putting filter drivers into the BDS
graph is basically the same as appending an overlay to a backing chain.
But the main child BDS of a filter driver is not "backing" but "file",
so restricting the no-loop rule to backing nodes would fail here.
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
Message-id: 20180613181823.13618-7-mreitz@redhat.com
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
block.c | 44 ++++++++++++++++++++++++++++++++++----------
1 file changed, 34 insertions(+), 10 deletions(-)
diff --git a/block.c b/block.c
index 6c128007fd..1b8147c1b3 100644
--- a/block.c
+++ b/block.c
@@ -3427,16 +3427,39 @@ static bool should_update_child(BdrvChild *c, BlockDriverState *to)
return false;
}
- if (c->role == &child_backing) {
- /* If @from is a backing file of @to, ignore the child to avoid
- * creating a loop. We only want to change the pointer of other
- * parents. */
- QLIST_FOREACH(to_c, &to->children, next) {
- if (to_c == c) {
- break;
- }
- }
- if (to_c) {
+ /* If the child @c belongs to the BDS @to, replacing the current
+ * c->bs by @to would mean to create a loop.
+ *
+ * Such a case occurs when appending a BDS to a backing chain.
+ * For instance, imagine the following chain:
+ *
+ * guest device -> node A -> further backing chain...
+ *
+ * Now we create a new BDS B which we want to put on top of this
+ * chain, so we first attach A as its backing node:
+ *
+ * node B
+ * |
+ * v
+ * guest device -> node A -> further backing chain...
+ *
+ * Finally we want to replace A by B. When doing that, we want to
+ * replace all pointers to A by pointers to B -- except for the
+ * pointer from B because (1) that would create a loop, and (2)
+ * that pointer should simply stay intact:
+ *
+ * guest device -> node B
+ * |
+ * v
+ * node A -> further backing chain...
+ *
+ * In general, when replacing a node A (c->bs) by a node B (@to),
+ * if A is a child of B, that means we cannot replace A by B there
+ * because that would create a loop. Silently detaching A from B
+ * is also not really an option. So overall just leaving A in
+ * place there is the most sensible choice. */
+ QLIST_FOREACH(to_c, &to->children, next) {
+ if (to_c == c) {
return false;
}
}
@@ -3462,6 +3485,7 @@ void bdrv_replace_node(BlockDriverState *from, BlockDriverState *to,
/* Put all parents into @list and calculate their cumulative permissions */
QLIST_FOREACH_SAFE(c, &from->parents, next_parent, next) {
+ assert(c->bs == from);
if (!should_update_child(c, to)) {
continue;
}
--
2.13.6
^ permalink raw reply related [flat|nested] 47+ messages in thread* [Qemu-devel] [PULL 28/35] hbitmap: Add @advance param to hbitmap_iter_next()
2018-06-18 16:44 [Qemu-devel] [PULL 00/35] Block layer patches Kevin Wolf
` (26 preceding siblings ...)
2018-06-18 16:44 ` [Qemu-devel] [PULL 27/35] block: Generalize should_update_child() rule Kevin Wolf
@ 2018-06-18 16:44 ` Kevin Wolf
2018-06-18 16:44 ` [Qemu-devel] [PULL 29/35] test-hbitmap: Add non-advancing iter_next tests Kevin Wolf
` (8 subsequent siblings)
36 siblings, 0 replies; 47+ messages in thread
From: Kevin Wolf @ 2018-06-18 16:44 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel
From: Max Reitz <mreitz@redhat.com>
This new parameter allows the caller to just query the next dirty
position without moving the iterator.
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>
Message-id: 20180613181823.13618-8-mreitz@redhat.com
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
include/qemu/hbitmap.h | 5 ++++-
block/backup.c | 2 +-
block/dirty-bitmap.c | 2 +-
tests/test-hbitmap.c | 26 +++++++++++++-------------
util/hbitmap.c | 10 +++++++---
5 files changed, 26 insertions(+), 19 deletions(-)
diff --git a/include/qemu/hbitmap.h b/include/qemu/hbitmap.h
index 6b6490ecad..ddca52c48e 100644
--- a/include/qemu/hbitmap.h
+++ b/include/qemu/hbitmap.h
@@ -324,11 +324,14 @@ void hbitmap_free_meta(HBitmap *hb);
/**
* hbitmap_iter_next:
* @hbi: HBitmapIter to operate on.
+ * @advance: If true, advance the iterator. Otherwise, the next call
+ * of this function will return the same result (if that
+ * position is still dirty).
*
* Return the next bit that is set in @hbi's associated HBitmap,
* or -1 if all remaining bits are zero.
*/
-int64_t hbitmap_iter_next(HBitmapIter *hbi);
+int64_t hbitmap_iter_next(HBitmapIter *hbi, bool advance);
/**
* hbitmap_iter_next_word:
diff --git a/block/backup.c b/block/backup.c
index 5661435675..d18be40caf 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -354,7 +354,7 @@ static int coroutine_fn backup_run_incremental(BackupBlockJob *job)
HBitmapIter hbi;
hbitmap_iter_init(&hbi, job->copy_bitmap, 0);
- while ((cluster = hbitmap_iter_next(&hbi)) != -1) {
+ while ((cluster = hbitmap_iter_next(&hbi, true)) != -1) {
do {
if (yield_and_check(job)) {
return 0;
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 383d742cdb..cedb971765 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -519,7 +519,7 @@ void bdrv_dirty_iter_free(BdrvDirtyBitmapIter *iter)
int64_t bdrv_dirty_iter_next(BdrvDirtyBitmapIter *iter)
{
- return hbitmap_iter_next(&iter->hbi);
+ return hbitmap_iter_next(&iter->hbi, true);
}
/* Called within bdrv_dirty_bitmap_lock..unlock */
diff --git a/tests/test-hbitmap.c b/tests/test-hbitmap.c
index f29631f939..f2158f767d 100644
--- a/tests/test-hbitmap.c
+++ b/tests/test-hbitmap.c
@@ -46,7 +46,7 @@ static void hbitmap_test_check(TestHBitmapData *data,
i = first;
for (;;) {
- next = hbitmap_iter_next(&hbi);
+ next = hbitmap_iter_next(&hbi, true);
if (next < 0) {
next = data->size;
}
@@ -435,25 +435,25 @@ static void test_hbitmap_iter_granularity(TestHBitmapData *data,
/* Note that hbitmap_test_check has to be invoked manually in this test. */
hbitmap_test_init(data, 131072 << 7, 7);
hbitmap_iter_init(&hbi, data->hb, 0);
- g_assert_cmpint(hbitmap_iter_next(&hbi), <, 0);
+ g_assert_cmpint(hbitmap_iter_next(&hbi, true), <, 0);
hbitmap_test_set(data, ((L2 + L1 + 1) << 7) + 8, 8);
hbitmap_iter_init(&hbi, data->hb, 0);
- g_assert_cmpint(hbitmap_iter_next(&hbi), ==, (L2 + L1 + 1) << 7);
- g_assert_cmpint(hbitmap_iter_next(&hbi), <, 0);
+ g_assert_cmpint(hbitmap_iter_next(&hbi, true), ==, (L2 + L1 + 1) << 7);
+ g_assert_cmpint(hbitmap_iter_next(&hbi, true), <, 0);
hbitmap_iter_init(&hbi, data->hb, (L2 + L1 + 2) << 7);
- g_assert_cmpint(hbitmap_iter_next(&hbi), <, 0);
+ g_assert_cmpint(hbitmap_iter_next(&hbi, true), <, 0);
hbitmap_test_set(data, (131072 << 7) - 8, 8);
hbitmap_iter_init(&hbi, data->hb, 0);
- g_assert_cmpint(hbitmap_iter_next(&hbi), ==, (L2 + L1 + 1) << 7);
- g_assert_cmpint(hbitmap_iter_next(&hbi), ==, 131071 << 7);
- g_assert_cmpint(hbitmap_iter_next(&hbi), <, 0);
+ g_assert_cmpint(hbitmap_iter_next(&hbi, true), ==, (L2 + L1 + 1) << 7);
+ g_assert_cmpint(hbitmap_iter_next(&hbi, true), ==, 131071 << 7);
+ g_assert_cmpint(hbitmap_iter_next(&hbi, true), <, 0);
hbitmap_iter_init(&hbi, data->hb, (L2 + L1 + 2) << 7);
- g_assert_cmpint(hbitmap_iter_next(&hbi), ==, 131071 << 7);
- g_assert_cmpint(hbitmap_iter_next(&hbi), <, 0);
+ g_assert_cmpint(hbitmap_iter_next(&hbi, true), ==, 131071 << 7);
+ g_assert_cmpint(hbitmap_iter_next(&hbi, true), <, 0);
}
static void hbitmap_test_set_boundary_bits(TestHBitmapData *data, ssize_t diff)
@@ -893,7 +893,7 @@ static void test_hbitmap_serialize_zeroes(TestHBitmapData *data,
for (i = 0; i < num_positions; i++) {
hbitmap_deserialize_zeroes(data->hb, positions[i], min_l1, true);
hbitmap_iter_init(&iter, data->hb, 0);
- next = hbitmap_iter_next(&iter);
+ next = hbitmap_iter_next(&iter, true);
if (i == num_positions - 1) {
g_assert_cmpint(next, ==, -1);
} else {
@@ -919,10 +919,10 @@ static void test_hbitmap_iter_and_reset(TestHBitmapData *data,
hbitmap_iter_init(&hbi, data->hb, BITS_PER_LONG - 1);
- hbitmap_iter_next(&hbi);
+ hbitmap_iter_next(&hbi, true);
hbitmap_reset_all(data->hb);
- hbitmap_iter_next(&hbi);
+ hbitmap_iter_next(&hbi, true);
}
static void test_hbitmap_next_zero_check(TestHBitmapData *data, int64_t start)
diff --git a/util/hbitmap.c b/util/hbitmap.c
index 58a2c93842..bcd304041a 100644
--- a/util/hbitmap.c
+++ b/util/hbitmap.c
@@ -141,7 +141,7 @@ unsigned long hbitmap_iter_skip_words(HBitmapIter *hbi)
return cur;
}
-int64_t hbitmap_iter_next(HBitmapIter *hbi)
+int64_t hbitmap_iter_next(HBitmapIter *hbi, bool advance)
{
unsigned long cur = hbi->cur[HBITMAP_LEVELS - 1] &
hbi->hb->levels[HBITMAP_LEVELS - 1][hbi->pos];
@@ -154,8 +154,12 @@ int64_t hbitmap_iter_next(HBitmapIter *hbi)
}
}
- /* The next call will resume work from the next bit. */
- hbi->cur[HBITMAP_LEVELS - 1] = cur & (cur - 1);
+ if (advance) {
+ /* The next call will resume work from the next bit. */
+ hbi->cur[HBITMAP_LEVELS - 1] = cur & (cur - 1);
+ } else {
+ hbi->cur[HBITMAP_LEVELS - 1] = cur;
+ }
item = ((uint64_t)hbi->pos << BITS_PER_LEVEL) + ctzl(cur);
return item << hbi->granularity;
--
2.13.6
^ permalink raw reply related [flat|nested] 47+ messages in thread* [Qemu-devel] [PULL 29/35] test-hbitmap: Add non-advancing iter_next tests
2018-06-18 16:44 [Qemu-devel] [PULL 00/35] Block layer patches Kevin Wolf
` (27 preceding siblings ...)
2018-06-18 16:44 ` [Qemu-devel] [PULL 28/35] hbitmap: Add @advance param to hbitmap_iter_next() Kevin Wolf
@ 2018-06-18 16:44 ` Kevin Wolf
2018-06-18 16:44 ` [Qemu-devel] [PULL 30/35] block/dirty-bitmap: Add bdrv_dirty_iter_next_area Kevin Wolf
` (7 subsequent siblings)
36 siblings, 0 replies; 47+ messages in thread
From: Kevin Wolf @ 2018-06-18 16:44 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel
From: Max Reitz <mreitz@redhat.com>
Add a function that wraps hbitmap_iter_next() and always calls it in
non-advancing mode first, and in advancing mode next. The result should
always be the same.
By using this function everywhere we called hbitmap_iter_next() before,
we should get good test coverage for non-advancing hbitmap_iter_next().
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>
Message-id: 20180613181823.13618-9-mreitz@redhat.com
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
tests/test-hbitmap.c | 36 ++++++++++++++++++++++++------------
1 file changed, 24 insertions(+), 12 deletions(-)
diff --git a/tests/test-hbitmap.c b/tests/test-hbitmap.c
index f2158f767d..5e67ac1d3a 100644
--- a/tests/test-hbitmap.c
+++ b/tests/test-hbitmap.c
@@ -30,6 +30,18 @@ typedef struct TestHBitmapData {
} TestHBitmapData;
+static int64_t check_hbitmap_iter_next(HBitmapIter *hbi)
+{
+ int next0, next1;
+
+ next0 = hbitmap_iter_next(hbi, false);
+ next1 = hbitmap_iter_next(hbi, true);
+
+ g_assert_cmpint(next0, ==, next1);
+
+ return next0;
+}
+
/* Check that the HBitmap and the shadow bitmap contain the same data,
* ignoring the same "first" bits.
*/
@@ -46,7 +58,7 @@ static void hbitmap_test_check(TestHBitmapData *data,
i = first;
for (;;) {
- next = hbitmap_iter_next(&hbi, true);
+ next = check_hbitmap_iter_next(&hbi);
if (next < 0) {
next = data->size;
}
@@ -435,25 +447,25 @@ static void test_hbitmap_iter_granularity(TestHBitmapData *data,
/* Note that hbitmap_test_check has to be invoked manually in this test. */
hbitmap_test_init(data, 131072 << 7, 7);
hbitmap_iter_init(&hbi, data->hb, 0);
- g_assert_cmpint(hbitmap_iter_next(&hbi, true), <, 0);
+ g_assert_cmpint(check_hbitmap_iter_next(&hbi), <, 0);
hbitmap_test_set(data, ((L2 + L1 + 1) << 7) + 8, 8);
hbitmap_iter_init(&hbi, data->hb, 0);
- g_assert_cmpint(hbitmap_iter_next(&hbi, true), ==, (L2 + L1 + 1) << 7);
- g_assert_cmpint(hbitmap_iter_next(&hbi, true), <, 0);
+ g_assert_cmpint(check_hbitmap_iter_next(&hbi), ==, (L2 + L1 + 1) << 7);
+ g_assert_cmpint(check_hbitmap_iter_next(&hbi), <, 0);
hbitmap_iter_init(&hbi, data->hb, (L2 + L1 + 2) << 7);
g_assert_cmpint(hbitmap_iter_next(&hbi, true), <, 0);
hbitmap_test_set(data, (131072 << 7) - 8, 8);
hbitmap_iter_init(&hbi, data->hb, 0);
- g_assert_cmpint(hbitmap_iter_next(&hbi, true), ==, (L2 + L1 + 1) << 7);
- g_assert_cmpint(hbitmap_iter_next(&hbi, true), ==, 131071 << 7);
- g_assert_cmpint(hbitmap_iter_next(&hbi, true), <, 0);
+ g_assert_cmpint(check_hbitmap_iter_next(&hbi), ==, (L2 + L1 + 1) << 7);
+ g_assert_cmpint(check_hbitmap_iter_next(&hbi), ==, 131071 << 7);
+ g_assert_cmpint(check_hbitmap_iter_next(&hbi), <, 0);
hbitmap_iter_init(&hbi, data->hb, (L2 + L1 + 2) << 7);
- g_assert_cmpint(hbitmap_iter_next(&hbi, true), ==, 131071 << 7);
- g_assert_cmpint(hbitmap_iter_next(&hbi, true), <, 0);
+ g_assert_cmpint(check_hbitmap_iter_next(&hbi), ==, 131071 << 7);
+ g_assert_cmpint(check_hbitmap_iter_next(&hbi), <, 0);
}
static void hbitmap_test_set_boundary_bits(TestHBitmapData *data, ssize_t diff)
@@ -893,7 +905,7 @@ static void test_hbitmap_serialize_zeroes(TestHBitmapData *data,
for (i = 0; i < num_positions; i++) {
hbitmap_deserialize_zeroes(data->hb, positions[i], min_l1, true);
hbitmap_iter_init(&iter, data->hb, 0);
- next = hbitmap_iter_next(&iter, true);
+ next = check_hbitmap_iter_next(&iter);
if (i == num_positions - 1) {
g_assert_cmpint(next, ==, -1);
} else {
@@ -919,10 +931,10 @@ static void test_hbitmap_iter_and_reset(TestHBitmapData *data,
hbitmap_iter_init(&hbi, data->hb, BITS_PER_LONG - 1);
- hbitmap_iter_next(&hbi, true);
+ check_hbitmap_iter_next(&hbi);
hbitmap_reset_all(data->hb);
- hbitmap_iter_next(&hbi, true);
+ check_hbitmap_iter_next(&hbi);
}
static void test_hbitmap_next_zero_check(TestHBitmapData *data, int64_t start)
--
2.13.6
^ permalink raw reply related [flat|nested] 47+ messages in thread* [Qemu-devel] [PULL 30/35] block/dirty-bitmap: Add bdrv_dirty_iter_next_area
2018-06-18 16:44 [Qemu-devel] [PULL 00/35] Block layer patches Kevin Wolf
` (28 preceding siblings ...)
2018-06-18 16:44 ` [Qemu-devel] [PULL 29/35] test-hbitmap: Add non-advancing iter_next tests Kevin Wolf
@ 2018-06-18 16:44 ` Kevin Wolf
2018-08-03 15:17 ` Vladimir Sementsov-Ogievskiy
2018-06-18 16:45 ` [Qemu-devel] [PULL 31/35] block/mirror: Add MirrorBDSOpaque Kevin Wolf
` (6 subsequent siblings)
36 siblings, 1 reply; 47+ messages in thread
From: Kevin Wolf @ 2018-06-18 16:44 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel
From: Max Reitz <mreitz@redhat.com>
This new function allows to look for a consecutively dirty area in a
dirty bitmap.
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>
Message-id: 20180613181823.13618-10-mreitz@redhat.com
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
include/block/dirty-bitmap.h | 2 ++
block/dirty-bitmap.c | 55 ++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 57 insertions(+)
diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index 02e0cbabd2..288dc6adb6 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -82,6 +82,8 @@ void bdrv_set_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap,
void bdrv_reset_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap,
int64_t offset, int64_t bytes);
int64_t bdrv_dirty_iter_next(BdrvDirtyBitmapIter *iter);
+bool bdrv_dirty_iter_next_area(BdrvDirtyBitmapIter *iter, uint64_t max_offset,
+ uint64_t *offset, int *bytes);
void bdrv_set_dirty_iter(BdrvDirtyBitmapIter *hbi, int64_t offset);
int64_t bdrv_get_dirty_count(BdrvDirtyBitmap *bitmap);
int64_t bdrv_get_meta_dirty_count(BdrvDirtyBitmap *bitmap);
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index cedb971765..db1782ec1f 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -522,6 +522,61 @@ int64_t bdrv_dirty_iter_next(BdrvDirtyBitmapIter *iter)
return hbitmap_iter_next(&iter->hbi, true);
}
+/**
+ * Return the next consecutively dirty area in the dirty bitmap
+ * belonging to the given iterator @iter.
+ *
+ * @max_offset: Maximum value that may be returned for
+ * *offset + *bytes
+ * @offset: Will contain the start offset of the next dirty area
+ * @bytes: Will contain the length of the next dirty area
+ *
+ * Returns: True if a dirty area could be found before max_offset
+ * (which means that *offset and *bytes then contain valid
+ * values), false otherwise.
+ *
+ * Note that @iter is never advanced if false is returned. If an area
+ * is found (which means that true is returned), it will be advanced
+ * past that area.
+ */
+bool bdrv_dirty_iter_next_area(BdrvDirtyBitmapIter *iter, uint64_t max_offset,
+ uint64_t *offset, int *bytes)
+{
+ uint32_t granularity = bdrv_dirty_bitmap_granularity(iter->bitmap);
+ uint64_t gran_max_offset;
+ int64_t ret;
+ int size;
+
+ if (max_offset == iter->bitmap->size) {
+ /* If max_offset points to the image end, round it up by the
+ * bitmap granularity */
+ gran_max_offset = ROUND_UP(max_offset, granularity);
+ } else {
+ gran_max_offset = max_offset;
+ }
+
+ ret = hbitmap_iter_next(&iter->hbi, false);
+ if (ret < 0 || ret + granularity > gran_max_offset) {
+ return false;
+ }
+
+ *offset = ret;
+ size = 0;
+
+ assert(granularity <= INT_MAX);
+
+ do {
+ /* Advance iterator */
+ ret = hbitmap_iter_next(&iter->hbi, true);
+ size += granularity;
+ } while (ret + granularity <= gran_max_offset &&
+ hbitmap_iter_next(&iter->hbi, false) == ret + granularity &&
+ size <= INT_MAX - granularity);
+
+ *bytes = MIN(size, max_offset - *offset);
+ return true;
+}
+
/* Called within bdrv_dirty_bitmap_lock..unlock */
void bdrv_set_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap,
int64_t offset, int64_t bytes)
--
2.13.6
^ permalink raw reply related [flat|nested] 47+ messages in thread* Re: [Qemu-devel] [PULL 30/35] block/dirty-bitmap: Add bdrv_dirty_iter_next_area
2018-06-18 16:44 ` [Qemu-devel] [PULL 30/35] block/dirty-bitmap: Add bdrv_dirty_iter_next_area Kevin Wolf
@ 2018-08-03 15:17 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 47+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2018-08-03 15:17 UTC (permalink / raw)
To: Kevin Wolf, qemu-block; +Cc: qemu-devel
18.06.2018 19:44, Kevin Wolf wrote:
> From: Max Reitz <mreitz@redhat.com>
>
> This new function allows to look for a consecutively dirty area in a
> dirty bitmap.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> Reviewed-by: Fam Zheng <famz@redhat.com>
> Reviewed-by: John Snow <jsnow@redhat.com>
> Message-id: 20180613181823.13618-10-mreitz@redhat.com
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
> include/block/dirty-bitmap.h | 2 ++
> block/dirty-bitmap.c | 55 ++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 57 insertions(+)
>
> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
> index 02e0cbabd2..288dc6adb6 100644
> --- a/include/block/dirty-bitmap.h
> +++ b/include/block/dirty-bitmap.h
> @@ -82,6 +82,8 @@ void bdrv_set_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap,
> void bdrv_reset_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap,
> int64_t offset, int64_t bytes);
> int64_t bdrv_dirty_iter_next(BdrvDirtyBitmapIter *iter);
> +bool bdrv_dirty_iter_next_area(BdrvDirtyBitmapIter *iter, uint64_t max_offset,
> + uint64_t *offset, int *bytes);
> void bdrv_set_dirty_iter(BdrvDirtyBitmapIter *hbi, int64_t offset);
> int64_t bdrv_get_dirty_count(BdrvDirtyBitmap *bitmap);
> int64_t bdrv_get_meta_dirty_count(BdrvDirtyBitmap *bitmap);
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index cedb971765..db1782ec1f 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -522,6 +522,61 @@ int64_t bdrv_dirty_iter_next(BdrvDirtyBitmapIter *iter)
> return hbitmap_iter_next(&iter->hbi, true);
> }
>
> +/**
> + * Return the next consecutively dirty area in the dirty bitmap
> + * belonging to the given iterator @iter.
> + *
> + * @max_offset: Maximum value that may be returned for
> + * *offset + *bytes
> + * @offset: Will contain the start offset of the next dirty area
> + * @bytes: Will contain the length of the next dirty area
> + *
> + * Returns: True if a dirty area could be found before max_offset
> + * (which means that *offset and *bytes then contain valid
> + * values), false otherwise.
> + *
> + * Note that @iter is never advanced if false is returned. If an area
> + * is found (which means that true is returned), it will be advanced
> + * past that area.
> + */
> +bool bdrv_dirty_iter_next_area(BdrvDirtyBitmapIter *iter, uint64_t max_offset,
> + uint64_t *offset, int *bytes)
> +{
> + uint32_t granularity = bdrv_dirty_bitmap_granularity(iter->bitmap);
> + uint64_t gran_max_offset;
> + int64_t ret;
> + int size;
> +
> + if (max_offset == iter->bitmap->size) {
> + /* If max_offset points to the image end, round it up by the
> + * bitmap granularity */
> + gran_max_offset = ROUND_UP(max_offset, granularity);
> + } else {
> + gran_max_offset = max_offset;
> + }
> +
> + ret = hbitmap_iter_next(&iter->hbi, false);
> + if (ret < 0 || ret + granularity > gran_max_offset) {
If max_offset was not mutiply of granularity and was not equal to
iter->bitmap-size, we can skip a dirty region here, from ret to
max_offset, which is smaller than granularity.. Isn't it a bug?
> + return false;
> + }
> +
> + *offset = ret;
> + size = 0;
> +
> + assert(granularity <= INT_MAX);
> +
> + do {
> + /* Advance iterator */
> + ret = hbitmap_iter_next(&iter->hbi, true);
> + size += granularity;
> + } while (ret + granularity <= gran_max_offset &&
> + hbitmap_iter_next(&iter->hbi, false) == ret + granularity &&
> + size <= INT_MAX - granularity);
> +
> + *bytes = MIN(size, max_offset - *offset);
> + return true;
> +}
> +
> /* Called within bdrv_dirty_bitmap_lock..unlock */
> void bdrv_set_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap,
> int64_t offset, int64_t bytes)
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 47+ messages in thread
* [Qemu-devel] [PULL 31/35] block/mirror: Add MirrorBDSOpaque
2018-06-18 16:44 [Qemu-devel] [PULL 00/35] Block layer patches Kevin Wolf
` (29 preceding siblings ...)
2018-06-18 16:44 ` [Qemu-devel] [PULL 30/35] block/dirty-bitmap: Add bdrv_dirty_iter_next_area Kevin Wolf
@ 2018-06-18 16:45 ` Kevin Wolf
2018-06-18 16:45 ` [Qemu-devel] [PULL 32/35] job: Add job_progress_increase_remaining() Kevin Wolf
` (5 subsequent siblings)
36 siblings, 0 replies; 47+ messages in thread
From: Kevin Wolf @ 2018-06-18 16:45 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel
From: Max Reitz <mreitz@redhat.com>
This will allow us to access the block job data when the mirror block
driver becomes more complex.
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
Message-id: 20180613181823.13618-11-mreitz@redhat.com
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
block/mirror.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/block/mirror.c b/block/mirror.c
index abf231c478..7da5e43c0d 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -76,6 +76,10 @@ typedef struct MirrorBlockJob {
bool initial_zeroing_ongoing;
} MirrorBlockJob;
+typedef struct MirrorBDSOpaque {
+ MirrorBlockJob *job;
+} MirrorBDSOpaque;
+
struct MirrorOp {
MirrorBlockJob *s;
QEMUIOVector qiov;
@@ -599,6 +603,7 @@ static void mirror_exit(Job *job, void *opaque)
MirrorBlockJob *s = container_of(job, MirrorBlockJob, common.job);
BlockJob *bjob = &s->common;
MirrorExitData *data = opaque;
+ MirrorBDSOpaque *bs_opaque = s->mirror_top_bs->opaque;
AioContext *replace_aio_context = NULL;
BlockDriverState *src = s->mirror_top_bs->backing->bs;
BlockDriverState *target_bs = blk_bs(s->target);
@@ -691,6 +696,7 @@ static void mirror_exit(Job *job, void *opaque)
blk_set_perm(bjob->blk, 0, BLK_PERM_ALL, &error_abort);
blk_insert_bs(bjob->blk, mirror_top_bs, &error_abort);
+ bs_opaque->job = NULL;
job_completed(job, data->ret, NULL);
g_free(data);
@@ -1230,6 +1236,7 @@ static void mirror_start_job(const char *job_id, BlockDriverState *bs,
Error **errp)
{
MirrorBlockJob *s;
+ MirrorBDSOpaque *bs_opaque;
BlockDriverState *mirror_top_bs;
bool target_graph_mod;
bool target_is_backing;
@@ -1265,6 +1272,8 @@ static void mirror_start_job(const char *job_id, BlockDriverState *bs,
mirror_top_bs->total_sectors = bs->total_sectors;
mirror_top_bs->supported_write_flags = BDRV_REQ_WRITE_UNCHANGED;
mirror_top_bs->supported_zero_flags = BDRV_REQ_WRITE_UNCHANGED;
+ bs_opaque = g_new0(MirrorBDSOpaque, 1);
+ mirror_top_bs->opaque = bs_opaque;
bdrv_set_aio_context(mirror_top_bs, bdrv_get_aio_context(bs));
/* bdrv_append takes ownership of the mirror_top_bs reference, need to keep
@@ -1289,6 +1298,8 @@ static void mirror_start_job(const char *job_id, BlockDriverState *bs,
if (!s) {
goto fail;
}
+ bs_opaque->job = s;
+
/* The block job now has a reference to this node */
bdrv_unref(mirror_top_bs);
@@ -1378,6 +1389,7 @@ fail:
g_free(s->replaces);
blk_unref(s->target);
+ bs_opaque->job = NULL;
job_early_fail(&s->common.job);
}
--
2.13.6
^ permalink raw reply related [flat|nested] 47+ messages in thread* [Qemu-devel] [PULL 32/35] job: Add job_progress_increase_remaining()
2018-06-18 16:44 [Qemu-devel] [PULL 00/35] Block layer patches Kevin Wolf
` (30 preceding siblings ...)
2018-06-18 16:45 ` [Qemu-devel] [PULL 31/35] block/mirror: Add MirrorBDSOpaque Kevin Wolf
@ 2018-06-18 16:45 ` Kevin Wolf
2018-06-18 16:45 ` [Qemu-devel] [PULL 33/35] block/mirror: Add active mirroring Kevin Wolf
` (4 subsequent siblings)
36 siblings, 0 replies; 47+ messages in thread
From: Kevin Wolf @ 2018-06-18 16:45 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel
From: Max Reitz <mreitz@redhat.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
Message-id: 20180613181823.13618-12-mreitz@redhat.com
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
include/qemu/job.h | 15 +++++++++++++++
job.c | 5 +++++
2 files changed, 20 insertions(+)
diff --git a/include/qemu/job.h b/include/qemu/job.h
index 1d820530fa..18c9223e31 100644
--- a/include/qemu/job.h
+++ b/include/qemu/job.h
@@ -335,6 +335,21 @@ void job_progress_update(Job *job, uint64_t done);
*/
void job_progress_set_remaining(Job *job, uint64_t remaining);
+/**
+ * @job: The job whose expected progress end value is updated
+ * @delta: Value which is to be added to the current expected end
+ * value
+ *
+ * Increases the expected end value of the progress counter of a job.
+ * This is useful for parenthesis operations: If a job has to
+ * conditionally perform a high-priority operation as part of its
+ * progress, it calls this function with the expected operation's
+ * length before, and job_progress_update() afterwards.
+ * (So the operation acts as a parenthesis in regards to the main job
+ * operation running in background.)
+ */
+void job_progress_increase_remaining(Job *job, uint64_t delta);
+
/** To be called when a cancelled job is finalised. */
void job_event_cancelled(Job *job);
diff --git a/job.c b/job.c
index 84e140238b..fa671b431a 100644
--- a/job.c
+++ b/job.c
@@ -385,6 +385,11 @@ void job_progress_set_remaining(Job *job, uint64_t remaining)
job->progress_total = job->progress_current + remaining;
}
+void job_progress_increase_remaining(Job *job, uint64_t delta)
+{
+ job->progress_total += delta;
+}
+
void job_event_cancelled(Job *job)
{
notifier_list_notify(&job->on_finalize_cancelled, job);
--
2.13.6
^ permalink raw reply related [flat|nested] 47+ messages in thread* [Qemu-devel] [PULL 33/35] block/mirror: Add active mirroring
2018-06-18 16:44 [Qemu-devel] [PULL 00/35] Block layer patches Kevin Wolf
` (31 preceding siblings ...)
2018-06-18 16:45 ` [Qemu-devel] [PULL 32/35] job: Add job_progress_increase_remaining() Kevin Wolf
@ 2018-06-18 16:45 ` Kevin Wolf
2018-08-03 15:20 ` Vladimir Sementsov-Ogievskiy
2018-06-18 16:45 ` [Qemu-devel] [PULL 34/35] block/mirror: Add copy mode QAPI interface Kevin Wolf
` (3 subsequent siblings)
36 siblings, 1 reply; 47+ messages in thread
From: Kevin Wolf @ 2018-06-18 16:45 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel
From: Max Reitz <mreitz@redhat.com>
This patch implements active synchronous mirroring. In active mode, the
passive mechanism will still be in place and is used to copy all
initially dirty clusters off the source disk; but every write request
will write data both to the source and the target disk, so the source
cannot be dirtied faster than data is mirrored to the target. Also,
once the block job has converged (BLOCK_JOB_READY sent), source and
target are guaranteed to stay in sync (unless an error occurs).
Active mode is completely optional and currently disabled at runtime. A
later patch will add a way for users to enable it.
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
Message-id: 20180613181823.13618-13-mreitz@redhat.com
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
qapi/block-core.json | 18 ++++
block/mirror.c | 252 ++++++++++++++++++++++++++++++++++++++++++++++++++-
2 files changed, 265 insertions(+), 5 deletions(-)
diff --git a/qapi/block-core.json b/qapi/block-core.json
index ab629d1647..96f8da1322 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1051,6 +1051,24 @@
'data': ['top', 'full', 'none', 'incremental'] }
##
+# @MirrorCopyMode:
+#
+# An enumeration whose values tell the mirror block job when to
+# trigger writes to the target.
+#
+# @background: copy data in background only.
+#
+# @write-blocking: when data is written to the source, write it
+# (synchronously) to the target as well. In
+# addition, data is copied in background just like in
+# @background mode.
+#
+# Since: 3.0
+##
+{ 'enum': 'MirrorCopyMode',
+ 'data': ['background', 'write-blocking'] }
+
+##
# @BlockJobInfo:
#
# Information about a long-running block device operation.
diff --git a/block/mirror.c b/block/mirror.c
index 7da5e43c0d..99b9b92c30 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -51,8 +51,12 @@ typedef struct MirrorBlockJob {
Error *replace_blocker;
bool is_none_mode;
BlockMirrorBackingMode backing_mode;
+ MirrorCopyMode copy_mode;
BlockdevOnError on_source_error, on_target_error;
bool synced;
+ /* Set when the target is synced (dirty bitmap is clean, nothing
+ * in flight) and the job is running in active mode */
+ bool actively_synced;
bool should_complete;
int64_t granularity;
size_t buf_size;
@@ -74,6 +78,7 @@ typedef struct MirrorBlockJob {
int target_cluster_size;
int max_iov;
bool initial_zeroing_ongoing;
+ int in_active_write_counter;
} MirrorBlockJob;
typedef struct MirrorBDSOpaque {
@@ -91,6 +96,7 @@ struct MirrorOp {
int64_t *bytes_handled;
bool is_pseudo_op;
+ bool is_active_write;
CoQueue waiting_requests;
QTAILQ_ENTRY(MirrorOp) next;
@@ -106,6 +112,7 @@ static BlockErrorAction mirror_error_action(MirrorBlockJob *s, bool read,
int error)
{
s->synced = false;
+ s->actively_synced = false;
if (read) {
return block_job_error_action(&s->common, s->on_source_error,
true, error);
@@ -272,7 +279,7 @@ static int mirror_cow_align(MirrorBlockJob *s, int64_t *offset,
return ret;
}
-static inline void mirror_wait_for_free_in_flight_slot(MirrorBlockJob *s)
+static inline void mirror_wait_for_any_operation(MirrorBlockJob *s, bool active)
{
MirrorOp *op;
@@ -282,7 +289,7 @@ static inline void mirror_wait_for_free_in_flight_slot(MirrorBlockJob *s)
* caller of this function. Since there is only one pseudo op
* at any given time, we will always find some real operation
* to wait on. */
- if (!op->is_pseudo_op) {
+ if (!op->is_pseudo_op && op->is_active_write == active) {
qemu_co_queue_wait(&op->waiting_requests, NULL);
return;
}
@@ -290,6 +297,12 @@ static inline void mirror_wait_for_free_in_flight_slot(MirrorBlockJob *s)
abort();
}
+static inline void mirror_wait_for_free_in_flight_slot(MirrorBlockJob *s)
+{
+ /* Only non-active operations use up in-flight slots */
+ mirror_wait_for_any_operation(s, false);
+}
+
/* Perform a mirror copy operation.
*
* *op->bytes_handled is set to the number of bytes copied after and
@@ -846,6 +859,7 @@ static void coroutine_fn mirror_run(void *opaque)
/* Transition to the READY state and wait for complete. */
job_transition_to_ready(&s->common.job);
s->synced = true;
+ s->actively_synced = true;
while (!job_is_cancelled(&s->common.job) && !s->should_complete) {
job_yield(&s->common.job);
}
@@ -897,6 +911,12 @@ static void coroutine_fn mirror_run(void *opaque)
int64_t cnt, delta;
bool should_complete;
+ /* Do not start passive operations while there are active
+ * writes in progress */
+ while (s->in_active_write_counter) {
+ mirror_wait_for_any_operation(s, true);
+ }
+
if (s->ret < 0) {
ret = s->ret;
goto immediate_exit;
@@ -942,6 +962,9 @@ static void coroutine_fn mirror_run(void *opaque)
*/
job_transition_to_ready(&s->common.job);
s->synced = true;
+ if (s->copy_mode != MIRROR_COPY_MODE_BACKGROUND) {
+ s->actively_synced = true;
+ }
}
should_complete = s->should_complete ||
@@ -1140,16 +1163,232 @@ static const BlockJobDriver commit_active_job_driver = {
.drain = mirror_drain,
};
+static void do_sync_target_write(MirrorBlockJob *job, MirrorMethod method,
+ uint64_t offset, uint64_t bytes,
+ QEMUIOVector *qiov, int flags)
+{
+ BdrvDirtyBitmapIter *iter;
+ QEMUIOVector target_qiov;
+ uint64_t dirty_offset;
+ int dirty_bytes;
+
+ if (qiov) {
+ qemu_iovec_init(&target_qiov, qiov->niov);
+ }
+
+ iter = bdrv_dirty_iter_new(job->dirty_bitmap);
+ bdrv_set_dirty_iter(iter, offset);
+
+ while (true) {
+ bool valid_area;
+ int ret;
+
+ bdrv_dirty_bitmap_lock(job->dirty_bitmap);
+ valid_area = bdrv_dirty_iter_next_area(iter, offset + bytes,
+ &dirty_offset, &dirty_bytes);
+ if (!valid_area) {
+ bdrv_dirty_bitmap_unlock(job->dirty_bitmap);
+ break;
+ }
+
+ bdrv_reset_dirty_bitmap_locked(job->dirty_bitmap,
+ dirty_offset, dirty_bytes);
+ bdrv_dirty_bitmap_unlock(job->dirty_bitmap);
+
+ job_progress_increase_remaining(&job->common.job, dirty_bytes);
+
+ assert(dirty_offset - offset <= SIZE_MAX);
+ if (qiov) {
+ qemu_iovec_reset(&target_qiov);
+ qemu_iovec_concat(&target_qiov, qiov,
+ dirty_offset - offset, dirty_bytes);
+ }
+
+ switch (method) {
+ case MIRROR_METHOD_COPY:
+ ret = blk_co_pwritev(job->target, dirty_offset, dirty_bytes,
+ qiov ? &target_qiov : NULL, flags);
+ break;
+
+ case MIRROR_METHOD_ZERO:
+ assert(!qiov);
+ ret = blk_co_pwrite_zeroes(job->target, dirty_offset, dirty_bytes,
+ flags);
+ break;
+
+ case MIRROR_METHOD_DISCARD:
+ assert(!qiov);
+ ret = blk_co_pdiscard(job->target, dirty_offset, dirty_bytes);
+ break;
+
+ default:
+ abort();
+ }
+
+ if (ret >= 0) {
+ job_progress_update(&job->common.job, dirty_bytes);
+ } else {
+ BlockErrorAction action;
+
+ bdrv_set_dirty_bitmap(job->dirty_bitmap, dirty_offset, dirty_bytes);
+ job->actively_synced = false;
+
+ action = mirror_error_action(job, false, -ret);
+ if (action == BLOCK_ERROR_ACTION_REPORT) {
+ if (!job->ret) {
+ job->ret = ret;
+ }
+ break;
+ }
+ }
+ }
+
+ bdrv_dirty_iter_free(iter);
+ if (qiov) {
+ qemu_iovec_destroy(&target_qiov);
+ }
+}
+
+static MirrorOp *coroutine_fn active_write_prepare(MirrorBlockJob *s,
+ uint64_t offset,
+ uint64_t bytes)
+{
+ MirrorOp *op;
+ uint64_t start_chunk = offset / s->granularity;
+ uint64_t end_chunk = DIV_ROUND_UP(offset + bytes, s->granularity);
+
+ op = g_new(MirrorOp, 1);
+ *op = (MirrorOp){
+ .s = s,
+ .offset = offset,
+ .bytes = bytes,
+ .is_active_write = true,
+ };
+ qemu_co_queue_init(&op->waiting_requests);
+ QTAILQ_INSERT_TAIL(&s->ops_in_flight, op, next);
+
+ s->in_active_write_counter++;
+
+ mirror_wait_on_conflicts(op, s, offset, bytes);
+
+ bitmap_set(s->in_flight_bitmap, start_chunk, end_chunk - start_chunk);
+
+ return op;
+}
+
+static void coroutine_fn active_write_settle(MirrorOp *op)
+{
+ uint64_t start_chunk = op->offset / op->s->granularity;
+ uint64_t end_chunk = DIV_ROUND_UP(op->offset + op->bytes,
+ op->s->granularity);
+
+ if (!--op->s->in_active_write_counter && op->s->actively_synced) {
+ BdrvChild *source = op->s->mirror_top_bs->backing;
+
+ if (QLIST_FIRST(&source->bs->parents) == source &&
+ QLIST_NEXT(source, next_parent) == NULL)
+ {
+ /* Assert that we are back in sync once all active write
+ * operations are settled.
+ * Note that we can only assert this if the mirror node
+ * is the source node's only parent. */
+ assert(!bdrv_get_dirty_count(op->s->dirty_bitmap));
+ }
+ }
+ bitmap_clear(op->s->in_flight_bitmap, start_chunk, end_chunk - start_chunk);
+ QTAILQ_REMOVE(&op->s->ops_in_flight, op, next);
+ qemu_co_queue_restart_all(&op->waiting_requests);
+ g_free(op);
+}
+
static int coroutine_fn bdrv_mirror_top_preadv(BlockDriverState *bs,
uint64_t offset, uint64_t bytes, QEMUIOVector *qiov, int flags)
{
return bdrv_co_preadv(bs->backing, offset, bytes, qiov, flags);
}
+static int coroutine_fn bdrv_mirror_top_do_write(BlockDriverState *bs,
+ MirrorMethod method, uint64_t offset, uint64_t bytes, QEMUIOVector *qiov,
+ int flags)
+{
+ MirrorOp *op = NULL;
+ MirrorBDSOpaque *s = bs->opaque;
+ int ret = 0;
+ bool copy_to_target;
+
+ copy_to_target = s->job->ret >= 0 &&
+ s->job->copy_mode == MIRROR_COPY_MODE_WRITE_BLOCKING;
+
+ if (copy_to_target) {
+ op = active_write_prepare(s->job, offset, bytes);
+ }
+
+ switch (method) {
+ case MIRROR_METHOD_COPY:
+ ret = bdrv_co_pwritev(bs->backing, offset, bytes, qiov, flags);
+ break;
+
+ case MIRROR_METHOD_ZERO:
+ ret = bdrv_co_pwrite_zeroes(bs->backing, offset, bytes, flags);
+ break;
+
+ case MIRROR_METHOD_DISCARD:
+ ret = bdrv_co_pdiscard(bs->backing->bs, offset, bytes);
+ break;
+
+ default:
+ abort();
+ }
+
+ if (ret < 0) {
+ goto out;
+ }
+
+ if (copy_to_target) {
+ do_sync_target_write(s->job, method, offset, bytes, qiov, flags);
+ }
+
+out:
+ if (copy_to_target) {
+ active_write_settle(op);
+ }
+ return ret;
+}
+
static int coroutine_fn bdrv_mirror_top_pwritev(BlockDriverState *bs,
uint64_t offset, uint64_t bytes, QEMUIOVector *qiov, int flags)
{
- return bdrv_co_pwritev(bs->backing, offset, bytes, qiov, flags);
+ MirrorBDSOpaque *s = bs->opaque;
+ QEMUIOVector bounce_qiov;
+ void *bounce_buf;
+ int ret = 0;
+ bool copy_to_target;
+
+ copy_to_target = s->job->ret >= 0 &&
+ s->job->copy_mode == MIRROR_COPY_MODE_WRITE_BLOCKING;
+
+ if (copy_to_target) {
+ /* The guest might concurrently modify the data to write; but
+ * the data on source and destination must match, so we have
+ * to use a bounce buffer if we are going to write to the
+ * target now. */
+ bounce_buf = qemu_blockalign(bs, bytes);
+ iov_to_buf_full(qiov->iov, qiov->niov, 0, bounce_buf, bytes);
+
+ qemu_iovec_init(&bounce_qiov, 1);
+ qemu_iovec_add(&bounce_qiov, bounce_buf, bytes);
+ qiov = &bounce_qiov;
+ }
+
+ ret = bdrv_mirror_top_do_write(bs, MIRROR_METHOD_COPY, offset, bytes, qiov,
+ flags);
+
+ if (copy_to_target) {
+ qemu_iovec_destroy(&bounce_qiov);
+ qemu_vfree(bounce_buf);
+ }
+
+ return ret;
}
static int coroutine_fn bdrv_mirror_top_flush(BlockDriverState *bs)
@@ -1164,13 +1403,15 @@ static int coroutine_fn bdrv_mirror_top_flush(BlockDriverState *bs)
static int coroutine_fn bdrv_mirror_top_pwrite_zeroes(BlockDriverState *bs,
int64_t offset, int bytes, BdrvRequestFlags flags)
{
- return bdrv_co_pwrite_zeroes(bs->backing, offset, bytes, flags);
+ return bdrv_mirror_top_do_write(bs, MIRROR_METHOD_ZERO, offset, bytes, NULL,
+ flags);
}
static int coroutine_fn bdrv_mirror_top_pdiscard(BlockDriverState *bs,
int64_t offset, int bytes)
{
- return bdrv_co_pdiscard(bs->backing->bs, offset, bytes);
+ return bdrv_mirror_top_do_write(bs, MIRROR_METHOD_DISCARD, offset, bytes,
+ NULL, 0);
}
static void bdrv_mirror_top_refresh_filename(BlockDriverState *bs, QDict *opts)
@@ -1340,6 +1581,7 @@ static void mirror_start_job(const char *job_id, BlockDriverState *bs,
s->on_target_error = on_target_error;
s->is_none_mode = is_none_mode;
s->backing_mode = backing_mode;
+ s->copy_mode = MIRROR_COPY_MODE_BACKGROUND;
s->base = base;
s->granularity = granularity;
s->buf_size = ROUND_UP(buf_size, granularity);
--
2.13.6
^ permalink raw reply related [flat|nested] 47+ messages in thread* Re: [Qemu-devel] [PULL 33/35] block/mirror: Add active mirroring
2018-06-18 16:45 ` [Qemu-devel] [PULL 33/35] block/mirror: Add active mirroring Kevin Wolf
@ 2018-08-03 15:20 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 47+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2018-08-03 15:20 UTC (permalink / raw)
To: Kevin Wolf, qemu-block; +Cc: qemu-devel
18.06.2018 19:45, Kevin Wolf wrote:
> From: Max Reitz <mreitz@redhat.com>
>
> This patch implements active synchronous mirroring. In active mode, the
> passive mechanism will still be in place and is used to copy all
> initially dirty clusters off the source disk; but every write request
> will write data both to the source and the target disk, so the source
> cannot be dirtied faster than data is mirrored to the target. Also,
> once the block job has converged (BLOCK_JOB_READY sent), source and
> target are guaranteed to stay in sync (unless an error occurs).
>
> Active mode is completely optional and currently disabled at runtime. A
> later patch will add a way for users to enable it.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> Reviewed-by: Fam Zheng <famz@redhat.com>
> Message-id: 20180613181823.13618-13-mreitz@redhat.com
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
> qapi/block-core.json | 18 ++++
> block/mirror.c | 252 ++++++++++++++++++++++++++++++++++++++++++++++++++-
> 2 files changed, 265 insertions(+), 5 deletions(-)
>
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index ab629d1647..96f8da1322 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -1051,6 +1051,24 @@
> 'data': ['top', 'full', 'none', 'incremental'] }
>
> ##
> +# @MirrorCopyMode:
> +#
> +# An enumeration whose values tell the mirror block job when to
> +# trigger writes to the target.
> +#
> +# @background: copy data in background only.
> +#
> +# @write-blocking: when data is written to the source, write it
> +# (synchronously) to the target as well. In
> +# addition, data is copied in background just like in
> +# @background mode.
> +#
> +# Since: 3.0
> +##
> +{ 'enum': 'MirrorCopyMode',
> + 'data': ['background', 'write-blocking'] }
> +
> +##
> # @BlockJobInfo:
> #
> # Information about a long-running block device operation.
> diff --git a/block/mirror.c b/block/mirror.c
> index 7da5e43c0d..99b9b92c30 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -51,8 +51,12 @@ typedef struct MirrorBlockJob {
> Error *replace_blocker;
> bool is_none_mode;
> BlockMirrorBackingMode backing_mode;
> + MirrorCopyMode copy_mode;
> BlockdevOnError on_source_error, on_target_error;
> bool synced;
> + /* Set when the target is synced (dirty bitmap is clean, nothing
> + * in flight) and the job is running in active mode */
> + bool actively_synced;
> bool should_complete;
> int64_t granularity;
> size_t buf_size;
> @@ -74,6 +78,7 @@ typedef struct MirrorBlockJob {
> int target_cluster_size;
> int max_iov;
> bool initial_zeroing_ongoing;
> + int in_active_write_counter;
> } MirrorBlockJob;
>
> typedef struct MirrorBDSOpaque {
> @@ -91,6 +96,7 @@ struct MirrorOp {
> int64_t *bytes_handled;
>
> bool is_pseudo_op;
> + bool is_active_write;
> CoQueue waiting_requests;
>
> QTAILQ_ENTRY(MirrorOp) next;
> @@ -106,6 +112,7 @@ static BlockErrorAction mirror_error_action(MirrorBlockJob *s, bool read,
> int error)
> {
> s->synced = false;
> + s->actively_synced = false;
> if (read) {
> return block_job_error_action(&s->common, s->on_source_error,
> true, error);
> @@ -272,7 +279,7 @@ static int mirror_cow_align(MirrorBlockJob *s, int64_t *offset,
> return ret;
> }
>
> -static inline void mirror_wait_for_free_in_flight_slot(MirrorBlockJob *s)
> +static inline void mirror_wait_for_any_operation(MirrorBlockJob *s, bool active)
> {
> MirrorOp *op;
>
> @@ -282,7 +289,7 @@ static inline void mirror_wait_for_free_in_flight_slot(MirrorBlockJob *s)
> * caller of this function. Since there is only one pseudo op
> * at any given time, we will always find some real operation
> * to wait on. */
> - if (!op->is_pseudo_op) {
> + if (!op->is_pseudo_op && op->is_active_write == active) {
> qemu_co_queue_wait(&op->waiting_requests, NULL);
> return;
> }
> @@ -290,6 +297,12 @@ static inline void mirror_wait_for_free_in_flight_slot(MirrorBlockJob *s)
> abort();
> }
>
> +static inline void mirror_wait_for_free_in_flight_slot(MirrorBlockJob *s)
> +{
> + /* Only non-active operations use up in-flight slots */
> + mirror_wait_for_any_operation(s, false);
> +}
> +
> /* Perform a mirror copy operation.
> *
> * *op->bytes_handled is set to the number of bytes copied after and
> @@ -846,6 +859,7 @@ static void coroutine_fn mirror_run(void *opaque)
> /* Transition to the READY state and wait for complete. */
> job_transition_to_ready(&s->common.job);
> s->synced = true;
> + s->actively_synced = true;
> while (!job_is_cancelled(&s->common.job) && !s->should_complete) {
> job_yield(&s->common.job);
> }
> @@ -897,6 +911,12 @@ static void coroutine_fn mirror_run(void *opaque)
> int64_t cnt, delta;
> bool should_complete;
>
> + /* Do not start passive operations while there are active
> + * writes in progress */
> + while (s->in_active_write_counter) {
> + mirror_wait_for_any_operation(s, true);
> + }
> +
> if (s->ret < 0) {
> ret = s->ret;
> goto immediate_exit;
> @@ -942,6 +962,9 @@ static void coroutine_fn mirror_run(void *opaque)
> */
> job_transition_to_ready(&s->common.job);
> s->synced = true;
> + if (s->copy_mode != MIRROR_COPY_MODE_BACKGROUND) {
> + s->actively_synced = true;
> + }
> }
>
> should_complete = s->should_complete ||
> @@ -1140,16 +1163,232 @@ static const BlockJobDriver commit_active_job_driver = {
> .drain = mirror_drain,
> };
>
> +static void do_sync_target_write(MirrorBlockJob *job, MirrorMethod method,
> + uint64_t offset, uint64_t bytes,
> + QEMUIOVector *qiov, int flags)
> +{
> + BdrvDirtyBitmapIter *iter;
> + QEMUIOVector target_qiov;
> + uint64_t dirty_offset;
> + int dirty_bytes;
> +
> + if (qiov) {
> + qemu_iovec_init(&target_qiov, qiov->niov);
> + }
> +
> + iter = bdrv_dirty_iter_new(job->dirty_bitmap);
> + bdrv_set_dirty_iter(iter, offset);
if offset is not a multiple of bitmap granularity, first result of
hbitmap_iter_next may be less than offset (actually, offset rounded down
to granularity bound). Is it ok?
> +
> + while (true) {
> + bool valid_area;
> + int ret;
> +
> + bdrv_dirty_bitmap_lock(job->dirty_bitmap);
> + valid_area = bdrv_dirty_iter_next_area(iter, offset + bytes,
> + &dirty_offset, &dirty_bytes);
> + if (!valid_area) {
we can skip here a small peace of dirty area, look at may comment to
"[PULL 30/35] block/dirty-bitmap: Add bdrv_dirty_iter_next_area", is it
ok too?
> + bdrv_dirty_bitmap_unlock(job->dirty_bitmap);
> + break;
> + }
> +
> + bdrv_reset_dirty_bitmap_locked(job->dirty_bitmap,
> + dirty_offset, dirty_bytes);
> + bdrv_dirty_bitmap_unlock(job->dirty_bitmap);
> +
> + job_progress_increase_remaining(&job->common.job, dirty_bytes);
> +
> + assert(dirty_offset - offset <= SIZE_MAX);
> + if (qiov) {
> + qemu_iovec_reset(&target_qiov);
> + qemu_iovec_concat(&target_qiov, qiov,
> + dirty_offset - offset, dirty_bytes);
> + }
> +
> + switch (method) {
> + case MIRROR_METHOD_COPY:
> + ret = blk_co_pwritev(job->target, dirty_offset, dirty_bytes,
> + qiov ? &target_qiov : NULL, flags);
> + break;
> +
> + case MIRROR_METHOD_ZERO:
> + assert(!qiov);
> + ret = blk_co_pwrite_zeroes(job->target, dirty_offset, dirty_bytes,
> + flags);
> + break;
> +
> + case MIRROR_METHOD_DISCARD:
> + assert(!qiov);
> + ret = blk_co_pdiscard(job->target, dirty_offset, dirty_bytes);
> + break;
> +
> + default:
> + abort();
> + }
> +
> + if (ret >= 0) {
> + job_progress_update(&job->common.job, dirty_bytes);
> + } else {
> + BlockErrorAction action;
> +
> + bdrv_set_dirty_bitmap(job->dirty_bitmap, dirty_offset, dirty_bytes);
> + job->actively_synced = false;
> +
> + action = mirror_error_action(job, false, -ret);
> + if (action == BLOCK_ERROR_ACTION_REPORT) {
> + if (!job->ret) {
> + job->ret = ret;
> + }
> + break;
> + }
> + }
> + }
> +
> + bdrv_dirty_iter_free(iter);
> + if (qiov) {
> + qemu_iovec_destroy(&target_qiov);
> + }
> +}
> +
> +static MirrorOp *coroutine_fn active_write_prepare(MirrorBlockJob *s,
> + uint64_t offset,
> + uint64_t bytes)
> +{
> + MirrorOp *op;
> + uint64_t start_chunk = offset / s->granularity;
> + uint64_t end_chunk = DIV_ROUND_UP(offset + bytes, s->granularity);
> +
> + op = g_new(MirrorOp, 1);
> + *op = (MirrorOp){
> + .s = s,
> + .offset = offset,
> + .bytes = bytes,
> + .is_active_write = true,
> + };
> + qemu_co_queue_init(&op->waiting_requests);
> + QTAILQ_INSERT_TAIL(&s->ops_in_flight, op, next);
> +
> + s->in_active_write_counter++;
> +
> + mirror_wait_on_conflicts(op, s, offset, bytes);
> +
> + bitmap_set(s->in_flight_bitmap, start_chunk, end_chunk - start_chunk);
> +
> + return op;
> +}
> +
> +static void coroutine_fn active_write_settle(MirrorOp *op)
> +{
> + uint64_t start_chunk = op->offset / op->s->granularity;
> + uint64_t end_chunk = DIV_ROUND_UP(op->offset + op->bytes,
> + op->s->granularity);
> +
> + if (!--op->s->in_active_write_counter && op->s->actively_synced) {
> + BdrvChild *source = op->s->mirror_top_bs->backing;
> +
> + if (QLIST_FIRST(&source->bs->parents) == source &&
> + QLIST_NEXT(source, next_parent) == NULL)
> + {
> + /* Assert that we are back in sync once all active write
> + * operations are settled.
> + * Note that we can only assert this if the mirror node
> + * is the source node's only parent. */
> + assert(!bdrv_get_dirty_count(op->s->dirty_bitmap));
> + }
> + }
> + bitmap_clear(op->s->in_flight_bitmap, start_chunk, end_chunk - start_chunk);
> + QTAILQ_REMOVE(&op->s->ops_in_flight, op, next);
> + qemu_co_queue_restart_all(&op->waiting_requests);
> + g_free(op);
> +}
> +
> static int coroutine_fn bdrv_mirror_top_preadv(BlockDriverState *bs,
> uint64_t offset, uint64_t bytes, QEMUIOVector *qiov, int flags)
> {
> return bdrv_co_preadv(bs->backing, offset, bytes, qiov, flags);
> }
>
> +static int coroutine_fn bdrv_mirror_top_do_write(BlockDriverState *bs,
> + MirrorMethod method, uint64_t offset, uint64_t bytes, QEMUIOVector *qiov,
> + int flags)
> +{
> + MirrorOp *op = NULL;
> + MirrorBDSOpaque *s = bs->opaque;
> + int ret = 0;
> + bool copy_to_target;
> +
> + copy_to_target = s->job->ret >= 0 &&
> + s->job->copy_mode == MIRROR_COPY_MODE_WRITE_BLOCKING;
> +
> + if (copy_to_target) {
> + op = active_write_prepare(s->job, offset, bytes);
> + }
> +
> + switch (method) {
> + case MIRROR_METHOD_COPY:
> + ret = bdrv_co_pwritev(bs->backing, offset, bytes, qiov, flags);
> + break;
> +
> + case MIRROR_METHOD_ZERO:
> + ret = bdrv_co_pwrite_zeroes(bs->backing, offset, bytes, flags);
> + break;
> +
> + case MIRROR_METHOD_DISCARD:
> + ret = bdrv_co_pdiscard(bs->backing->bs, offset, bytes);
> + break;
> +
> + default:
> + abort();
> + }
> +
> + if (ret < 0) {
> + goto out;
> + }
> +
> + if (copy_to_target) {
> + do_sync_target_write(s->job, method, offset, bytes, qiov, flags);
> + }
> +
> +out:
> + if (copy_to_target) {
> + active_write_settle(op);
> + }
> + return ret;
> +}
> +
> static int coroutine_fn bdrv_mirror_top_pwritev(BlockDriverState *bs,
> uint64_t offset, uint64_t bytes, QEMUIOVector *qiov, int flags)
> {
> - return bdrv_co_pwritev(bs->backing, offset, bytes, qiov, flags);
> + MirrorBDSOpaque *s = bs->opaque;
> + QEMUIOVector bounce_qiov;
> + void *bounce_buf;
> + int ret = 0;
> + bool copy_to_target;
> +
> + copy_to_target = s->job->ret >= 0 &&
> + s->job->copy_mode == MIRROR_COPY_MODE_WRITE_BLOCKING;
> +
> + if (copy_to_target) {
> + /* The guest might concurrently modify the data to write; but
> + * the data on source and destination must match, so we have
> + * to use a bounce buffer if we are going to write to the
> + * target now. */
> + bounce_buf = qemu_blockalign(bs, bytes);
> + iov_to_buf_full(qiov->iov, qiov->niov, 0, bounce_buf, bytes);
> +
> + qemu_iovec_init(&bounce_qiov, 1);
> + qemu_iovec_add(&bounce_qiov, bounce_buf, bytes);
> + qiov = &bounce_qiov;
> + }
> +
> + ret = bdrv_mirror_top_do_write(bs, MIRROR_METHOD_COPY, offset, bytes, qiov,
> + flags);
> +
> + if (copy_to_target) {
> + qemu_iovec_destroy(&bounce_qiov);
> + qemu_vfree(bounce_buf);
> + }
> +
> + return ret;
> }
>
> static int coroutine_fn bdrv_mirror_top_flush(BlockDriverState *bs)
> @@ -1164,13 +1403,15 @@ static int coroutine_fn bdrv_mirror_top_flush(BlockDriverState *bs)
> static int coroutine_fn bdrv_mirror_top_pwrite_zeroes(BlockDriverState *bs,
> int64_t offset, int bytes, BdrvRequestFlags flags)
> {
> - return bdrv_co_pwrite_zeroes(bs->backing, offset, bytes, flags);
> + return bdrv_mirror_top_do_write(bs, MIRROR_METHOD_ZERO, offset, bytes, NULL,
> + flags);
> }
>
> static int coroutine_fn bdrv_mirror_top_pdiscard(BlockDriverState *bs,
> int64_t offset, int bytes)
> {
> - return bdrv_co_pdiscard(bs->backing->bs, offset, bytes);
> + return bdrv_mirror_top_do_write(bs, MIRROR_METHOD_DISCARD, offset, bytes,
> + NULL, 0);
> }
>
> static void bdrv_mirror_top_refresh_filename(BlockDriverState *bs, QDict *opts)
> @@ -1340,6 +1581,7 @@ static void mirror_start_job(const char *job_id, BlockDriverState *bs,
> s->on_target_error = on_target_error;
> s->is_none_mode = is_none_mode;
> s->backing_mode = backing_mode;
> + s->copy_mode = MIRROR_COPY_MODE_BACKGROUND;
> s->base = base;
> s->granularity = granularity;
> s->buf_size = ROUND_UP(buf_size, granularity);
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 47+ messages in thread
* [Qemu-devel] [PULL 34/35] block/mirror: Add copy mode QAPI interface
2018-06-18 16:44 [Qemu-devel] [PULL 00/35] Block layer patches Kevin Wolf
` (32 preceding siblings ...)
2018-06-18 16:45 ` [Qemu-devel] [PULL 33/35] block/mirror: Add active mirroring Kevin Wolf
@ 2018-06-18 16:45 ` Kevin Wolf
2018-06-18 16:45 ` [Qemu-devel] [PULL 35/35] iotests: Add test for active mirroring Kevin Wolf
` (2 subsequent siblings)
36 siblings, 0 replies; 47+ messages in thread
From: Kevin Wolf @ 2018-06-18 16:45 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel
From: Max Reitz <mreitz@redhat.com>
This patch allows the user to specify whether to use active or only
background mode for mirror block jobs. Currently, this setting will
remain constant for the duration of the entire block job.
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
Message-id: 20180613181823.13618-14-mreitz@redhat.com
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
qapi/block-core.json | 11 +++++++++--
include/block/block_int.h | 4 +++-
block/mirror.c | 12 +++++++-----
blockdev.c | 9 ++++++++-
4 files changed, 27 insertions(+), 9 deletions(-)
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 96f8da1322..cc3ede0630 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1710,6 +1710,9 @@
# written. Both will result in identical contents.
# Default is true. (Since 2.4)
#
+# @copy-mode: when to copy data to the destination; defaults to 'background'
+# (Since: 3.0)
+#
# Since: 1.3
##
{ 'struct': 'DriveMirror',
@@ -1719,7 +1722,7 @@
'*speed': 'int', '*granularity': 'uint32',
'*buf-size': 'int', '*on-source-error': 'BlockdevOnError',
'*on-target-error': 'BlockdevOnError',
- '*unmap': 'bool' } }
+ '*unmap': 'bool', '*copy-mode': 'MirrorCopyMode' } }
##
# @BlockDirtyBitmap:
@@ -1982,6 +1985,9 @@
# above @device. If this option is not given, a node name is
# autogenerated. (Since: 2.9)
#
+# @copy-mode: when to copy data to the destination; defaults to 'background'
+# (Since: 3.0)
+#
# Returns: nothing on success.
#
# Since: 2.6
@@ -2002,7 +2008,8 @@
'*speed': 'int', '*granularity': 'uint32',
'*buf-size': 'int', '*on-source-error': 'BlockdevOnError',
'*on-target-error': 'BlockdevOnError',
- '*filter-node-name': 'str' } }
+ '*filter-node-name': 'str',
+ '*copy-mode': 'MirrorCopyMode' } }
##
# @block_set_io_throttle:
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 7cd7eed83b..74646ed722 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -1031,6 +1031,7 @@ void commit_active_start(const char *job_id, BlockDriverState *bs,
* @filter_node_name: The node name that should be assigned to the filter
* driver that the mirror job inserts into the graph above @bs. NULL means that
* a node name should be autogenerated.
+ * @copy_mode: When to trigger writes to the target.
* @errp: Error object.
*
* Start a mirroring operation on @bs. Clusters that are allocated
@@ -1044,7 +1045,8 @@ void mirror_start(const char *job_id, BlockDriverState *bs,
MirrorSyncMode mode, BlockMirrorBackingMode backing_mode,
BlockdevOnError on_source_error,
BlockdevOnError on_target_error,
- bool unmap, const char *filter_node_name, Error **errp);
+ bool unmap, const char *filter_node_name,
+ MirrorCopyMode copy_mode, Error **errp);
/*
* backup_job_create:
diff --git a/block/mirror.c b/block/mirror.c
index 99b9b92c30..61bd9f3cf1 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1473,7 +1473,7 @@ static void mirror_start_job(const char *job_id, BlockDriverState *bs,
const BlockJobDriver *driver,
bool is_none_mode, BlockDriverState *base,
bool auto_complete, const char *filter_node_name,
- bool is_mirror,
+ bool is_mirror, MirrorCopyMode copy_mode,
Error **errp)
{
MirrorBlockJob *s;
@@ -1581,7 +1581,7 @@ static void mirror_start_job(const char *job_id, BlockDriverState *bs,
s->on_target_error = on_target_error;
s->is_none_mode = is_none_mode;
s->backing_mode = backing_mode;
- s->copy_mode = MIRROR_COPY_MODE_BACKGROUND;
+ s->copy_mode = copy_mode;
s->base = base;
s->granularity = granularity;
s->buf_size = ROUND_UP(buf_size, granularity);
@@ -1648,7 +1648,8 @@ void mirror_start(const char *job_id, BlockDriverState *bs,
MirrorSyncMode mode, BlockMirrorBackingMode backing_mode,
BlockdevOnError on_source_error,
BlockdevOnError on_target_error,
- bool unmap, const char *filter_node_name, Error **errp)
+ bool unmap, const char *filter_node_name,
+ MirrorCopyMode copy_mode, Error **errp)
{
bool is_none_mode;
BlockDriverState *base;
@@ -1663,7 +1664,7 @@ void mirror_start(const char *job_id, BlockDriverState *bs,
speed, granularity, buf_size, backing_mode,
on_source_error, on_target_error, unmap, NULL, NULL,
&mirror_job_driver, is_none_mode, base, false,
- filter_node_name, true, errp);
+ filter_node_name, true, copy_mode, errp);
}
void commit_active_start(const char *job_id, BlockDriverState *bs,
@@ -1686,7 +1687,8 @@ void commit_active_start(const char *job_id, BlockDriverState *bs,
MIRROR_LEAVE_BACKING_CHAIN,
on_error, on_error, true, cb, opaque,
&commit_active_job_driver, false, base, auto_complete,
- filter_node_name, false, &local_err);
+ filter_node_name, false, MIRROR_COPY_MODE_BACKGROUND,
+ &local_err);
if (local_err) {
error_propagate(errp, local_err);
goto error_restore_flags;
diff --git a/blockdev.c b/blockdev.c
index 7f65cd7497..58d7570932 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3586,6 +3586,7 @@ static void blockdev_mirror_common(const char *job_id, BlockDriverState *bs,
bool has_unmap, bool unmap,
bool has_filter_node_name,
const char *filter_node_name,
+ bool has_copy_mode, MirrorCopyMode copy_mode,
Error **errp)
{
@@ -3610,6 +3611,9 @@ static void blockdev_mirror_common(const char *job_id, BlockDriverState *bs,
if (!has_filter_node_name) {
filter_node_name = NULL;
}
+ if (!has_copy_mode) {
+ copy_mode = MIRROR_COPY_MODE_BACKGROUND;
+ }
if (granularity != 0 && (granularity < 512 || granularity > 1048576 * 64)) {
error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "granularity",
@@ -3640,7 +3644,7 @@ static void blockdev_mirror_common(const char *job_id, BlockDriverState *bs,
has_replaces ? replaces : NULL,
speed, granularity, buf_size, sync, backing_mode,
on_source_error, on_target_error, unmap, filter_node_name,
- errp);
+ copy_mode, errp);
}
void qmp_drive_mirror(DriveMirror *arg, Error **errp)
@@ -3786,6 +3790,7 @@ void qmp_drive_mirror(DriveMirror *arg, Error **errp)
arg->has_on_target_error, arg->on_target_error,
arg->has_unmap, arg->unmap,
false, NULL,
+ arg->has_copy_mode, arg->copy_mode,
&local_err);
bdrv_unref(target_bs);
error_propagate(errp, local_err);
@@ -3806,6 +3811,7 @@ void qmp_blockdev_mirror(bool has_job_id, const char *job_id,
BlockdevOnError on_target_error,
bool has_filter_node_name,
const char *filter_node_name,
+ bool has_copy_mode, MirrorCopyMode copy_mode,
Error **errp)
{
BlockDriverState *bs;
@@ -3838,6 +3844,7 @@ void qmp_blockdev_mirror(bool has_job_id, const char *job_id,
has_on_target_error, on_target_error,
true, true,
has_filter_node_name, filter_node_name,
+ has_copy_mode, copy_mode,
&local_err);
error_propagate(errp, local_err);
--
2.13.6
^ permalink raw reply related [flat|nested] 47+ messages in thread* [Qemu-devel] [PULL 35/35] iotests: Add test for active mirroring
2018-06-18 16:44 [Qemu-devel] [PULL 00/35] Block layer patches Kevin Wolf
` (33 preceding siblings ...)
2018-06-18 16:45 ` [Qemu-devel] [PULL 34/35] block/mirror: Add copy mode QAPI interface Kevin Wolf
@ 2018-06-18 16:45 ` Kevin Wolf
2018-06-18 18:50 ` [Qemu-devel] [PULL 00/35] Block layer patches no-reply
2018-06-19 15:57 ` Peter Maydell
36 siblings, 0 replies; 47+ messages in thread
From: Kevin Wolf @ 2018-06-18 16:45 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel
From: Max Reitz <mreitz@redhat.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
Message-id: 20180613181823.13618-15-mreitz@redhat.com
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
tests/qemu-iotests/151 | 120 +++++++++++++++++++++++++++++++++++++++++++++
tests/qemu-iotests/151.out | 5 ++
tests/qemu-iotests/group | 1 +
3 files changed, 126 insertions(+)
create mode 100755 tests/qemu-iotests/151
create mode 100644 tests/qemu-iotests/151.out
diff --git a/tests/qemu-iotests/151 b/tests/qemu-iotests/151
new file mode 100755
index 0000000000..fe53b9f446
--- /dev/null
+++ b/tests/qemu-iotests/151
@@ -0,0 +1,120 @@
+#!/usr/bin/env python
+#
+# Tests for active mirroring
+#
+# Copyright (C) 2018 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+#
+
+import os
+import iotests
+from iotests import qemu_img
+
+source_img = os.path.join(iotests.test_dir, 'source.' + iotests.imgfmt)
+target_img = os.path.join(iotests.test_dir, 'target.' + iotests.imgfmt)
+
+class TestActiveMirror(iotests.QMPTestCase):
+ image_len = 128 * 1024 * 1024 # MB
+ potential_writes_in_flight = True
+
+ def setUp(self):
+ qemu_img('create', '-f', iotests.imgfmt, source_img, '128M')
+ qemu_img('create', '-f', iotests.imgfmt, target_img, '128M')
+
+ blk_source = {'id': 'source',
+ 'if': 'none',
+ 'node-name': 'source-node',
+ 'driver': iotests.imgfmt,
+ 'file': {'driver': 'file',
+ 'filename': source_img}}
+
+ blk_target = {'node-name': 'target-node',
+ 'driver': iotests.imgfmt,
+ 'file': {'driver': 'file',
+ 'filename': target_img}}
+
+ self.vm = iotests.VM()
+ self.vm.add_drive_raw(self.vm.qmp_to_opts(blk_source))
+ self.vm.add_blockdev(self.vm.qmp_to_opts(blk_target))
+ self.vm.add_device('virtio-blk,drive=source')
+ self.vm.launch()
+
+ def tearDown(self):
+ self.vm.shutdown()
+
+ if not self.potential_writes_in_flight:
+ self.assertTrue(iotests.compare_images(source_img, target_img),
+ 'mirror target does not match source')
+
+ os.remove(source_img)
+ os.remove(target_img)
+
+ def doActiveIO(self, sync_source_and_target):
+ # Fill the source image
+ self.vm.hmp_qemu_io('source',
+ 'write -P 1 0 %i' % self.image_len);
+
+ # Start some background requests
+ for offset in range(1 * self.image_len / 8, 3 * self.image_len / 8, 1024 * 1024):
+ self.vm.hmp_qemu_io('source', 'aio_write -P 2 %i 1M' % offset)
+ for offset in range(2 * self.image_len / 8, 3 * self.image_len / 8, 1024 * 1024):
+ self.vm.hmp_qemu_io('source', 'aio_write -z %i 1M' % offset)
+
+ # Start the block job
+ result = self.vm.qmp('blockdev-mirror',
+ job_id='mirror',
+ filter_node_name='mirror-node',
+ device='source-node',
+ target='target-node',
+ sync='full',
+ copy_mode='write-blocking')
+ self.assert_qmp(result, 'return', {})
+
+ # Start some more requests
+ for offset in range(3 * self.image_len / 8, 5 * self.image_len / 8, 1024 * 1024):
+ self.vm.hmp_qemu_io('source', 'aio_write -P 3 %i 1M' % offset)
+ for offset in range(4 * self.image_len / 8, 5 * self.image_len / 8, 1024 * 1024):
+ self.vm.hmp_qemu_io('source', 'aio_write -z %i 1M' % offset)
+
+ # Wait for the READY event
+ self.wait_ready(drive='mirror')
+
+ # Now start some final requests; all of these (which land on
+ # the source) should be settled using the active mechanism.
+ # The mirror code itself asserts that the source BDS's dirty
+ # bitmap will stay clean between READY and COMPLETED.
+ for offset in range(5 * self.image_len / 8, 7 * self.image_len / 8, 1024 * 1024):
+ self.vm.hmp_qemu_io('source', 'aio_write -P 3 %i 1M' % offset)
+ for offset in range(6 * self.image_len / 8, 7 * self.image_len / 8, 1024 * 1024):
+ self.vm.hmp_qemu_io('source', 'aio_write -z %i 1M' % offset)
+
+ if sync_source_and_target:
+ # If source and target should be in sync after the mirror,
+ # we have to flush before completion
+ self.vm.hmp_qemu_io('source', 'aio_flush')
+ self.potential_writes_in_flight = False
+
+ self.complete_and_wait(drive='mirror', wait_ready=False)
+
+ def testActiveIO(self):
+ self.doActiveIO(False)
+
+ def testActiveIOFlushed(self):
+ self.doActiveIO(True)
+
+
+
+if __name__ == '__main__':
+ iotests.main(supported_fmts=['qcow2', 'raw'])
diff --git a/tests/qemu-iotests/151.out b/tests/qemu-iotests/151.out
new file mode 100644
index 0000000000..fbc63e62f8
--- /dev/null
+++ b/tests/qemu-iotests/151.out
@@ -0,0 +1,5 @@
+..
+----------------------------------------------------------------------
+Ran 2 tests
+
+OK
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 937a3d0a4d..eea75819d2 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -157,6 +157,7 @@
148 rw auto quick
149 rw auto sudo
150 rw auto quick
+151 rw auto
152 rw auto quick
153 rw auto quick
154 rw auto backing quick
--
2.13.6
^ permalink raw reply related [flat|nested] 47+ messages in thread* Re: [Qemu-devel] [PULL 00/35] Block layer patches
2018-06-18 16:44 [Qemu-devel] [PULL 00/35] Block layer patches Kevin Wolf
` (34 preceding siblings ...)
2018-06-18 16:45 ` [Qemu-devel] [PULL 35/35] iotests: Add test for active mirroring Kevin Wolf
@ 2018-06-18 18:50 ` no-reply
2018-06-19 15:57 ` Peter Maydell
36 siblings, 0 replies; 47+ messages in thread
From: no-reply @ 2018-06-18 18:50 UTC (permalink / raw)
To: kwolf; +Cc: famz, qemu-block, qemu-devel
Hi,
This series seems to have some coding style problems. See output below for
more information:
Type: series
Message-id: 20180618164504.24488-1-kwolf@redhat.com
Subject: [Qemu-devel] [PULL 00/35] Block layer patches
=== TEST SCRIPT BEGIN ===
#!/bin/bash
BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))
done
exit $failed
=== TEST SCRIPT END ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
* [new tag] patchew/20180618164504.24488-1-kwolf@redhat.com -> patchew/20180618164504.24488-1-kwolf@redhat.com
Switched to a new branch 'test'
261e944402 iotests: Add test for active mirroring
d3a0ca0eea block/mirror: Add copy mode QAPI interface
b764786814 block/mirror: Add active mirroring
8895e3d343 job: Add job_progress_increase_remaining()
bbf2bb9443 block/mirror: Add MirrorBDSOpaque
ba9ab133e7 block/dirty-bitmap: Add bdrv_dirty_iter_next_area
e30cbdb941 test-hbitmap: Add non-advancing iter_next tests
5e2182782b hbitmap: Add @advance param to hbitmap_iter_next()
36f4266813 block: Generalize should_update_child() rule
ba264cc757 block/mirror: Use source as a BdrvChild
063fc71753 block/mirror: Wait for in-flight op conflicts
af955a4481 block/mirror: Use CoQueue to wait on in-flight ops
997541f920 block/mirror: Convert to coroutines
b64d9ae31a block/mirror: Pull out mirror_perform()
663ef8298d block: fix QEMU crash with scsi-hd and drive_del
6c3f8c482a test-bdrv-drain: Test graph changes in drain_all section
68fc9aff1c block: Allow graph changes in bdrv_drain_all_begin/end sections
d1d3ed6904 block: ignore_bds_parents parameter for drain functions
f8c0b911e0 block: Move bdrv_drain_all_begin() out of coroutine context
a3788f4f63 block: Allow AIO_WAIT_WHILE with NULL ctx
b81b248bc3 test-bdrv-drain: Test that bdrv_drain_invoke() doesn't poll
ded108f0fe block: Defer .bdrv_drain_begin callback to polling phase
bba3bdfe5c test-bdrv-drain: Graph change through parent callback
caee2b495b block: Don't poll in parent drain callbacks
b53788a5c5 test-bdrv-drain: Test node deletion in subtree recursion
9463657eee block: Drain recursively with a single BDRV_POLL_WHILE()
7d60930932 test-bdrv-drain: Add test for node deletion
bf34a48b66 block: Remove bdrv_drain_recurse()
2b0389a675 block: Really pause block jobs on drain
f47738d5fb block: Avoid unnecessary aio_poll() in AIO_WAIT_WHILE()
b942cc966d tests/test-bdrv-drain: bdrv_drain_all() works in coroutines now
b42befd0f8 block: Don't manually poll in bdrv_drain_all()
8e5f337a97 block: Remove 'recursive' parameter from bdrv_drain_invoke()
dbe66b7ff7 block: Use bdrv_do_drain_begin/end in bdrv_drain_all()
e2fdbc78df test-bdrv-drain: bdrv_drain() works with cross-AioContext events
=== OUTPUT BEGIN ===
Checking PATCH 1/35: test-bdrv-drain: bdrv_drain() works with cross-AioContext events...
Checking PATCH 2/35: block: Use bdrv_do_drain_begin/end in bdrv_drain_all()...
Checking PATCH 3/35: block: Remove 'recursive' parameter from bdrv_drain_invoke()...
Checking PATCH 4/35: block: Don't manually poll in bdrv_drain_all()...
Checking PATCH 5/35: tests/test-bdrv-drain: bdrv_drain_all() works in coroutines now...
Checking PATCH 6/35: block: Avoid unnecessary aio_poll() in AIO_WAIT_WHILE()...
ERROR: trailing statements should be on next line
#38: FILE: block/io.c:190:
+ while (aio_poll(bs->aio_context, false));
ERROR: braces {} are necessary for all arms of this statement
#38: FILE: block/io.c:190:
+ while (aio_poll(bs->aio_context, false));
[...]
total: 2 errors, 0 warnings, 60 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Checking PATCH 7/35: block: Really pause block jobs on drain...
Checking PATCH 8/35: block: Remove bdrv_drain_recurse()...
Checking PATCH 9/35: test-bdrv-drain: Add test for node deletion...
Checking PATCH 10/35: block: Drain recursively with a single BDRV_POLL_WHILE()...
Checking PATCH 11/35: test-bdrv-drain: Test node deletion in subtree recursion...
WARNING: line over 80 characters
#85: FILE: tests/test-bdrv-drain.c:1034:
+ g_test_add_func("/bdrv-drain/detach/drain_subtree", test_detach_by_drain_subtree);
total: 0 errors, 1 warnings, 68 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Checking PATCH 12/35: block: Don't poll in parent drain callbacks...
Checking PATCH 13/35: test-bdrv-drain: Graph change through parent callback...
WARNING: line over 80 characters
#81: FILE: tests/test-bdrv-drain.c:1049:
+ child_a = bdrv_attach_child(parent_b, a, "PB-A", &child_backing, &error_abort);
total: 0 errors, 1 warnings, 142 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Checking PATCH 14/35: block: Defer .bdrv_drain_begin callback to polling phase...
Checking PATCH 15/35: test-bdrv-drain: Test that bdrv_drain_invoke() doesn't poll...
Checking PATCH 16/35: block: Allow AIO_WAIT_WHILE with NULL ctx...
Checking PATCH 17/35: block: Move bdrv_drain_all_begin() out of coroutine context...
WARNING: line over 80 characters
#27: FILE: block/io.c:270:
+ bdrv_do_drained_begin(bs, data->recursive, data->parent, data->poll);
total: 0 errors, 1 warnings, 41 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Checking PATCH 18/35: block: ignore_bds_parents parameter for drain functions...
Checking PATCH 19/35: block: Allow graph changes in bdrv_drain_all_begin/end sections...
ERROR: do not initialise globals to 0 or NULL
#123: FILE: block/io.c:477:
+unsigned int bdrv_drain_all_count = 0;
ERROR: trailing statements should be on next line
#132: FILE: block/io.c:486:
+ while (aio_poll(qemu_get_aio_context(), false));
ERROR: braces {} are necessary for all arms of this statement
#132: FILE: block/io.c:486:
+ while (aio_poll(qemu_get_aio_context(), false));
[...]
total: 3 errors, 0 warnings, 193 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Checking PATCH 20/35: test-bdrv-drain: Test graph changes in drain_all section...
Checking PATCH 21/35: block: fix QEMU crash with scsi-hd and drive_del...
Checking PATCH 22/35: block/mirror: Pull out mirror_perform()...
Checking PATCH 23/35: block/mirror: Convert to coroutines...
Checking PATCH 24/35: block/mirror: Use CoQueue to wait on in-flight ops...
Checking PATCH 25/35: block/mirror: Wait for in-flight op conflicts...
Checking PATCH 26/35: block/mirror: Use source as a BdrvChild...
Checking PATCH 27/35: block: Generalize should_update_child() rule...
Checking PATCH 28/35: hbitmap: Add @advance param to hbitmap_iter_next()...
Checking PATCH 29/35: test-hbitmap: Add non-advancing iter_next tests...
Checking PATCH 30/35: block/dirty-bitmap: Add bdrv_dirty_iter_next_area...
Checking PATCH 31/35: block/mirror: Add MirrorBDSOpaque...
Checking PATCH 32/35: job: Add job_progress_increase_remaining()...
Checking PATCH 33/35: block/mirror: Add active mirroring...
Checking PATCH 34/35: block/mirror: Add copy mode QAPI interface...
Checking PATCH 35/35: iotests: Add test for active mirroring...
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#14:
new file mode 100755
total: 0 errors, 1 warnings, 132 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===
Test command exited with code: 1
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [Qemu-devel] [PULL 00/35] Block layer patches
2018-06-18 16:44 [Qemu-devel] [PULL 00/35] Block layer patches Kevin Wolf
` (35 preceding siblings ...)
2018-06-18 18:50 ` [Qemu-devel] [PULL 00/35] Block layer patches no-reply
@ 2018-06-19 15:57 ` Peter Maydell
36 siblings, 0 replies; 47+ messages in thread
From: Peter Maydell @ 2018-06-19 15:57 UTC (permalink / raw)
To: Kevin Wolf; +Cc: Qemu-block, QEMU Developers
On 18 June 2018 at 17:44, Kevin Wolf <kwolf@redhat.com> wrote:
> The following changes since commit 2ef2f16781af9dee6ba6517755e9073ba5799fa2:
>
> Merge remote-tracking branch 'remotes/dgilbert/tags/pull-migration-20180615a' into staging (2018-06-15 18:13:35 +0100)
>
> are available in the git repository at:
>
> git://repo.or.cz/qemu/kevin.git tags/for-upstream
>
> for you to fetch changes up to 4c790afe2503eab12874508acab5b388d7babfd2:
>
> Merge remote-tracking branch 'mreitz/tags/pull-block-2018-06-18' into queue-block (2018-06-18 17:20:42 +0200)
>
> ----------------------------------------------------------------
> Block layer patches:
>
> - Active mirror (blockdev-mirror copy-mode=write-blocking)
> - bdrv_drain_*() fixes and test cases
> - Fix crash with scsi-hd and drive_del
>
Applied, thanks.
-- PMM
^ permalink raw reply [flat|nested] 47+ messages in thread