Re: [PATCH 1/2] virtio-blk: Acquire context while switching them on dataplane start

[Kevin Wolf]

Am 09.12.2020 um 17:51 hat Sergio Lopez geschrieben:
> On Mon, Dec 07, 2020 at 04:37:53PM +0100, Kevin Wolf wrote:
> > Am 04.12.2020 um 17:53 hat Sergio Lopez geschrieben:
> > > On dataplane start, acquire the new AIO context before calling
> > > 'blk_set_aio_context', releasing it immediately afterwards. This
> > > prevents reaching the AIO context attach/detach notifier functions
> > > without having acquired it first.
> > > 
> > > It was also the only place where 'blk_set_aio_context' was called with
> > > an unprotected AIO context.
> > > 
> > > Signed-off-by: Sergio Lopez <slp@redhat.com>
> > > ---
> > >  hw/block/dataplane/virtio-blk.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/hw/block/dataplane/virtio-blk.c 
> > > b/hw/block/dataplane/virtio-blk.c
> > > index 37499c5564..034e43cb1f 100644
> > > --- a/hw/block/dataplane/virtio-blk.c
> > > +++ b/hw/block/dataplane/virtio-blk.c
> > > @@ -214,7 +214,9 @@ int virtio_blk_data_plane_start(VirtIODevice *vdev)
> > >      vblk->dataplane_started = true;
> > >      trace_virtio_blk_data_plane_start(s);
> > >  
> > > +    aio_context_acquire(s->ctx);
> > >      r = blk_set_aio_context(s->conf->conf.blk, s->ctx, &local_err);
> > > +    aio_context_release(s->ctx);
> > 
> > bdrv_set_aio_context_ignore() is documented like this:
> > 
> >  * The caller must own the AioContext lock for the old AioContext of bs, 
> > but it
> >  * must not own the AioContext lock for new_context (unless new_context is 
> > the
> >  * same as the current context of bs).
> 
> Does that rule apply to blk_set_aio_context too?

bdrv_set_aio_context_ignore() is what blk_set_aio_context() calls, so I
would say yes.

> All use cases I can find in the code are acquiring the new context:
> [...]

Hm... That's unfortunate.

I think the reason why you shouldn't hold it is that the
bdrv_drained_begin() call in bdrv_set_aio_context_ignore() could
deadlock if you hold the lock of a context that is not the current
context of the BlockDriverState.

Maybe there are more reasons, I'm not sure.

Kevin

signature.asc
Description: PGP signature