Disable notifications right before scheduling, instead of waiting until
the work is running.

After applying this patch, fio read test goes from 1191MiB/s to
1263.14Mib/s

Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
---
 fs/fuse/virtio_fs.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
index e19c78f2480e..3d6981c0f3c3 100644
--- a/fs/fuse/virtio_fs.c
+++ b/fs/fuse/virtio_fs.c
@@ -915,6 +915,8 @@ static void virtio_fs_vq_done(struct virtqueue *vq)
 
 	dev_dbg(&vq->vdev->dev, "%s %s\n", __func__, fsvq->name);
 
+	/* Shceduled, don't send more notifications */
+	virtqueue_disable_cb(vq);
 	schedule_work(&fsvq->done_work);
 }
 
-- 
2.48.1

On Sat, Feb 22, 2025 at 1:07 AM Eugenio Pérez <eperezma@redhat.com> wrote:
>
> Disable notifications right before scheduling, instead of waiting until
> the work is running.
>
> After applying this patch, fio read test goes from 1191MiB/s to
> 1263.14Mib/s

Let's describe more about the testing. E.g FIO parameters, test setups.

>
> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> ---
>  fs/fuse/virtio_fs.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
> index e19c78f2480e..3d6981c0f3c3 100644
> --- a/fs/fuse/virtio_fs.c
> +++ b/fs/fuse/virtio_fs.c
> @@ -915,6 +915,8 @@ static void virtio_fs_vq_done(struct virtqueue *vq)
>
>         dev_dbg(&vq->vdev->dev, "%s %s\n", __func__, fsvq->name);
>
> +       /* Shceduled, don't send more notifications */

Typo, and I think we can just drop this commnet.

> +       virtqueue_disable_cb(vq);

Do we need to tweak the virtio_fs_requests_done_work() as well?

>         schedule_work(&fsvq->done_work);
>  }

Thanks

>
> --
> 2.48.1
>

On Mon, Feb 24, 2025 at 3:01 AM Jason Wang <jasowang@redhat.com> wrote:
>
> On Sat, Feb 22, 2025 at 1:07 AM Eugenio Pérez <eperezma@redhat.com> wrote:
> >
> > Disable notifications right before scheduling, instead of waiting until
> > the work is running.
> >
> > After applying this patch, fio read test goes from 1191MiB/s to
> > 1263.14Mib/s
>
> Let's describe more about the testing. E.g FIO parameters, test setups.
>
> >
> > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > ---
> >  fs/fuse/virtio_fs.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
> > index e19c78f2480e..3d6981c0f3c3 100644
> > --- a/fs/fuse/virtio_fs.c
> > +++ b/fs/fuse/virtio_fs.c
> > @@ -915,6 +915,8 @@ static void virtio_fs_vq_done(struct virtqueue *vq)
> >
> >         dev_dbg(&vq->vdev->dev, "%s %s\n", __func__, fsvq->name);
> >
> > +       /* Shceduled, don't send more notifications */
>
> Typo, and I think we can just drop this commnet.

Agree.

>
> > +       virtqueue_disable_cb(vq);
>
> Do we need to tweak the virtio_fs_requests_done_work() as well?

I don't think so, as virtqueue_disable_cb can be called multiple times
without consequences.

To control it in virtio_fs_requests_done_work imply to create a new
boolean somewhere and add a conditional there, redundant with the
conditional in virtio_fs_requests_done_work.

On Mon, Feb 24, 2025 at 7:44 AM Eugenio Perez Martin
<eperezma@redhat.com> wrote:
>
> On Mon, Feb 24, 2025 at 3:01 AM Jason Wang <jasowang@redhat.com> wrote:
> >
> > On Sat, Feb 22, 2025 at 1:07 AM Eugenio Pérez <eperezma@redhat.com> wrote:
> > >
> > > Disable notifications right before scheduling, instead of waiting until
> > > the work is running.
> > >
> > > After applying this patch, fio read test goes from 1191MiB/s to
> > > 1263.14Mib/s
> >
> > Let's describe more about the testing. E.g FIO parameters, test setups.
> >
> > >
> > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > > ---
> > >  fs/fuse/virtio_fs.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > >
> > > diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
> > > index e19c78f2480e..3d6981c0f3c3 100644
> > > --- a/fs/fuse/virtio_fs.c
> > > +++ b/fs/fuse/virtio_fs.c
> > > @@ -915,6 +915,8 @@ static void virtio_fs_vq_done(struct virtqueue *vq)
> > >
> > >         dev_dbg(&vq->vdev->dev, "%s %s\n", __func__, fsvq->name);
> > >
> > > +       /* Shceduled, don't send more notifications */
> >
> > Typo, and I think we can just drop this commnet.
>
> Agree.
>
> >
> > > +       virtqueue_disable_cb(vq);
> >
> > Do we need to tweak the virtio_fs_requests_done_work() as well?
>
> I don't think so, as virtqueue_disable_cb can be called multiple times
> without consequences.
>
> To control it in virtio_fs_requests_done_work imply to create a new
> boolean somewhere and add a conditional there, redundant with the
> conditional in virtio_fs_requests_done_work.

I meant it would be redundant with the conditional in
virtqueue_disable_cb_split and _packed, as they store the avail flags
set so the operation is idempotent :).

Is it worth reordering this patch so it can be profiled and applied in
current code?