Re: [PATCH] virtiofs: Enable multiple request queues

[Vivek Goyal]

On Mon, May 10, 2021 at 11:15:21AM -0500, Connor Kuehl wrote:
> On 5/10/21 10:25 AM, Vivek Goyal wrote:
> > On Fri, May 07, 2021 at 03:15:27PM -0700, Connor Kuehl wrote:
> >> Distribute requests across the multiqueue complex automatically based
> >> on the IRQ affinity.
> > 
> > Hi Connor,
> > 
> > Thanks for the patch. I will look into it and also test it.
> > 
> > How did you test it? Did you modify vitiofsd to support multiqueue. Did
> > you also run some performance numbers. Does it provide better/worse
> > performance as compared to single queue.
> 
> Thanks, Vivek! I need to NACK this version of the patch for inclusion
> though since I think the way I did per-CPU state will not work for
> multiple virtio-fs mounts because it will be overwritten with each new
> mount, but for testing purposes this should be OK with just one mount.

Hi Connor,

Ok. Will wait for next version which fixes the multiple mount issue.

> 
> I need to do more benchmarking on this.

That would be nice.

> 
> I had to hack multiqueue support into virtiofsd, which runs against the
> warning in the virtiofsd source code that instructs people to *not*
> enable multiqueue due to thread-safety concerns. I didn't audit
> virtiofsd for correctness, so I also worry this has the potential of
> affecting benchmarks if there are races.

filesystem code already can handle multiple threads because on a single
queue we can have a thread pool processing requests in parallel. I am
not aware of any issues about supporting multiple queues. I think
may be fuse_virtio.c might require a little closer inspection to make
sure nothing is dependent on single queue. 

> 
> For testing, QEMU needs to be invoked with `num-request-queues` like
> this:
> 
> 	-device vhost-user-fs-pci,chardev=char0,tag=myfs,num-request-queues=2 
> 
> And obviously you can choose any value >= 1 for num-request-queues.
> 
> and I also made a quick-and-dirty hack to let me pass in the number of
> total queues to virtiofsd on the command line:

Ok. May be there is some inspiration to take from virtio-blk. How do they
specific number of queues by default and how many. I thought stefan mentioned
that by default there is one queue per vcpu.

Vivek

