Re: [PATCH] fuse: cleanup request queuing towards virtiofs

[Peter-Jan Gootzen <pgootzen@nvidia.com>]

On Wed, 2024-05-29 at 14:32 -0400, Stefan Hajnoczi wrote:
> On Wed, May 29, 2024 at 05:52:07PM +0200, Miklos Szeredi wrote:
> > Virtiofs has its own queing mechanism, but still requests are first
> > queued
> > on fiq->pending to be immediately dequeued and queued onto the
> > virtio
> > queue.
> > 
> > The queuing on fiq->pending is unnecessary and might even have some
> > performance impact due to being a contention point.
> > 
> > Forget requests are handled similarly.
> > 
> > Move the queuing of requests and forgets into the fiq->ops->*.
> > fuse_iqueue_ops are renamed to reflect the new semantics.
> > 
> > Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
> > ---
> >  fs/fuse/dev.c       | 159 ++++++++++++++++++++++++-----------------
> > ---
> >  fs/fuse/fuse_i.h    |  19 ++----
> >  fs/fuse/virtio_fs.c |  41 ++++--------
> >  3 files changed, 106 insertions(+), 113 deletions(-)
> 
> This is a little scary but I can't think of a scenario where directly
> dispatching requests to virtqueues is a problem.
> 
> Is there someone who can run single and multiqueue virtiofs
> performance
> benchmarks?
> 
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

I ran some tests and experiments on the patch (on top of v6.10-rc2) with
our multi-queue capable virtio-fs device. No issues were found.

Experimental system setup (which is not the fastest possible setup nor
the most optimized setup!):
# Host:
   - Dell PowerEdge R7525
   - CPU: 2x AMD EPYC 7413 24-Core
   - VM: QEMU KVM with 24 cores, vCPUs locked to the NUMA nodes on which
the DPU is attached. VFIO-pci device to passthrough the DPU.           
Running a default x86_64 ext4 buildroot with fio installed.
# Virtio-fs device:
   - BlueField-3 DPU
   - CPU: ARM Cortex-A78AE, 16 cores
   - One thread per queue, each busy polling on one request queue
   - Each queue is 1024 descriptors deep
# Workload (deviations are specified in the table):
   - fio 3.34
   - sequential read
   - ioengine=io_uring, single 4GiB file, iodepth=128, bs=256KiB,    
runtime=30s, ramp_time=10s, direct=1
   - T is the number of threads (numjobs=T with thread=1)
   - Q is the number of request queues

| Workload           | Before patch | After patch |
| ------------------ | ------------ | ----------- |
| T=1 Q=1            | 9216MiB/s    | 9201MiB/s   |
| T=2 Q=2            | 10.8GiB/s    | 10.7GiB/s   |
| T=4 Q=4            | 12.6GiB/s    | 12.2GiB/s   |
| T=8 Q=8            | 19.5GiB/s    | 19.7GiB/s   |
| T=16 Q=1           | 9451MiB/s    | 9558MiB/s   |
| T=16 Q=2           | 13.5GiB/s    | 13.4GiB/s   |
| T=16 Q=4           | 11.8GiB/s    | 11.4GiB/s   |
| T=16 Q=8           | 11.1GiB/s    | 10.8GiB/s   |
| T=24 Q=24          | 26.5GiB/s    | 26.5GiB/s   |
| T=24 Q=24 24 files | 26.5GiB/s    | 26.6GiB/s   |
| T=24 Q=24 4k       | 948MiB/s     | 955MiB/s    |

Averaging out those results, the difference is within a reasonable
margin of a error (less than 1%). So in this setup's
case we see no difference in performance.
However if the virtio-fs device was more optimized, e.g. if it didn't
copy the data to its memory, then the bottleneck could possibly be on
the driver-side and this patch could show some benefit at those higher
message rates.

So although I would have hoped for some performance increase already
with this setup, I still think this is a good patch and a logical
optimization for high performance virtio-fs devices that might show a
benefit in the future.

Tested-by: Peter-Jan Gootzen <pgootzen@nvidia.com>
Reviewed-by: Peter-Jan Gootzen <pgootzen@nvidia.com>