Re: [PATCH RFC v2 10/19] fuse: {uring} Handle SQEs - register commands

[Josef Bacik <josef@toxicpanda.com>]

On Wed, May 29, 2024 at 08:00:45PM +0200, Bernd Schubert wrote:
> This adds basic support for ring SQEs (with opcode=IORING_OP_URING_CMD).
> For now only FUSE_URING_REQ_FETCH is handled to register queue entries.
> 
> Signed-off-by: Bernd Schubert <bschubert@ddn.com>
> ---
>  fs/fuse/dev.c             |   1 +
>  fs/fuse/dev_uring.c       | 267 ++++++++++++++++++++++++++++++++++++++++++++++
>  fs/fuse/dev_uring_i.h     |  12 +++
>  include/uapi/linux/fuse.h |  33 ++++++
>  4 files changed, 313 insertions(+)
> 
> diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
> index cd5dc6ae9272..05a87731b5c3 100644
> --- a/fs/fuse/dev.c
> +++ b/fs/fuse/dev.c
> @@ -2510,6 +2510,7 @@ const struct file_operations fuse_dev_operations = {
>  	.compat_ioctl   = compat_ptr_ioctl,
>  #if IS_ENABLED(CONFIG_FUSE_IO_URING)
>  	.mmap		= fuse_uring_mmap,
> +	.uring_cmd	= fuse_uring_cmd,
>  #endif
>  };
>  EXPORT_SYMBOL_GPL(fuse_dev_operations);
> diff --git a/fs/fuse/dev_uring.c b/fs/fuse/dev_uring.c
> index 2c0ccb378908..48b1118b64f4 100644
> --- a/fs/fuse/dev_uring.c
> +++ b/fs/fuse/dev_uring.c
> @@ -31,6 +31,27 @@
>  #include <linux/topology.h>
>  #include <linux/io_uring/cmd.h>
>  
> +static void fuse_ring_ring_ent_unset_userspace(struct fuse_ring_ent *ent)
> +{
> +	clear_bit(FRRS_USERSPACE, &ent->state);
> +	list_del_init(&ent->list);
> +}
> +
> +/* Update conn limits according to ring values */
> +static void fuse_uring_conn_cfg_limits(struct fuse_ring *ring)
> +{
> +	struct fuse_conn *fc = ring->fc;
> +
> +	WRITE_ONCE(fc->max_pages, min_t(unsigned int, fc->max_pages,
> +					ring->req_arg_len / PAGE_SIZE));
> +
> +	/* This not ideal, as multiplication with nr_queue assumes the limit
> +	 * gets reached when all queues are used, but a single threaded
> +	 * application might already do that.
> +	 */
> +	WRITE_ONCE(fc->max_background, ring->nr_queues * ring->max_nr_async);
> +}
> +
>  /*
>   * Basic ring setup for this connection based on the provided configuration
>   */
> @@ -329,3 +350,249 @@ int fuse_uring_queue_cfg(struct fuse_ring *ring,
>  	return 0;
>  }
>  
> +/*
> + * Put a ring request onto hold, it is no longer used for now.
> + */
> +static void fuse_uring_ent_avail(struct fuse_ring_ent *ring_ent,
> +				 struct fuse_ring_queue *queue)
> +	__must_hold(&queue->lock)

Sorry I'm just now bringing this up, but I'd love to see a

lockdep_assert_held(<whatever lock>);

in every place where you use __must_hold, so I get a nice big warning when I'm
running stuff.  I don't always run sparse, but I always test with lockdep on,
and that'll help me notice problems.

