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

[Sergio Lopez]

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? All use cases I can
find in the code are acquiring the new context:

hw/block/dataplane/xen-block.c:
 719 void xen_block_dataplane_start(XenBlockDataPlane *dataplane,
 720                                const unsigned int ring_ref[],
 721                                unsigned int nr_ring_ref,
 722                                unsigned int event_channel,
 723                                unsigned int protocol,
 724                                Error **errp)
 725 {
 ...
 811     aio_context_acquire(dataplane->ctx);
 812     /* If other users keep the BlockBackend in the iothread, that's ok */
 813     blk_set_aio_context(dataplane->blk, dataplane->ctx, NULL);
 814     /* Only reason for failure is a NULL channel */
 815     xen_device_set_event_channel_context(xendev, dataplane->event_channel,
 816                                          dataplane->ctx, &error_abort);
 817     aio_context_release(dataplane->ctx);

hw/scsi/virtio-scsi.c:
 818 static void virtio_scsi_hotplug(HotplugHandler *hotplug_dev, DeviceState 
*dev,
 819                                 Error **errp)
 820 {
 ...
 830         virtio_scsi_acquire(s);
 831         ret = blk_set_aio_context(sd->conf.blk, s->ctx, errp);
 832         virtio_scsi_release(s);

Thanks,
Sergio.

signature.asc
Description: PGP signature