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);
     } else {
         return true;
-- 
2.31.1

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