> +{
> +	struct fuse_ring *ring = queue->ring;
> +
> +	/* unsets all previous flags - basically resets */
> +	pr_devel("%s ring=%p qid=%d tag=%d state=%lu async=%d\n", __func__,
> +		 ring, ring_ent->queue->qid, ring_ent->tag, ring_ent->state,
> +		 ring_ent->async);
> +
> +	if (WARN_ON(test_bit(FRRS_USERSPACE, &ring_ent->state))) {
> +		pr_warn("%s qid=%d tag=%d state=%lu async=%d\n", __func__,
> +			ring_ent->queue->qid, ring_ent->tag, ring_ent->state,
> +			ring_ent->async);
> +		return;
> +	}
> +
> +	WARN_ON_ONCE(!list_empty(&ring_ent->list));
> +
> +	if (ring_ent->async)
> +		list_add(&ring_ent->list, &queue->async_ent_avail_queue);
> +	else
> +		list_add(&ring_ent->list, &queue->sync_ent_avail_queue);
> +
> +	set_bit(FRRS_WAIT, &ring_ent->state);
> +}
> +
> +/*
> + * fuse_uring_req_fetch command handling
> + */
> +static int fuse_uring_fetch(struct fuse_ring_ent *ring_ent,
> +			    struct io_uring_cmd *cmd, unsigned int issue_flags)
> +__must_hold(ring_ent->queue->lock)
> +{
> +	struct fuse_ring_queue *queue = ring_ent->queue;
> +	struct fuse_ring *ring = queue->ring;
> +	int ret = 0;
> +	int nr_ring_sqe;
> +
> +	/* register requests for foreground requests first, then backgrounds */
> +	if (queue->nr_req_sync >= ring->max_nr_sync) {
> +		queue->nr_req_async++;
> +		ring_ent->async = 1;
> +	} else
> +		queue->nr_req_sync++;

IIRC the style guidelines say if you use { in any part of the if, you've got to
use them for all of it.  But that may just be what we do in btrfs.  Normally I
wouldn't nit about it but I have comments elsewhere for this patch.

> +
> +	fuse_uring_ent_avail(ring_ent, queue);
> +
> +	if (queue->nr_req_sync + queue->nr_req_async > ring->queue_depth) {
> +		/* should be caught by ring state before and queue depth
> +		 * check before
> +		 */
> +		WARN_ON(1);
> +		pr_info("qid=%d tag=%d req cnt (fg=%d async=%d exceeds depth=%zu",
> +			queue->qid, ring_ent->tag, queue->nr_req_sync,
> +			queue->nr_req_async, ring->queue_depth);
> +		ret = -ERANGE;
> +	}
> +
> +	if (ret)
> +		goto out; /* erange */

This can just be

if (whatever) {
	WARN_ON_ONCE(1);
	return -ERANGE;
}

instead of the goto out thing.

> +
> +	WRITE_ONCE(ring_ent->cmd, cmd);
> +
> +	nr_ring_sqe = ring->queue_depth * ring->nr_queues;
> +	if (atomic_inc_return(&ring->nr_sqe_init) == nr_ring_sqe) {
> +		fuse_uring_conn_cfg_limits(ring);
> +		ring->ready = 1;
> +	}
> +
> +out:
> +	return ret;

And this can just be return 0 here with the above change.

> +}
> +
> +static struct fuse_ring_queue *
> +fuse_uring_get_verify_queue(struct fuse_ring *ring,
> +			    const struct fuse_uring_cmd_req *cmd_req,
> +			    unsigned int issue_flags)
> +{
> +	struct fuse_conn *fc = ring->fc;
> +	struct fuse_ring_queue *queue;
> +	int ret;
> +
> +	if (!(issue_flags & IO_URING_F_SQE128)) {
> +		pr_info("qid=%d tag=%d SQE128 not set\n", cmd_req->qid,
> +			cmd_req->tag);
> +		ret = -EINVAL;
> +		goto err;
> +	}
> +
> +	if (unlikely(!fc->connected)) {
> +		ret = -ENOTCONN;
> +		goto err;
> +	}
> +
> +	if (unlikely(!ring->configured)) {
> +		pr_info("command for a connection that is not ring configured\n");
> +		ret = -ENODEV;
> +		goto err;
> +	}
> +
> +	if (unlikely(cmd_req->qid >= ring->nr_queues)) {
> +		pr_devel("qid=%u >= nr-queues=%zu\n", cmd_req->qid,
> +			 ring->nr_queues);
> +		ret = -EINVAL;
> +		goto err;
> +	}
> +
> +	queue = fuse_uring_get_queue(ring, cmd_req->qid);
> +	if (unlikely(queue == NULL)) {
> +		pr_info("Got NULL queue for qid=%d\n", cmd_req->qid);
> +		ret = -EIO;
> +		goto err;
> +	}
> +
> +	if (unlikely(!queue->configured || queue->stopped)) {
> +		pr_info("Ring or queue (qid=%u) not ready.\n", cmd_req->qid);
> +		ret = -ENOTCONN;
> +		goto err;
> +	}
> +
> +	if (cmd_req->tag > ring->queue_depth) {
> +		pr_info("tag=%u > queue-depth=%zu\n", cmd_req->tag,
> +			ring->queue_depth);
> +		ret = -EINVAL;
> +		goto err;
> +	}
> +
> +	return queue;
> +
> +err:
> +	return ERR_PTR(ret);

There's no cleanup here, so just make all the above

return ERR_PTR(-whatever)

instead of the goto err thing.

> +}
> +
> +/**
> + * Entry function from io_uring to handle the given passthrough command
> + * (op cocde IORING_OP_URING_CMD)
> + */

Docstyle thing.

> +int fuse_uring_cmd(struct io_uring_cmd *cmd, unsigned int issue_flags)
> +{
> +	const struct fuse_uring_cmd_req *cmd_req = io_uring_sqe_cmd(cmd->sqe);
> +	struct fuse_dev *fud = fuse_get_dev(cmd->file);
> +	struct fuse_conn *fc = fud->fc;
> +	struct fuse_ring *ring = fc->ring;
> +	struct fuse_ring_queue *queue;
> +	struct fuse_ring_ent *ring_ent = NULL;
> +	u32 cmd_op = cmd->cmd_op;
> +	int ret = 0;
> +
> +	if (!ring) {
> +		ret = -ENODEV;
> +		goto out;
> +	}
> +
> +	queue = fuse_uring_get_verify_queue(ring, cmd_req, issue_flags);
> +	if (IS_ERR(queue)) {
> +		ret = PTR_ERR(queue);
> +		goto out;
> +	}
> +
> +	ring_ent = &queue->ring_ent[cmd_req->tag];
> +
> +	pr_devel("%s:%d received: cmd op %d qid %d (%p) tag %d  (%p)\n",
> +		 __func__, __LINE__, cmd_op, cmd_req->qid, queue, cmd_req->tag,
> +		 ring_ent);
> +
> +	spin_lock(&queue->lock);
> +	if (unlikely(queue->stopped)) {
> +		/* XXX how to ensure queue still exists? Add
> +		 * an rw ring->stop lock? And take that at the beginning
> +		 * of this function? Better would be to advise uring
> +		 * not to call this function at all? Or free the queue memory
> +		 * only, on daemon PF_EXITING?
> +		 */
> +		ret = -ENOTCONN;
> +		goto err_unlock;
> +	}
> +
> +	if (current == queue->server_task)
> +		queue->uring_cmd_issue_flags = issue_flags;
> +
> +	switch (cmd_op) {
> +	case FUSE_URING_REQ_FETCH:

This is all organized kind of oddly, I think I'd prefer if you put all the code
from above where we grab the queue lock and the bit below into a helper.

So instead of

spin_lock(&queue->lock);
blah

switch (cmd_op) {
case FUSE_URING_REQ_FETCH:
	blah
default:
	ret = -EINVAL;
}

you have

static int fuse_uring_req_fetch(queue, cmd, issue_flags)
{
	ring_ent = blah;
	spin_lock(&queue->lock);
	<blah>
	spin_unlock(&que->lock);
	return ret;
}

then

switch (cmd_op) {
case FUSE_URING_REQ_FETCH:
	ret = fuse_uring_req_fetch(queue, cmd, issue_flags);
	break;
default:
	ret = -EINVAL;
	break;
}

Alternatively just pushe all the queue stuff down into the case
FUSE_URING_REQ_FETCH part, but I think the helper is cleaner.

> +		if (queue->server_task == NULL) {
> +			queue->server_task = current;
> +			queue->uring_cmd_issue_flags = issue_flags;
> +		}
> +
> +		/* No other bit must be set here */
> +		if (ring_ent->state != BIT(FRRS_INIT)) {
> +			pr_info_ratelimited(
> +				"qid=%d tag=%d register req state %lu expected %lu",
> +				cmd_req->qid, cmd_req->tag, ring_ent->state,
> +				BIT(FRRS_INIT));
> +			ret = -EINVAL;
> +			goto err_unlock;
> +		}
> +
> +		fuse_ring_ring_ent_unset_userspace(ring_ent);
> +
> +		ret = fuse_uring_fetch(ring_ent, cmd, issue_flags);
> +		if (ret)
> +			goto err_unlock;
> +
> +		/*
> +		 * The ring entry is registered now and needs to be handled
> +		 * for shutdown.
> +		 */
> +		atomic_inc(&ring->queue_refs);
> +
> +		spin_unlock(&queue->lock);
> +		break;
> +	default:
> +		ret = -EINVAL;
> +		pr_devel("Unknown uring command %d", cmd_op);
> +		goto err_unlock;
> +	}
> +out:
> +	pr_devel("uring cmd op=%d, qid=%d tag=%d ret=%d\n", cmd_op,
> +		 cmd_req->qid, cmd_req->tag, ret);
> +
> +	if (ret < 0) {
> +		if (ring_ent != NULL) {

You don't pull anything from ring_ent in the pr_info, so maybe drop the extra
if statement?  Thanks,

Josef