Commit d3f6f294aeadd5f88caf0155e4360808c95b3146 ("virtio-blk: always set
ioeventfd during startup") has made virtio_blk_start_ioeventfd() always
kick the virtqueue (set the ioeventfd), regardless of whether the BB is
drained.  That is no longer necessary, because attaching the host
notifier will now set the ioeventfd, too; this happens either
immediately right here in virtio_blk_start_ioeventfd(), or later when
the drain ends, in virtio_blk_ioeventfd_attach().

With event_notifier_set() removed, the code becomes the same as the one
in virtio_blk_ioeventfd_attach(), so we can reuse that function.

Signed-off-by: Hanna Czenczek <hreitz@redhat.com>
---
 hw/block/virtio-blk.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 227d83569f..22b8eef69b 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -37,6 +37,8 @@
 #include "hw/virtio/virtio-blk-common.h"
 #include "qemu/coroutine.h"
 
+static void virtio_blk_ioeventfd_attach(VirtIOBlock *s);
+
 static void virtio_blk_init_request(VirtIOBlock *s, VirtQueue *vq,
                                     VirtIOBlockReq *req)
 {
@@ -1808,17 +1810,14 @@ static int virtio_blk_start_ioeventfd(VirtIODevice *vdev)
     s->ioeventfd_started = true;
     smp_wmb(); /* paired with aio_notify_accept() on the read side */
 
-    /* Get this show started by hooking up our callbacks */
-    for (i = 0; i < nvqs; i++) {
-        VirtQueue *vq = virtio_get_queue(vdev, i);
-        AioContext *ctx = s->vq_aio_context[i];
-
-        /* Kick right away to begin processing requests already in vring */
-        event_notifier_set(virtio_queue_get_host_notifier(vq));
-
-        if (!blk_in_drain(s->conf.conf.blk)) {
-            virtio_queue_aio_attach_host_notifier(vq, ctx);
-        }
+    /*
+     * Get this show started by hooking up our callbacks.  If drained now,
+     * virtio_blk_drained_end() will do this later.
+     * Attaching the notifier also kicks the virtqueues, processing any requests
+     * they may already have.
+     */
+    if (!blk_in_drain(s->conf.conf.blk)) {
+        virtio_blk_ioeventfd_attach(s);
     }
     return 0;
 
-- 
2.43.0

02.02.2024 18:31, Hanna Czenczek :
> Commit d3f6f294aeadd5f88caf0155e4360808c95b3146 ("virtio-blk: always set
> ioeventfd during startup") has made virtio_blk_start_ioeventfd() always
> kick the virtqueue (set the ioeventfd), regardless of whether the BB is
> drained.  That is no longer necessary, because attaching the host
> notifier will now set the ioeventfd, too; this happens either
> immediately right here in virtio_blk_start_ioeventfd(), or later when
> the drain ends, in virtio_blk_ioeventfd_attach().
> 
> With event_notifier_set() removed, the code becomes the same as the one
> in virtio_blk_ioeventfd_attach(), so we can reuse that function.

The mentioned comit is v8.2.0-812-gd3f6f294ae, - ie, past 8.2.
Is this new change still relevant for stable?

Thanks,

/mjt

On 09.02.24 15:38, Michael Tokarev wrote:
> 02.02.2024 18:31, Hanna Czenczek :
>> Commit d3f6f294aeadd5f88caf0155e4360808c95b3146 ("virtio-blk: always set
>> ioeventfd during startup") has made virtio_blk_start_ioeventfd() always
>> kick the virtqueue (set the ioeventfd), regardless of whether the BB is
>> drained.  That is no longer necessary, because attaching the host
>> notifier will now set the ioeventfd, too; this happens either
>> immediately right here in virtio_blk_start_ioeventfd(), or later when
>> the drain ends, in virtio_blk_ioeventfd_attach().
>>
>> With event_notifier_set() removed, the code becomes the same as the one
>> in virtio_blk_ioeventfd_attach(), so we can reuse that function.
>
> The mentioned comit is v8.2.0-812-gd3f6f294ae, - ie, past 8.2.
> Is this new change still relevant for stable?

Sorry again. :/  This patch is a clean-up patch that won’t apply to 
8.2.  Now, 8.2 does have basically the same logic as described in the 
patch message (d3f6f294aea restored it after it was broken), so a 
similar patch could be made for it (removing the event_notifier_set() 
from virtio_blk_data_plane_start()), but whether we kick the virtqueues 
once or twice on start-up probably won’t make a difference, certainly 
not in terms of correctness.

Hanna

09.02.2024 20:11, Hanna Czenczek :

>> The mentioned comit is v8.2.0-812-gd3f6f294ae, - ie, past 8.2.
>> Is this new change still relevant for stable?
> 
> Sorry again. :/ 

There's nothing to be sorry about here - it's regular work, and is quite
good at it, - I just asked to be sure, maybe I misunderstood something.

>     This patch is a clean-up patch that won’t apply to 8.2.  Now, 8.2 does have basically the same logic as described in the patch 
> message (d3f6f294aea restored it after it was broken), so a similar patch could be made for it (removing the event_notifier_set() from 
> virtio_blk_data_plane_start()), but whether we kick the virtqueues once or twice on start-up probably won’t make a difference, certainly not in terms 
> of correctness.

Ok, excellent, this makes good sense now.
I'm not including this one in stable-8.2 :)

Thank you very much for the excellent work and
the clarification!

/mjt