Re: [Qemu-devel] [PATCH for-2.9 3/5] block: Quiesce old aio context during bdrv_set_aio_context

[Fam Zheng]

On Thu, 04/06 17:17, Kevin Wolf wrote:
> Am 06.04.2017 um 16:25 hat Fam Zheng geschrieben:
> > The fact that the bs->aio_context is changing can confuse the dataplane
> > iothread, because of the now fine granularity aio context lock.
> > bdrv_drain should rather be a bdrv_drained_begin/end pair, but since
> > bs->aio_context is changing, we can just use aio_disable_external and
> > block_job_pause.
> > 
> > Reported-by: Ed Swierk <address@hidden>
> > Signed-off-by: Fam Zheng <address@hidden>
> > ---
> >  block.c | 11 +++++++++--
> >  1 file changed, 9 insertions(+), 2 deletions(-)
> > 
> > diff --git a/block.c b/block.c
> > index 8893ac1..e70684a 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -4395,11 +4395,14 @@ void bdrv_attach_aio_context(BlockDriverState *bs,
> >  
> >  void bdrv_set_aio_context(BlockDriverState *bs, AioContext *new_context)
> >  {
> > -    AioContext *ctx;
> > +    AioContext *ctx = bdrv_get_aio_context(bs);
> >  
> > +    aio_disable_external(ctx);
> > +    if (bs->job) {
> > +        block_job_pause(bs->job);
> > +    }
> 
> Even more bs->job users... :-(
> 
> But is this one actually necessary? We drain all pending BHs below, so
> the job shouldn't have any requests in flight or be able to submit new
> ones while we switch.

I'm not 100% sure, but I think the aio_poll() loop below can still fire
co_sleep_cb if we don't do block_job_pause()?

> 
> >      bdrv_drain(bs); /* ensure there are no in-flight requests */
> >  
> > -    ctx = bdrv_get_aio_context(bs);
> >      while (aio_poll(ctx, false)) {
> >          /* wait for all bottom halves to execute */
> >      }
> > @@ -4412,6 +4415,10 @@ void bdrv_set_aio_context(BlockDriverState *bs, 
> > AioContext *new_context)
> >      aio_context_acquire(new_context);
> >      bdrv_attach_aio_context(bs, new_context);
> >      aio_context_release(new_context);
> > +    if (bs->job) {
> > +        block_job_resume(bs->job);
> > +    }
> > +    aio_enable_external(ctx);
> >  }
> 
> The aio_disabe/enable_external() pair seems to make sense anyway.
> 
> Kevin