[PATCH 0/2] virtio-fs: change handling of failure at request enqueue

[PATCH 0/2] virtio-fs: change handling of failure at request enqueue

[Peter-Jan Gootzen]

This patch set aims to improve the latencies of virtio-fs requests when
the system is under high load, namely when the application's IO depth
is greater than the size of the Virtio queue.

We found that the handling of -ENOMEM when enqueueing requests is
inconsistent with other parts of the kernel (e.g. FUSE) and also
obstructs optimizing the enqueueing behavior.

It is important to first remove the -ENOMEM behavior as the new style of
retrying virtio-fs requests in patch 2/2 is only correct in case of
-ENOSPC. With -ENOMEM the failed enqueue might never be retried as there
might not be another completion to trigger retrying the enqueue.

Note that this patch series is a revival of my patch that was last
discussed in the mailing list on 2023-08-16.

Peter-Jan Gootzen (2):
  virtio-fs: let -ENOMEM bubble up or burst gently
  virtio-fs: improved request latencies when Virtio queue is full

 fs/fuse/virtio_fs.c | 40 ++++++++++++++++++++++------------------
 1 file changed, 22 insertions(+), 18 deletions(-)

-- 
2.34.1

[PATCH 1/2] virtio-fs: let -ENOMEM bubble up or burst gently

[Peter-Jan Gootzen]

Currently, when the enqueueing of a request or forget operation fails
with -ENOMEM, the enqueueing is retried after a timeout. This patch
removes this behavior and treats -ENOMEM in these scenarios like any
other error. By bubbling up the error to user space in the case of a
request, and by dropping the operation in case of a forget. This
behavior matches that of the FUSE layer above, and also simplifies the
error handling. The latter will come in handy for upcoming patches that
optimize the retrying of operations in case of -ENOSPC.

Signed-off-by: Peter-Jan Gootzen <pgootzen@nvidia.com>
Reviewed-by: Max Gurtovoy <mgurtovoy@nvidia.com>
Reviewed-by: Yoray Zack <yorayz@nvidia.com>
---
 fs/fuse/virtio_fs.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
