Re: [PATCH v3] fuse: virtio-fs: stop new requests before draining queues

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] fuse: virtio-fs: hang in virtio_fs_drain_queue() on stranded
  queued_reqs
--

commit 01cb846c58c4f5ed59cf9d9ee391ec0c2e114c82
Author: Qihang <q.h.hack.winter@gmail.com>

fuse: virtio-fs: stop new requests before draining queues

This commit adds a per-virtqueue aborting flag to reject new requests
(other than FUSE_DESTROY) before draining queues during teardown, and
offloads blocking request end callbacks from the dispatch worker.

> diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
> index ba8d79c4bcac6..a46536c9fdc8e 100644
> --- a/fs/fuse/virtio_fs.c
> +++ b/fs/fuse/virtio_fs.c
[ ... ]
> @@ -593,9 +633,17 @@ static void virtio_fs_request_dispatch_work(struct work_struct *work)
>  		if (ret < 0) {
>  			if (ret == -ENOSPC) {
>  				spin_lock(&fsvq->lock);
> -				list_add_tail(&req->list, &fsvq->queued_reqs);
> -				spin_unlock(&fsvq->lock);
> -				return;
> +				if (!fsvq->connected ||
> +				    (fsvq->aborting &&
> +				     req->in.h.opcode != FUSE_DESTROY)) {
> +					spin_unlock(&fsvq->lock);
> +					ret = -ENOTCONN;
> +				} else {
> +					list_add_tail(&req->list,
> +						      &fsvq->queued_reqs);
> +					spin_unlock(&fsvq->lock);
> +					return;
> +				}
>  			}
[ ... ]
> @@ -1226,6 +1274,28 @@ static void virtio_fs_stop_all_queues(struct virtio_fs *fs)
>  	}
>  }
>  
> +/*
> + * Stop admitting new requests before draining the queues.  Admission and
> + * virtqueue submission are serialized by fsvq->lock, so a request is
> + * either rejected before reaching the device or accounted in in_flight
> + * and covered by the drain.
> + *
> + * FUSE_DESTROY is exempt because fuse_conn_destroy() sends it after the
> + * first drain and waits for its completion before aborting the channel.
> + */
> +static void virtio_fs_stop_new_requests(struct virtio_fs *fs)
> +{
> +	struct virtio_fs_vq *fsvq;
> +	int i;
> +
> +	for (i = 0; i < fs->nvqs; i++) {
> +		fsvq = &fs->vqs[i];
> +		spin_lock(&fsvq->lock);
> +		fsvq->aborting = true;
> +		spin_unlock(&fsvq->lock);
> +	}
> +}
[ ... ]
> @@ -1551,12 +1622,22 @@ static void virtio_fs_send_req(struct fuse_iqueue *fiq, struct fuse_req *req)
>  			/*
>  			 * Virtqueue full. Retry submission from worker
>  			 * context as we might be holding fc->chan->bg_lock.
> +			 * Re-check admission under the same lock that
> +			 * increments in_flight: teardown may have started
> +			 * between enqueue_req() dropping its lock and here.
>  			 */
>  			spin_lock(&fsvq->lock);
> -			list_add_tail(&req->list, &fsvq->queued_reqs);
> -			inc_in_flight_req(fsvq);
> -			spin_unlock(&fsvq->lock);
> -			return;
> +			if (!fsvq->connected ||
> +			    (fsvq->aborting &&
> +			     req->in.h.opcode != FUSE_DESTROY)) {
> +				spin_unlock(&fsvq->lock);
> +				ret = -ENOTCONN;
> +			} else {
> +				list_add_tail(&req->list, &fsvq->queued_reqs);
> +				inc_in_flight_req(fsvq);
> +				spin_unlock(&fsvq->lock);
> +				return;
> +			}
>  		}

[Severity: Medium]
Can a request added to fsvq->queued_reqs on -ENOSPC still be stranded with
fsvq->in_flight > 0 and hang virtio_fs_drain_queue()?

virtio_fs_enqueue_req() drops fsvq->lock before returning -ENOSPC, and
neither virtio_fs_send_req() nor virtio_fs_request_dispatch_work()
schedules fsvq->dispatch_work when placing req on fsvq->queued_reqs; they
rely on virtio_fs_requests_done_work() seeing a non-empty queued_reqs when
in-flight virtqueue requests complete.

If the virtqueue drains during the window where fsvq->lock is dropped,
and the caller then re-acquires fsvq->lock before teardown sets
fsvq->aborting, nothing kicks fsvq->dispatch_work:

CPU0                              CPU1 (kworker)
virtio_fs_send_req()
  virtio_fs_enqueue_req()
    spin_lock(&fsvq->lock)
    virtqueue_add_sgs() -> -ENOSPC
    spin_unlock(&fsvq->lock)
                                  virtio_fs_requests_done_work()
                                    spin_lock(&fsvq->lock)
                                    /* drains all vq buffers */
                                    !list_empty(&fsvq->queued_reqs) -> false
                                    spin_unlock(&fsvq->lock)
  spin_lock(&fsvq->lock)
  /* connected && !aborting */
  list_add_tail(..., &fsvq->queued_reqs)
  inc_in_flight_req(fsvq)
  spin_unlock(&fsvq->lock)

When virtio_fs_conn_destroy() then runs virtio_fs_stop_new_requests() and
virtio_fs_drain_all_queues(), fsvq->vq is already empty so
virtio_fs_requests_done_work() never runs again, and virtio_fs_drain_queue()
waits forever in wait_for_completion(&fsvq->in_flight_zero) before
reaching flush_work(&fsvq->dispatch_work).  (The same window exists in
virtio_fs_request_dispatch_work() after it removes req from queued_reqs,
and against virtio_fs_stop_all_queues() in virtio_fs_remove().)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109023957.34500-1-q.h.hack.winter@gmail.com?part=1