| Message ID | 20201204165347.73542-2-slp@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] virtio-blk: Acquire context while switching them on dataplane start | expand |
On 12/4/20 10:53 AM, Sergio Lopez wrote: > 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(+) Reviewed-by: Eric Blake <eblake@redhat.com> I'll queue through my NBD tree, but will wait a couple days to see if other block developers want to add review comments. > > 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); > if (r < 0) { > error_report_err(local_err); > goto fail_guest_notifiers; >
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). Doesn't this patch lock the new context instead of the old one? Kevin
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.
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
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); if (r < 0) { error_report_err(local_err); goto fail_guest_notifiers;
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(+)