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

[Kevin Wolf]

Am 07.04.2017 um 01:44 hat Fam Zheng geschrieben:
> 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()?

Somehow I thought that aio_poll() doesn't run timers, but that seems to
be wrong. I think in the pre-AioContext world it used to be this way.

Okay, let's leave this patch as it is. We can then clean things up in
the 2.10 timeframe and propagate things cleanly upwards through the
BlockBackend to its users. We want to this for drain anyway.

Hm, maybe we should already call bdrv_parent_drained_begin/end? I don't
think that image throttling can submit new requests while we're changing
the AioContext (because it probably doesn't have any at that point), but
I'm not completely sure.

Kevin