Re: [PATCH RFC v2 13/19] fuse: {uring} Handle uring shutdown

[Josef Bacik <josef@toxicpanda.com>]

On Wed, May 29, 2024 at 08:00:48PM +0200, Bernd Schubert wrote:
> Signed-off-by: Bernd Schubert <bschubert@ddn.com>
> ---
>  fs/fuse/dev.c         |  10 +++
>  fs/fuse/dev_uring.c   | 194 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  fs/fuse/dev_uring_i.h |  67 +++++++++++++++++
>  3 files changed, 271 insertions(+)
> 
> diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
> index a7d26440de39..6ffd216b27c8 100644
> --- a/fs/fuse/dev.c
> +++ b/fs/fuse/dev.c
> @@ -2202,6 +2202,8 @@ void fuse_abort_conn(struct fuse_conn *fc)
>  		fc->connected = 0;
>  		spin_unlock(&fc->bg_lock);
>  
> +		fuse_uring_set_stopped(fc);
> +
>  		fuse_set_initialized(fc);
>  		list_for_each_entry(fud, &fc->devices, entry) {
>  			struct fuse_pqueue *fpq = &fud->pq;
> @@ -2245,6 +2247,12 @@ void fuse_abort_conn(struct fuse_conn *fc)
>  		spin_unlock(&fc->lock);
>  
>  		fuse_dev_end_requests(&to_end);
> +
> +		/*
> +		 * fc->lock must not be taken to avoid conflicts with io-uring
> +		 * locks
> +		 */
> +		fuse_uring_abort(fc);

Perhaps a 

lockdep_assert_not_held(&fc->lock)

in fuse_uring_abort() then?

>  	} else {
>  		spin_unlock(&fc->lock);
>  	}
> @@ -2256,6 +2264,8 @@ void fuse_wait_aborted(struct fuse_conn *fc)
>  	/* matches implicit memory barrier in fuse_drop_waiting() */
>  	smp_mb();
>  	wait_event(fc->blocked_waitq, atomic_read(&fc->num_waiting) == 0);
> +
> +	fuse_uring_wait_stopped_queues(fc);
>  }
>  
>  int fuse_dev_release(struct inode *inode, struct file *file)
> diff --git a/fs/fuse/dev_uring.c b/fs/fuse/dev_uring.c
> index 5269b3f8891e..6001ba4d6e82 100644
> --- a/fs/fuse/dev_uring.c
> +++ b/fs/fuse/dev_uring.c
> @@ -48,6 +48,44 @@ fuse_uring_async_send_to_ring(struct io_uring_cmd *cmd,
>  	io_uring_cmd_done(cmd, 0, 0, issue_flags);
>  }
>  
> +/* Abort all list queued request on the given ring queue */
> +static void fuse_uring_abort_end_queue_requests(struct fuse_ring_queue *queue)
> +{
> +	struct fuse_req *req;
> +	LIST_HEAD(sync_list);
> +	LIST_HEAD(async_list);
> +
> +	spin_lock(&queue->lock);
> +
> +	list_for_each_entry(req, &queue->sync_fuse_req_queue, list)
> +		clear_bit(FR_PENDING, &req->flags);
> +	list_for_each_entry(req, &queue->async_fuse_req_queue, list)
> +		clear_bit(FR_PENDING, &req->flags);
> +
> +	list_splice_init(&queue->async_fuse_req_queue, &sync_list);
> +	list_splice_init(&queue->sync_fuse_req_queue, &async_list);
> +
> +	spin_unlock(&queue->lock);
> +
> +	/* must not hold queue lock to avoid order issues with fi->lock */
> +	fuse_dev_end_requests(&sync_list);
> +	fuse_dev_end_requests(&async_list);
> +}
> +
> +void fuse_uring_abort_end_requests(struct fuse_ring *ring)
> +{
> +	int qid;
> +
> +	for (qid = 0; qid < ring->nr_queues; qid++) {
> +		struct fuse_ring_queue *queue = fuse_uring_get_queue(ring, qid);
> +
> +		if (!queue->configured)
> +			continue;
> +
> +		fuse_uring_abort_end_queue_requests(queue);
> +	}
> +}
> +
>  /* Update conn limits according to ring values */
>  static void fuse_uring_conn_cfg_limits(struct fuse_ring *ring)
>  {
> @@ -361,6 +399,162 @@ int fuse_uring_queue_cfg(struct fuse_ring *ring,
>  	return 0;
>  }
>  
> +static void fuse_uring_stop_fuse_req_end(struct fuse_ring_ent *ent)
> +{
> +	struct fuse_req *req = ent->fuse_req;
> +
> +	ent->fuse_req = NULL;
> +	clear_bit(FRRS_FUSE_REQ, &ent->state);
> +	clear_bit(FR_SENT, &req->flags);
> +	req->out.h.error = -ECONNABORTED;
> +	fuse_request_end(req);
> +}
> +
> +/*
> + * Release a request/entry on connection shutdown
> + */
> +static bool fuse_uring_try_entry_stop(struct fuse_ring_ent *ent,
> +				      bool need_cmd_done)
> +	__must_hold(ent->queue->lock)
> +{
> +	struct fuse_ring_queue *queue = ent->queue;
> +	bool released = false;
> +
> +	if (test_bit(FRRS_FREED, &ent->state))
> +		goto out; /* no work left, freed before */

Just return false;

> +
> +	if (ent->state == BIT(FRRS_INIT) || test_bit(FRRS_WAIT, &ent->state) ||
> +	    test_bit(FRRS_USERSPACE, &ent->state)) {

Again, apologies for just now noticing this, but this is kind of a complicated
state machine.

I think I'd rather you just use ent->state as an actual state machine, so it has
one value and one value only at any given time, which appears to be what happens
except in that we have FRRS_INIT set in addition to whatever other bit is set.

Rework this so it's less complicated, because it's quite difficult to follow in
it's current form.  Thanks,

Josef