| Message ID | 20220301142113.163174-5-eesposit@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Removal of AioContext lock, bs->parents and ->children: proof of concept | expand |
On Tue, Mar 01, 2022 at 09:21:12AM -0500, Emanuele Giuseppe Esposito wrote: > drv->drained_poll() is only implemented in mirror, and allows > it to drain from the coroutine. The mirror implementation uses > in_drain flag to recognize when it is draining from coroutine, > and consequently avoid deadlocking (wait the poll condition in > child_job_drained_poll to wait for itself). > > The problem is that this flag is dangerous, because it breaks > bdrv_drained_begin() invariants: once drained_begin ends, all > jobs, in_flight requests, and anything running in the iothread > are blocked. > > This can be broken in such way: > iothread(mirror): s->in_drain = true; // mirror.c:1112 > main loop: bdrv_drained_begin(mirror_bs); > /* > * drained_begin wait for bdrv_drain_poll_top_level() condition, > * that translates in child_job_drained_poll() for jobs, but > * mirror implements drv->drained_poll() so it returns > * !!in_flight_requests, which his 0 (assertion in mirror.c:1105). > */ > main loop: thinks iothread is stopped and is modifying the graph... > iothread(mirror): *continues*, as nothing is stopping it > iothread(mirror): bdrv_drained_begin(bs); > /* draining reads the graph while it is modified!! */ > main loop: done modifying the graph... > > In order to fix this, we can simply allow drv->drained_poll() > to be called only by the iothread, and not the main loop. > We distinguish it by using in_aio_context_home_thread(), that > returns false if @ctx is not the same as the thread that runs it. > > Co-Developed-by: Paolo Bonzini <pbonzini@redhat.com> > Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> > --- > blockjob.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/blockjob.c b/blockjob.c > index 10815a89fe..e132d9587e 100644 > --- a/blockjob.c > +++ b/blockjob.c > @@ -107,6 +107,7 @@ static bool child_job_drained_poll(BdrvChild *c) > BlockJob *bjob = c->opaque; > Job *job = &bjob->job; > const BlockJobDriver *drv = block_job_driver(bjob); > + AioContext *ctx = block_job_get_aio_context(bjob); > > /* An inactive or completed job doesn't have any pending requests. Jobs > * with !job->busy are either already paused or have a pause point after > @@ -117,7 +118,7 @@ static bool child_job_drained_poll(BdrvChild *c) > > /* Otherwise, assume that it isn't fully stopped yet, but allow the job to > * override this assumption. */ > - if (drv->drained_poll) { > + if (in_aio_context_home_thread(ctx) && drv->drained_poll) { > return drv->drained_poll(bjob); The purpose of in_aio_context_home_thread() is not obvious from the code. Please add a comment explaining this case. Thanks, Stefan
diff --git a/blockjob.c b/blockjob.c index 10815a89fe..e132d9587e 100644 --- a/blockjob.c +++ b/blockjob.c @@ -107,6 +107,7 @@ static bool child_job_drained_poll(BdrvChild *c) BlockJob *bjob = c->opaque; Job *job = &bjob->job; const BlockJobDriver *drv = block_job_driver(bjob); + AioContext *ctx = block_job_get_aio_context(bjob); /* An inactive or completed job doesn't have any pending requests. Jobs * with !job->busy are either already paused or have a pause point after @@ -117,7 +118,7 @@ static bool child_job_drained_poll(BdrvChild *c) /* Otherwise, assume that it isn't fully stopped yet, but allow the job to * override this assumption. */ - if (drv->drained_poll) { + if (in_aio_context_home_thread(ctx) && drv->drained_poll) { return drv->drained_poll(bjob); } else { return true;
drv->drained_poll() is only implemented in mirror, and allows it to drain from the coroutine. The mirror implementation uses in_drain flag to recognize when it is draining from coroutine, and consequently avoid deadlocking (wait the poll condition in child_job_drained_poll to wait for itself). The problem is that this flag is dangerous, because it breaks bdrv_drained_begin() invariants: once drained_begin ends, all jobs, in_flight requests, and anything running in the iothread are blocked. This can be broken in such way: iothread(mirror): s->in_drain = true; // mirror.c:1112 main loop: bdrv_drained_begin(mirror_bs); /* * drained_begin wait for bdrv_drain_poll_top_level() condition, * that translates in child_job_drained_poll() for jobs, but * mirror implements drv->drained_poll() so it returns * !!in_flight_requests, which his 0 (assertion in mirror.c:1105). */ main loop: thinks iothread is stopped and is modifying the graph... iothread(mirror): *continues*, as nothing is stopping it iothread(mirror): bdrv_drained_begin(bs); /* draining reads the graph while it is modified!! */ main loop: done modifying the graph... In order to fix this, we can simply allow drv->drained_poll() to be called only by the iothread, and not the main loop. We distinguish it by using in_aio_context_home_thread(), that returns false if @ctx is not the same as the thread that runs it. Co-Developed-by: Paolo Bonzini <pbonzini@redhat.com> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> --- blockjob.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)