index 8ffa4f063a37..55126df202d6 100644
--- a/fs/fuse/virtio_fs.c
+++ b/fs/fuse/virtio_fs.c
@@ -447,7 +447,7 @@ static void virtio_fs_request_dispatch_work(struct work_struct *work)
 
 		ret = virtio_fs_enqueue_req(fsvq, req, true);
 		if (ret < 0) {
-			if (ret == -ENOMEM || ret == -ENOSPC) {
+			if (ret == -ENOSPC) {
 				spin_lock(&fsvq->lock);
 				list_add_tail(&req->list, &fsvq->queued_reqs);
 				schedule_delayed_work(&fsvq->dispatch_work,
@@ -494,7 +494,7 @@ static int send_forget_request(struct virtio_fs_vq *fsvq,
 
 	ret = virtqueue_add_outbuf(vq, &sg, 1, forget, GFP_ATOMIC);
 	if (ret < 0) {
-		if (ret == -ENOMEM || ret == -ENOSPC) {
+		if (ret == -ENOSPC) {
 			pr_debug("virtio-fs: Could not queue FORGET: err=%d. Will try later\n",
 				 ret);
 			list_add_tail(&forget->list, &fsvq->queued_reqs);
@@ -1368,7 +1368,7 @@ __releases(fiq->lock)
 	fsvq = &fs->vqs[queue_id];
 	ret = virtio_fs_enqueue_req(fsvq, req, false);
 	if (ret < 0) {
-		if (ret == -ENOMEM || ret == -ENOSPC) {
+		if (ret == -ENOSPC) {
 			/*
 			 * Virtqueue full. Retry submission from worker
 			 * context as we might be holding fc->bg_lock.
-- 
2.34.1

[PATCH 2/2] virtio-fs: improved request latencies when Virtio queue is full

[Peter-Jan Gootzen]

Currently, when the Virtio queue is full, a work item is scheduled
to execute in 1ms that retries adding the request to the queue.
This is a large amount of time on the scale on which a
virtio-fs device can operate. When using a DPU this is around
30-40us baseline without going to a remote server (4k, QD=1).

This patch changes the retrying behavior to immediately filling the
Virtio queue up again when a completion has been received. 

This reduces the 99.9th percentile latencies in our tests by
60x and slightly increases the overall throughput, when using a
workload IO depth 2x the size of the Virtio queue and a
DPU-powered virtio-fs device (NVIDIA BlueField DPU).

Signed-off-by: Peter-Jan Gootzen <pgootzen@nvidia.com>
Reviewed-by: Max Gurtovoy <mgurtovoy@nvidia.com>
Reviewed-by: Yoray Zack <yorayz@nvidia.com>
---
 fs/fuse/virtio_fs.c | 34 +++++++++++++++++++---------------
 1 file changed, 19 insertions(+), 15 deletions(-)

diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
index 55126df202d6..fdeb7e34ec01 100644
--- a/fs/fuse/virtio_fs.c
+++ b/fs/fuse/virtio_fs.c
@@ -51,7 +51,7 @@ struct virtio_fs_vq {
 	struct work_struct done_work;
 	struct list_head queued_reqs;
 	struct list_head end_reqs;	/* End these requests */
-	struct delayed_work dispatch_work;
+	struct work_struct dispatch_work;
 	struct fuse_dev *fud;
 	bool connected;
 	long in_flight;
@@ -233,7 +233,7 @@ static void virtio_fs_drain_queue(struct virtio_fs_vq *fsvq)
 	}
 
 	flush_work(&fsvq->done_work);
-	flush_delayed_work(&fsvq->dispatch_work);
+	flush_work(&fsvq->dispatch_work);
 }
 
 static void virtio_fs_drain_all_queues_locked(struct virtio_fs *fs)
@@ -408,6 +408,10 @@ static void virtio_fs_hiprio_done_work(struct work_struct *work)
 			dec_in_flight_req(fsvq);
 		}
 	} while (!virtqueue_enable_cb(vq));
+
+	if (!list_empty(&fsvq->queued_reqs))
+		schedule_work(&fsvq->dispatch_work);
+
 	spin_unlock(&fsvq->lock);
 }
 
@@ -415,7 +419,7 @@ static void virtio_fs_request_dispatch_work(struct work_struct *work)
 {
 	struct fuse_req *req;
 	struct virtio_fs_vq *fsvq = container_of(work, struct virtio_fs_vq,
-						 dispatch_work.work);
+						 dispatch_work);
 	int ret;
 
 	pr_debug("virtio-fs: worker %s called.\n", __func__);
@@ -450,8 +454,6 @@ static void virtio_fs_request_dispatch_work(struct work_struct *work)
 			if (ret == -ENOSPC) {
 				spin_lock(&fsvq->lock);
 				list_add_tail(&req->list, &fsvq->queued_reqs);
-				schedule_delayed_work(&fsvq->dispatch_work,
-						      msecs_to_jiffies(1));
 				spin_unlock(&fsvq->lock);
 				return;
 			}
@@ -498,8 +500,6 @@ static int send_forget_request(struct virtio_fs_vq *fsvq,
 			pr_debug("virtio-fs: Could not queue FORGET: err=%d. Will try later\n",
 				 ret);
 			list_add_tail(&forget->list, &fsvq->queued_reqs);
-			schedule_delayed_work(&fsvq->dispatch_work,
-					      msecs_to_jiffies(1));
 			if (!in_flight)
 				inc_in_flight_req(fsvq);
 			/* Queue is full */
@@ -531,7 +531,7 @@ static void virtio_fs_hiprio_dispatch_work(struct work_struct *work)
 {
 	struct virtio_fs_forget *forget;
 	struct virtio_fs_vq *fsvq = container_of(work, struct virtio_fs_vq,
-						 dispatch_work.work);
+						 dispatch_work);
 	pr_debug("virtio-fs: worker %s called.\n", __func__);
 	while (1) {
 		spin_lock(&fsvq->lock);
@@ -709,6 +709,12 @@ static void virtio_fs_requests_done_work(struct work_struct *work)
 			virtio_fs_request_complete(req, fsvq);
 		}
 	}
+
+	/* Try to push previously queued requests, as the queue might no longer be full */
+	spin_lock(&fsvq->lock);
+	if (!list_empty(&fsvq->queued_reqs))
+		schedule_work(&fsvq->dispatch_work);
+	spin_unlock(&fsvq->lock);
 }
 
 static void virtio_fs_map_queues(struct virtio_device *vdev, struct virtio_fs *fs)
@@ -770,12 +776,12 @@ static void virtio_fs_init_vq(struct virtio_fs_vq *fsvq, char *name,
 
 	if (vq_type == VQ_REQUEST) {
 		INIT_WORK(&fsvq->done_work, virtio_fs_requests_done_work);
-		INIT_DELAYED_WORK(&fsvq->dispatch_work,
-				  virtio_fs_request_dispatch_work);
+		INIT_WORK(&fsvq->dispatch_work,
+				virtio_fs_request_dispatch_work);
 	} else {
 		INIT_WORK(&fsvq->done_work, virtio_fs_hiprio_done_work);
-		INIT_DELAYED_WORK(&fsvq->dispatch_work,
-				  virtio_fs_hiprio_dispatch_work);
+		INIT_WORK(&fsvq->dispatch_work,
+				virtio_fs_hiprio_dispatch_work);
 	}
 }
 
@@ -1376,8 +1382,6 @@ __releases(fiq->lock)
 			spin_lock(&fsvq->lock);
 			list_add_tail(&req->list, &fsvq->queued_reqs);
 			inc_in_flight_req(fsvq);
-			schedule_delayed_work(&fsvq->dispatch_work,
-						msecs_to_jiffies(1));
 			spin_unlock(&fsvq->lock);
 			return;
 		}
@@ -1387,7 +1391,7 @@ __releases(fiq->lock)
 		/* Can't end request in submission context. Use a worker */
 		spin_lock(&fsvq->lock);
 		list_add_tail(&req->list, &fsvq->end_reqs);
-		schedule_delayed_work(&fsvq->dispatch_work, 0);
+		schedule_work(&fsvq->dispatch_work);
 		spin_unlock(&fsvq->lock);
 		return;
 	}
-- 
2.34.1

Re: [PATCH 0/2] virtio-fs: change handling of failure at request enqueue

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1274 bytes --]

On Fri, May 17, 2024 at 09:04:33PM +0200, Peter-Jan Gootzen wrote:
> This patch set aims to improve the latencies of virtio-fs requests when
> the system is under high load, namely when the application's IO depth
> is greater than the size of the Virtio queue.
> 
> We found that the handling of -ENOMEM when enqueueing requests is
> inconsistent with other parts of the kernel (e.g. FUSE) and also
> obstructs optimizing the enqueueing behavior.
> 
> It is important to first remove the -ENOMEM behavior as the new style of
> retrying virtio-fs requests in patch 2/2 is only correct in case of
> -ENOSPC. With -ENOMEM the failed enqueue might never be retried as there
> might not be another completion to trigger retrying the enqueue.
> 
> Note that this patch series is a revival of my patch that was last
> discussed in the mailing list on 2023-08-16.
> 
> Peter-Jan Gootzen (2):
>   virtio-fs: let -ENOMEM bubble up or burst gently
>   virtio-fs: improved request latencies when Virtio queue is full
> 
>  fs/fuse/virtio_fs.c | 40 ++++++++++++++++++++++------------------
>  1 file changed, 22 insertions(+), 18 deletions(-)
> 
> -- 
> 2.34.1
> 

This is a nice improvement, thank you!

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/2] virtio-fs: let -ENOMEM bubble up or burst gently

[Miklos Szeredi]

On Fri, 17 May 2024 at 21:05, Peter-Jan Gootzen <pgootzen@nvidia.com> wrote:
>
> Currently, when the enqueueing of a request or forget operation fails
> with -ENOMEM, the enqueueing is retried after a timeout. This patch
> removes this behavior and treats -ENOMEM in these scenarios like any
> other error. By bubbling up the error to user space in the case of a
> request, and by dropping the operation in case of a forget. This
> behavior matches that of the FUSE layer above, and also simplifies the
> error handling. The latter will come in handy for upcoming patches that
> optimize the retrying of operations in case of -ENOSPC.
>
> Signed-off-by: Peter-Jan Gootzen <pgootzen@nvidia.com>
> Reviewed-by: Max Gurtovoy <mgurtovoy@nvidia.com>
> Reviewed-by: Yoray Zack <yorayz@nvidia.com>
> ---
>  fs/fuse/virtio_fs.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
> index 8ffa4f063a37..55126df202d6 100644
> --- a/fs/fuse/virtio_fs.c
> +++ b/fs/fuse/virtio_fs.c
> @@ -447,7 +447,7 @@ static void virtio_fs_request_dispatch_work(struct work_struct *work)
>
>                 ret = virtio_fs_enqueue_req(fsvq, req, true);
>                 if (ret < 0) {
> -                       if (ret == -ENOMEM || ret == -ENOSPC) {
> +                       if (ret == -ENOSPC) {
>                                 spin_lock(&fsvq->lock);
>                                 list_add_tail(&req->list, &fsvq->queued_reqs);
>                                 schedule_delayed_work(&fsvq->dispatch_work,
> @@ -494,7 +494,7 @@ static int send_forget_request(struct virtio_fs_vq *fsvq,
>
>         ret = virtqueue_add_outbuf(vq, &sg, 1, forget, GFP_ATOMIC);
>         if (ret < 0) {
> -               if (ret == -ENOMEM || ret == -ENOSPC) {
> +               if (ret == -ENOSPC) {

Wasn't retrying on ENOMEM added because GFP_ATOMIC can fail with a
significantly larger probability?

Thanks,
Miklos