[RFC v2 0/5] virtiofs: map buffer out of virtqueue lock

[RFC v2 0/5] virtiofs: map buffer out of virtqueue lock

[Eugenio Pérez]

This is useful for some setups like swiotlb or VDUSE where the DMA
operations are expensive and/or need to be performed with a write lock.

After applying these patches, fio read test goes from 1124MiB/s to
1263.14MiB/s.

v2:
* Follow current add_premapped virtio API
* Disable notification more aggressive too.

---
Sending this series to obtain feedback if this is the right way to go &
to profile it properly.

TODO: Profile multiqueue.
TODO: Handling errors.
TODO: Do the same for hiprio queue.
TODO: Can we batch maps? virtiofs always sends many buffers.

Eugenio Pérez (5):
  vduse: add virtio_fs to allowed dev id
  virtiofs: Move stack sg to fuse_req
  virtio_ring: add api for premapped out and in buffer chain
  virtiofs: perform DMA operations out of the spinlock
  virtiofs: Disable notifications more aggresively

 drivers/vdpa/vdpa_user/vduse_dev.c |  1 +
 drivers/virtio/virtio_ring.c       | 35 ++++++++++++
 fs/fuse/fuse_i.h                   |  7 +++
 fs/fuse/virtio_fs.c                | 85 +++++++++++++++++++++---------
 include/linux/virtio.h             |  7 +++
 5 files changed, 110 insertions(+), 25 deletions(-)

-- 
2.48.1

[RFC v2 1/5] vduse: add virtio_fs to allowed dev id

[Eugenio Pérez]

A VDUSE device that implements virtiofs device works fine just by
adding the device id to the whitelist.

Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
Acked-by: Jason Wang <jasowang@redhat.com>
---
 drivers/vdpa/vdpa_user/vduse_dev.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
index 7ae99691efdf..6a9a37351310 100644
--- a/drivers/vdpa/vdpa_user/vduse_dev.c
+++ b/drivers/vdpa/vdpa_user/vduse_dev.c
@@ -144,6 +144,7 @@ static struct workqueue_struct *vduse_irq_bound_wq;
 static u32 allowed_device_id[] = {
 	VIRTIO_ID_BLOCK,
 	VIRTIO_ID_NET,
+	VIRTIO_ID_FS,
 };
 
 static inline struct vduse_dev *vdpa_to_vduse(struct vdpa_device *vdpa)
-- 
2.48.1

[RFC v2 2/5] virtiofs: Move stack sg to fuse_req

[Eugenio Pérez]

We need to move the map and unmap out of virtiofs queue lock.  We need to
store the unmap sgs to call virtqueue_unmap_sgs, so use the request
itself.

TODO: We need to store the forget sgs too.  In my tests it is not called
so we're safe.  Find a way to force-call it.

Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
---
 fs/fuse/fuse_i.h    |  7 +++++++
 fs/fuse/virtio_fs.c | 45 +++++++++++++++++++++------------------------
 2 files changed, 28 insertions(+), 24 deletions(-)

diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
index fee96fe7887b..0a77e28d8da6 100644
--- a/fs/fuse/fuse_i.h
+++ b/fs/fuse/fuse_i.h
@@ -18,6 +18,7 @@
 #include <linux/mount.h>
 #include <linux/wait.h>
 #include <linux/list.h>
+#include <linux/scatterlist.h>
 #include <linux/spinlock.h>
 #include <linux/mm.h>
 #include <linux/backing-dev.h>
