Re: [PATCH 5/5] virtiofs: Retry request submission from worker context

[Miklos Szeredi <miklos@szeredi.hu>]

On Tue, Oct 15, 2019 at 7:46 PM Vivek Goyal <vgoyal@redhat.com> wrote:
>
> If regular request queue gets full, currently we sleep for a bit and
> retrying submission in submitter's context. This assumes submitter is
> not holding any spin lock. But this assumption is not true for background
> requests. For background requests, we are called with fc->bg_lock held.
>
> This can lead to deadlock where one thread is trying submission with
> fc->bg_lock held while request completion thread has called fuse_request_end()
> which tries to acquire fc->bg_lock and gets blocked. As request completion
> thread gets blocked, it does not make further progress and that means queue
> does not get empty and submitter can't submit more requests.
>
> To solve this issue, retry submission with the help of a worker, instead of
> retrying in submitter's context. We already do this for hiprio/forget
> requests.
>
> Reported-by: Chirantan Ekbote <chirantan@chromium.org>
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  fs/fuse/virtio_fs.c | 59 ++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 50 insertions(+), 9 deletions(-)
>
> diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
> index 625de45fa471..58e568ef54ef 100644
> --- a/fs/fuse/virtio_fs.c
> +++ b/fs/fuse/virtio_fs.c
> @@ -55,6 +55,9 @@ struct virtio_fs_forget {
>         struct list_head list;
>  };
>
> +static int virtio_fs_enqueue_req(struct virtio_fs_vq *fsvq,
> +                                struct fuse_req *req, bool in_flight);
> +
>  static inline struct virtio_fs_vq *vq_to_fsvq(struct virtqueue *vq)
>  {
>         struct virtio_fs *fs = vq->vdev->priv;
> @@ -260,6 +263,7 @@ static void virtio_fs_request_dispatch_work(struct work_struct *work)
>         struct virtio_fs_vq *fsvq = container_of(work, struct virtio_fs_vq,
>                                                  dispatch_work.work);
>         struct fuse_conn *fc = fsvq->fud->fc;
> +       int ret;
>
>         pr_debug("virtio-fs: worker %s called.\n", __func__);
>         while (1) {
> @@ -268,13 +272,43 @@ static void virtio_fs_request_dispatch_work(struct work_struct *work)
>                                                list);
>                 if (!req) {
>                         spin_unlock(&fsvq->lock);
> -                       return;
> +                       break;
>                 }
>
>                 list_del_init(&req->list);
>                 spin_unlock(&fsvq->lock);
>                 fuse_request_end(fc, req);
>         }
> +
> +       /* Dispatch pending requests */
> +       while (1) {
> +               spin_lock(&fsvq->lock);
> +               req = list_first_entry_or_null(&fsvq->queued_reqs,
> +                                              struct fuse_req, list);
> +               if (!req) {
> +                       spin_unlock(&fsvq->lock);
> +                       return;
> +               }
> +               list_del_init(&req->list);
> +               spin_unlock(&fsvq->lock);
> +
> +               ret = virtio_fs_enqueue_req(fsvq, req, true);
> +               if (ret < 0) {
> +                       if (ret == -ENOMEM || 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;
> +                       }
> +                       req->out.h.error = ret;
> +                       dec_in_flight_req(fsvq);

Missing locking.  Fixed.

Thanks,
Miklos