[PATCH] virtio-blk: avoid using ioeventfd state in irqfd conditional

[PATCH] virtio-blk: avoid using ioeventfd state in irqfd conditional

[Stefan Hajnoczi]

Requests that complete in an IOThread use irqfd to notify the guest
while requests that complete in the main loop thread use the traditional
qdev irq code path. The reason for this conditional is that the irq code
path requires the BQL:

  if (s->ioeventfd_started && !s->ioeventfd_disabled) {
      virtio_notify_irqfd(vdev, req->vq);
  } else {
      virtio_notify(vdev, req->vq);
  }

There is a corner case where the conditional invokes the irq code path
instead of the irqfd code path:

  static void virtio_blk_stop_ioeventfd(VirtIODevice *vdev)
  {
      ...
      /*
       * Set ->ioeventfd_started to false before draining so that host notifiers
       * are not detached/attached anymore.
       */
      s->ioeventfd_started = false;

      /* Wait for virtio_blk_dma_restart_bh() and in flight I/O to complete */
      blk_drain(s->conf.conf.blk);

During blk_drain() the conditional produces the wrong result because
ioeventfd_started is false.

Use qemu_in_iothread() instead of checking the ioeventfd state.

Buglink: https://issues.redhat.com/browse/RHEL-15394
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
Based-on: https://repo.or.cz/qemu/kevin.git/shortlog/c14962c3ea6f0998d028142ed14affcb9dfccf28

Stable backport notes: dataplane_started is being renamed to
ioeventfd_started in the next block pull request. This patch can be
safely applied to -stable although the variable name has changed and
git-am will complain.

 hw/block/virtio-blk.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 227d83569f..287c31ee3c 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -64,7 +64,7 @@ static void virtio_blk_req_complete(VirtIOBlockReq *req, unsigned char status)
     iov_discard_undo(&req->inhdr_undo);
     iov_discard_undo(&req->outhdr_undo);
     virtqueue_push(req->vq, &req->elem, req->in_len);
-    if (s->ioeventfd_started && !s->ioeventfd_disabled) {
+    if (qemu_in_iothread()) {
         virtio_notify_irqfd(vdev, req->vq);
     } else {
         virtio_notify(vdev, req->vq);
-- 
2.43.0

Re: [PATCH] virtio-blk: avoid using ioeventfd state in irqfd conditional

[Stefan Hajnoczi]

On Mon, 22 Jan 2024 at 12:27, Stefan Hajnoczi <stefanha@redhat.com> wrote:
>
> Requests that complete in an IOThread use irqfd to notify the guest
> while requests that complete in the main loop thread use the traditional
> qdev irq code path. The reason for this conditional is that the irq code
> path requires the BQL:
>
>   if (s->ioeventfd_started && !s->ioeventfd_disabled) {
>       virtio_notify_irqfd(vdev, req->vq);
>   } else {
>       virtio_notify(vdev, req->vq);
>   }
>
> There is a corner case where the conditional invokes the irq code path
> instead of the irqfd code path:
>
>   static void virtio_blk_stop_ioeventfd(VirtIODevice *vdev)
>   {
>       ...
>       /*
>        * Set ->ioeventfd_started to false before draining so that host notifiers
>        * are not detached/attached anymore.
>        */
>       s->ioeventfd_started = false;
>
>       /* Wait for virtio_blk_dma_restart_bh() and in flight I/O to complete */
>       blk_drain(s->conf.conf.blk);
>
> During blk_drain() the conditional produces the wrong result because
> ioeventfd_started is false.
>
> Use qemu_in_iothread() instead of checking the ioeventfd state.
>
> Buglink: https://issues.redhat.com/browse/RHEL-15394
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---

Ping?

> Based-on: https://repo.or.cz/qemu/kevin.git/shortlog/c14962c3ea6f0998d028142ed14affcb9dfccf28
>
> Stable backport notes: dataplane_started is being renamed to
> ioeventfd_started in the next block pull request. This patch can be
> safely applied to -stable although the variable name has changed and
> git-am will complain.
>
>  hw/block/virtio-blk.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index 227d83569f..287c31ee3c 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -64,7 +64,7 @@ static void virtio_blk_req_complete(VirtIOBlockReq *req, unsigned char status)
>      iov_discard_undo(&req->inhdr_undo);
>      iov_discard_undo(&req->outhdr_undo);
>      virtqueue_push(req->vq, &req->elem, req->in_len);
> -    if (s->ioeventfd_started && !s->ioeventfd_disabled) {
> +    if (qemu_in_iothread()) {
>          virtio_notify_irqfd(vdev, req->vq);
>      } else {
>          virtio_notify(vdev, req->vq);
> --
> 2.43.0
>
>

Re: [PATCH] virtio-blk: avoid using ioeventfd state in irqfd conditional

[Kevin Wolf]

Am 22.01.2024 um 18:26 hat Stefan Hajnoczi geschrieben:
> Requests that complete in an IOThread use irqfd to notify the guest
> while requests that complete in the main loop thread use the traditional
> qdev irq code path. The reason for this conditional is that the irq code
> path requires the BQL:
> 
>   if (s->ioeventfd_started && !s->ioeventfd_disabled) {
>       virtio_notify_irqfd(vdev, req->vq);
>   } else {
>       virtio_notify(vdev, req->vq);
>   }
> 
> There is a corner case where the conditional invokes the irq code path
> instead of the irqfd code path:
> 
>   static void virtio_blk_stop_ioeventfd(VirtIODevice *vdev)
>   {
>       ...
>       /*
>        * Set ->ioeventfd_started to false before draining so that host notifiers
>        * are not detached/attached anymore.
>        */
>       s->ioeventfd_started = false;
> 
>       /* Wait for virtio_blk_dma_restart_bh() and in flight I/O to complete */
>       blk_drain(s->conf.conf.blk);
> 
> During blk_drain() the conditional produces the wrong result because
> ioeventfd_started is false.
> 
> Use qemu_in_iothread() instead of checking the ioeventfd state.
> 
> Buglink: https://issues.redhat.com/browse/RHEL-15394
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

Thanks, applied to the block branch.

Kevin