[PULL 0/1] Block patches

[PULL 0/1] Block patches

[Stefan Hajnoczi]

The following changes since commit 39a6e4f87e7b75a45b08d6dc8b8b7c2954c87440:

  Merge tag 'pull-qapi-2024-02-03' of https://repo.or.cz/qemu/armbru into staging (2024-02-03 13:31:58 +0000)

are available in the Git repository at:

  https://gitlab.com/stefanha/qemu.git tags/block-pull-request

for you to fetch changes up to 1d52cc0ac27761e296b14655c2f5b2649ee69491:

  virtio-blk: avoid using ioeventfd state in irqfd conditional (2024-02-06 10:22:18 -0500)

----------------------------------------------------------------
Pull request

A bug fix for in-flight I/O during ioeventfd shutdown.

----------------------------------------------------------------

Stefan Hajnoczi (1):
  virtio-blk: avoid using ioeventfd state in irqfd conditional

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

-- 
2.43.0

[PULL 1/1] 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>
Message-id: 20240122172625.415386-1-stefanha@redhat.com
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 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: [PULL 0/1] Block patches

[Kevin Wolf]

Am 06.02.2024 um 16:31 hat Stefan Hajnoczi geschrieben:
> The following changes since commit 39a6e4f87e7b75a45b08d6dc8b8b7c2954c87440:
> 
>   Merge tag 'pull-qapi-2024-02-03' of https://repo.or.cz/qemu/armbru into staging (2024-02-03 13:31:58 +0000)
> 
> are available in the Git repository at:
> 
>   https://gitlab.com/stefanha/qemu.git tags/block-pull-request
> 
> for you to fetch changes up to 1d52cc0ac27761e296b14655c2f5b2649ee69491:
> 
>   virtio-blk: avoid using ioeventfd state in irqfd conditional (2024-02-06 10:22:18 -0500)
> 
> ----------------------------------------------------------------
> Pull request
> 
> A bug fix for in-flight I/O during ioeventfd shutdown.
> 
> ----------------------------------------------------------------
> 
> Stefan Hajnoczi (1):
>   virtio-blk: avoid using ioeventfd state in irqfd conditional

I noticed that this patch is also in the pull request I sent, so I
guess if mine goes through, you don't have to process this one
separately.

Kevin

Re: [PULL 1/1] virtio-blk: avoid using ioeventfd state in irqfd conditional

[Michael Tokarev]

06.02.2024 18:31, 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>
> Message-id: 20240122172625.415386-1-stefanha@redhat.com
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

Cc qemu-stable?  This smells like a stable material, please let me know
if it is not.

(And yes I've seen it also included in another pullreq)

Thanks,

/mjt

Re: [PULL 1/1] virtio-blk: avoid using ioeventfd state in irqfd conditional

[Kevin Wolf]

Am 08.02.2024 um 06:37 hat Michael Tokarev geschrieben:
> 06.02.2024 18:31, 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>
> > Message-id: 20240122172625.415386-1-stefanha@redhat.com
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> 
> Cc qemu-stable?  This smells like a stable material, please let me know
> if it is not.

The patch email itself was CCed to qemu-stable and even contained a note
for backporting to stable:

https://lists.gnu.org/archive/html/qemu-block/2024-01/msg00278.html

It's only missing in the commit message. I'll add the Cc: line to
my pull request (for Stefan's pull request it seems too late because
Peter is already processing it, so we'll probably end up having both
versions in the git history).

Kevin

Re: [PULL 1/1] virtio-blk: avoid using ioeventfd state in irqfd conditional

[Michael Tokarev]

08.02.2024 11:42, Kevin Wolf wrote:

> The patch email itself was CCed to qemu-stable and even contained a note
> for backporting to stable:
> 
> https://lists.gnu.org/archive/html/qemu-block/2024-01/msg00278.html

Ahh. Yes.  I'm having a large(ish) queue in stable and missed the fact
I already has this one in there.  Was a -ENOCOFFEE issue this morning.
Sorry for the noise.

> It's only missing in the commit message. I'll add the Cc: line to
> my pull request (for Stefan's pull request it seems too late because
> Peter is already processing it, so we'll probably end up having both
> versions in the git history).

That's ok I guess :)

Thanks,

/mjt