* [Qemu-devel] [RFC PATCH 0/5] Fix some jobs/drain/aio_poll related hangs
@ 2018-08-17 17:02 Kevin Wolf
2018-08-17 17:02 ` [Qemu-devel] [RFC PATCH 1/5] blockjob: Wake up BDS when job becomes idle Kevin Wolf
` (6 more replies)
0 siblings, 7 replies; 13+ messages in thread
From: Kevin Wolf @ 2018-08-17 17:02 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, famz, mreitz, qemu-devel
I'm running out of time and will be offline for the next two weeks, so
I'm just sending out what I have right now. This is probably not ready
to be merged yet, but if need be, someone else can pick it up. Otherwise
I'll do that myself when I return.
This is related to the following bug reports:
https://bugzilla.redhat.com/show_bug.cgi?id=1614623
https://bugzilla.redhat.com/show_bug.cgi?id=1601212
Essentially it tries to fix a few deadlocks into which we run when
running two commit jobs (probably just one example for problematic
scenarios) on nodes that are in a non-mainloop AioContext (i.e. with
dataplane).
Obviously, besides verifying that the code changes are actually correct
as they are, we also want to add some test cases for this. I suppose
test-bdrv-drain should test for the individual bugs, and a qemu-iotests
case could try the higher level scenario of multiple commit jobs with
dataplane.
Kevin Wolf (5):
blockjob: Wake up BDS when job becomes idle
tests: Acquire AioContext around job_finish_sync()
job: Drop AioContext lock around aio_poll()
block: Drop AioContext lock in bdrv_drain_poll_top_level()
[WIP] Lock AioContext in bdrv_co_drain_bh_cb()
include/block/blockjob.h | 13 +++++++++++++
include/qemu/job.h | 9 +++++++++
block/io.c | 31 ++++++++++++++++++++++++++++++-
blockjob.c | 18 ++++++++++++++++++
job.c | 10 ++++++++++
tests/test-bdrv-drain.c | 6 ++++++
tests/test-blockjob.c | 6 ++++++
7 files changed, 92 insertions(+), 1 deletion(-)
--
2.13.6
^ permalink raw reply [flat|nested] 13+ messages in thread
* [Qemu-devel] [RFC PATCH 1/5] blockjob: Wake up BDS when job becomes idle
2018-08-17 17:02 [Qemu-devel] [RFC PATCH 0/5] Fix some jobs/drain/aio_poll related hangs Kevin Wolf
@ 2018-08-17 17:02 ` Kevin Wolf
2018-08-17 17:02 ` [Qemu-devel] [RFC PATCH 2/5] tests: Acquire AioContext around job_finish_sync() Kevin Wolf
` (5 subsequent siblings)
6 siblings, 0 replies; 13+ messages in thread
From: Kevin Wolf @ 2018-08-17 17:02 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, famz, mreitz, qemu-devel
In the context of draining a BDS, the .drained_poll callback of block
jobs is called. If this returns true (i.e. there is still some activity
pending), the drain operation may call aio_poll() with blocking=true to
wait for completion.
As soon as the pending activity is completed and the job finally arrives
in a quiescent state (i.e. its coroutine either yields with busy=false
or terminates), the block job must notify the aio_poll() loop to wake
up, otherwise we get a deadlock if both are running in different
threads.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
include/block/blockjob.h | 13 +++++++++++++
include/qemu/job.h | 3 +++
blockjob.c | 18 ++++++++++++++++++
job.c | 7 +++++++
4 files changed, 41 insertions(+)
diff --git a/include/block/blockjob.h b/include/block/blockjob.h
index 32c00b7dc0..2290bbb824 100644
--- a/include/block/blockjob.h
+++ b/include/block/blockjob.h
@@ -70,6 +70,9 @@ typedef struct BlockJob {
/** Called when the job transitions to READY */
Notifier ready_notifier;
+ /** Called when the job coroutine yields or terminates */
+ Notifier idle_notifier;
+
/** BlockDriverStates that are involved in this block job */
GSList *nodes;
} BlockJob;
@@ -119,6 +122,16 @@ int block_job_add_bdrv(BlockJob *job, const char *name, BlockDriverState *bs,
void block_job_remove_all_bdrv(BlockJob *job);
/**
+ * block_job_wakeup_all_bdrv:
+ * @job: The block job
+ *
+ * Calls bdrv_wakeup() for all BlockDriverStates that have been added to the
+ * job. This function is to be called whenever child_job_drained_poll() would
+ * go from true to false to notify waiting drain requests.
+ */
+void block_job_wakeup_all_bdrv(BlockJob *job);
+
+/**
* block_job_set_speed:
* @job: The job to set the speed for.
* @speed: The new value
diff --git a/include/qemu/job.h b/include/qemu/job.h
index 18c9223e31..0dae5b8481 100644
--- a/include/qemu/job.h
+++ b/include/qemu/job.h
@@ -148,6 +148,9 @@ typedef struct Job {
/** Notifiers called when the job transitions to READY */
NotifierList on_ready;
+ /** Notifiers called when the job coroutine yields or terminates */
+ NotifierList on_idle;
+
/** Element of the list of jobs */
QLIST_ENTRY(Job) job_list;
diff --git a/blockjob.c b/blockjob.c
index be5903aa96..8d27e8e1ea 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -221,6 +221,22 @@ int block_job_add_bdrv(BlockJob *job, const char *name, BlockDriverState *bs,
return 0;
}
+void block_job_wakeup_all_bdrv(BlockJob *job)
+{
+ GSList *l;
+
+ for (l = job->nodes; l; l = l->next) {
+ BdrvChild *c = l->data;
+ bdrv_wakeup(c->bs);
+ }
+}
+
+static void block_job_on_idle(Notifier *n, void *opaque)
+{
+ BlockJob *job = opaque;
+ block_job_wakeup_all_bdrv(job);
+}
+
bool block_job_is_internal(BlockJob *job)
{
return (job->job.id == NULL);
@@ -419,6 +435,7 @@ void *block_job_create(const char *job_id, const BlockJobDriver *driver,
job->finalize_completed_notifier.notify = block_job_event_completed;
job->pending_notifier.notify = block_job_event_pending;
job->ready_notifier.notify = block_job_event_ready;
+ job->idle_notifier.notify = block_job_on_idle;
notifier_list_add(&job->job.on_finalize_cancelled,
&job->finalize_cancelled_notifier);
@@ -426,6 +443,7 @@ void *block_job_create(const char *job_id, const BlockJobDriver *driver,
&job->finalize_completed_notifier);
notifier_list_add(&job->job.on_pending, &job->pending_notifier);
notifier_list_add(&job->job.on_ready, &job->ready_notifier);
+ notifier_list_add(&job->job.on_idle, &job->idle_notifier);
error_setg(&job->blocker, "block device is in use by block job: %s",
job_type_str(&job->job));
diff --git a/job.c b/job.c
index fa671b431a..a746bfe70b 100644
--- a/job.c
+++ b/job.c
@@ -410,6 +410,11 @@ static void job_event_ready(Job *job)
notifier_list_notify(&job->on_ready, job);
}
+static void job_event_idle(Job *job)
+{
+ notifier_list_notify(&job->on_idle, job);
+}
+
void job_enter_cond(Job *job, bool(*fn)(Job *job))
{
if (!job_started(job)) {
@@ -455,6 +460,7 @@ static void coroutine_fn job_do_yield(Job *job, uint64_t ns)
timer_mod(&job->sleep_timer, ns);
}
job->busy = false;
+ job_event_idle(job);
job_unlock();
qemu_coroutine_yield();
@@ -547,6 +553,7 @@ static void coroutine_fn job_co_entry(void *opaque)
assert(job && job->driver && job->driver->start);
job_pause_point(job);
job->driver->start(job);
+ job_event_idle(job);
}
--
2.13.6
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [Qemu-devel] [RFC PATCH 2/5] tests: Acquire AioContext around job_finish_sync()
2018-08-17 17:02 [Qemu-devel] [RFC PATCH 0/5] Fix some jobs/drain/aio_poll related hangs Kevin Wolf
2018-08-17 17:02 ` [Qemu-devel] [RFC PATCH 1/5] blockjob: Wake up BDS when job becomes idle Kevin Wolf
@ 2018-08-17 17:02 ` Kevin Wolf
2018-08-24 7:15 ` Fam Zheng
2018-08-17 17:02 ` [Qemu-devel] [RFC PATCH 3/5] job: Drop AioContext lock around aio_poll() Kevin Wolf
` (4 subsequent siblings)
6 siblings, 1 reply; 13+ messages in thread
From: Kevin Wolf @ 2018-08-17 17:02 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, famz, mreitz, qemu-devel
All callers in QEMU proper hold the AioContext lock when calling
job_finish_sync(). The tests should do the same.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
include/qemu/job.h | 6 ++++++
tests/test-bdrv-drain.c | 6 ++++++
tests/test-blockjob.c | 6 ++++++
3 files changed, 18 insertions(+)
diff --git a/include/qemu/job.h b/include/qemu/job.h
index 0dae5b8481..8ac48dbd28 100644
--- a/include/qemu/job.h
+++ b/include/qemu/job.h
@@ -520,6 +520,8 @@ void job_user_cancel(Job *job, bool force, Error **errp);
*
* Returns the return value from the job if the job actually completed
* during the call, or -ECANCELED if it was canceled.
+ *
+ * Callers must hold the AioContext lock of job->aio_context.
*/
int job_cancel_sync(Job *job);
@@ -537,6 +539,8 @@ void job_cancel_sync_all(void);
* function).
*
* Returns the return value from the job.
+ *
+ * Callers must hold the AioContext lock of job->aio_context.
*/
int job_complete_sync(Job *job, Error **errp);
@@ -579,6 +583,8 @@ void job_defer_to_main_loop(Job *job, JobDeferToMainLoopFn *fn, void *opaque);
*
* Returns 0 if the job is successfully completed, -ECANCELED if the job was
* cancelled before completing, and -errno in other error cases.
+ *
+ * Callers must hold the AioContext lock of job->aio_context.
*/
int job_finish_sync(Job *job, void (*finish)(Job *, Error **errp), Error **errp);
diff --git a/tests/test-bdrv-drain.c b/tests/test-bdrv-drain.c
index 17bb8508ae..30294038ef 100644
--- a/tests/test-bdrv-drain.c
+++ b/tests/test-bdrv-drain.c
@@ -795,6 +795,7 @@ static void test_blockjob_common(enum drain_type drain_type)
BlockBackend *blk_src, *blk_target;
BlockDriverState *src, *target;
BlockJob *job;
+ AioContext *ctx;
int ret;
src = bdrv_new_open_driver(&bdrv_test, "source", BDRV_O_RDWR,
@@ -807,6 +808,9 @@ static void test_blockjob_common(enum drain_type drain_type)
blk_target = blk_new(BLK_PERM_ALL, BLK_PERM_ALL);
blk_insert_bs(blk_target, target, &error_abort);
+ ctx = qemu_get_aio_context();
+ aio_context_acquire(ctx);
+
job = block_job_create("job0", &test_job_driver, NULL, src, 0, BLK_PERM_ALL,
0, 0, NULL, NULL, &error_abort);
block_job_add_bdrv(job, "target", target, 0, BLK_PERM_ALL, &error_abort);
@@ -853,6 +857,8 @@ static void test_blockjob_common(enum drain_type drain_type)
ret = job_complete_sync(&job->job, &error_abort);
g_assert_cmpint(ret, ==, 0);
+ aio_context_release(ctx);
+
blk_unref(blk_src);
blk_unref(blk_target);
bdrv_unref(src);
diff --git a/tests/test-blockjob.c b/tests/test-blockjob.c
index cb42f06e61..8c2babbe35 100644
--- a/tests/test-blockjob.c
+++ b/tests/test-blockjob.c
@@ -230,6 +230,10 @@ static void cancel_common(CancelJob *s)
BlockJob *job = &s->common;
BlockBackend *blk = s->blk;
JobStatus sts = job->job.status;
+ AioContext *ctx;
+
+ ctx = job->job.aio_context;
+ aio_context_acquire(ctx);
job_cancel_sync(&job->job);
if (sts != JOB_STATUS_CREATED && sts != JOB_STATUS_CONCLUDED) {
@@ -239,6 +243,8 @@ static void cancel_common(CancelJob *s)
assert(job->job.status == JOB_STATUS_NULL);
job_unref(&job->job);
destroy_blk(blk);
+
+ aio_context_release(ctx);
}
static void test_cancel_created(void)
--
2.13.6
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [Qemu-devel] [RFC PATCH 3/5] job: Drop AioContext lock around aio_poll()
2018-08-17 17:02 [Qemu-devel] [RFC PATCH 0/5] Fix some jobs/drain/aio_poll related hangs Kevin Wolf
2018-08-17 17:02 ` [Qemu-devel] [RFC PATCH 1/5] blockjob: Wake up BDS when job becomes idle Kevin Wolf
2018-08-17 17:02 ` [Qemu-devel] [RFC PATCH 2/5] tests: Acquire AioContext around job_finish_sync() Kevin Wolf
@ 2018-08-17 17:02 ` Kevin Wolf
2018-08-24 7:22 ` Fam Zheng
2018-08-17 17:02 ` [Qemu-devel] [RFC PATCH 4/5] block: Drop AioContext lock in bdrv_drain_poll_top_level() Kevin Wolf
` (3 subsequent siblings)
6 siblings, 1 reply; 13+ messages in thread
From: Kevin Wolf @ 2018-08-17 17:02 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, famz, mreitz, qemu-devel
Simimlar to AIO_WAIT_WHILE(), job_finish_sync() needs to release the
AioContext lock of the job before calling aio_poll(). Otherwise,
callbacks called by aio_poll() would possibly take the lock a second
time and run into a deadlock with a nested AIO_WAIT_WHILE() call.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
job.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/job.c b/job.c
index a746bfe70b..6acf55bceb 100644
--- a/job.c
+++ b/job.c
@@ -1016,7 +1016,10 @@ int job_finish_sync(Job *job, void (*finish)(Job *, Error **errp), Error **errp)
job_drain(job);
}
while (!job_is_completed(job)) {
+ AioContext *aio_context = job->aio_context;
+ aio_context_release(aio_context);
aio_poll(qemu_get_aio_context(), true);
+ aio_context_acquire(aio_context);
}
ret = (job_is_cancelled(job) && job->ret == 0) ? -ECANCELED : job->ret;
job_unref(job);
--
2.13.6
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [Qemu-devel] [RFC PATCH 4/5] block: Drop AioContext lock in bdrv_drain_poll_top_level()
2018-08-17 17:02 [Qemu-devel] [RFC PATCH 0/5] Fix some jobs/drain/aio_poll related hangs Kevin Wolf
` (2 preceding siblings ...)
2018-08-17 17:02 ` [Qemu-devel] [RFC PATCH 3/5] job: Drop AioContext lock around aio_poll() Kevin Wolf
@ 2018-08-17 17:02 ` Kevin Wolf
2018-08-24 7:24 ` Fam Zheng
2018-08-17 17:02 ` [Qemu-devel] [RFC PATCH 5/5] [WIP] Lock AioContext in bdrv_co_drain_bh_cb() Kevin Wolf
` (2 subsequent siblings)
6 siblings, 1 reply; 13+ messages in thread
From: Kevin Wolf @ 2018-08-17 17:02 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, famz, mreitz, qemu-devel
Simimlar to AIO_WAIT_WHILE(), bdrv_drain_poll_top_level() needs to
release the AioContext lock of the node to be drained before calling
aio_poll(). Otherwise, callbacks called by aio_poll() would possibly
take the lock a second time and run into a deadlock with a nested
AIO_WAIT_WHILE() call.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
block/io.c | 25 ++++++++++++++++++++++++-
1 file changed, 24 insertions(+), 1 deletion(-)
diff --git a/block/io.c b/block/io.c
index 7100344c7b..832d2536bf 100644
--- a/block/io.c
+++ b/block/io.c
@@ -268,9 +268,32 @@ bool bdrv_drain_poll(BlockDriverState *bs, bool recursive,
static bool bdrv_drain_poll_top_level(BlockDriverState *bs, bool recursive,
BdrvChild *ignore_parent)
{
+ AioContext *ctx = bdrv_get_aio_context(bs);
+
+ /*
+ * We cannot easily release the lock unconditionally here because many
+ * callers of drain function (like qemu initialisation, tools, etc.) don't
+ * even hold the main context lock.
+ *
+ * This means that we fix potential deadlocks for the case where we are in
+ * the main context and polling a BDS in a different AioContext, but
+ * draining a BDS in the main context from a different I/O thread would
+ * still have this problem. Fortunately, this isn't supposed to happen
+ * anyway.
+ */
+ if (ctx != qemu_get_aio_context()) {
+ aio_context_release(ctx);
+ } else {
+ assert(qemu_get_current_aio_context() == qemu_get_aio_context());
+ }
+
/* Execute pending BHs first and check everything else only after the BHs
* have executed. */
- while (aio_poll(bs->aio_context, false));
+ while (aio_poll(ctx, false));
+
+ if (ctx != qemu_get_aio_context()) {
+ aio_context_acquire(ctx);
+ }
return bdrv_drain_poll(bs, recursive, ignore_parent, false);
}
--
2.13.6
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [Qemu-devel] [RFC PATCH 5/5] [WIP] Lock AioContext in bdrv_co_drain_bh_cb()
2018-08-17 17:02 [Qemu-devel] [RFC PATCH 0/5] Fix some jobs/drain/aio_poll related hangs Kevin Wolf
` (3 preceding siblings ...)
2018-08-17 17:02 ` [Qemu-devel] [RFC PATCH 4/5] block: Drop AioContext lock in bdrv_drain_poll_top_level() Kevin Wolf
@ 2018-08-17 17:02 ` Kevin Wolf
2018-08-18 10:46 ` [Qemu-devel] [RFC PATCH 0/5] Fix some jobs/drain/aio_poll related hangs no-reply
2018-08-21 6:08 ` Fam Zheng
6 siblings, 0 replies; 13+ messages in thread
From: Kevin Wolf @ 2018-08-17 17:02 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, famz, mreitz, qemu-devel
Not sure if this is correct, but at least it makes qemu-iotests 127 pass
again.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
block/io.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/block/io.c b/block/io.c
index 832d2536bf..d3dde4d7fd 100644
--- a/block/io.c
+++ b/block/io.c
@@ -309,6 +309,10 @@ static void bdrv_co_drain_bh_cb(void *opaque)
BdrvCoDrainData *data = opaque;
Coroutine *co = data->co;
BlockDriverState *bs = data->bs;
+ AioContext *ctx;
+
+ ctx = bdrv_get_aio_context(bs);
+ aio_context_acquire(ctx);
if (bs) {
bdrv_dec_in_flight(bs);
@@ -324,6 +328,8 @@ static void bdrv_co_drain_bh_cb(void *opaque)
bdrv_drain_all_begin();
}
+ aio_context_release(ctx);
+
data->done = true;
aio_co_wake(co);
}
--
2.13.6
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 0/5] Fix some jobs/drain/aio_poll related hangs
2018-08-17 17:02 [Qemu-devel] [RFC PATCH 0/5] Fix some jobs/drain/aio_poll related hangs Kevin Wolf
` (4 preceding siblings ...)
2018-08-17 17:02 ` [Qemu-devel] [RFC PATCH 5/5] [WIP] Lock AioContext in bdrv_co_drain_bh_cb() Kevin Wolf
@ 2018-08-18 10:46 ` no-reply
2018-08-21 6:08 ` Fam Zheng
6 siblings, 0 replies; 13+ messages in thread
From: no-reply @ 2018-08-18 10:46 UTC (permalink / raw)
To: kwolf; +Cc: famz, qemu-block
Hi,
This series seems to have some coding style problems. See output below for
more information:
Type: series
Message-id: 20180817170246.14641-1-kwolf@redhat.com
Subject: [Qemu-devel] [RFC PATCH 0/5] Fix some jobs/drain/aio_poll related hangs
=== 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
Switched to a new branch 'test'
fb25339a46 Lock AioContext in bdrv_co_drain_bh_cb()
11c6cc6aa9 block: Drop AioContext lock in bdrv_drain_poll_top_level()
1b110e2bb8 job: Drop AioContext lock around aio_poll()
52269c1c01 tests: Acquire AioContext around job_finish_sync()
25292b40af blockjob: Wake up BDS when job becomes idle
=== OUTPUT BEGIN ===
Checking PATCH 1/5: blockjob: Wake up BDS when job becomes idle...
Checking PATCH 2/5: tests: Acquire AioContext around job_finish_sync()...
Checking PATCH 3/5: job: Drop AioContext lock around aio_poll()...
Checking PATCH 4/5: block: Drop AioContext lock in bdrv_drain_poll_top_level()...
ERROR: trailing statements should be on next line
#45: FILE: block/io.c:292:
+ while (aio_poll(ctx, false));
ERROR: braces {} are necessary for all arms of this statement
#45: FILE: block/io.c:292:
+ while (aio_poll(ctx, false));
[...]
total: 2 errors, 0 warnings, 33 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 5/5: Lock AioContext in bdrv_co_drain_bh_cb()...
=== 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] 13+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 0/5] Fix some jobs/drain/aio_poll related hangs
2018-08-17 17:02 [Qemu-devel] [RFC PATCH 0/5] Fix some jobs/drain/aio_poll related hangs Kevin Wolf
` (5 preceding siblings ...)
2018-08-18 10:46 ` [Qemu-devel] [RFC PATCH 0/5] Fix some jobs/drain/aio_poll related hangs no-reply
@ 2018-08-21 6:08 ` Fam Zheng
6 siblings, 0 replies; 13+ messages in thread
From: Fam Zheng @ 2018-08-21 6:08 UTC (permalink / raw)
To: Kevin Wolf, pbonzini; +Cc: qemu-block, mreitz, qemu-devel
On Fri, 08/17 19:02, Kevin Wolf wrote:
> I'm running out of time and will be offline for the next two weeks, so
> I'm just sending out what I have right now. This is probably not ready
> to be merged yet, but if need be, someone else can pick it up. Otherwise
> I'll do that myself when I return.
I'll try take a look at these. But I'll start from
https://bugzilla.redhat.com/show_bug.cgi?id=1609137
>
> This is related to the following bug reports:
> https://bugzilla.redhat.com/show_bug.cgi?id=1614623
> https://bugzilla.redhat.com/show_bug.cgi?id=1601212
>
> Essentially it tries to fix a few deadlocks into which we run when
> running two commit jobs (probably just one example for problematic
> scenarios) on nodes that are in a non-mainloop AioContext (i.e. with
> dataplane).
>
> Obviously, besides verifying that the code changes are actually correct
> as they are, we also want to add some test cases for this. I suppose
> test-bdrv-drain should test for the individual bugs, and a qemu-iotests
> case could try the higher level scenario of multiple commit jobs with
> dataplane.
The test coverage of dataplane is terribly low. Let's think about adding
variants to existing iotests around block jobs at least.
Fam
>
> Kevin Wolf (5):
> blockjob: Wake up BDS when job becomes idle
> tests: Acquire AioContext around job_finish_sync()
> job: Drop AioContext lock around aio_poll()
> block: Drop AioContext lock in bdrv_drain_poll_top_level()
> [WIP] Lock AioContext in bdrv_co_drain_bh_cb()
>
> include/block/blockjob.h | 13 +++++++++++++
> include/qemu/job.h | 9 +++++++++
> block/io.c | 31 ++++++++++++++++++++++++++++++-
> blockjob.c | 18 ++++++++++++++++++
> job.c | 10 ++++++++++
> tests/test-bdrv-drain.c | 6 ++++++
> tests/test-blockjob.c | 6 ++++++
> 7 files changed, 92 insertions(+), 1 deletion(-)
>
> --
> 2.13.6
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 2/5] tests: Acquire AioContext around job_finish_sync()
2018-08-17 17:02 ` [Qemu-devel] [RFC PATCH 2/5] tests: Acquire AioContext around job_finish_sync() Kevin Wolf
@ 2018-08-24 7:15 ` Fam Zheng
0 siblings, 0 replies; 13+ messages in thread
From: Fam Zheng @ 2018-08-24 7:15 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-block, mreitz, qemu-devel
On Fri, 08/17 19:02, Kevin Wolf wrote:
> All callers in QEMU proper hold the AioContext lock when calling
> job_finish_sync(). The tests should do the same.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
A similar patch is needed for job_finalize() too, I think.
Fam
> ---
> include/qemu/job.h | 6 ++++++
> tests/test-bdrv-drain.c | 6 ++++++
> tests/test-blockjob.c | 6 ++++++
> 3 files changed, 18 insertions(+)
>
> diff --git a/include/qemu/job.h b/include/qemu/job.h
> index 0dae5b8481..8ac48dbd28 100644
> --- a/include/qemu/job.h
> +++ b/include/qemu/job.h
> @@ -520,6 +520,8 @@ void job_user_cancel(Job *job, bool force, Error **errp);
> *
> * Returns the return value from the job if the job actually completed
> * during the call, or -ECANCELED if it was canceled.
> + *
> + * Callers must hold the AioContext lock of job->aio_context.
> */
> int job_cancel_sync(Job *job);
>
> @@ -537,6 +539,8 @@ void job_cancel_sync_all(void);
> * function).
> *
> * Returns the return value from the job.
> + *
> + * Callers must hold the AioContext lock of job->aio_context.
> */
> int job_complete_sync(Job *job, Error **errp);
>
> @@ -579,6 +583,8 @@ void job_defer_to_main_loop(Job *job, JobDeferToMainLoopFn *fn, void *opaque);
> *
> * Returns 0 if the job is successfully completed, -ECANCELED if the job was
> * cancelled before completing, and -errno in other error cases.
> + *
> + * Callers must hold the AioContext lock of job->aio_context.
> */
> int job_finish_sync(Job *job, void (*finish)(Job *, Error **errp), Error **errp);
>
> diff --git a/tests/test-bdrv-drain.c b/tests/test-bdrv-drain.c
> index 17bb8508ae..30294038ef 100644
> --- a/tests/test-bdrv-drain.c
> +++ b/tests/test-bdrv-drain.c
> @@ -795,6 +795,7 @@ static void test_blockjob_common(enum drain_type drain_type)
> BlockBackend *blk_src, *blk_target;
> BlockDriverState *src, *target;
> BlockJob *job;
> + AioContext *ctx;
> int ret;
>
> src = bdrv_new_open_driver(&bdrv_test, "source", BDRV_O_RDWR,
> @@ -807,6 +808,9 @@ static void test_blockjob_common(enum drain_type drain_type)
> blk_target = blk_new(BLK_PERM_ALL, BLK_PERM_ALL);
> blk_insert_bs(blk_target, target, &error_abort);
>
> + ctx = qemu_get_aio_context();
> + aio_context_acquire(ctx);
> +
> job = block_job_create("job0", &test_job_driver, NULL, src, 0, BLK_PERM_ALL,
> 0, 0, NULL, NULL, &error_abort);
> block_job_add_bdrv(job, "target", target, 0, BLK_PERM_ALL, &error_abort);
> @@ -853,6 +857,8 @@ static void test_blockjob_common(enum drain_type drain_type)
> ret = job_complete_sync(&job->job, &error_abort);
> g_assert_cmpint(ret, ==, 0);
>
> + aio_context_release(ctx);
> +
> blk_unref(blk_src);
> blk_unref(blk_target);
> bdrv_unref(src);
> diff --git a/tests/test-blockjob.c b/tests/test-blockjob.c
> index cb42f06e61..8c2babbe35 100644
> --- a/tests/test-blockjob.c
> +++ b/tests/test-blockjob.c
> @@ -230,6 +230,10 @@ static void cancel_common(CancelJob *s)
> BlockJob *job = &s->common;
> BlockBackend *blk = s->blk;
> JobStatus sts = job->job.status;
> + AioContext *ctx;
> +
> + ctx = job->job.aio_context;
> + aio_context_acquire(ctx);
>
> job_cancel_sync(&job->job);
> if (sts != JOB_STATUS_CREATED && sts != JOB_STATUS_CONCLUDED) {
> @@ -239,6 +243,8 @@ static void cancel_common(CancelJob *s)
> assert(job->job.status == JOB_STATUS_NULL);
> job_unref(&job->job);
> destroy_blk(blk);
> +
> + aio_context_release(ctx);
> }
>
> static void test_cancel_created(void)
> --
> 2.13.6
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 3/5] job: Drop AioContext lock around aio_poll()
2018-08-17 17:02 ` [Qemu-devel] [RFC PATCH 3/5] job: Drop AioContext lock around aio_poll() Kevin Wolf
@ 2018-08-24 7:22 ` Fam Zheng
2018-09-04 14:44 ` Kevin Wolf
0 siblings, 1 reply; 13+ messages in thread
From: Fam Zheng @ 2018-08-24 7:22 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-block, mreitz, qemu-devel
On Fri, 08/17 19:02, Kevin Wolf wrote:
> Simimlar to AIO_WAIT_WHILE(), job_finish_sync() needs to release the
> AioContext lock of the job before calling aio_poll(). Otherwise,
> callbacks called by aio_poll() would possibly take the lock a second
> time and run into a deadlock with a nested AIO_WAIT_WHILE() call.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> job.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/job.c b/job.c
> index a746bfe70b..6acf55bceb 100644
> --- a/job.c
> +++ b/job.c
> @@ -1016,7 +1016,10 @@ int job_finish_sync(Job *job, void (*finish)(Job *, Error **errp), Error **errp)
> job_drain(job);
> }
> while (!job_is_completed(job)) {
> + AioContext *aio_context = job->aio_context;
> + aio_context_release(aio_context);
> aio_poll(qemu_get_aio_context(), true);
> + aio_context_acquire(aio_context);
> }
> ret = (job_is_cancelled(job) && job->ret == 0) ? -ECANCELED : job->ret;
> job_unref(job);
Why doesn't this function just use AIO_WAIT_WHILE()?
Fam
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 4/5] block: Drop AioContext lock in bdrv_drain_poll_top_level()
2018-08-17 17:02 ` [Qemu-devel] [RFC PATCH 4/5] block: Drop AioContext lock in bdrv_drain_poll_top_level() Kevin Wolf
@ 2018-08-24 7:24 ` Fam Zheng
2018-09-04 14:50 ` Kevin Wolf
0 siblings, 1 reply; 13+ messages in thread
From: Fam Zheng @ 2018-08-24 7:24 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-block, mreitz, qemu-devel
On Fri, 08/17 19:02, Kevin Wolf wrote:
> Simimlar to AIO_WAIT_WHILE(), bdrv_drain_poll_top_level() needs to
> release the AioContext lock of the node to be drained before calling
> aio_poll(). Otherwise, callbacks called by aio_poll() would possibly
> take the lock a second time and run into a deadlock with a nested
> AIO_WAIT_WHILE() call.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> block/io.c | 25 ++++++++++++++++++++++++-
> 1 file changed, 24 insertions(+), 1 deletion(-)
>
> diff --git a/block/io.c b/block/io.c
> index 7100344c7b..832d2536bf 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -268,9 +268,32 @@ bool bdrv_drain_poll(BlockDriverState *bs, bool recursive,
> static bool bdrv_drain_poll_top_level(BlockDriverState *bs, bool recursive,
> BdrvChild *ignore_parent)
> {
> + AioContext *ctx = bdrv_get_aio_context(bs);
> +
> + /*
> + * We cannot easily release the lock unconditionally here because many
> + * callers of drain function (like qemu initialisation, tools, etc.) don't
> + * even hold the main context lock.
> + *
> + * This means that we fix potential deadlocks for the case where we are in
> + * the main context and polling a BDS in a different AioContext, but
> + * draining a BDS in the main context from a different I/O thread would
> + * still have this problem. Fortunately, this isn't supposed to happen
> + * anyway.
> + */
> + if (ctx != qemu_get_aio_context()) {
> + aio_context_release(ctx);
> + } else {
> + assert(qemu_get_current_aio_context() == qemu_get_aio_context());
> + }
> +
> /* Execute pending BHs first and check everything else only after the BHs
> * have executed. */
> - while (aio_poll(bs->aio_context, false));
> + while (aio_poll(ctx, false));
> +
> + if (ctx != qemu_get_aio_context()) {
> + aio_context_acquire(ctx);
> + }
>
> return bdrv_drain_poll(bs, recursive, ignore_parent, false);
> }
> --
> 2.13.6
>
The same question as patch 3: why not just use AIO_WAIT_WHILE() here? It takes
care to not release any lock if both running and polling in the main context
(taking the in_aio_context_home_thread() branch).
Fam
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 3/5] job: Drop AioContext lock around aio_poll()
2018-08-24 7:22 ` Fam Zheng
@ 2018-09-04 14:44 ` Kevin Wolf
0 siblings, 0 replies; 13+ messages in thread
From: Kevin Wolf @ 2018-09-04 14:44 UTC (permalink / raw)
To: Fam Zheng; +Cc: qemu-block, mreitz, qemu-devel
Am 24.08.2018 um 09:22 hat Fam Zheng geschrieben:
> On Fri, 08/17 19:02, Kevin Wolf wrote:
> > Simimlar to AIO_WAIT_WHILE(), job_finish_sync() needs to release the
> > AioContext lock of the job before calling aio_poll(). Otherwise,
> > callbacks called by aio_poll() would possibly take the lock a second
> > time and run into a deadlock with a nested AIO_WAIT_WHILE() call.
> >
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> > job.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/job.c b/job.c
> > index a746bfe70b..6acf55bceb 100644
> > --- a/job.c
> > +++ b/job.c
> > @@ -1016,7 +1016,10 @@ int job_finish_sync(Job *job, void (*finish)(Job *, Error **errp), Error **errp)
> > job_drain(job);
> > }
> > while (!job_is_completed(job)) {
> > + AioContext *aio_context = job->aio_context;
> > + aio_context_release(aio_context);
> > aio_poll(qemu_get_aio_context(), true);
> > + aio_context_acquire(aio_context);
> > }
> > ret = (job_is_cancelled(job) && job->ret == 0) ? -ECANCELED : job->ret;
> > job_unref(job);
>
> Why doesn't this function just use AIO_WAIT_WHILE()?
I don't have an AioWait here, so this seemed the simplest way. But
thinking more about it, a dummy AioWait should do because otherwise the
code would already hang as it is.
Of course, we need to #include "block/aio-wait.h", which doesn't feel
completely right outside the block layer. But maybe that just means that
the header should be moved somewhere else.
Kevin
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 4/5] block: Drop AioContext lock in bdrv_drain_poll_top_level()
2018-08-24 7:24 ` Fam Zheng
@ 2018-09-04 14:50 ` Kevin Wolf
0 siblings, 0 replies; 13+ messages in thread
From: Kevin Wolf @ 2018-09-04 14:50 UTC (permalink / raw)
To: Fam Zheng; +Cc: qemu-block, mreitz, qemu-devel
Am 24.08.2018 um 09:24 hat Fam Zheng geschrieben:
> On Fri, 08/17 19:02, Kevin Wolf wrote:
> > Simimlar to AIO_WAIT_WHILE(), bdrv_drain_poll_top_level() needs to
> > release the AioContext lock of the node to be drained before calling
> > aio_poll(). Otherwise, callbacks called by aio_poll() would possibly
> > take the lock a second time and run into a deadlock with a nested
> > AIO_WAIT_WHILE() call.
> >
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> > block/io.c | 25 ++++++++++++++++++++++++-
> > 1 file changed, 24 insertions(+), 1 deletion(-)
> >
> > diff --git a/block/io.c b/block/io.c
> > index 7100344c7b..832d2536bf 100644
> > --- a/block/io.c
> > +++ b/block/io.c
> > @@ -268,9 +268,32 @@ bool bdrv_drain_poll(BlockDriverState *bs, bool recursive,
> > static bool bdrv_drain_poll_top_level(BlockDriverState *bs, bool recursive,
> > BdrvChild *ignore_parent)
> > {
> > + AioContext *ctx = bdrv_get_aio_context(bs);
> > +
> > + /*
> > + * We cannot easily release the lock unconditionally here because many
> > + * callers of drain function (like qemu initialisation, tools, etc.) don't
> > + * even hold the main context lock.
> > + *
> > + * This means that we fix potential deadlocks for the case where we are in
> > + * the main context and polling a BDS in a different AioContext, but
> > + * draining a BDS in the main context from a different I/O thread would
> > + * still have this problem. Fortunately, this isn't supposed to happen
> > + * anyway.
> > + */
> > + if (ctx != qemu_get_aio_context()) {
> > + aio_context_release(ctx);
> > + } else {
> > + assert(qemu_get_current_aio_context() == qemu_get_aio_context());
> > + }
> > +
> > /* Execute pending BHs first and check everything else only after the BHs
> > * have executed. */
> > - while (aio_poll(bs->aio_context, false));
> > + while (aio_poll(ctx, false));
> > +
> > + if (ctx != qemu_get_aio_context()) {
> > + aio_context_acquire(ctx);
> > + }
> >
> > return bdrv_drain_poll(bs, recursive, ignore_parent, false);
> > }
>
> The same question as patch 3: why not just use AIO_WAIT_WHILE() here? It takes
> care to not release any lock if both running and polling in the main context
> (taking the in_aio_context_home_thread() branch).
I don't think AIO_WAIT_WHILE() can be non-blocking, though?
There is also no real condition here to check. It's just polling as long
as there is activity to get the pending BH callbacks run. I'm not sure
how I could possibly write this as a AIO_WAIT_WHILE() condition.
After all, this one just doesn't feel like the right use case for
AIO_WAIT_WHILE().
Kevin
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2018-09-04 14:50 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-08-17 17:02 [Qemu-devel] [RFC PATCH 0/5] Fix some jobs/drain/aio_poll related hangs Kevin Wolf
2018-08-17 17:02 ` [Qemu-devel] [RFC PATCH 1/5] blockjob: Wake up BDS when job becomes idle Kevin Wolf
2018-08-17 17:02 ` [Qemu-devel] [RFC PATCH 2/5] tests: Acquire AioContext around job_finish_sync() Kevin Wolf
2018-08-24 7:15 ` Fam Zheng
2018-08-17 17:02 ` [Qemu-devel] [RFC PATCH 3/5] job: Drop AioContext lock around aio_poll() Kevin Wolf
2018-08-24 7:22 ` Fam Zheng
2018-09-04 14:44 ` Kevin Wolf
2018-08-17 17:02 ` [Qemu-devel] [RFC PATCH 4/5] block: Drop AioContext lock in bdrv_drain_poll_top_level() Kevin Wolf
2018-08-24 7:24 ` Fam Zheng
2018-09-04 14:50 ` Kevin Wolf
2018-08-17 17:02 ` [Qemu-devel] [RFC PATCH 5/5] [WIP] Lock AioContext in bdrv_co_drain_bh_cb() Kevin Wolf
2018-08-18 10:46 ` [Qemu-devel] [RFC PATCH 0/5] Fix some jobs/drain/aio_poll related hangs no-reply
2018-08-21 6:08 ` Fam Zheng
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.