Re: [Qemu-devel] [PATCH v2 2/4] block: Pause block jobs in bdrv_drain_all

[Stefan Hajnoczi]

On Fri, Apr 03, 2015 at 10:05:19PM +0800, Fam Zheng wrote:
> This is necessary to suppress more IO requests from being generated from
> block job coroutines.
> 
> Signed-off-by: Fam Zheng <address@hidden>
> ---
>  block.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/block.c b/block.c
> index f2f8ae7..00cd91e 100644
> --- a/block.c
> +++ b/block.c
> @@ -2033,6 +2033,16 @@ void bdrv_drain_all(void)
>      bool busy = true;
>      BlockDriverState *bs;
>  
> +    QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
> +        AioContext *aio_context = bdrv_get_aio_context(bs);
> +
> +        aio_context_acquire(aio_context);
> +        if (bs->job) {
> +            block_job_pause(bs->job);
> +        }
> +        aio_context_release(aio_context);
> +    }
> +
>      while (busy) {
>          busy = false;
>  
> @@ -2044,6 +2054,16 @@ void bdrv_drain_all(void)
>              aio_context_release(aio_context);
>          }
>      }
> +
> +    QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
> +        AioContext *aio_context = bdrv_get_aio_context(bs);
> +
> +        aio_context_acquire(aio_context);
> +        if (bs->job) {
> +            block_job_resume(bs->job);
> +        }
> +        aio_context_release(aio_context);
> +    }
>  }

There is a tiny chance that we pause a job (which actually just sets
job->paused = true but there's no guarantee the job's coroutine reacts
to this) right before it terminates.  Then aio_poll() enters the
coroutine one last time and the job terminates.

We then reach the resume portion of bdrv_drain_all() and the job no
longer exists.  Hopefully nothing started a new job in the meantime.
bs->job should either be the original block job or NULL.

This code seems under current assumptions, but I just wanted to raise
these issues in case someone sees problems that I've missed.

Stefan

pgp8LFrkSS7pE.pgp
Description: PGP signature