On Thu, May 04, 2023 at 11:13:42PM +0200, Kevin Wolf wrote:
> Am 25.04.2023 um 19:27 hat Stefan Hajnoczi geschrieben:
> > Detach ioeventfds during drained sections to stop I/O submission from
> > the guest. virtio-blk is no longer reliant on aio_disable_external()
> > after this patch. This will allow us to remove the
> > aio_disable_external() API once all other code that relies on it is
> > converted.
> >
> > Take extra care to avoid attaching/detaching ioeventfds if the data
> > plane is started/stopped during a drained section. This should be rare,
> > but maybe the mirror block job can trigger it.
> >
> > Signed-off-by: Stefan Hajnoczi <stefa...@redhat.com>
> > ---
> > hw/block/dataplane/virtio-blk.c | 17 +++++++++------
> > hw/block/virtio-blk.c | 38 ++++++++++++++++++++++++++++++++-
> > 2 files changed, 48 insertions(+), 7 deletions(-)
> >
> > diff --git a/hw/block/dataplane/virtio-blk.c
> > b/hw/block/dataplane/virtio-blk.c
> > index bd7cc6e76b..d77fc6028c 100644
> > --- a/hw/block/dataplane/virtio-blk.c
> > +++ b/hw/block/dataplane/virtio-blk.c
> > @@ -245,13 +245,15 @@ int virtio_blk_data_plane_start(VirtIODevice *vdev)
> > }
> >
> > /* Get this show started by hooking up our callbacks */
> > - aio_context_acquire(s->ctx);
> > - for (i = 0; i < nvqs; i++) {
> > - VirtQueue *vq = virtio_get_queue(s->vdev, i);
> > + if (!blk_in_drain(s->conf->conf.blk)) {
> > + aio_context_acquire(s->ctx);
> > + for (i = 0; i < nvqs; i++) {
> > + VirtQueue *vq = virtio_get_queue(s->vdev, i);
> >
> > - virtio_queue_aio_attach_host_notifier(vq, s->ctx);
> > + virtio_queue_aio_attach_host_notifier(vq, s->ctx);
> > + }
> > + aio_context_release(s->ctx);
> > }
> > - aio_context_release(s->ctx);
> > return 0;
> >
> > fail_aio_context:
> > @@ -317,7 +319,10 @@ void virtio_blk_data_plane_stop(VirtIODevice *vdev)
> > trace_virtio_blk_data_plane_stop(s);
> >
> > aio_context_acquire(s->ctx);
> > - aio_wait_bh_oneshot(s->ctx, virtio_blk_data_plane_stop_bh, s);
> > +
> > + if (!blk_in_drain(s->conf->conf.blk)) {
> > + aio_wait_bh_oneshot(s->ctx, virtio_blk_data_plane_stop_bh, s);
> > + }
>
> So here we actually get a semantic change: What you described as the
> second part in the previous patch, processing the virtqueue one last
> time, isn't done any more if the device is drained.
>
> If it's okay to just skip this during drain, why do we need to do it
> outside of drain?Yes, it's safe because virtio_blk_data_plane_stop() has two cases: 1. The device is being reset. It is not necessary to process new requests. 2. 'stop'/'cont'. 'cont' will call virtio_blk_data_plane_start() -> event_notifier_set() so new requests will be processed when the guest resumes exection. That's why I think this is safe and the right thing to do. However, your question led me to a pre-existing drain bug when a vCPU resets the device during a drained section (e.g. when a mirror block job has started a drained section and the main loop runs until the block job exits). New requests must not be processed by virtio_blk_data_plane_stop() because that would violate drain semantics. It turns out requests are still processed because of virtio_blk_data_plane_stop() -> virtio_bus_cleanup_host_notifier() -> virtio_queue_host_notifier_read(). I think that should be handled in a separate patch series. It's not related to aio_disable_external(). Stefan
signature.asc
Description: PGP signature