> 
> diff --git a/tools/virtiofsd/fuse_lowlevel.c b/tools/virtiofsd/fuse_lowlevel.c
> index 58e32fc963..cf8f132efd 100644
> --- a/tools/virtiofsd/fuse_lowlevel.c
> +++ b/tools/virtiofsd/fuse_lowlevel.c
> @@ -2565,9 +2565,9 @@ out1:
>      return NULL;
>  }
>  
> -int fuse_session_mount(struct fuse_session *se)
> +int fuse_session_mount(struct fuse_session *se, unsigned int num_queues)
>  {
> -    return virtio_session_mount(se);
> +    return virtio_session_mount(se, num_queues);
>  }
>  
>  int fuse_session_fd(struct fuse_session *se)
> diff --git a/tools/virtiofsd/fuse_lowlevel.h b/tools/virtiofsd/fuse_lowlevel.h
> index 3bf786b034..50bf86113d 100644
> --- a/tools/virtiofsd/fuse_lowlevel.h
> +++ b/tools/virtiofsd/fuse_lowlevel.h
> @@ -1842,7 +1842,7 @@ struct fuse_session *fuse_session_new(struct fuse_args *args,
>   *
>   * @return 0 on success, -1 on failure.
>   **/
> -int fuse_session_mount(struct fuse_session *se);
> +int fuse_session_mount(struct fuse_session *se, unsigned int num_queues);
>  
>  /**
>   * Enter a single threaded, blocking event loop.
> diff --git a/tools/virtiofsd/fuse_virtio.c b/tools/virtiofsd/fuse_virtio.c
> index 3e13997406..8622c3dce6 100644
> --- a/tools/virtiofsd/fuse_virtio.c
> +++ b/tools/virtiofsd/fuse_virtio.c
> @@ -747,20 +747,6 @@ static void fv_queue_set_started(VuDev *dev, int qidx, bool started)
>               started);
>      assert(qidx >= 0);
>  
> -    /*
> -     * Ignore additional request queues for now.  passthrough_ll.c must be
> -     * audited for thread-safety issues first.  It was written with a
> -     * well-behaved client in mind and may not protect against all types of
> -     * races yet.
> -     */
> -    if (qidx > 1) {
> -        fuse_log(FUSE_LOG_ERR,
> -                 "%s: multiple request queues not yet implemented, please only "
> -                 "configure 1 request queue\n",
> -                 __func__);
> -        exit(EXIT_FAILURE);
> -    }
> -
>      if (started) {
>          /* Fire up a thread to watch this queue */
>          if (qidx >= vud->nqueues) {
> @@ -997,7 +983,7 @@ static int fv_create_listen_socket(struct fuse_session *se)
>      return 0;
>  }
>  
> -int virtio_session_mount(struct fuse_session *se)
> +int virtio_session_mount(struct fuse_session *se, unsigned int num_queues)
>  {
>      int ret;
>  
> @@ -1048,8 +1034,8 @@ int virtio_session_mount(struct fuse_session *se)
>      se->vu_socketfd = data_sock;
>      se->virtio_dev->se = se;
>      pthread_rwlock_init(&se->virtio_dev->vu_dispatch_rwlock, NULL);
> -    if (!vu_init(&se->virtio_dev->dev, 2, se->vu_socketfd, fv_panic, NULL,
> -                 fv_set_watch, fv_remove_watch, &fv_iface)) {
> +    if (!vu_init(&se->virtio_dev->dev, num_queues, se->vu_socketfd,
> +		 fv_panic, NULL, fv_set_watch, fv_remove_watch, &fv_iface)) {
>          fuse_log(FUSE_LOG_ERR, "%s: vu_init failed\n", __func__);
>          return -1;
>      }
> diff --git a/tools/virtiofsd/fuse_virtio.h b/tools/virtiofsd/fuse_virtio.h
> index 111684032c..a0e78b9b84 100644
> --- a/tools/virtiofsd/fuse_virtio.h
> +++ b/tools/virtiofsd/fuse_virtio.h
> @@ -18,7 +18,7 @@
>  
>  struct fuse_session;
>  
> -int virtio_session_mount(struct fuse_session *se);
> +int virtio_session_mount(struct fuse_session *se, unsigned int num_queues);
>  void virtio_session_close(struct fuse_session *se);
>  int virtio_loop(struct fuse_session *se);
>  
> diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
> index 1553d2ef45..9fd4e34980 100644
> --- a/tools/virtiofsd/passthrough_ll.c
> +++ b/tools/virtiofsd/passthrough_ll.c
> @@ -161,6 +161,7 @@ struct lo_data {
>      int allow_direct_io;
>      int announce_submounts;
>      bool use_statx;
> +    int num_vqs;
>      struct lo_inode root;
>      GHashTable *inodes; /* protected by lo->mutex */
>      struct lo_map ino_map; /* protected by lo->mutex */
> @@ -204,6 +205,7 @@ static const struct fuse_opt lo_opts[] = {
>      { "announce_submounts", offsetof(struct lo_data, announce_submounts), 1 },
>      { "killpriv_v2", offsetof(struct lo_data, user_killpriv_v2), 1 },
>      { "no_killpriv_v2", offsetof(struct lo_data, user_killpriv_v2), 0 },
> +    { "num_queues=%d", offsetof(struct lo_data, num_vqs), 2 },
>      FUSE_OPT_END
>  };
>  static bool use_syslog = false;
> @@ -3848,6 +3850,12 @@ int main(int argc, char *argv[])
>          exit(1);
>      }
>  
> +    if (lo.num_vqs < 2) {
> +        fuse_log(FUSE_LOG_ERR, "num_queues must be at least 2 (got %d)\n",
> +                 lo.num_vqs);
> +        exit(1);
> +    }
> +
>      lo.use_statx = true;
>  
>      se = fuse_session_new(&args, &lo_oper, sizeof(lo_oper), &lo);
> @@ -3859,7 +3867,7 @@ int main(int argc, char *argv[])
>          goto err_out2;
>      }
>  
> -    if (fuse_session_mount(se) != 0) {
> +    if (fuse_session_mount(se, lo.num_vqs) != 0) {
>          goto err_out3;
>      }
>  
>