Drain fixes and cleanups, part 2
cover Drain fixes and cleanups, part 2
This is the second part of my work to fix drain and hopefully to prevent it from attracting bugs as much as it did in the past. There is definitely at least a third part coming after this, see below.
In this series, the following improvments are made:
* Fix several bugs and inconsistencies
* Create lots of unit tests for the drain functions
* Introduce bdrv_subtree_drained_begin/end() to drain a whole subtree rather than only a single node
* Use this to make bdrv_reopen() safe (graph changes in callbacks called during its internal bdrv_drain_all() frequently broke it)
Planned for part three: Make graph modifications in callbacks safe by avoiding BDRV_POLL_WHILE() calls while we're recursing through the graph. We can recurse before BDRV_POLL_WHILE(), after it or while evaluating its condition, but never call any callbacks while we're iterating child or parent lists.
Fam Zheng (1):
block: Remove unused bdrv_requests_pending
Kevin Wolf (18):
block: Assert drain_all is only called from main AioContext
block: Make bdrv_drain() driver callbacks non-recursive
test-bdrv-drain: Test callback for bdrv_drain
test-bdrv-drain: Test bs->quiesce_counter
blockjob: Pause job on draining any job BDS
test-bdrv-drain: Test drain vs. block jobs
block: Don't block_job_pause_all() in bdrv_drain_all()
block: Nested drain_end must still call callbacks
test-bdrv-drain: Test nested drain sections
block: Don't notify parents in drain call chain
block: Add bdrv_subtree_drained_begin/end()
test-bdrv-drain: Tests for bdrv_subtree_drain
test-bdrv-drain: Test behaviour in coroutine context
test-bdrv-drain: Recursive draining with multiple parents
block: Allow graph changes in subtree drained section
test-bdrv-drain: Test graph changes in drained section
commit: Simplify reopen of base
block: Keep nodes drained between reopen_queue/multiple
include/block/block.h | 20 +- include/block/block_int.h | 3 +- block.c | 74 +++++-- block/commit.c | 8 +- block/io.c | 128 ++++++++---- block/replication.c | 6 + blockjob.c | 22 +- qemu-io-cmds.c | 3 + tests/test-bdrv-drain.c | 520 +++++++++++++++++++++++++++++++++++++++++++++- 9 files changed, 696 insertions(+), 88 deletions(-)
Signature · 1 line
2.13.6
PATCH 1/19 block: Remove unused bdrv_requests_pending
From: Fam Zheng <[email protected]>
--- include/block/block_int.h | 1 - block/io.c | 18 ------------------ 2 files changed, 19 deletions(-)
include/block/block_int.h +0 -1
@@ -1045,7 +1045,6 @@ bool blk_dev_is_tray_open(BlockBackend *blk);bool blk_dev_is_medium_locked (BlockBackend *blk);void bdrv_set_dirty (BlockDriverState *bs,int64_t offset,int64_t bytes); -bool bdrv_requests_pending (BlockDriverState *bs);void bdrv_clear_dirty_bitmap (BdrvDirtyBitmap *bitmap,HBitmap **out);void bdrv_undo_clear_dirty_bitmap (BdrvDirtyBitmap *bitmap,HBitmap *in);
block/io.c +0 -18
@@ -134,24 +134,6 @@ void bdrv_disable_copy_on_read(BlockDriverState *bs)assert (old >=1 ); } -/* Check if any requests are in-flight (including throttled requests) */ -bool bdrv_requests_pending (BlockDriverState *bs) -{ -BdrvChild *child; - -if (atomic_read (&bs->in_flight)) { -return true; - } - - QLIST_FOREACH(child, &bs->children, next) { -if (bdrv_requests_pending (child->bs)) { -return true; - } - } - -return false; -} - typedef struct { Coroutine *co; BlockDriverState *bs;
Signature · 1 line
2.13.6
PATCH 2/19 block: Assert drain_all is only called from main AioContext
In reply to Kevin Wolf--- block/io.c | 6 ++++++ 1 file changed, 6 insertions(+)
block/io.c +6 -0
@@ -330,6 +330,12 @@ void bdrv_drain_all_begin(void)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 +* nodes in several different AioContexts, so make sure we're in the main +* context. */ +assert (qemu_get_current_aio_context () ==qemu_get_aio_context ()); +block_job_pause_all ();for (bs =bdrv_first (&it); bs; bs =bdrv_next (&it)) {
Signature · 1 line
2.13.6
Re: [Qemu-devel] [PATCH 02/19] block: Assert drain_all is only called from main AioContext
Quoted 25 lines · Kevin Wolf
Signed-off-by Kevin Wolf <[email protected]>--- block/io.c | 6 ++++++ 1 file changed, 6 insertions(+)block/io.c +6
-0@@ -330,6 +330,12 @@ void bdrv_drain_all_begin(void)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 +* nodes in several different AioContexts, so make sure we're in the main +* context. */ +assert (qemu_get_current_aio_context () ==qemu_get_aio_context ()); +block_job_pause_all ();for (bs =bdrv_first (&it); bs; bs =bdrv_next (&it)) {Signature · 1 line
2.13.6
PATCH 3/19 block: Make bdrv_drain() driver callbacks non-recursive
In reply to Kevin Wolfbdrv_drain_begin() doesn't increase bs->quiesce_counter recursively, which means that the child nodes are not actually drained. To keep this consistent, we also shouldn't call the block driver callbacks.
--- block/io.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-)
block/io.c +9 -7
@@ -158,7 +158,7 @@ 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) +static void bdrv_drain_invoke (BlockDriverState *bs,bool begin,bool recursive) {BdrvChild *child, *tmp;BdrvCoDrainData data = { .bs = bs, .done = false, .begin = begin};
@@ -172,8 +172,10 @@ static void bdrv_drain_invoke(BlockDriverState *bs, bool begin)bdrv_coroutine_enter (bs, data.co);BDRV_POLL_WHILE (bs, !data.done); - QLIST_FOREACH_SAFE(child, &bs->children, next, tmp) { -bdrv_drain_invoke (child->bs, begin); +if (recursive) { + QLIST_FOREACH_SAFE(child, &bs->children, next, tmp) { +bdrv_drain_invoke (child->bs, begin, true); + } } }
@@ -265,7 +267,7 @@ void bdrv_drained_begin(BlockDriverState *bs)bdrv_parent_drained_begin (bs); } -bdrv_drain_invoke (bs, true); +bdrv_drain_invoke (bs, true, false);bdrv_drain_recurse (bs); }
@@ -281,7 +283,7 @@ void bdrv_drained_end(BlockDriverState *bs) }/* Re-enable things in child-to-parent order */ -bdrv_drain_invoke (bs, false); +bdrv_drain_invoke (bs, false, false);bdrv_parent_drained_end (bs);aio_enable_external (bdrv_get_aio_context (bs)); }
@@ -345,7 +347,7 @@ void bdrv_drain_all_begin(void)aio_context_acquire (aio_context);aio_disable_external (aio_context);bdrv_parent_drained_begin (bs); -bdrv_drain_invoke (bs, true); +bdrv_drain_invoke (bs, true, true);aio_context_release (aio_context);if (!g_slist_find (aio_ctxs, aio_context)) {
@@ -388,7 +390,7 @@ void bdrv_drain_all_end(void)/* Re-enable things in child-to-parent order */ aio_context_acquire (aio_context); -bdrv_drain_invoke (bs, false); +bdrv_drain_invoke (bs, false, true);bdrv_parent_drained_end (bs);aio_enable_external (aio_context);aio_context_release (aio_context);
Signature · 1 line
2.13.6
Re: [Qemu-devel] [PATCH 03/19] block: Make bdrv_drain() driver callbacks non-recursive
Quoted 1 line · Kevin Wolf
bdrv_drain_begin()doesn't increasebs->quiesce_counterrecursively,
Pretty trivial but:
s/bdrv_drain_begin/bdrv_drained_begin/
Quoted 35 lines
which means that the child nodes are not actually drained. To keep this consistent, we also shouldn't call the block driver callbacks.
Signed-off-by Kevin Wolf <[email protected]>--- block/io.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-)block/io.c +5
-3@@ -158,7 +158,7 @@ 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) +static void bdrv_drain_invoke (BlockDriverState *bs,bool begin,bool recursive) {BdrvChild *child, *tmp;BdrvCoDrainData data = { .bs = bs, .done = false, .begin = begin};@@ -172,8 +172,10 @@ static void bdrv_drain_invoke(BlockDriverState *bs, bool begin)bdrv_coroutine_enter (bs, data.co);BDRV_POLL_WHILE (bs, !data.done); - QLIST_FOREACH_SAFE(child, &bs->children, next, tmp) { -bdrv_drain_invoke (child->bs, begin); +if (recursive) { + QLIST_FOREACH_SAFE(child, &bs->children, next, tmp) { +bdrv_drain_invoke (child->bs, begin, true); + } } }
block/io.c void bdrv_drained_begin(BlockDriverState *bs) · line 270
Quoted 5 lines
block/io.c +1
-1@@ -265,7 +267,7 @@ void bdrv_drained_begin(BlockDriverState *bs)bdrv_parent_drained_begin (bs); } -bdrv_drain_invoke (bs, true); +bdrv_drain_invoke (bs, true, false);
I'm not sure I understand this patch. If we call bdrv_drained_begin on a quorum BDS that has a QED child, we do want to call bdrv_qed_co_drain_begin() on the child, don't we? I think after this patch, we don't do that any more?
The same question arises when I look at throttle_co_drain_begin()..
Fam
Re: [Qemu-devel] [PATCH 03/19] block: Make bdrv_drain() driver callbacks non-recursive
Am 20.12.2017 um 14:16 hat Fam Zheng geschrieben:
Quoted 6 lines · Kevin Wolf
Quoted 1 line
bdrv_drain_begin()doesn't increasebs->quiesce_counterrecursively,Pretty trivial but:
s/bdrv_drain_begin/bdrv_drained_begin/
Yes, thanks.
Quoted 38 lines
Quoted 38 lines
which means that the child nodes are not actually drained. To keep this consistent, we also shouldn't call the block driver callbacks.
Signed-off-by Kevin Wolf <[email protected]>--- block/io.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-)block/io.c +5
-3@@ -158,7 +158,7 @@ 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) +static void bdrv_drain_invoke (BlockDriverState *bs,bool begin,bool recursive) {BdrvChild *child, *tmp;BdrvCoDrainData data = { .bs = bs, .done = false, .begin = begin};@@ -172,8 +172,10 @@ static void bdrv_drain_invoke(BlockDriverState *bs, bool begin)bdrv_coroutine_enter (bs, data.co);BDRV_POLL_WHILE (bs, !data.done); - QLIST_FOREACH_SAFE(child, &bs->children, next, tmp) { -bdrv_drain_invoke (child->bs, begin); +if (recursive) { + QLIST_FOREACH_SAFE(child, &bs->children, next, tmp) { +bdrv_drain_invoke (child->bs, begin, true); + } } }@@ -265,7 +267,7 @@ void bdrv_drained_begin(BlockDriverState *bs)bdrv_parent_drained_begin (bs); }
Quoted 8 lines
block/io.c void bdrv_drained_begin(BlockDriverState *bs) · line 270
Quoted 2 lines
block/io.c +1
-1-bdrv_drain_invoke (bs, true); +bdrv_drain_invoke (bs, true, false);I'm not sure I understand this patch. If we call
bdrv_drained_beginon a quorum BDS that has a QED child, we do want to callbdrv_qed_co_drain_begin()on the child, don't we? I think after this patch, we don't do that any more?The same question arises when I look at
throttle_co_drain_begin()..
We don't increase bs->quiesce_counter for child nodes and don't notify their parents, so they aren't really drained. Calling the BlcokDriver callbacks is the only thing we did recursively. We should do one thing consistently, and for bdrv_drained_begin/end() that is draining a single node without the children.
Later in the series, I introduce a bdrv_subtree_drained_begin/end() which doesn't only call the callbacks recursively, but also increases bs->quiesce_counter etc. so that the child nodes are actually drained.
There are use cases for both functions.
Kevin
Fam Zheng
Reviewed-byRe: [Qemu-devel] [PATCH 03/19] block: Make bdrv_drain() driver callbacks non-recursive
Quoted 63 lines · Kevin Wolf
Am 20.12.2017 um 14:16 hat Fam Zheng geschrieben:
Quoted 6 lines · Kevin Wolf
Quoted 1 line
bdrv_drain_begin()doesn't increasebs->quiesce_counterrecursively,Pretty trivial but:
s/bdrv_drain_begin/bdrv_drained_begin/
Yes, thanks.
Quoted 38 lines
Quoted 38 lines
which means that the child nodes are not actually drained. To keep this consistent, we also shouldn't call the block driver callbacks.
Signed-off-by Kevin Wolf <[email protected]>--- block/io.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-)block/io.c +5
-3@@ -158,7 +158,7 @@ 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) +static void bdrv_drain_invoke (BlockDriverState *bs,bool begin,bool recursive) {BdrvChild *child, *tmp;BdrvCoDrainData data = { .bs = bs, .done = false, .begin = begin};@@ -172,8 +172,10 @@ static void bdrv_drain_invoke(BlockDriverState *bs, bool begin)bdrv_coroutine_enter (bs, data.co);BDRV_POLL_WHILE (bs, !data.done); - QLIST_FOREACH_SAFE(child, &bs->children, next, tmp) { -bdrv_drain_invoke (child->bs, begin); +if (recursive) { + QLIST_FOREACH_SAFE(child, &bs->children, next, tmp) { +bdrv_drain_invoke (child->bs, begin, true); + } } }@@ -265,7 +267,7 @@ void bdrv_drained_begin(BlockDriverState *bs)bdrv_parent_drained_begin (bs); }Quoted 8 lines
block/io.c void bdrv_drained_begin(BlockDriverState *bs) · line 270
Quoted 2 lines
block/io.c +1
-1-bdrv_drain_invoke (bs, true); +bdrv_drain_invoke (bs, true, false);I'm not sure I understand this patch. If we call
bdrv_drained_beginon a quorum BDS that has a QED child, we do want to callbdrv_qed_co_drain_begin()on the child, don't we? I think after this patch, we don't do that any more?The same question arises when I look at
throttle_co_drain_begin()..We don't increase
bs->quiesce_counterfor child nodes and don't notify their parents, so they aren't really drained. Calling the BlcokDriver callbacks is the only thing we did recursively. We should do one thing consistently, and for bdrv_drained_begin/end() that is draining a single node without the children.
Quoted 6 lines
Later in the series, I introduce a bdrv_subtree_drained_begin/end() which doesn't only call the callbacks recursively, but also increases
bs->quiesce_counteretc. so that the child nodes are actually drained.There are use cases for both functions.
OK, thanks.
PATCH 4/19 test-bdrv-drain: Test callback for bdrv_drain
In reply to Kevin WolfThe existing test is for bdrv_drain_all_begin/end() only. Generalise the test case so that it can be run for the other variants as well. At the moment this is only bdrv_drain_begin/end(), but in a while, we'll add another one.
Also, add a backing file to the test node to test whether the operations work recursively.
--- tests/test-bdrv-drain.c | 69 ++++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 62 insertions(+), 7 deletions(-)
tests/test-bdrv-drain.c +62 -7
@@ -71,6 +71,8 @@ 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, };static void aio_ret_cb (void *opaque,int ret)
@@ -79,11 +81,34 @@ static void aio_ret_cb(void *opaque, int ret) *aio_ret = ret; } -static void test_drv_cb_drain_all (void ) +enum drain_type { +BDRV_DRAIN_ALL , +BDRV_DRAIN , +}; + +static void do_drain_begin (enum drain_type drain_type,BlockDriverState *bs) +{ +switch (drain_type) { +case BDRV_DRAIN_ALL :bdrv_drain_all_begin ();break ; +case BDRV_DRAIN :bdrv_drained_begin (bs);break ; +default :g_assert_not_reached (); + } +} + +static void do_drain_end (enum drain_type drain_type,BlockDriverState *bs) +{ +switch (drain_type) { +case BDRV_DRAIN_ALL :bdrv_drain_all_end ();break ; +case BDRV_DRAIN :bdrv_drained_end (bs);break ; +default :g_assert_not_reached (); + } +} + +static void test_drv_cb_common (enum drain_type drain_type,bool recursive) {BlockBackend *blk; -BlockDriverState *bs; -BDRVTestState *s; +BlockDriverState *bs, *backing; +BDRVTestState *s, *backing_s;BlockAIOCB *acb;int aio_ret;
@@ -100,12 +125,23 @@ static void test_drv_cb_drain_all(void) s = bs->opaque;blk_insert_bs (blk, bs, &error_abort); + backing =bdrv_new_open_driver (&bdrv_test,"backing" ,0 , &error_abort); + backing_s = backing->opaque; +bdrv_set_backing_hd (bs, backing, &error_abort); +/* Simple bdrv_drain_all_begin/end pair, check that CBs are called */ g_assert_cmpint(s->drain_count, ==, 0); -bdrv_drain_all_begin (); + g_assert_cmpint(backing_s->drain_count, ==, 0); + +do_drain_begin (drain_type, bs); + g_assert_cmpint(s->drain_count, ==, 1); -bdrv_drain_all_end (); + g_assert_cmpint(backing_s->drain_count, ==, !!recursive); + +do_drain_end (drain_type, bs); + g_assert_cmpint(s->drain_count, ==, 0); + g_assert_cmpint(backing_s->drain_count, ==, 0);/* Now do the same while a request is pending */ aio_ret = -EINPROGRESS ;
@@ -114,16 +150,34 @@ static void test_drv_cb_drain_all(void) g_assert_cmpint(aio_ret, ==, -EINPROGRESS); g_assert_cmpint(s->drain_count, ==, 0); -bdrv_drain_all_begin (); + g_assert_cmpint(backing_s->drain_count, ==, 0); + +do_drain_begin (drain_type, bs); + g_assert_cmpint(aio_ret, ==, 0); g_assert_cmpint(s->drain_count, ==, 1); -bdrv_drain_all_end (); + g_assert_cmpint(backing_s->drain_count, ==, !!recursive); + +do_drain_end (drain_type, bs); + g_assert_cmpint(s->drain_count, ==, 0); + g_assert_cmpint(backing_s->drain_count, ==, 0); +bdrv_unref (backing);bdrv_unref (bs);blk_unref (blk); } +static void test_drv_cb_drain_all (void ) +{ +test_drv_cb_common (BDRV_DRAIN_ALL , true); +} + +static void test_drv_cb_drain (void ) +{ +test_drv_cb_common (BDRV_DRAIN , false); +} +int main (int argc,char **argv) {bdrv_init ();
@@ -132,6 +186,7 @@ int main(int argc, char **argv)g_test_init (&argc, &argv,NULL );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);return g_test_run (); }
Signature · 1 line
2.13.6
PATCH 5/19 test-bdrv-drain: Test bs->quiesce_counter
In reply to Kevin WolfThis is currently only working correctly for bdrv_drain(), not for bdrv_drain_all(). Leave a comment for the drain_all case, we'll address it later.
--- tests/test-bdrv-drain.c | 45 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 45 insertions(+)
tests/test-bdrv-drain.c +45 -0
@@ -178,6 +178,48 @@ static void test_drv_cb_drain(void)test_drv_cb_common (BDRV_DRAIN , false); } +static void test_quiesce_common (enum drain_type drain_type,bool recursive) +{ +BlockBackend *blk; +BlockDriverState *bs, *backing; + + blk =blk_new (BLK_PERM_ALL ,BLK_PERM_ALL ); + bs =bdrv_new_open_driver (&bdrv_test,"test-node" ,BDRV_O_RDWR , + &error_abort); +blk_insert_bs (blk, bs, &error_abort); + + backing =bdrv_new_open_driver (&bdrv_test,"backing" ,0 , &error_abort); +bdrv_set_backing_hd (bs, backing, &error_abort); + + g_assert_cmpint(bs->quiesce_counter, ==, 0); + g_assert_cmpint(backing->quiesce_counter, ==, 0); + +do_drain_begin (drain_type, bs); + + g_assert_cmpint(bs->quiesce_counter, ==, 1); + g_assert_cmpint(backing->quiesce_counter, ==, !!recursive); + +do_drain_end (drain_type, bs); + + g_assert_cmpint(bs->quiesce_counter, ==, 0); + g_assert_cmpint(backing->quiesce_counter, ==, 0); + +bdrv_unref (backing); +bdrv_unref (bs); +blk_unref (blk); +} + +static void test_quiesce_drain_all (void ) +{ +// XXX drain_all doesn't quiesce +//test_quiesce_common(BDRV_DRAIN_ALL, true); +} + +static void test_quiesce_drain (void ) +{ +test_quiesce_common (BDRV_DRAIN , false); +} +int main (int argc,char **argv) {bdrv_init ();
@@ -188,5 +230,8 @@ int main(int argc, char **argv)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); +g_test_add_func ("/bdrv-drain/quiesce/drain_all" , test_quiesce_drain_all); +g_test_add_func ("/bdrv-drain/quiesce/drain" , test_quiesce_drain); +return g_test_run (); }
Signature · 1 line
2.13.6
PATCH 6/19 blockjob: Pause job on draining any job BDS
In reply to Kevin WolfBlock jobs already paused themselves when their main BlockBackend entered a drained section. This is not good enough: We also want to pause a block job and may not submit new requests if, for example, the mirror target node should be drained.
This implements .drained_begin/end callbacks in child_job in order to consider all block nodes related to the job, and removes the BlockBackend callbacks which are unnecessary now because the root of the job main BlockBackend is always referenced with a child_job, too.
--- blockjob.c | 22 +++++++++------------- 1 file changed, 9 insertions(+), 13 deletions(-)
blockjob.c +9 -13
@@ -234,26 +234,23 @@ static char *child_job_get_parent_desc(BdrvChild *c) job->id); } -static const BdrvChildRole child_job = { - .get_parent_desc = child_job_get_parent_desc, - .stay_at_node = true, -}; - -static void block_job_drained_begin (void *opaque) +static void child_job_drained_begin (BdrvChild *c) { -BlockJob *job = opaque; +BlockJob *job = c->opaque;block_job_pause (job); } -static void block_job_drained_end (void *opaque) +static void child_job_drained_end (BdrvChild *c) { -BlockJob *job = opaque; +BlockJob *job = c->opaque;block_job_resume (job); } -static const BlockDevOps block_job_dev_ops = { - .drained_begin = block_job_drained_begin, - .drained_end = block_job_drained_end, +static const BdrvChildRole child_job = { + .get_parent_desc = child_job_get_parent_desc, + .drained_begin = child_job_drained_begin, + .drained_end = child_job_drained_end, + .stay_at_node = true, };void block_job_remove_all_bdrv (BlockJob *job)
@@ -715,7 +712,6 @@ void *block_job_create(const char *job_id, const BlockJobDriver *driver,block_job_add_bdrv (job,"main node" , bs,0 ,BLK_PERM_ALL , &error_abort); bs->job = job; -blk_set_dev_ops (blk, &block_job_dev_ops, job);bdrv_op_unblock (bs,BLOCK_OP_TYPE_DATAPLANE , job->blocker);QLIST_INSERT_HEAD (&block_jobs, job, job_list);
Signature · 1 line
2.13.6
PATCH 7/19 test-bdrv-drain: Test drain vs. block jobs
In reply to Kevin WolfBlock jobs must be paused if any of the involved nodes are drained.
--- tests/test-bdrv-drain.c | 121 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 121 insertions(+)
tests/test-bdrv-drain.c +121 -0
@@ -24,6 +24,7 @@#include "qemu/osdep.h" #include "block/block.h" +#include "block/blockjob_int.h" #include "sysemu/block-backend.h" #include "qapi/error.h"
@@ -220,6 +221,123 @@ static void test_quiesce_drain(void)test_quiesce_common (BDRV_DRAIN , false); } + +typedef struct TestBlockJob { +BlockJob common; +bool should_complete; +}TestBlockJob ; + +static void test_job_completed (BlockJob *job,void *opaque) +{ +block_job_completed (job,0 ); +} + +static void coroutine_fn test_job_start(void *opaque) +{ +TestBlockJob *s = opaque; + +while (!s->should_complete) { +block_job_sleep_ns (&s->common,100000 ); + } + +block_job_defer_to_main_loop (&s->common, test_job_completed,NULL ); +} + +static void test_job_complete (BlockJob *job,Error **errp) +{ +TestBlockJob *s =container_of (job, TestBlockJob, common); + s->should_complete = true; +} + +BlockJobDriver test_job_driver = { + .instance_size =sizeof (TestBlockJob), + .start = test_job_start, + .complete = test_job_complete, +}; + +static void test_blockjob_common (enum drain_type drain_type) +{ +BlockBackend *blk_src, *blk_target; +BlockDriverState *src, *target; +BlockJob *job; +int ret; + + src =bdrv_new_open_driver (&bdrv_test,"source" ,BDRV_O_RDWR , + &error_abort); + blk_src =blk_new (BLK_PERM_ALL ,BLK_PERM_ALL ); +blk_insert_bs (blk_src, src, &error_abort); + + target =bdrv_new_open_driver (&bdrv_test,"target" ,BDRV_O_RDWR , + &error_abort); + blk_target =blk_new (BLK_PERM_ALL ,BLK_PERM_ALL ); +blk_insert_bs (blk_target, target, &error_abort); + + job =block_job_create ("job0" , &test_job_driver, src,0 ,BLK_PERM_ALL ,0 , +0 ,NULL ,NULL , &error_abort); +block_job_add_bdrv (job,"target" , target,0 ,BLK_PERM_ALL , &error_abort); +block_job_start (job); + + g_assert_cmpint(job->pause_count, ==, 0); +g_assert_false (job->paused); +g_assert_false (job->busy);/* We're in block_job_sleep_ns() */ + +do_drain_begin (drain_type, src); + +if (drain_type ==BDRV_DRAIN_ALL ) { +/* bdrv_drain_all() drains both src and target, and involves an +* additional block_job_pause_all() */ + g_assert_cmpint(job->pause_count, ==, 3); + }else { + g_assert_cmpint(job->pause_count, ==, 1); + } +/* XXX We don't wait until the job is actually paused. Is this okay? */ +/* g_assert_true(job->paused); */ +g_assert_false (job->busy);/* The job is paused */ + +do_drain_end (drain_type, src); + + g_assert_cmpint(job->pause_count, ==, 0); +g_assert_false (job->paused); +g_assert_false (job->busy);/* We're in block_job_sleep_ns() */ + +do_drain_begin (drain_type, target); + +if (drain_type ==BDRV_DRAIN_ALL ) { +/* bdrv_drain_all() drains both src and target, and involves an +* additional block_job_pause_all() */ + g_assert_cmpint(job->pause_count, ==, 3); + }else { + g_assert_cmpint(job->pause_count, ==, 1); + } +/* XXX We don't wait until the job is actually paused. Is this okay? */ +/* g_assert_true(job->paused); */ +g_assert_false (job->busy);/* The job is paused */ + +do_drain_end (drain_type, target); + + g_assert_cmpint(job->pause_count, ==, 0); +g_assert_false (job->paused); +g_assert_false (job->busy);/* We're in block_job_sleep_ns() */ + + ret =block_job_complete_sync (job, &error_abort); + g_assert_cmpint(ret, ==, 0); + +blk_unref (blk_src); +blk_unref (blk_target); +bdrv_unref (src); +bdrv_unref (target); +} + +static void test_blockjob_drain_all (void ) +{ +test_blockjob_common (BDRV_DRAIN_ALL ); +} + +static void test_blockjob_drain (void ) +{ +test_blockjob_common (BDRV_DRAIN ); +} +int main (int argc,char **argv) {bdrv_init ();
@@ -233,5 +351,8 @@ int main(int argc, char **argv)g_test_add_func ("/bdrv-drain/quiesce/drain_all" , test_quiesce_drain_all);g_test_add_func ("/bdrv-drain/quiesce/drain" , test_quiesce_drain); +g_test_add_func ("/bdrv-drain/blockjob/drain_all" , test_blockjob_drain_all); +g_test_add_func ("/bdrv-drain/blockjob/drain" , test_blockjob_drain); +return g_test_run (); }
Signature · 1 line
2.13.6
PATCH 8/19 block: Don't block_job_pause_all() in bdrv_drain_all()
In reply to Kevin WolfBlock jobs are already paused using the BdrvChildRole drain callbacks, so we don't need an additionall block_job_pause_all() call.
--- block/io.c | 4 ---- tests/test-bdrv-drain.c | 10 ++++------ 2 files changed, 4 insertions(+), 10 deletions(-)
block/io.c +0 -4
@@ -338,8 +338,6 @@ void bdrv_drain_all_begin(void)* context. */ assert (qemu_get_current_aio_context () ==qemu_get_aio_context ()); -block_job_pause_all (); -for (bs =bdrv_first (&it); bs; bs =bdrv_next (&it)) {AioContext *aio_context =bdrv_get_aio_context (bs);
@@ -395,8 +393,6 @@ void bdrv_drain_all_end(void)aio_enable_external (aio_context);aio_context_release (aio_context); } - -block_job_resume_all (); }void bdrv_drain_all (void )
tests/test-bdrv-drain.c +4 -6
@@ -284,9 +284,8 @@ static void test_blockjob_common(enum drain_type drain_type)do_drain_begin (drain_type, src);if (drain_type ==BDRV_DRAIN_ALL ) { -/* bdrv_drain_all() drains both src and target, and involves an -* additional block_job_pause_all() */ - g_assert_cmpint(job->pause_count, ==, 3); +/* bdrv_drain_all() drains both src and target */ + g_assert_cmpint(job->pause_count, ==, 2); }else { g_assert_cmpint(job->pause_count, ==, 1); }
@@ -303,9 +302,8 @@ static void test_blockjob_common(enum drain_type drain_type)do_drain_begin (drain_type, target);if (drain_type ==BDRV_DRAIN_ALL ) { -/* bdrv_drain_all() drains both src and target, and involves an -* additional block_job_pause_all() */ - g_assert_cmpint(job->pause_count, ==, 3); +/* bdrv_drain_all() drains both src and target */ + g_assert_cmpint(job->pause_count, ==, 2); }else { g_assert_cmpint(job->pause_count, ==, 1); }
Signature · 1 line
2.13.6
Re: [Qemu-devel] [PATCH 08/19] block: Don't block_job_pause_all() in bdrv_drain_all()
Quoted 2 lines · Kevin Wolf
Block jobs are already paused using the BdrvChildRole drain callbacks, so we don't need an additionall
block_job_pause_all()call.
s/additionall/additional/
Quoted 7 lines
Signed-off-by Kevin Wolf <[email protected]>--- block/io.c | 4 ---- tests/test-bdrv-drain.c | 10 ++++------ 2 files changed, 4 insertions(+), 10 deletions(-)
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
PATCH 9/19 block: Nested drain_end must still call callbacks
In reply to Kevin Wolfbdrv_do_drained_begin() restricts the call of parent callbacks and aio_disable_external() to the outermost drain section, but the block driver callbacks are always called. bdrv_do_drained_end() must match this behaviour, otherwise nodes stay drained even if begin/end calls were balanced.
--- block/io.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)
block/io.c +7 -5
@@ -273,19 +273,21 @@ void bdrv_drained_begin(BlockDriverState *bs)void bdrv_drained_end (BlockDriverState *bs) { +int old_quiesce_counter; +if (qemu_in_coroutine ()) {bdrv_co_yield_to_drain (bs, false);return ; }assert (bs->quiesce_counter >0 ); -if (atomic_fetch_dec (&bs->quiesce_counter) >1 ) { -return ; - } + old_quiesce_counter =atomic_fetch_dec (&bs->quiesce_counter);/* Re-enable things in child-to-parent order */ bdrv_drain_invoke (bs, false, false); -bdrv_parent_drained_end (bs); -aio_enable_external (bdrv_get_aio_context (bs)); +if (old_quiesce_counter ==1 ) { +bdrv_parent_drained_end (bs); +aio_enable_external (bdrv_get_aio_context (bs)); + } }/*
Signature · 1 line
2.13.6
PATCH 10/19 test-bdrv-drain: Test nested drain sections
In reply to Kevin Wolf--- tests/test-bdrv-drain.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 57 insertions(+)
tests/test-bdrv-drain.c +57 -0
@@ -85,6 +85,7 @@ static void aio_ret_cb(void *opaque, int ret)enum drain_type {BDRV_DRAIN_ALL ,BDRV_DRAIN , +DRAIN_TYPE_MAX , };static void do_drain_begin (enum drain_type drain_type,BlockDriverState *bs)
@@ -221,6 +222,60 @@ static void test_quiesce_drain(void)test_quiesce_common (BDRV_DRAIN , false); } +static void test_nested (void ) +{ +BlockBackend *blk; +BlockDriverState *bs, *backing; +BDRVTestState *s, *backing_s; +enum drain_type outer, inner; + + 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); + + backing =bdrv_new_open_driver (&bdrv_test,"backing" ,0 , &error_abort); + backing_s = backing->opaque; +bdrv_set_backing_hd (bs, backing, &error_abort); + +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 =0 ; +int backing_cb_cnt = (outer !=BDRV_DRAIN ) + + (inner !=BDRV_DRAIN ); + + g_assert_cmpint(bs->quiesce_counter, ==, 0); + g_assert_cmpint(backing->quiesce_counter, ==, 0); + g_assert_cmpint(s->drain_count, ==, 0); + g_assert_cmpint(backing_s->drain_count, ==, 0); + +do_drain_begin (outer, bs); +do_drain_begin (inner, bs); + + g_assert_cmpint(bs->quiesce_counter, ==, bs_quiesce); + 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); + +do_drain_end (inner, bs); +do_drain_end (outer, bs); + + g_assert_cmpint(bs->quiesce_counter, ==, 0); + g_assert_cmpint(backing->quiesce_counter, ==, 0); + g_assert_cmpint(s->drain_count, ==, 0); + g_assert_cmpint(backing_s->drain_count, ==, 0); + } + } + +bdrv_unref (backing); +bdrv_unref (bs); +blk_unref (blk); +} + typedef struct TestBlockJob { BlockJob common;
@@ -349,6 +404,8 @@ int main(int argc, char **argv)g_test_add_func ("/bdrv-drain/quiesce/drain_all" , test_quiesce_drain_all);g_test_add_func ("/bdrv-drain/quiesce/drain" , test_quiesce_drain); +g_test_add_func ("/bdrv-drain/nested" , test_nested); +g_test_add_func ("/bdrv-drain/blockjob/drain_all" , test_blockjob_drain_all);g_test_add_func ("/bdrv-drain/blockjob/drain" , test_blockjob_drain);
Signature · 1 line
2.13.6
PATCH 11/19 block: Don't notify parents in drain call chain
In reply to Kevin WolfThis is in preparation for subtree drains, i.e. drained sections that affect not only a single node, but recursively all child nodes, too.
Calling the parent callbacks for drain is pointless when we just came from that parent node recursively and leads to multiple increases of bs->quiesce_counter in a single drain call. Don't do it.
In order for this to work correctly, the parent callback must be called for every bdrv_drain_begin/end() call, not only for the outermost one:
If we have a node N with two parents A and B, recursive draining of A should cause the quiesce_counter of B to increased because its child N is drained independently of B. If now B is recursively drained, too, A must increase its quiesce_counter because N is drained independently of A only now, even if N is going from quiesce_counter 1 to 2.
--- include/block/block.h | 4 ++-- block.c | 13 +++++++++---- block/io.c | 47 ++++++++++++++++++++++++++++++++++------------- 3 files changed, 45 insertions(+), 19 deletions(-)
include/block/block.h +2 -2
@@ -585,7 +585,7 @@ 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); +void bdrv_parent_drained_begin (BlockDriverState *bs,BdrvChild *ignore);/** * bdrv_parent_drained_end:
@@ -593,7 +593,7 @@ void bdrv_parent_drained_begin(BlockDriverState *bs);* End a quiesced section of all users of @bs. This is part of * bdrv_drained_end. */ -void bdrv_parent_drained_end (BlockDriverState *bs); +void bdrv_parent_drained_end (BlockDriverState *bs,BdrvChild *ignore);/** * bdrv_drained_begin:
block.c +9 -4
@@ -1972,13 +1972,16 @@ static void bdrv_replace_child_noperm(BdrvChild *child, BlockDriverState *new_bs) {BlockDriverState *old_bs = child->bs; +int i;if (old_bs && new_bs) {assert (bdrv_get_aio_context (old_bs) ==bdrv_get_aio_context (new_bs)); }if (old_bs) {if (old_bs->quiesce_counter && child->role->drained_end) { - child->role->drained_end (child); +for (i =0 ; i < old_bs->quiesce_counter; i++) { + child->role->drained_end (child); + } }if (child->role->detach) { child->role->detach (child);
@@ -1991,7 +1994,9 @@ 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) { - child->role->drained_begin (child); +for (i =0 ; i < new_bs->quiesce_counter; i++) { + child->role->drained_begin (child); + } }if (child->role->attach) {
@@ -4750,7 +4755,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); +bdrv_parent_drained_begin (bs,NULL );bdrv_drain (bs);/* ensure there are no in-flight requests */ while (aio_poll (ctx, false)) {
@@ -4764,7 +4769,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); +bdrv_parent_drained_end (bs,NULL );aio_enable_external (ctx);aio_context_release (new_context); }
block/io.c +34 -13
145 lines not on this page · raw message
Signature · 1 line
2.13.6
Re: [Qemu-devel] [PATCH 11/19] block: Don't notify parents in drain call chain
Quoted 7 lines · Kevin Wolf
This is in preparation for subtree drains, i.e. drained sections that affect not only a single node, but recursively all child nodes, too.
Calling the parent callbacks for drain is pointless when we just came from that parent node recursively and leads to multiple increases of
bs->quiesce_counterin a single drain call. Don't do it.
Quoted 5 lines
In order for this to work correctly, the parent callback must be called for every bdrv_drain_begin/end() call, not only for the outermost one:
If we have a node N with two parents A and B, recursive draining of A should cause the
quiesce_counterof B to increased because its child N
either 'to be increased' or 'to increase'
Quoted 11 lines
is drained independently of B. If now B is recursively drained, too, A must increase its
quiesce_counterbecause N is drained independently of A only now, even if N is going fromquiesce_counter1 to 2.Signed-off-by Kevin Wolf <[email protected]>--- include/block/block.h | 4 ++-- block.c | 13 +++++++++---- block/io.c | 47 ++++++++++++++++++++++++++++++++++------------- 3 files changed, 45 insertions(+), 19 deletions(-)
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
PATCH 12/19 block: Add bdrv_subtree_drained_begin/end()
In reply to Kevin Wolfbdrv_drained_begin() waits for the completion of requests in the whole subtree, but it only actually keeps its immediate bs parameter quiesced until bdrv_drained_end().
Add a version that keeps the whole subtree drained. As of this commit, graph changes cannot be allowed during a subtree drained section, but this will be fixed soon.
--- include/block/block.h | 13 +++++++++++++ block/io.c | 54 ++++++++++++++++++++++++++++++++++++++++----------- 2 files changed, 56 insertions(+), 11 deletions(-)
include/block/block.h +13 -0
@@ -608,12 +608,25 @@ void bdrv_parent_drained_end(BlockDriverState *bs, BdrvChild *ignore);void bdrv_drained_begin (BlockDriverState *bs);/** +* Like bdrv_drained_begin, but recursively begins a quiesced section for +* exclusive access to all child nodes as well. +* +* Graph changes are not allowed during a subtree drain section. +*/ +void bdrv_subtree_drained_begin (BlockDriverState *bs); + +/** * bdrv_drained_end: * * End a quiescent section started by bdrv_drained_begin(). */ void bdrv_drained_end (BlockDriverState *bs); +/** +* End a quiescent section started by bdrv_subtree_drained_begin(). +*/ +void bdrv_subtree_drained_end (BlockDriverState *bs); +void bdrv_add_child (BlockDriverState *parent,BlockDriverState *child,Error **errp);void bdrv_del_child (BlockDriverState *parent,BdrvChild *child,Error **errp);
block/io.c +43 -11
119 lines not on this page · raw message
Signature · 1 line
2.13.6
PATCH 13/19 test-bdrv-drain: Tests for bdrv_subtree_drain
In reply to Kevin WolfAdd a subtree drain version to the existing test cases.
--- tests/test-bdrv-drain.c | 27 ++++++++++++++++++++++++++- 1 file changed, 26 insertions(+), 1 deletion(-)
tests/test-bdrv-drain.c +26 -1
@@ -85,6 +85,7 @@ static void aio_ret_cb(void *opaque, int ret)enum drain_type {BDRV_DRAIN_ALL ,BDRV_DRAIN , +BDRV_SUBTREE_DRAIN ,DRAIN_TYPE_MAX , };
@@ -93,6 +94,7 @@ static void do_drain_begin(enum drain_type drain_type, BlockDriverState *bs)switch (drain_type) {case BDRV_DRAIN_ALL :bdrv_drain_all_begin ();break ;case BDRV_DRAIN :bdrv_drained_begin (bs);break ; +case BDRV_SUBTREE_DRAIN :bdrv_subtree_drained_begin (bs);break ;default :g_assert_not_reached (); } }
@@ -102,6 +104,7 @@ static void do_drain_end(enum drain_type drain_type, BlockDriverState *bs)switch (drain_type) {case BDRV_DRAIN_ALL :bdrv_drain_all_end ();break ;case BDRV_DRAIN :bdrv_drained_end (bs);break ; +case BDRV_SUBTREE_DRAIN :bdrv_subtree_drained_end (bs);break ;default :g_assert_not_reached (); } }
@@ -180,6 +183,11 @@ static void test_drv_cb_drain(void)test_drv_cb_common (BDRV_DRAIN , false); } +static void test_drv_cb_drain_subtree (void ) +{ +test_drv_cb_common (BDRV_SUBTREE_DRAIN , true); +} +static void test_quiesce_common (enum drain_type drain_type,bool recursive) {BlockBackend *blk;
@@ -222,6 +230,11 @@ static void test_quiesce_drain(void)test_quiesce_common (BDRV_DRAIN , false); } +static void test_quiesce_drain_subtree (void ) +{ +test_quiesce_common (BDRV_SUBTREE_DRAIN , true); +} +static void test_nested (void ) {BlockBackend *blk;
@@ -244,7 +257,8 @@ static void test_nested(void)/* XXX bdrv_drain_all() doesn't increase the quiesce_counter */ int bs_quiesce = (outer !=BDRV_DRAIN_ALL ) + (inner !=BDRV_DRAIN_ALL ); -int backing_quiesce =0 ; +int backing_quiesce = (outer ==BDRV_SUBTREE_DRAIN ) + + (inner ==BDRV_SUBTREE_DRAIN );int backing_cb_cnt = (outer !=BDRV_DRAIN ) + (inner !=BDRV_DRAIN );
@@ -391,6 +405,11 @@ static void test_blockjob_drain(void)test_blockjob_common (BDRV_DRAIN ); } +static void test_blockjob_drain_subtree (void ) +{ +test_blockjob_common (BDRV_SUBTREE_DRAIN ); +} +int main (int argc,char **argv) {bdrv_init ();
@@ -400,14 +419,20 @@ int main(int argc, char **argv)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); +g_test_add_func ("/bdrv-drain/driver-cb/drain_subtree" , + test_drv_cb_drain_subtree);g_test_add_func ("/bdrv-drain/quiesce/drain_all" , test_quiesce_drain_all);g_test_add_func ("/bdrv-drain/quiesce/drain" , test_quiesce_drain); +g_test_add_func ("/bdrv-drain/quiesce/drain_subtree" , + test_quiesce_drain_subtree);g_test_add_func ("/bdrv-drain/nested" , test_nested);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 (); }
Signature · 1 line
2.13.6
PATCH 14/19 test-bdrv-drain: Test behaviour in coroutine context
In reply to Kevin WolfIf bdrv_do_drained_begin/end() are called in coroutine context, they first use a BH to get out of the coroutine context. Call some existing tests again from a coroutine to cover this code path.
--- tests/test-bdrv-drain.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 51 insertions(+)
tests/test-bdrv-drain.c +51 -0
@@ -188,6 +188,30 @@ static void test_drv_cb_drain_subtree(void)test_drv_cb_common (BDRV_SUBTREE_DRAIN , true); } +static coroutine_fn void test_drv_cb_coroutine_entry(void *opaque) +{ +bool *done = opaque; + +// XXX bdrv_drain_all() doesn't work in coroutine context +//test_drv_cb_drain_all(); +test_drv_cb_drain (); +test_drv_cb_drain_subtree (); + + *done = true; +} + +static void test_drv_cb_coroutine (void ) +{ +Coroutine *co; +bool done; + + co =qemu_coroutine_create (test_drv_cb_coroutine_entry, &done); +qemu_coroutine_enter (co); +while (!done) { +aio_poll (qemu_get_aio_context (), true); + } +} +static void test_quiesce_common (enum drain_type drain_type,bool recursive) {BlockBackend *blk;
@@ -235,6 +259,30 @@ static void test_quiesce_drain_subtree(void)test_quiesce_common (BDRV_SUBTREE_DRAIN , true); } +static coroutine_fn void test_quiesce_coroutine_entry(void *opaque) +{ +bool *done = opaque; + +// XXX bdrv_drain_all() doesn't work in coroutine context +//test_quiesce_drain_all(); +test_quiesce_drain (); +test_quiesce_drain_subtree (); + + *done = true; +} + +static void test_quiesce_coroutine (void ) +{ +Coroutine *co; +bool done; + + co =qemu_coroutine_create (test_quiesce_coroutine_entry, &done); +qemu_coroutine_enter (co); +while (!done) { +aio_poll (qemu_get_aio_context (), true); + } +} +static void test_nested (void ) {BlockBackend *blk;
@@ -421,11 +469,14 @@ int main(int argc, char **argv)g_test_add_func ("/bdrv-drain/driver-cb/drain" , test_drv_cb_drain);g_test_add_func ("/bdrv-drain/driver-cb/drain_subtree" , test_drv_cb_drain_subtree); +g_test_add_func ("/bdrv-drain/driver-cb/coroutine" , test_drv_cb_coroutine); +g_test_add_func ("/bdrv-drain/quiesce/drain_all" , test_quiesce_drain_all);g_test_add_func ("/bdrv-drain/quiesce/drain" , test_quiesce_drain);g_test_add_func ("/bdrv-drain/quiesce/drain_subtree" , test_quiesce_drain_subtree); +g_test_add_func ("/bdrv-drain/quiesce/coroutine" , test_quiesce_coroutine);g_test_add_func ("/bdrv-drain/nested" , test_nested);
Signature · 1 line
2.13.6
Re: [Qemu-devel] [PATCH 14/19] test-bdrv-drain: Test behaviour in coroutine context
Quoted 77 lines · Kevin Wolf
If bdrv_do_drained_begin/end() are called in coroutine context, they first use a BH to get out of the coroutine context. Call some existing tests again from a coroutine to cover this code path.
Signed-off-by Kevin Wolf <[email protected]>--- tests/test-bdrv-drain.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 51 insertions(+)tests/test-bdrv-drain.c +48
-0@@ -188,6 +188,30 @@ static void test_drv_cb_drain_subtree(void)test_drv_cb_common (BDRV_SUBTREE_DRAIN , true); } +static coroutine_fn void test_drv_cb_coroutine_entry(void *opaque) +{ +bool *done = opaque; + +// XXX bdrv_drain_all() doesn't work in coroutine context +//test_drv_cb_drain_all(); +test_drv_cb_drain (); +test_drv_cb_drain_subtree (); + + *done = true; +} + +static void test_drv_cb_coroutine (void ) +{ +Coroutine *co; +bool done; + + co =qemu_coroutine_create (test_drv_cb_coroutine_entry, &done); +qemu_coroutine_enter (co); +while (!done) { +aio_poll (qemu_get_aio_context (), true); + } +} +static void test_quiesce_common (enum drain_type drain_type,bool recursive) {BlockBackend *blk;@@ -235,6 +259,30 @@ static void test_quiesce_drain_subtree(void)test_quiesce_common (BDRV_SUBTREE_DRAIN , true); } +static coroutine_fn void test_quiesce_coroutine_entry(void *opaque) +{ +bool *done = opaque; + +// XXX bdrv_drain_all() doesn't work in coroutine context +//test_quiesce_drain_all(); +test_quiesce_drain (); +test_quiesce_drain_subtree (); + + *done = true; +} + +static void test_quiesce_coroutine (void ) +{ +Coroutine *co; +bool done; + + co =qemu_coroutine_create (test_quiesce_coroutine_entry, &done); +qemu_coroutine_enter (co); +while (!done) { +aio_poll (qemu_get_aio_context (), true); + } +} +static void test_nested (void ) {BlockBackend *blk;@@ -421,11 +469,14 @@ int main(int argc, char **argv)g_test_add_func ("/bdrv-drain/driver-cb/drain" , test_drv_cb_drain);
tests/test-bdrv-drain.c int main(int argc, char **argv) · line 479
Quoted 10 lines
tests/test-bdrv-drain.c +3
-0g_test_add_func ("/bdrv-drain/driver-cb/drain_subtree" , test_drv_cb_drain_subtree); +g_test_add_func ("/bdrv-drain/driver-cb/coroutine" , test_drv_cb_coroutine); +g_test_add_func ("/bdrv-drain/quiesce/drain_all" , test_quiesce_drain_all);g_test_add_func ("/bdrv-drain/quiesce/drain" , test_quiesce_drain);g_test_add_func ("/bdrv-drain/quiesce/drain_subtree" , test_quiesce_drain_subtree); +g_test_add_func ("/bdrv-drain/quiesce/coroutine" , test_quiesce_coroutine);
Maybe split the two sub-tests of test_{drv_cb,quiesce}_coroutine into separate g_test_add_func?
Thanks,
Paolo
Quoted 3 lines
tests/test-bdrv-drain.c +0
-0g_test_add_func ("/bdrv-drain/nested" , test_nested);
PATCH 15/19 test-bdrv-drain: Recursive draining with multiple parents
In reply to Kevin WolfTest that drain sections are correctly propagated through the graph.
--- tests/test-bdrv-drain.c | 74 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 74 insertions(+)
tests/test-bdrv-drain.c +74 -0
@@ -338,6 +338,79 @@ static void test_nested(void)blk_unref (blk); } +static void test_multiparent (void ) +{ +BlockBackend *blk_a, *blk_b; +BlockDriverState *bs_a, *bs_b, *backing; +BDRVTestState *a_s, *b_s, *backing_s; + + 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); + + 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); + + backing =bdrv_new_open_driver (&bdrv_test,"backing" ,0 , &error_abort); + backing_s = backing->opaque; +bdrv_set_backing_hd (bs_a, backing, &error_abort); +bdrv_set_backing_hd (bs_b, backing, &error_abort); + + g_assert_cmpint(bs_a->quiesce_counter, ==, 0); + g_assert_cmpint(bs_b->quiesce_counter, ==, 0); + g_assert_cmpint(backing->quiesce_counter, ==, 0); + g_assert_cmpint(a_s->drain_count, ==, 0); + g_assert_cmpint(b_s->drain_count, ==, 0); + g_assert_cmpint(backing_s->drain_count, ==, 0); + +do_drain_begin (BDRV_SUBTREE_DRAIN , bs_a); + + g_assert_cmpint(bs_a->quiesce_counter, ==, 1); + g_assert_cmpint(bs_b->quiesce_counter, ==, 1); + g_assert_cmpint(backing->quiesce_counter, ==, 1); + g_assert_cmpint(a_s->drain_count, ==, 1); + g_assert_cmpint(b_s->drain_count, ==, 1); + g_assert_cmpint(backing_s->drain_count, ==, 1); + +do_drain_begin (BDRV_SUBTREE_DRAIN , bs_b); + + g_assert_cmpint(bs_a->quiesce_counter, ==, 2); + g_assert_cmpint(bs_b->quiesce_counter, ==, 2); + g_assert_cmpint(backing->quiesce_counter, ==, 2); + g_assert_cmpint(a_s->drain_count, ==, 2); + g_assert_cmpint(b_s->drain_count, ==, 2); + g_assert_cmpint(backing_s->drain_count, ==, 2); + +do_drain_end (BDRV_SUBTREE_DRAIN , bs_b); + + g_assert_cmpint(bs_a->quiesce_counter, ==, 1); + g_assert_cmpint(bs_b->quiesce_counter, ==, 1); + g_assert_cmpint(backing->quiesce_counter, ==, 1); + g_assert_cmpint(a_s->drain_count, ==, 1); + g_assert_cmpint(b_s->drain_count, ==, 1); + g_assert_cmpint(backing_s->drain_count, ==, 1); + +do_drain_end (BDRV_SUBTREE_DRAIN , bs_a); + + g_assert_cmpint(bs_a->quiesce_counter, ==, 0); + g_assert_cmpint(bs_b->quiesce_counter, ==, 0); + g_assert_cmpint(backing->quiesce_counter, ==, 0); + g_assert_cmpint(a_s->drain_count, ==, 0); + g_assert_cmpint(b_s->drain_count, ==, 0); + g_assert_cmpint(backing_s->drain_count, ==, 0); + +bdrv_unref (backing); +bdrv_unref (bs_a); +bdrv_unref (bs_b); +blk_unref (blk_a); +blk_unref (blk_b); +} + typedef struct TestBlockJob { BlockJob common;
@@ -479,6 +552,7 @@ int main(int argc, char **argv)g_test_add_func ("/bdrv-drain/quiesce/coroutine" , test_quiesce_coroutine);g_test_add_func ("/bdrv-drain/nested" , test_nested); +g_test_add_func ("/bdrv-drain/multiparent" , test_multiparent);g_test_add_func ("/bdrv-drain/blockjob/drain_all" , test_blockjob_drain_all);g_test_add_func ("/bdrv-drain/blockjob/drain" , test_blockjob_drain);
Signature · 1 line
2.13.6
PATCH 16/19 block: Allow graph changes in subtree drained section
In reply to Kevin WolfWe need to remember how many of the drain sections in which a node is were recursive (i.e. subtree drain rather than node drain), so that they can be correctly applied when children are added or removed during the drained section.
With this change, it is safe to modify the graph even inside a bdrv_subtree_drained_begin/end() section.
--- include/block/block.h | 7 +++++-- include/block/block_int.h | 2 ++ block.c | 40 +++++++++++++++++++++++++++++++++++++--- block/io.c | 15 ++++++--------- 4 files changed, 50 insertions(+), 14 deletions(-)
include/block/block.h +5 -2
@@ -610,8 +610,6 @@ void bdrv_drained_begin(BlockDriverState *bs);/** * Like bdrv_drained_begin, but recursively begins a quiesced section for * exclusive access to all child nodes as well. -* -* Graph changes are not allowed during a subtree drain section. */ void bdrv_subtree_drained_begin (BlockDriverState *bs);
@@ -627,6 +625,11 @@ void bdrv_drained_end(BlockDriverState *bs);*/ void bdrv_subtree_drained_end (BlockDriverState *bs); +void bdrv_do_drained_begin (BlockDriverState *bs,bool recursive, +BdrvChild *parent); +void bdrv_do_drained_end (BlockDriverState *bs,bool recursive, +BdrvChild *parent); +void bdrv_add_child (BlockDriverState *parent,BlockDriverState *child,Error **errp);void bdrv_del_child (BlockDriverState *parent,BdrvChild *child,Error **errp);
include/block/block_int.h +2 -0
@@ -717,6 +717,8 @@ struct BlockDriverState {/* Accessed with atomic ops. */ int quiesce_counter; +int recursive_quiesce_counter; +unsigned int write_gen;/* Current data generation */ /* Protected by reqs_lock. */
block.c +37 -3
@@ -822,6 +822,26 @@ static void bdrv_child_cb_drained_end(BdrvChild *child)bdrv_drained_end (bs); } +static void bdrv_child_cb_attach (BdrvChild *child) +{ +BlockDriverState *bs = child->opaque; +int i; + +for (i =0 ; i < bs->recursive_quiesce_counter; i++) { +bdrv_do_drained_begin (child->bs, true, child); + } +} + +static void bdrv_child_cb_detach (BdrvChild *child) +{ +BlockDriverState *bs = child->opaque; +int i; + +for (i =0 ; i < bs->recursive_quiesce_counter; i++) { +bdrv_do_drained_end (child->bs, true, child); + } +} +static int bdrv_child_cb_inactivate (BdrvChild *child) {BlockDriverState *bs = child->opaque;
@@ -889,6 +909,8 @@ const BdrvChildRole child_file = { .inherit_options = bdrv_inherited_options, .drained_begin = bdrv_child_cb_drained_begin, .drained_end = bdrv_child_cb_drained_end, + .attach = bdrv_child_cb_attach, + .detach = bdrv_child_cb_detach, .inactivate = bdrv_child_cb_inactivate, };
@@ -911,6 +933,8 @@ const BdrvChildRole child_format = { .inherit_options = bdrv_inherited_fmt_options, .drained_begin = bdrv_child_cb_drained_begin, .drained_end = bdrv_child_cb_drained_end, + .attach = bdrv_child_cb_attach, + .detach = bdrv_child_cb_detach, .inactivate = bdrv_child_cb_inactivate, };
@@ -953,6 +977,8 @@ static void bdrv_backing_attach(BdrvChild *c) parent->backing_blocker);bdrv_op_unblock (backing_hd,BLOCK_OP_TYPE_BACKUP_TARGET , parent->backing_blocker); + +bdrv_child_cb_attach (c); }static void bdrv_backing_detach (BdrvChild *c)
@@ -963,6 +989,8 @@ static void bdrv_backing_detach(BdrvChild *c)bdrv_op_unblock_all (c->bs, parent->backing_blocker);error_free (parent->backing_blocker); parent->backing_blocker =NULL ; + +bdrv_child_cb_detach (c); }/*
@@ -1978,14 +2006,17 @@ static void bdrv_replace_child_noperm(BdrvChild *child,assert (bdrv_get_aio_context (old_bs) ==bdrv_get_aio_context (new_bs)); }if (old_bs) { +/* Detach first so that the recursive drain sections coming from @child +* are already gone and we only end the drain sections that came from +* elsewhere. */ +if (child->role->detach) { + child->role->detach (child); + }if (old_bs->quiesce_counter && child->role->drained_end) {for (i =0 ; i < old_bs->quiesce_counter; i++) { child->role->drained_end (child); } } -if (child->role->detach) { - child->role->detach (child); - }QLIST_REMOVE (child, next_parent); }
@@ -1999,6 +2030,9 @@ static void bdrv_replace_child_noperm(BdrvChild *child, } } +/* Attach only after starting new drained sections, so that recursive +* drain sections coming from @child don't get an extra .drained_begin +* callback. */ if (child->role->attach) { child->role->attach (child); }
block/io.c +6 -9
@@ -219,11 +219,6 @@ static bool bdrv_drain_recurse(BlockDriverState *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, -BdrvChild *parent); -static void bdrv_co_drain_bh_cb (void *opaque) {BdrvCoDrainData *data = opaque;
@@ -270,8 +265,8 @@ static void coroutine_fn bdrv_co_yield_to_drain(BlockDriverState *bs,assert (data.done); } -static void bdrv_do_drained_begin (BlockDriverState *bs,bool recursive, -BdrvChild *parent) +void bdrv_do_drained_begin (BlockDriverState *bs,bool recursive, +BdrvChild *parent) {BdrvChild *child, *next;
@@ -290,6 +285,7 @@ static void bdrv_do_drained_begin(BlockDriverState *bs, bool recursive,bdrv_drain_recurse (bs);if (recursive) { + bs->recursive_quiesce_counter++; QLIST_FOREACH_SAFE(child, &bs->children, next, next) {bdrv_do_drained_begin (child->bs, true, child); }
@@ -306,8 +302,8 @@ void bdrv_subtree_drained_begin(BlockDriverState *bs)bdrv_do_drained_begin (bs, true,NULL ); } -static void bdrv_do_drained_end (BlockDriverState *bs,bool recursive, -BdrvChild *parent) +void bdrv_do_drained_end (BlockDriverState *bs,bool recursive, +BdrvChild *parent) {BdrvChild *child, *next;int old_quiesce_counter;
@@ -327,6 +323,7 @@ static void bdrv_do_drained_end(BlockDriverState *bs, bool recursive, }if (recursive) { + bs->recursive_quiesce_counter--; QLIST_FOREACH_SAFE(child, &bs->children, next, next) {bdrv_do_drained_end (child->bs, true, child); }
Signature · 1 line
2.13.6
Re: [Qemu-devel] [PATCH 16/19] block: Allow graph changes in subtree drained section
Quoted 9 lines · Kevin Wolf
.inherit_options =
bdrv_inherited_options,
.drained_begin =bdrv_child_cb_drained_begin,
.drained_end =bdrv_child_cb_drained_end,block.c +2
-0+ .attach = bdrv_child_cb_attach, + .detach = bdrv_child_cb_detach, .inactivate = bdrv_child_cb_inactivate, };
block.c const BdrvChildRole child_format = { · line 938
Quoted 6 lines
block.c +2
-0@@ -911,6 +933,8 @@ const BdrvChildRole child_format = { .inherit_options = bdrv_inherited_fmt_options, .drained_begin = bdrv_child_cb_drained_begin, .drained_end = bdrv_child_cb_drained_end, + .attach = bdrv_child_cb_attach, + .detach = bdrv_child_cb_detach, .inactivate = bdrv_child_cb_inactivate,
Is there any case of a BdrvChildRole that doesn't want these callbacks? Maybe the functions should be called after ->attach and before ->detach (e.g. bdrv_{,un}apply_subtree_drain), rather than modifying the BdrvChildRole implementations.
Then they can be put in block/io.c, and bdrv_do_drained_* can remain static. (I would also consider extracting block/drain.c, but it is painful to do it now that you have this nice series---so let's do it after).
Paolo
Re: [Qemu-devel] [PATCH 16/19] block: Allow graph changes in subtree drained section
Am 20.12.2017 um 11:51 hat Paolo Bonzini geschrieben:
Quoted 9 lines · Kevin Wolf
Quoted 8 lines
.inherit_options =
bdrv_inherited_options,
.drained_begin =bdrv_child_cb_drained_begin,
.drained_end =bdrv_child_cb_drained_end,block.c +2
-0+ .attach = bdrv_child_cb_attach, + .detach = bdrv_child_cb_detach, .inactivate = bdrv_child_cb_inactivate, };
Quoted 12 lines
block.c const BdrvChildRole child_format = { · line 938
Quoted 7 lines
block.c +2
-0@@ -911,6 +933,8 @@ const BdrvChildRole child_format = { .inherit_options = bdrv_inherited_fmt_options, .drained_begin = bdrv_child_cb_drained_begin, .drained_end = bdrv_child_cb_drained_end, + .attach = bdrv_child_cb_attach, + .detach = bdrv_child_cb_detach, .inactivate = bdrv_child_cb_inactivate,Is there any case of a BdrvChildRole that doesn't want these callbacks? Maybe the functions should be called after ->attach and before ->detach (e.g. bdrv_{,un}apply_subtree_drain), rather than modifying the BdrvChildRole implementations.
At first I intended to implement it directly in bdrv_replace_child_noperm(), but the thing is that you need the bs->recursive_quiesce_counter of the parent BDS - but not all parents of a BdrvChild are even a BDS. It could also be a BB root child or a block job child. This is why we only have a void *opaque rather than a BDS pointer for the parent.
The other option would be an additional BdrvChildRole callback like .get_recursive_quiesce_counter, but compared to that, I like some code in .attach/.detach better.
Quoted 3 lines
Then they can be put in
block/io.c, and bdrv_do_drained_* can remain static. (I would also consider extractingblock/drain.c, but it is painful to do it now that you have this nice series---so let's do it after).
I can keep that in mind for part 3 (or 4, whatever it may become).
Kevin
Re: [Qemu-devel] [Qemu-block] [PATCH 16/19] block: Allow graph changes in subtree drained section
Quoted 30 lines · Kevin Wolf
Am 20.12.2017 um 11:51 hat Paolo Bonzini geschrieben:
Quoted 9 lines · Kevin Wolf
Quoted 8 lines
.inherit_options =
bdrv_inherited_options,
.drained_begin =bdrv_child_cb_drained_begin,
.drained_end =bdrv_child_cb_drained_end,block.c +2
-0+ .attach = bdrv_child_cb_attach, + .detach = bdrv_child_cb_detach, .inactivate = bdrv_child_cb_inactivate, };Quoted 12 lines
block.c const BdrvChildRole child_format = { · line 938
Quoted 7 lines
block.c +2
-0@@ -911,6 +933,8 @@ const BdrvChildRole child_format = { .inherit_options = bdrv_inherited_fmt_options, .drained_begin = bdrv_child_cb_drained_begin, .drained_end = bdrv_child_cb_drained_end, + .attach = bdrv_child_cb_attach, + .detach = bdrv_child_cb_detach, .inactivate = bdrv_child_cb_inactivate,Is there any case of a BdrvChildRole that doesn't want these callbacks? Maybe the functions should be called after ->attach and before ->detach (e.g. bdrv_{,un}apply_subtree_drain), rather than modifying the BdrvChildRole implementations.
At first I intended to implement it directly in
bdrv_replace_child_noperm(), but the thing is that you need thebs->recursive_quiesce_counterof the parent BDS - but not all parents of a BdrvChild are even a BDS. It could also be a BB root child or a block job child. This is why we only have a void *opaque rather than a BDS pointer for the parent.
Quoted 3 lines
The other option would be an additional BdrvChildRole callback like .get_recursive_quiesce_counter, but compared to that, I like some code in .attach/.detach better.
I see. What about keeping the callbacks, but exporting
void bdrv_apply_subtree_drain(BlockDriverState *child,
BlockDriverState *new_parent); void bdrv_unapply_subtree_drain(BlockDriverState *child, BlockDriverState *old_parent);
instead of bdrv_do_drained_{begin,end}?
Thanks,
Paolo
Quoted 8 lines
Quoted 3 lines
Then they can be put in
block/io.c, and bdrv_do_drained_* can remain static. (I would also consider extractingblock/drain.c, but it is painful to do it now that you have this nice series---so let's do it after).I can keep that in mind for part 3 (or 4, whatever it may become).
Kevin
Re: [Qemu-devel] [Qemu-block] [PATCH 16/19] block: Allow graph changes in subtree drained section
Am 20.12.2017 um 12:31 hat Paolo Bonzini geschrieben:
Quoted 35 lines · Kevin Wolf
Quoted 30 lines
Am 20.12.2017 um 11:51 hat Paolo Bonzini geschrieben:
Quoted 9 lines · Kevin Wolf
Quoted 8 lines
.inherit_options =
bdrv_inherited_options,
.drained_begin =bdrv_child_cb_drained_begin,
.drained_end =bdrv_child_cb_drained_end,block.c +2
-0+ .attach = bdrv_child_cb_attach, + .detach = bdrv_child_cb_detach, .inactivate = bdrv_child_cb_inactivate, };Quoted 12 lines
block.c const BdrvChildRole child_format = { · line 938
Quoted 7 lines
block.c +2
-0@@ -911,6 +933,8 @@ const BdrvChildRole child_format = { .inherit_options = bdrv_inherited_fmt_options, .drained_begin = bdrv_child_cb_drained_begin, .drained_end = bdrv_child_cb_drained_end, + .attach = bdrv_child_cb_attach, + .detach = bdrv_child_cb_detach, .inactivate = bdrv_child_cb_inactivate,Is there any case of a BdrvChildRole that doesn't want these callbacks? Maybe the functions should be called after ->attach and before ->detach (e.g. bdrv_{,un}apply_subtree_drain), rather than modifying the BdrvChildRole implementations.
At first I intended to implement it directly in
bdrv_replace_child_noperm(), but the thing is that you need thebs->recursive_quiesce_counterof the parent BDS - but not all parents of a BdrvChild are even a BDS. It could also be a BB root child or a block job child. This is why we only have a void *opaque rather than a BDS pointer for the parent.Quoted 3 lines
The other option would be an additional BdrvChildRole callback like .get_recursive_quiesce_counter, but compared to that, I like some code in .attach/.detach better.
Quoted 8 lines
I see. What about keeping the callbacks, but exporting
void bdrv_apply_subtree_drain(BlockDriverState *child,
BlockDriverState *new_parent); void bdrv_unapply_subtree_drain(BlockDriverState *child, BlockDriverState *old_parent);instead of bdrv_do_drained_{begin,end}?
Sure, that can be done. (BdrvChild *child, though.)
Kevin
PATCH 17/19 test-bdrv-drain: Test graph changes in drained section
In reply to Kevin Wolf--- tests/test-bdrv-drain.c | 80 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 80 insertions(+)
tests/test-bdrv-drain.c +80 -0
@@ -411,6 +411,85 @@ static void test_multiparent(void)blk_unref (blk_b); } +static void test_graph_change (void ) +{ +BlockBackend *blk_a, *blk_b; +BlockDriverState *bs_a, *bs_b, *backing; +BDRVTestState *a_s, *b_s, *backing_s; + + 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); + + 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); + + backing =bdrv_new_open_driver (&bdrv_test,"backing" ,0 , &error_abort); + backing_s = backing->opaque; +bdrv_set_backing_hd (bs_a, backing, &error_abort); + + g_assert_cmpint(bs_a->quiesce_counter, ==, 0); + g_assert_cmpint(bs_b->quiesce_counter, ==, 0); + g_assert_cmpint(backing->quiesce_counter, ==, 0); + g_assert_cmpint(a_s->drain_count, ==, 0); + g_assert_cmpint(b_s->drain_count, ==, 0); + g_assert_cmpint(backing_s->drain_count, ==, 0); + +do_drain_begin (BDRV_SUBTREE_DRAIN , bs_a); +do_drain_begin (BDRV_SUBTREE_DRAIN , bs_a); +do_drain_begin (BDRV_SUBTREE_DRAIN , bs_a); +do_drain_begin (BDRV_SUBTREE_DRAIN , bs_b); +do_drain_begin (BDRV_SUBTREE_DRAIN , bs_b); + +bdrv_set_backing_hd (bs_b, backing, &error_abort); + g_assert_cmpint(bs_a->quiesce_counter, ==, 5); + g_assert_cmpint(bs_b->quiesce_counter, ==, 5); + g_assert_cmpint(backing->quiesce_counter, ==, 5); + g_assert_cmpint(a_s->drain_count, ==, 5); + g_assert_cmpint(b_s->drain_count, ==, 5); + g_assert_cmpint(backing_s->drain_count, ==, 5); + +bdrv_set_backing_hd (bs_b,NULL , &error_abort); + g_assert_cmpint(bs_a->quiesce_counter, ==, 3); + g_assert_cmpint(bs_b->quiesce_counter, ==, 2); + g_assert_cmpint(backing->quiesce_counter, ==, 3); + g_assert_cmpint(a_s->drain_count, ==, 3); + g_assert_cmpint(b_s->drain_count, ==, 2); + g_assert_cmpint(backing_s->drain_count, ==, 3); + +bdrv_set_backing_hd (bs_b, backing, &error_abort); + g_assert_cmpint(bs_a->quiesce_counter, ==, 5); + g_assert_cmpint(bs_b->quiesce_counter, ==, 5); + g_assert_cmpint(backing->quiesce_counter, ==, 5); + g_assert_cmpint(a_s->drain_count, ==, 5); + g_assert_cmpint(b_s->drain_count, ==, 5); + g_assert_cmpint(backing_s->drain_count, ==, 5); + +do_drain_end (BDRV_SUBTREE_DRAIN , bs_b); +do_drain_end (BDRV_SUBTREE_DRAIN , bs_b); +do_drain_end (BDRV_SUBTREE_DRAIN , bs_a); +do_drain_end (BDRV_SUBTREE_DRAIN , bs_a); +do_drain_end (BDRV_SUBTREE_DRAIN , bs_a); + + g_assert_cmpint(bs_a->quiesce_counter, ==, 0); + g_assert_cmpint(bs_b->quiesce_counter, ==, 0); + g_assert_cmpint(backing->quiesce_counter, ==, 0); + g_assert_cmpint(a_s->drain_count, ==, 0); + g_assert_cmpint(b_s->drain_count, ==, 0); + g_assert_cmpint(backing_s->drain_count, ==, 0); + +bdrv_unref (backing); +bdrv_unref (bs_a); +bdrv_unref (bs_b); +blk_unref (blk_a); +blk_unref (blk_b); +} + typedef struct TestBlockJob { BlockJob common;
@@ -553,6 +632,7 @@ 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/blockjob/drain_all" , test_blockjob_drain_all);g_test_add_func ("/bdrv-drain/blockjob/drain" , test_blockjob_drain);
Signature · 1 line
2.13.6
PATCH 18/19 commit: Simplify reopen of base
In reply to Kevin WolfSince commit bde70715, base is the only node that is reopened in commit_start(). This means that the code, which still involves an explicit BlockReopenQueue, can now be simplified by using bdrv_reopen().
--- block/commit.c | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-)
block/commit.c +1 -7
@@ -277,7 +277,6 @@ void commit_start(const char *job_id, BlockDriverState *bs, const char *filter_node_name, Error **errp) { CommitBlockJob *s; -BlockReopenQueue *reopen_queue =NULL ;int orig_base_flags;BlockDriverState *iter;BlockDriverState *commit_top_bs =NULL ;
@@ -299,12 +298,7 @@ void commit_start(const char *job_id, BlockDriverState *bs,/* convert base to r/w, if necessary */ orig_base_flags =bdrv_get_flags (base);if (!(orig_base_flags &BDRV_O_RDWR )) { - reopen_queue =bdrv_reopen_queue (reopen_queue, base,NULL , - orig_base_flags |BDRV_O_RDWR ); - } - -if (reopen_queue) { -bdrv_reopen_multiple (bdrv_get_aio_context (bs), reopen_queue, &local_err); +bdrv_reopen (base, orig_base_flags |BDRV_O_RDWR , &local_err);if (local_err !=NULL ) {error_propagate (errp, local_err); goto fail;
Signature · 1 line
2.13.6
Re: [Qemu-devel] [PATCH 18/19] commit: Simplify reopen of base
Quoted 38 lines · Kevin Wolf
Since commit
bde70715, base is the only node that is reopened incommit_start(). This means that the code, which still involves an explicit BlockReopenQueue, can now be simplified by usingbdrv_reopen().Signed-off-by Kevin Wolf <[email protected]>--- block/commit.c | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-)block/commit.c +1
-7@@ -277,7 +277,6 @@ void commit_start(const char *job_id, BlockDriverState *bs, const char *filter_node_name, Error **errp) { CommitBlockJob *s; -BlockReopenQueue *reopen_queue =NULL ;int orig_base_flags;BlockDriverState *iter;BlockDriverState *commit_top_bs =NULL ;@@ -299,12 +298,7 @@ void commit_start(const char *job_id, BlockDriverState *bs,/* convert base to r/w, if necessary */ orig_base_flags =bdrv_get_flags (base);if (!(orig_base_flags &BDRV_O_RDWR )) { - reopen_queue =bdrv_reopen_queue (reopen_queue, base,NULL , - orig_base_flags |BDRV_O_RDWR ); - } - -if (reopen_queue) { -bdrv_reopen_multiple (bdrv_get_aio_context (bs), reopen_queue, &local_err); +bdrv_reopen (base, orig_base_flags |BDRV_O_RDWR , &local_err);if (local_err !=NULL ) {error_propagate (errp, local_err); goto fail;Signature · 1 line
2.13.6
PATCH 19/19 block: Keep nodes drained between reopen_queue/multiple
In reply to Kevin WolfThe bdrv_reopen*() implementation doesn't like it if the graph is changed between queuing nodes for reopen and actually reopening them (one of the reasons is that queuing can be recursive).
So instead of draining the device only in bdrv_reopen_multiple(), require that callers already drained all affected nodes, and assert this in bdrv_reopen_queue().
--- block.c | 23 ++++++++++++++++------- block/replication.c | 6 ++++++ qemu-io-cmds.c | 3 +++ 3 files changed, 25 insertions(+), 7 deletions(-)
block.c +16 -7
@@ -2774,6 +2774,7 @@ BlockDriverState *bdrv_open(const char *filename, const char *reference,* returns a pointer to bs_queue, which is either the newly allocated * bs_queue, or the existing bs_queue being used. * +* bs must be drained between bdrv_reopen_queue() and bdrv_reopen_multiple(). */ static BlockReopenQueue *bdrv_reopen_queue_child(BlockReopenQueue *bs_queue, BlockDriverState *bs,
@@ -2789,6 +2790,11 @@ static BlockReopenQueue *bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,BdrvChild *child;QDict *old_options, *explicit_options; +/* Make sure that the caller remembered to use a drained section. This is +* important to avoid graph changes between the recursive queuing here and +* bdrv_reopen_multiple(). */ +assert (bs->quiesce_counter >0 ); +if (bs_queue ==NULL ) { bs_queue =g_new0 (BlockReopenQueue,1 );QSIMPLEQ_INIT (bs_queue);
@@ -2913,6 +2919,8 @@ BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue,* If all devices prepare successfully, then the changes are committed * to all devices. * +* All affected nodes must be drained between bdrv_reopen_queue() and +* bdrv_reopen_multiple(). */ int bdrv_reopen_multiple (AioContext *ctx,BlockReopenQueue *bs_queue,Error **errp) {
@@ -2922,11 +2930,8 @@ int bdrv_reopen_multiple(AioContext *ctx, BlockReopenQueue *bs_queue, Error **erassert (bs_queue !=NULL ); -aio_context_release (ctx); -bdrv_drain_all_begin (); -aio_context_acquire (ctx); - QSIMPLEQ_FOREACH(bs_entry, bs_queue, entry) { +assert (bs_entry->state.bs->quiesce_counter >0 );if (bdrv_reopen_prepare (&bs_entry->state, bs_queue, &local_err)) {error_propagate (errp, local_err); goto cleanup;
@@ -2955,8 +2960,6 @@ cleanup: }g_free (bs_queue); -bdrv_drain_all_end (); -return ret; }
@@ -2966,12 +2969,18 @@ int bdrv_reopen(BlockDriverState *bs, int bdrv_flags, Error **errp) {int ret =-1 ;Error *local_err =NULL ; -BlockReopenQueue *queue =bdrv_reopen_queue (NULL , bs,NULL , bdrv_flags); +BlockReopenQueue *queue; +bdrv_subtree_drained_begin (bs); + + queue =bdrv_reopen_queue (NULL , bs,NULL , bdrv_flags); ret =bdrv_reopen_multiple (bdrv_get_aio_context (bs), queue, &local_err);if (local_err !=NULL ) {error_propagate (errp, local_err); } + +bdrv_subtree_drained_end (bs); +return ret; }
block/replication.c +6 -0
@@ -394,6 +394,9 @@ static void reopen_backing_file(BlockDriverState *bs, bool writable, new_secondary_flags = s->orig_secondary_flags; } +bdrv_subtree_drained_begin (s->hidden_disk->bs); +bdrv_subtree_drained_begin (s->secondary_disk->bs); +if (orig_hidden_flags != new_hidden_flags) { reopen_queue =bdrv_reopen_queue (reopen_queue, s->hidden_disk->bs,NULL , new_hidden_flags);
@@ -409,6 +412,9 @@ static void reopen_backing_file(BlockDriverState *bs, bool writable, reopen_queue, &local_err);error_propagate (errp, local_err); } + +bdrv_subtree_drained_end (s->hidden_disk->bs); +bdrv_subtree_drained_end (s->secondary_disk->bs); }static void backup_job_cleanup (BlockDriverState *bs)
qemu-io-cmds.c +3 -0
@@ -2013,8 +2013,11 @@ static int reopen_f(BlockBackend *blk, int argc, char **argv) opts = qopts ?qemu_opts_to_qdict (qopts,NULL ) :NULL ;qemu_opts_reset (&reopen_opts); +bdrv_subtree_drained_begin (bs); brq =bdrv_reopen_queue (NULL , bs, opts, flags);bdrv_reopen_multiple (bdrv_get_aio_context (bs), brq, &local_err); +bdrv_subtree_drained_end (bs); +if (local_err) {error_report_err (local_err); }else {
Signature · 1 line
2.13.6
Re: [Qemu-devel] [PATCH 19/19] block: Keep nodes drained between reopen_queue/multiple
Quoted 133 lines · Kevin Wolf
The bdrv_reopen*() implementation doesn't like it if the graph is changed between queuing nodes for reopen and actually reopening them (one of the reasons is that queuing can be recursive).
So instead of draining the device only in
bdrv_reopen_multiple(), require that callers already drained all affected nodes, and assert this inbdrv_reopen_queue().Signed-off-by Kevin Wolf <[email protected]>--- block.c | 23 ++++++++++++++++------- block/replication.c | 6 ++++++ qemu-io-cmds.c | 3 +++ 3 files changed, 25 insertions(+), 7 deletions(-)block.c +16
-7@@ -2774,6 +2774,7 @@ BlockDriverState *bdrv_open(const char *filename, const char *reference,* returns a pointer to bs_queue, which is either the newly allocated * bs_queue, or the existing bs_queue being used. * +* bs must be drained between bdrv_reopen_queue() and bdrv_reopen_multiple(). */ static BlockReopenQueue *bdrv_reopen_queue_child(BlockReopenQueue *bs_queue, BlockDriverState *bs,@@ -2789,6 +2790,11 @@ static BlockReopenQueue *bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,BdrvChild *child;QDict *old_options, *explicit_options; +/* Make sure that the caller remembered to use a drained section. This is +* important to avoid graph changes between the recursive queuing here and +* bdrv_reopen_multiple(). */ +assert (bs->quiesce_counter >0 ); +if (bs_queue ==NULL ) { bs_queue =g_new0 (BlockReopenQueue,1 );QSIMPLEQ_INIT (bs_queue);@@ -2913,6 +2919,8 @@ BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue,* If all devices prepare successfully, then the changes are committed * to all devices. * +* All affected nodes must be drained between bdrv_reopen_queue() and +* bdrv_reopen_multiple(). */ int bdrv_reopen_multiple (AioContext *ctx,BlockReopenQueue *bs_queue,Error **errp) {@@ -2922,11 +2930,8 @@ int bdrv_reopen_multiple(AioContext *ctx, BlockReopenQueue *bs_queue, Error **erassert (bs_queue !=NULL ); -aio_context_release (ctx); -bdrv_drain_all_begin (); -aio_context_acquire (ctx); - QSIMPLEQ_FOREACH(bs_entry, bs_queue, entry) { +assert (bs_entry->state.bs->quiesce_counter >0 );if (bdrv_reopen_prepare (&bs_entry->state, bs_queue, &local_err)) {error_propagate (errp, local_err); goto cleanup;@@ -2955,8 +2960,6 @@ cleanup: }g_free (bs_queue); -bdrv_drain_all_end (); -return ret; }@@ -2966,12 +2969,18 @@ int bdrv_reopen(BlockDriverState *bs, int bdrv_flags, Error **errp) {int ret =-1 ;Error *local_err =NULL ; -BlockReopenQueue *queue =bdrv_reopen_queue (NULL , bs,NULL , bdrv_flags); +BlockReopenQueue *queue; +bdrv_subtree_drained_begin (bs); + + queue =bdrv_reopen_queue (NULL , bs,NULL , bdrv_flags); ret =bdrv_reopen_multiple (bdrv_get_aio_context (bs), queue, &local_err);if (local_err !=NULL ) {error_propagate (errp, local_err); } + +bdrv_subtree_drained_end (bs); +return ret; }block/replication.c +6
-0@@ -394,6 +394,9 @@ static void reopen_backing_file(BlockDriverState *bs, bool writable, new_secondary_flags = s->orig_secondary_flags; } +bdrv_subtree_drained_begin (s->hidden_disk->bs); +bdrv_subtree_drained_begin (s->secondary_disk->bs); +if (orig_hidden_flags != new_hidden_flags) { reopen_queue =bdrv_reopen_queue (reopen_queue, s->hidden_disk->bs,NULL , new_hidden_flags);@@ -409,6 +412,9 @@ static void reopen_backing_file(BlockDriverState *bs, bool writable, reopen_queue, &local_err);error_propagate (errp, local_err); } + +bdrv_subtree_drained_end (s->hidden_disk->bs); +bdrv_subtree_drained_end (s->secondary_disk->bs); }static void backup_job_cleanup (BlockDriverState *bs)qemu-io-cmds.c +3
-0@@ -2013,8 +2013,11 @@ static int reopen_f(BlockBackend *blk, int argc, char **argv) opts = qopts ?qemu_opts_to_qdict (qopts,NULL ) :NULL ;qemu_opts_reset (&reopen_opts); +bdrv_subtree_drained_begin (bs); brq =bdrv_reopen_queue (NULL , bs, opts, flags);bdrv_reopen_multiple (bdrv_get_aio_context (bs), brq, &local_err); +bdrv_subtree_drained_end (bs); +if (local_err) {error_report_err (local_err); }else {Signature · 1 line
2.13.6
Quoted 12 lines · Kevin Wolf
This is the second part of my work to fix drain and hopefully to prevent it from attracting bugs as much as it did in the past. There is definitely at least a third part coming after this, see below.
In this series, the following improvments are made:
* Fix several bugs and inconsistencies
* Create lots of unit tests for the drain functions
* Introduce bdrv_subtree_drained_begin/end() to drain a whole subtree rather than only a single node
* Use this to makebdrv_reopen()safe (graph changes in callbacks called during its internalbdrv_drain_all()frequently broke it)
Quoted 5 lines
Planned for part three: Make graph modifications in callbacks safe by avoiding
BDRV_POLL_WHILE()calls while we're recursing through the graph. We can recurse beforeBDRV_POLL_WHILE(), after it or while evaluating its condition, but never call any callbacks while we're iterating child or parent lists.
This is very nice, thanks for it! I only had two comments, one of them pretty minor.
Paolo
Quoted 34 lines
Fam Zheng (1):
block: Remove unusedbdrv_requests_pendingKevin Wolf (18):
block: Assertdrain_allis only called from main AioContext
block: Makebdrv_drain()driver callbacks non-recursive
test-bdrv-drain: Test callback forbdrv_drain
test-bdrv-drain: Testbs->quiesce_counter
blockjob: Pause job on draining any job BDS
test-bdrv-drain: Test drain vs. block jobs
block: Don'tblock_job_pause_all()inbdrv_drain_all()
block: Nesteddrain_endmust still call callbacks
test-bdrv-drain: Test nested drain sections
block: Don't notify parents in drain call chain
block: Add bdrv_subtree_drained_begin/end()
test-bdrv-drain: Tests forbdrv_subtree_drain
test-bdrv-drain: Test behaviour in coroutine context
test-bdrv-drain: Recursive draining with multiple parents
block: Allow graph changes in subtree drained section
test-bdrv-drain: Test graph changes in drained section
commit: Simplify reopen of base
block: Keep nodes drained between reopen_queue/multipleinclude/block/block.h | 20 +- include/block/block_int.h | 3 +- block.c | 74 +++++-- block/commit.c | 8 +- block/io.c | 128 ++++++++---- block/replication.c | 6 + blockjob.c | 22 +- qemu-io-cmds.c | 3 + tests/test-bdrv-drain.c | 520 +++++++++++++++++++++++++++++++++++++++++++++- 9 files changed, 696 insertions(+), 88 deletions(-)