[PATCH 1/2] fuse: virtiofs: Fix nullptr dereference

[PATCH 1/2] fuse: virtiofs: Fix nullptr dereference

[Chirantan Ekbote]

virtiofs device implementations are allowed to provide more than one
request queue.  In this case `fsvq->fud` would not be initialized,
leading to a nullptr dereference later during driver initialization.

Make sure that `fsvq->fud` is initialized for all request queues even if
the driver doesn't use them.

Signed-off-by: Chirantan Ekbote <chirantan@chromium.org>
---
 fs/fuse/virtio_fs.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
index bade747689033..d3c38222a7e4e 100644
--- a/fs/fuse/virtio_fs.c
+++ b/fs/fuse/virtio_fs.c
@@ -1066,10 +1066,13 @@ static int virtio_fs_fill_super(struct super_block *sb)
 	}
 
 	err = -ENOMEM;
-	/* Allocate fuse_dev for hiprio and notification queues */
-	for (i = 0; i < VQ_REQUEST; i++) {
+	/* Allocate fuse_dev for all queues except the first request queue. */
+	for (i = 0; i < fs->nvqs; i++) {
 		struct virtio_fs_vq *fsvq = &fs->vqs[i];
 
+		if (i == VQ_REQUEST)
+			continue;
+
 		fsvq->fud = fuse_dev_alloc();
 		if (!fsvq->fud)
 			goto err_free_fuse_devs;
-- 
2.26.2.303.gf8c07b1a785-goog

[PATCH 2/2] fuse: virtiofs: Add basic multiqueue support

[Chirantan Ekbote]

Use simple round-robin scheduling based on the `unique` field of the
fuse request to spread requests across multiple queues, if supported by
the device.

Signed-off-by: Chirantan Ekbote <chirantan@chromium.org>
---
 fs/fuse/dev.c       | 4 ----
 fs/fuse/fuse_i.h    | 4 ++++
 fs/fuse/virtio_fs.c | 4 +++-
 3 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
index 97eec7522bf20..cad9f76c2519c 100644
--- a/fs/fuse/dev.c
+++ b/fs/fuse/dev.c
@@ -25,10 +25,6 @@
 MODULE_ALIAS_MISCDEV(FUSE_MINOR);
 MODULE_ALIAS("devname:fuse");
 
-/* Ordinary requests have even IDs, while interrupts IDs are odd */
-#define FUSE_INT_REQ_BIT (1ULL << 0)
-#define FUSE_REQ_ID_STEP (1ULL << 1)
-
 static struct kmem_cache *fuse_req_cachep;
 
 static struct fuse_dev *fuse_get_dev(struct file *file)
diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
index ca344bf714045..110b917d950a8 100644
--- a/fs/fuse/fuse_i.h
+++ b/fs/fuse/fuse_i.h
@@ -47,6 +47,10 @@
 /** Number of dentries for each connection in the control filesystem */
 #define FUSE_CTL_NUM_DENTRIES 5
 
+/* Ordinary requests have even IDs, while interrupts IDs are odd */
+#define FUSE_INT_REQ_BIT (1ULL << 0)
+#define FUSE_REQ_ID_STEP (1ULL << 1)
+
 /** List of active connections */
 extern struct list_head fuse_conn_list;
 
diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
index d3c38222a7e4e..c5129fd27930c 100644
--- a/fs/fuse/virtio_fs.c
+++ b/fs/fuse/virtio_fs.c
@@ -980,7 +980,7 @@ static int virtio_fs_enqueue_req(struct virtio_fs_vq *fsvq,
 static void virtio_fs_wake_pending_and_unlock(struct fuse_iqueue *fiq)
 __releases(fiq->lock)
 {
-	unsigned int queue_id = VQ_REQUEST; /* TODO multiqueue */
+	unsigned int queue_id;
 	struct virtio_fs *fs;
 	struct fuse_req *req;
 	struct virtio_fs_vq *fsvq;
@@ -994,6 +994,8 @@ __releases(fiq->lock)
 	spin_unlock(&fiq->lock);
 
 	fs = fiq->priv;
+	queue_id = ((req->in.h.unique / FUSE_REQ_ID_STEP) %
+		    (uint64_t)fs->num_request_queues) + VQ_REQUEST;
 
 	pr_debug("%s: opcode %u unique %#llx nodeid %#llx in.len %u out.len %u\n",
 		  __func__, req->in.h.opcode, req->in.h.unique,
-- 
2.26.2.303.gf8c07b1a785-goog

Re: [PATCH 2/2] fuse: virtiofs: Add basic multiqueue support

[Stefan Hajnoczi]

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

On Fri, Apr 24, 2020 at 03:25:40PM +0900, Chirantan Ekbote wrote:
> Use simple round-robin scheduling based on the `unique` field of the
> fuse request to spread requests across multiple queues, if supported by
> the device.

Multiqueue is not intended to be used this way and this patch will
reduce performance*.  I don't think it should be merged.

* I know it increases performance for you :) but hear me out:

The purpose of multiqueue is for SMP scalability.  It allows queues to
be processed with CPU/NUMA affinity to the vCPU that submitted the
request (i.e. the virtqueue processing thread runs on a sibling physical
CPU core).  Each queue has its own MSI-X interrupt so that completion
interrupts can be processed on the same vCPU that submitted the request.

Spreading requests across queues defeats all this.  Virtqueue processing
threads that are located in the wrong place will now process the
requests.  Completion interrupts will wake up a vCPU that did not submit
the request and IPIs are necessary to notify the vCPU that originally
submitted the request.

Even if you don't care about SMP performance, using multiqueue as a
workaround for missing request parallelism still won't yield the best
results.  The guest should be able to submit up to the maximum queue
depth of the physical storage device.  Many Linux block drivers have max
queue depths of 64.  This would require 64 virtqueues (plus the queue
selection algorithm would have to utilize each one) and shows how
wasteful this approach is.

Instead of modifying the guest driver, please implement request
parallelism in your device implementation.  Device implementations
should pop a virtqueue element, submit I/O, and then move on to the next
virtqueue element instead of waiting for the I/O to complete.  This can
be done with a thread pool, coroutines, async I/O APIs, etc.

The C implementation of virtiofsd has request parallelism and there is
work underway to do it in Rust for cloud-hypervisor too.  Perhaps you
can try the cloud-hypervisor code, I have CCed Sergio Lopez?

Thanks,
Stefan

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

Re: [PATCH 2/2] fuse: virtiofs: Add basic multiqueue support

[Stefan Hajnoczi]

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

On Mon, Apr 27, 2020 at 04:19:34PM +0100, Stefan Hajnoczi wrote:
> On Fri, Apr 24, 2020 at 03:25:40PM +0900, Chirantan Ekbote wrote:
> The C implementation of virtiofsd has request parallelism and there is
> work underway to do it in Rust for cloud-hypervisor too.  Perhaps you
> can try the cloud-hypervisor code, I have CCed Sergio Lopez?

Sergio pointed me to this commit:

https://github.com/cloud-hypervisor/cloud-hypervisor/commit/710520e9a1860c587b4b3ec6aadc466ba1709557

Stefan

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

Re: [PATCH 2/2] fuse: virtiofs: Add basic multiqueue support

[Chirantan Ekbote]

Hi Stefan,

On Tue, Apr 28, 2020 at 12:20 AM Stefan Hajnoczi <stefanha@redhat.com> wrote:
>
> On Fri, Apr 24, 2020 at 03:25:40PM +0900, Chirantan Ekbote wrote:
> > Use simple round-robin scheduling based on the `unique` field of the
> > fuse request to spread requests across multiple queues, if supported by
> > the device.
>
> Multiqueue is not intended to be used this way and this patch will
> reduce performance*.  I don't think it should be merged.
>
> * I know it increases performance for you :) but hear me out:
>

It actually doesn't increase performance for me :-(.  It did increase
performance when I tested it on my 96-core workstation but on our
actual target devices, which only have 2 cores, using multiqueue or
having additional threads in the server actually made performance
worse.

> The purpose of multiqueue is for SMP scalability.  It allows queues to
> be processed with CPU/NUMA affinity to the vCPU that submitted the
> request (i.e. the virtqueue processing thread runs on a sibling physical
> CPU core).  Each queue has its own MSI-X interrupt so that completion
> interrupts can be processed on the same vCPU that submitted the request.
>
> Spreading requests across queues defeats all this.  Virtqueue processing
> threads that are located in the wrong place will now process the
> requests.  Completion interrupts will wake up a vCPU that did not submit
> the request and IPIs are necessary to notify the vCPU that originally
> submitted the request.
>

Thanks for the explanation.  I wasn't aware of this aspect of using
multiple queues but it makes sense now why we wouldn't want to spread
the requests across different queues.

> Even if you don't care about SMP performance, using multiqueue as a
> workaround for missing request parallelism still won't yield the best
> results.  The guest should be able to submit up to the maximum queue
> depth of the physical storage device.  Many Linux block drivers have max
> queue depths of 64.  This would require 64 virtqueues (plus the queue
> selection algorithm would have to utilize each one) and shows how
> wasteful this approach is.
>

I understand this but in practice unlike the virtio-blk workload,
which is nothing but reads and writes to a single file, the virtio-fs
workload tends to mix a bunch of metadata operations with data
transfers.  The metadata operations should be mostly handled out of
the host's file cache so it's unlikely virtio-fs would really be able
to fully utilize the underlying storage short of reading or writing a
really huge file.

> Instead of modifying the guest driver, please implement request
> parallelism in your device implementation.

Yes, we have tried this already [1][2].  As I mentioned above, having
additional threads in the server actually made performance worse.  My
theory is that when the device only has 2 cpus, having additional
threads on the host that need cpu time ends up taking time away from
the guest vcpu.  We're now looking at switching to io_uring so that we
can submit multiple requests from a single thread.

The multiqueue change was small and I wasn't aware of the SMP
implications, which is why I sent this patch.

Thanks,
Chirantan

[1] https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/2103602
[2] https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/2103603

Re: [PATCH 2/2] fuse: virtiofs: Add basic multiqueue support

[Stefan Hajnoczi]

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

On Fri, May 01, 2020 at 04:14:38PM +0900, Chirantan Ekbote wrote:
> On Tue, Apr 28, 2020 at 12:20 AM Stefan Hajnoczi <stefanha@redhat.com> wrote:
> > On Fri, Apr 24, 2020 at 03:25:40PM +0900, Chirantan Ekbote wrote:
> > Even if you don't care about SMP performance, using multiqueue as a
> > workaround for missing request parallelism still won't yield the best
> > results.  The guest should be able to submit up to the maximum queue
> > depth of the physical storage device.  Many Linux block drivers have max
> > queue depths of 64.  This would require 64 virtqueues (plus the queue
> > selection algorithm would have to utilize each one) and shows how
> > wasteful this approach is.
> >
> 
> I understand this but in practice unlike the virtio-blk workload,
> which is nothing but reads and writes to a single file, the virtio-fs
> workload tends to mix a bunch of metadata operations with data
> transfers.  The metadata operations should be mostly handled out of
> the host's file cache so it's unlikely virtio-fs would really be able
> to fully utilize the underlying storage short of reading or writing a
> really huge file.

I agree that a proportion of heavy I/O workloads on virtio-blk become
heavy metadata I/O workloads on virtio-fs.

However, workloads consisting mostly of READ, WRITE, and FLUSH
operations still exist on virtio-fs.  Databases, audio/video file
streaming, etc are bottlenecked on I/O performance.  They need to
perform well and virtio-fs should strive to do that.

> > Instead of modifying the guest driver, please implement request
> > parallelism in your device implementation.
> 
> Yes, we have tried this already [1][2].  As I mentioned above, having
> additional threads in the server actually made performance worse.  My
> theory is that when the device only has 2 cpus, having additional
> threads on the host that need cpu time ends up taking time away from
> the guest vcpu.  We're now looking at switching to io_uring so that we
> can submit multiple requests from a single thread.

The host has 2 CPUs?  How many vCPUs does the guest have?  What is the
physical storage device?  What is the host file system?

io_uring's vocabulary is expanding.  It can now do openat2(2), close(2),
statx(2), but not mkdir(2), unlink(2), rename(2), etc.

I guess there are two options:
1. Fall back to threads for FUSE operations that cannot yet be done via
   io_uring.
2. Process FUSE operations that cannot be done via io_uring
   synchronously.

Stefan

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

Re: [PATCH 2/2] fuse: virtiofs: Add basic multiqueue support

[Chirantan Ekbote]

On Sat, May 2, 2020 at 12:48 AM Stefan Hajnoczi <stefanha@redhat.com> wrote:
>
> On Fri, May 01, 2020 at 04:14:38PM +0900, Chirantan Ekbote wrote:
> > On Tue, Apr 28, 2020 at 12:20 AM Stefan Hajnoczi <stefanha@redhat.com> wrote:
> > > Instead of modifying the guest driver, please implement request
> > > parallelism in your device implementation.
> >
> > Yes, we have tried this already [1][2].  As I mentioned above, having
> > additional threads in the server actually made performance worse.  My
> > theory is that when the device only has 2 cpus, having additional
> > threads on the host that need cpu time ends up taking time away from
> > the guest vcpu.  We're now looking at switching to io_uring so that we
> > can submit multiple requests from a single thread.
>
> The host has 2 CPUs?  How many vCPUs does the guest have?  What is the
> physical storage device?  What is the host file system?

The host has 2 cpus.  The guest has 1 vcpu.  The physical storage
device is an internal ssd.  The file system is ext4 with directory
encryption.


>
> io_uring's vocabulary is expanding.  It can now do openat2(2), close(2),
> statx(2), but not mkdir(2), unlink(2), rename(2), etc.
>
> I guess there are two options:
> 1. Fall back to threads for FUSE operations that cannot yet be done via
>    io_uring.
> 2. Process FUSE operations that cannot be done via io_uring
>    synchronously.
>

I'm hoping that using io_uring for just the reads and writes should
give us a big enough improvement that we can do the rest of the
operations synchronously.

Chirantan

Re: [PATCH 2/2] fuse: virtiofs: Add basic multiqueue support

[Stefan Hajnoczi]

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

On Thu, May 07, 2020 at 05:10:15PM +0900, Chirantan Ekbote wrote:
> On Sat, May 2, 2020 at 12:48 AM Stefan Hajnoczi <stefanha@redhat.com> wrote:
> > On Fri, May 01, 2020 at 04:14:38PM +0900, Chirantan Ekbote wrote:
> > > On Tue, Apr 28, 2020 at 12:20 AM Stefan Hajnoczi <stefanha@redhat.com> wrote:
> > io_uring's vocabulary is expanding.  It can now do openat2(2), close(2),
> > statx(2), but not mkdir(2), unlink(2), rename(2), etc.
> >
> > I guess there are two options:
> > 1. Fall back to threads for FUSE operations that cannot yet be done via
> >    io_uring.
> > 2. Process FUSE operations that cannot be done via io_uring
> >    synchronously.
> >
> 
> I'm hoping that using io_uring for just the reads and writes should
> give us a big enough improvement that we can do the rest of the
> operations synchronously.

Sounds like a good idea.

Stefan

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

Re: [PATCH 1/2] fuse: virtiofs: Fix nullptr dereference

[Stefan Hajnoczi]

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

On Fri, Apr 24, 2020 at 03:25:39PM +0900, Chirantan Ekbote wrote:
> virtiofs device implementations are allowed to provide more than one
> request queue.  In this case `fsvq->fud` would not be initialized,
> leading to a nullptr dereference later during driver initialization.
> 
> Make sure that `fsvq->fud` is initialized for all request queues even if
> the driver doesn't use them.
> 
> Signed-off-by: Chirantan Ekbote <chirantan@chromium.org>
> ---
>  fs/fuse/virtio_fs.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)

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

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

Re: [PATCH 1/2] fuse: virtiofs: Fix nullptr dereference

[Vivek Goyal]

On Fri, Apr 24, 2020 at 03:25:39PM +0900, Chirantan Ekbote wrote:
> virtiofs device implementations are allowed to provide more than one
> request queue.  In this case `fsvq->fud` would not be initialized,
> leading to a nullptr dereference later during driver initialization.
> 
> Make sure that `fsvq->fud` is initialized for all request queues even if
> the driver doesn't use them.
> 
> Signed-off-by: Chirantan Ekbote <chirantan@chromium.org>
> ---
>  fs/fuse/virtio_fs.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
> index bade747689033..d3c38222a7e4e 100644
> --- a/fs/fuse/virtio_fs.c
> +++ b/fs/fuse/virtio_fs.c
> @@ -1066,10 +1066,13 @@ static int virtio_fs_fill_super(struct super_block *sb)
>  	}
>  
>  	err = -ENOMEM;
> -	/* Allocate fuse_dev for hiprio and notification queues */
> -	for (i = 0; i < VQ_REQUEST; i++) {
> +	/* Allocate fuse_dev for all queues except the first request queue. */
> +	for (i = 0; i < fs->nvqs; i++) {
>  		struct virtio_fs_vq *fsvq = &fs->vqs[i];
>  
> +		if (i == VQ_REQUEST)
> +			continue;
> +

These special conditions of initializing fuse device for one queue
fusing fill_super_common() and rest of the queues outside of it, are
bothering me. I am proposing a separate patch where all fuse device
initialization/cleanup is done by the caller. It makes code look
cleaner and easier to understand.

Vivek

>  		fsvq->fud = fuse_dev_alloc();
>  		if (!fsvq->fud)
>  			goto err_free_fuse_devs;
> -- 
> 2.26.2.303.gf8c07b1a785-goog
> 

Re: [PATCH 1/2] fuse: virtiofs: Fix nullptr dereference

[Stefan Hajnoczi]

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

On Mon, Apr 27, 2020 at 01:58:14PM -0400, Vivek Goyal wrote:
> On Fri, Apr 24, 2020 at 03:25:39PM +0900, Chirantan Ekbote wrote:
> > virtiofs device implementations are allowed to provide more than one
> > request queue.  In this case `fsvq->fud` would not be initialized,
> > leading to a nullptr dereference later during driver initialization.
> > 
> > Make sure that `fsvq->fud` is initialized for all request queues even if
> > the driver doesn't use them.
> > 
> > Signed-off-by: Chirantan Ekbote <chirantan@chromium.org>
> > ---
> >  fs/fuse/virtio_fs.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
> > index bade747689033..d3c38222a7e4e 100644
> > --- a/fs/fuse/virtio_fs.c
> > +++ b/fs/fuse/virtio_fs.c
> > @@ -1066,10 +1066,13 @@ static int virtio_fs_fill_super(struct super_block *sb)
> >  	}
> >  
> >  	err = -ENOMEM;
> > -	/* Allocate fuse_dev for hiprio and notification queues */
> > -	for (i = 0; i < VQ_REQUEST; i++) {
> > +	/* Allocate fuse_dev for all queues except the first request queue. */
> > +	for (i = 0; i < fs->nvqs; i++) {
> >  		struct virtio_fs_vq *fsvq = &fs->vqs[i];
> >  
> > +		if (i == VQ_REQUEST)
> > +			continue;
> > +
> 
> These special conditions of initializing fuse device for one queue
> fusing fill_super_common() and rest of the queues outside of it, are
> bothering me. I am proposing a separate patch where all fuse device
> initialization/cleanup is done by the caller. It makes code look
> cleaner and easier to understand.

Nice!

Stefan

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