@@ -434,6 +435,12 @@ struct fuse_req {
 #if IS_ENABLED(CONFIG_VIRTIO_FS)
 	/** virtio-fs's physically contiguous buffer for in and out args */
 	void *argbuf;
+
+	/** virtio-fs's pre-mapped stuff */
+	struct scatterlist sg_inline_data[6]; /* optimization for short requests */
+	struct scatterlist *sg;
+	unsigned int out_sgs;
+	unsigned int in_sgs;
 #endif
 
 	/** fuse_mount this request belongs to */
diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
index 82afe78ec542..1344c5782a7c 100644
--- a/fs/fuse/virtio_fs.c
+++ b/fs/fuse/virtio_fs.c
@@ -1377,14 +1377,10 @@ static int virtio_fs_enqueue_req(struct virtio_fs_vq *fsvq,
 {
 	/* requests need at least 4 elements */
 	struct scatterlist *stack_sgs[6];
-	struct scatterlist stack_sg[ARRAY_SIZE(stack_sgs)];
 	struct scatterlist **sgs = stack_sgs;
-	struct scatterlist *sg = stack_sg;
 	struct virtqueue *vq;
 	struct fuse_args *args = req->args;
 	unsigned int argbuf_used = 0;
-	unsigned int out_sgs = 0;
-	unsigned int in_sgs = 0;
 	unsigned int total_sgs;
 	unsigned int i;
 	int ret;
@@ -1392,11 +1388,13 @@ static int virtio_fs_enqueue_req(struct virtio_fs_vq *fsvq,
 	struct fuse_pqueue *fpq;
 
 	/* Does the sglist fit on the stack? */
+	/* TODO replace magic 6 by a macro */
+	req->sg = req->sg_inline_data;
 	total_sgs = sg_count_fuse_req(req);
-	if (total_sgs > ARRAY_SIZE(stack_sgs)) {
+	if (total_sgs > 6) {
 		sgs = kmalloc_array(total_sgs, sizeof(sgs[0]), gfp);
-		sg = kmalloc_array(total_sgs, sizeof(sg[0]), gfp);
-		if (!sgs || !sg) {
+		req->sg = kmalloc_array(total_sgs, sizeof(req->sg[0]), gfp);
+		if (!sgs || !req->sg) {
 			ret = -ENOMEM;
 			goto out;
 		}
@@ -1408,26 +1406,25 @@ static int virtio_fs_enqueue_req(struct virtio_fs_vq *fsvq,
 		goto out;
 
 	/* Request elements */
-	sg_init_one(&sg[out_sgs++], &req->in.h, sizeof(req->in.h));
-	out_sgs += sg_init_fuse_args(&sg[out_sgs], req,
-				     (struct fuse_arg *)args->in_args,
-				     args->in_numargs, args->in_pages,
-				     req->argbuf, &argbuf_used);
+	sg_init_one(&req->sg[req->out_sgs++], &req->in.h, sizeof(req->in.h));
+	req->out_sgs += sg_init_fuse_args(&req->sg[req->out_sgs], req,
+					  (struct fuse_arg *)args->in_args,
+					  args->in_numargs, args->in_pages,
+					  req->argbuf, &argbuf_used);
 
 	/* Reply elements */
 	if (test_bit(FR_ISREPLY, &req->flags)) {
-		sg_init_one(&sg[out_sgs + in_sgs++],
+		sg_init_one(&req->sg[req->out_sgs + req->in_sgs++],
 			    &req->out.h, sizeof(req->out.h));
-		in_sgs += sg_init_fuse_args(&sg[out_sgs + in_sgs], req,
-					    args->out_args, args->out_numargs,
-					    args->out_pages,
-					    req->argbuf + argbuf_used, NULL);
+		req->in_sgs += sg_init_fuse_args(&req->sg[req->out_sgs + req->in_sgs], req,
+						 args->out_args, args->out_numargs,
+						 args->out_pages,
+						 req->argbuf + argbuf_used, NULL);
 	}
 
-	WARN_ON(out_sgs + in_sgs != total_sgs);
-
 	for (i = 0; i < total_sgs; i++)
-		sgs[i] = &sg[i];
+		sgs[i] = &req->sg[i];
+	WARN_ON(req->out_sgs + req->in_sgs != total_sgs);
 
 	spin_lock(&fsvq->lock);
 
@@ -1438,7 +1435,7 @@ static int virtio_fs_enqueue_req(struct virtio_fs_vq *fsvq,
 	}
 
 	vq = fsvq->vq;
-	ret = virtqueue_add_sgs(vq, sgs, out_sgs, in_sgs, req, GFP_ATOMIC);
+	ret = virtqueue_add_sgs(vq, sgs, req->out_sgs, req->in_sgs, req, GFP_ATOMIC);
 	if (ret < 0) {
 		spin_unlock(&fsvq->lock);
 		goto out;
@@ -1467,10 +1464,10 @@ static int virtio_fs_enqueue_req(struct virtio_fs_vq *fsvq,
 		kfree(req->argbuf);
 		req->argbuf = NULL;
 	}
-	if (sgs != stack_sgs) {
+	if (ret < 0 && req->sg != req->sg_inline_data)
+		kfree(req->sg);
+	if (sgs != stack_sgs)
 		kfree(sgs);
-		kfree(sg);
-	}
 
 	return ret;
 }
-- 
2.48.1

[RFC v2 3/5] virtio_ring: add api for premapped out and in buffer chain

[Eugenio Pérez]

Commit 3ef66af31fea ("virtio_ring: introduce add api for premapped")
add functions to add premapped input or output chains to a virtqueue.
Add a generic one that supports out+in buffers chains.

Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
---
 drivers/virtio/virtio_ring.c | 35 +++++++++++++++++++++++++++++++++++
 include/linux/virtio.h       |  7 +++++++
 2 files changed, 42 insertions(+)

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index fdd2d2b07b5a..7ef1f37e025f 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -2318,6 +2318,41 @@ int virtqueue_add_sgs(struct virtqueue *_vq,
 }
 EXPORT_SYMBOL_GPL(virtqueue_add_sgs);
 
+/**
+ * virtqueue_add_sgs_premapped - expose buffers to other end
+ * @_vq: the struct virtqueue we're talking about.
+ * @sgs: array of terminated scatterlists.
+ * @out_sgs: the number of scatterlists readable by other side
+ * @in_sgs: the number of scatterlists which are writable (after readable ones)
+ * @data: the token identifying the buffer.
+ * @gfp: how to do memory allocations (if necessary).
+ *
+ * Caller must ensure we don't call this with other virtqueue operations
+ * at the same time (except where noted).
+ *
+ * Returns zero or a negative error (ie. ENOSPC, ENOMEM, EIO).
+ */
+int virtqueue_add_sgs_premapped(struct virtqueue *_vq,
+				struct scatterlist *sgs[],
+				unsigned int out_sgs,
+				unsigned int in_sgs,
+				void *data,
+				gfp_t gfp)
+{
+	unsigned int i, total_sg = 0;
+
+	/* Count them first. */
+	for (i = 0; i < out_sgs + in_sgs; i++) {
+		struct scatterlist *sg;
+
+		for (sg = sgs[i]; sg; sg = sg_next(sg))
+			total_sg++;
+	}
+	return virtqueue_add(_vq, sgs, total_sg, out_sgs, in_sgs,
+			     data, NULL, true, gfp);
+}
+EXPORT_SYMBOL_GPL(virtqueue_add_sgs_premapped);
+
 /**
  * virtqueue_add_outbuf - expose output buffers to other end
  * @vq: the struct virtqueue we're talking about.
diff --git a/include/linux/virtio.h b/include/linux/virtio.h
index 4d16c13d0df5..7f61f66ddaa2 100644
--- a/include/linux/virtio.h
+++ b/include/linux/virtio.h
@@ -74,6 +74,13 @@ int virtqueue_add_sgs(struct virtqueue *vq,
 		      void *data,
 		      gfp_t gfp);
 
+int virtqueue_add_sgs_premapped(struct virtqueue *vq,
+				struct scatterlist *sgs[],
+				unsigned int out_sgs,
+				unsigned int in_sgs,
+				void *data,
+				gfp_t gfp);
+
 struct device *virtqueue_dma_dev(struct virtqueue *vq);
 
 bool virtqueue_kick(struct virtqueue *vq);
-- 
2.48.1

[RFC v2 4/5] virtiofs: perform DMA operations out of the spinlock

[Eugenio Pérez]

This is useful for some setups like swiotlb or VDUSE where the DMA
operations are expensive and/or need to be performed with a write lock.

After applying this patch, fio read test goes from 1124MiB/s to 1191MiB/s.

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

diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
index 1344c5782a7c..e19c78f2480e 100644
--- a/fs/fuse/virtio_fs.c
+++ b/fs/fuse/virtio_fs.c
@@ -836,8 +836,19 @@ static void virtio_fs_requests_done_work(struct work_struct *work)
 
 	/* End requests */
 	list_for_each_entry_safe(req, next, &reqs, list) {
+		unsigned int total_sgs = req->out_sgs + req->in_sgs;
+
 		list_del_init(&req->list);
 
+		for (unsigned int i = 0; i < total_sgs; ++i) {
+			enum dma_data_direction dir = (i < req->out_sgs) ?
+				DMA_TO_DEVICE : DMA_FROM_DEVICE;
+			dma_unmap_page(vq->vdev->dev.parent,
+				       sg_dma_address(&req->sg[i]),
+				       sg_dma_len(&req->sg[i]), dir);
+
+		}
+
 		/* blocking async request completes in a worker context */
 		if (req->args->may_block) {
 			struct virtio_fs_req_work *w;
@@ -1426,6 +1437,24 @@ static int virtio_fs_enqueue_req(struct virtio_fs_vq *fsvq,
 		sgs[i] = &req->sg[i];
 	WARN_ON(req->out_sgs + req->in_sgs != total_sgs);
 
+	// TODO can we change this ptr out of the lock?
+	vq = fsvq->vq;
+	// TODO handle this and following errors
+	for (i = 0; i < total_sgs; i++) {
+		struct page *page = sg_page(&req->sg[i]);
+		enum dma_data_direction dir = (i < req->out_sgs) ?
+			DMA_TO_DEVICE : DMA_FROM_DEVICE;
+		dma_addr_t dma_addr = dma_map_page(vq->vdev->dev.parent, page,
+						   req->sg[i].offset, req->sg[i].length, dir);
+
+		if (dma_mapping_error(vq->vdev->dev.parent, dma_addr)) {
+			ret = -ENOMEM;
+			goto out;
+		}
+		sg_dma_address(&req->sg[i]) = dma_addr;
+		sg_dma_len(&req->sg[i]) = req->sg[i].length;
+	}
+
 	spin_lock(&fsvq->lock);
 
 	if (!fsvq->connected) {
@@ -1434,8 +1463,8 @@ static int virtio_fs_enqueue_req(struct virtio_fs_vq *fsvq,
 		goto out;
 	}
 
-	vq = fsvq->vq;
-	ret = virtqueue_add_sgs(vq, sgs, req->out_sgs, req->in_sgs, req, GFP_ATOMIC);
+	ret = virtqueue_add_sgs_premapped(vq, sgs, req->out_sgs,
+					  req->in_sgs, req, GFP_ATOMIC);
 	if (ret < 0) {
 		spin_unlock(&fsvq->lock);
 		goto out;
@@ -1460,6 +1489,13 @@ static int virtio_fs_enqueue_req(struct virtio_fs_vq *fsvq,
 		virtqueue_notify(vq);
 
 out:
+	for (unsigned int j = 0; ret && j < total_sgs; ++j) {
+		enum dma_data_direction dir = (j < req->out_sgs) ?
+			DMA_TO_DEVICE : DMA_FROM_DEVICE;
+		dma_unmap_page(vq->vdev->dev.parent,
+			       sg_dma_address(&req->sg[j]),
+			       sg_dma_len(&req->sg[j]), dir);
+	}
 	if (ret < 0 && req->argbuf) {
 		kfree(req->argbuf);
 		req->argbuf = NULL;
-- 
2.48.1

[RFC v2 5/5] virtiofs: Disable notifications more aggresively

[Eugenio Pérez]

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

Re: [RFC v2 1/5] vduse: add virtio_fs to allowed dev id

[Jason Wang]

On Sat, Feb 22, 2025 at 1:06 AM Eugenio Pérez <eperezma@redhat.com> wrote:
>
> A VDUSE device that implements virtiofs device works fine just by
> adding the device id to the whitelist.
>
> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> Acked-by: Jason Wang <jasowang@redhat.com>
> ---

Let's separate this from this series.

Thanks

Re: [RFC v2 5/5] virtiofs: Disable notifications more aggresively

[Jason Wang]

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
>

Re: [RFC v2 4/5] virtiofs: perform DMA operations out of the spinlock

[Jason Wang]

On Sat, Feb 22, 2025 at 1:07 AM Eugenio Pérez <eperezma@redhat.com> wrote:
>
> This is useful for some setups like swiotlb or VDUSE where the DMA
> operations are expensive and/or need to be performed with a write lock.
>
> After applying this patch, fio read test goes from 1124MiB/s to 1191MiB/s.

What FIO parameter have you used? It might be worth trying different
sizes. It seems to be more obvious when using larger requests when I'm
testing similar optimization for virtio-blk.

And we also need to test without VDUSE, to make sure no regression in
classical setups.

Thanks

>
> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> ---
>  fs/fuse/virtio_fs.c | 40 ++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 38 insertions(+), 2 deletions(-)
>
> diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
> index 1344c5782a7c..e19c78f2480e 100644
> --- a/fs/fuse/virtio_fs.c
> +++ b/fs/fuse/virtio_fs.c
> @@ -836,8 +836,19 @@ static void virtio_fs_requests_done_work(struct work_struct *work)
>
>         /* End requests */
>         list_for_each_entry_safe(req, next, &reqs, list) {
> +               unsigned int total_sgs = req->out_sgs + req->in_sgs;
> +
>                 list_del_init(&req->list);
>
> +               for (unsigned int i = 0; i < total_sgs; ++i) {
> +                       enum dma_data_direction dir = (i < req->out_sgs) ?
> +                               DMA_TO_DEVICE : DMA_FROM_DEVICE;
> +                       dma_unmap_page(vq->vdev->dev.parent,
> +                                      sg_dma_address(&req->sg[i]),
> +                                      sg_dma_len(&req->sg[i]), dir);
> +
> +               }
> +
>                 /* blocking async request completes in a worker context */
>                 if (req->args->may_block) {
>                         struct virtio_fs_req_work *w;
> @@ -1426,6 +1437,24 @@ static int virtio_fs_enqueue_req(struct virtio_fs_vq *fsvq,
>                 sgs[i] = &req->sg[i];
>         WARN_ON(req->out_sgs + req->in_sgs != total_sgs);
>
> +       // TODO can we change this ptr out of the lock?
> +       vq = fsvq->vq;
> +       // TODO handle this and following errors
> +       for (i = 0; i < total_sgs; i++) {
> +               struct page *page = sg_page(&req->sg[i]);
> +               enum dma_data_direction dir = (i < req->out_sgs) ?
> +                       DMA_TO_DEVICE : DMA_FROM_DEVICE;
> +               dma_addr_t dma_addr = dma_map_page(vq->vdev->dev.parent, page,
> +                                                  req->sg[i].offset, req->sg[i].length, dir);
> +
> +               if (dma_mapping_error(vq->vdev->dev.parent, dma_addr)) {
> +                       ret = -ENOMEM;
> +                       goto out;
> +               }
> +               sg_dma_address(&req->sg[i]) = dma_addr;
> +               sg_dma_len(&req->sg[i]) = req->sg[i].length;
> +       }
> +
>         spin_lock(&fsvq->lock);
>
>         if (!fsvq->connected) {
> @@ -1434,8 +1463,8 @@ static int virtio_fs_enqueue_req(struct virtio_fs_vq *fsvq,
>                 goto out;
>         }
>
> -       vq = fsvq->vq;
> -       ret = virtqueue_add_sgs(vq, sgs, req->out_sgs, req->in_sgs, req, GFP_ATOMIC);
> +       ret = virtqueue_add_sgs_premapped(vq, sgs, req->out_sgs,
> +                                         req->in_sgs, req, GFP_ATOMIC);
>         if (ret < 0) {
>                 spin_unlock(&fsvq->lock);
>                 goto out;
> @@ -1460,6 +1489,13 @@ static int virtio_fs_enqueue_req(struct virtio_fs_vq *fsvq,
>                 virtqueue_notify(vq);
>
>  out:
> +       for (unsigned int j = 0; ret && j < total_sgs; ++j) {
> +               enum dma_data_direction dir = (j < req->out_sgs) ?
> +                       DMA_TO_DEVICE : DMA_FROM_DEVICE;
> +               dma_unmap_page(vq->vdev->dev.parent,
> +                              sg_dma_address(&req->sg[j]),
> +                              sg_dma_len(&req->sg[j]), dir);
> +       }
>         if (ret < 0 && req->argbuf) {
>                 kfree(req->argbuf);
>                 req->argbuf = NULL;
> --
> 2.48.1
>

Re: [RFC v2 1/5] vduse: add virtio_fs to allowed dev id

[Eugenio Perez Martin]

On Mon, Feb 24, 2025 at 2:57 AM Jason Wang <jasowang@redhat.com> wrote:
>
> On Sat, Feb 22, 2025 at 1:06 AM Eugenio Pérez <eperezma@redhat.com> wrote:
> >
> > A VDUSE device that implements virtiofs device works fine just by
> > adding the device id to the whitelist.
> >
> > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > Acked-by: Jason Wang <jasowang@redhat.com>
> > ---
>
> Let's separate this from this series.
>

Right, actually I should have said this series should be applied on
top of this sent patch:

https://lore.kernel.org/lkml/CAJaqyWdC-Tte+ao6pk22fq-mUym8C9guQFThSnG5gMxWNqWyXw@mail.gmail.com/T/

Re: [RFC v2 5/5] virtiofs: Disable notifications more aggresively

[Eugenio Perez Martin]

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.

Re: [RFC v2 5/5] virtiofs: Disable notifications more aggresively

[Eugenio Perez Martin]

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?