Re: [PATCH RFC v6 06/16] fuse: {uring} Handle SQEs - register commands

[Miklos Szeredi <miklos@szeredi.hu>]

On Fri, 22 Nov 2024 at 00:44, Bernd Schubert <bschubert@ddn.com> wrote:

> +static struct fuse_ring *fuse_uring_create(struct fuse_conn *fc)
> +{
> +       struct fuse_ring *ring = NULL;
> +       size_t nr_queues = num_possible_cpus();
> +       struct fuse_ring *res = NULL;
> +
> +       ring = kzalloc(sizeof(*fc->ring) +
> +                              nr_queues * sizeof(struct fuse_ring_queue),

Left over from a previous version?

> +static void fuse_uring_ent_avail(struct fuse_ring_ent *ring_ent,
> +                                struct fuse_ring_queue *queue)
> +       __must_hold(&queue->lock)
> +{
> +       struct fuse_ring *ring = queue->ring;
> +
> +       lockdep_assert_held(&queue->lock);
> +
> +       /* unsets all previous flags - basically resets */
> +       pr_devel("%s ring=%p qid=%d state=%d\n", __func__, ring,
> +                ring_ent->queue->qid, ring_ent->state);
> +
> +       if (WARN_ON(ring_ent->state != FRRS_COMMIT)) {
> +               pr_warn("%s qid=%d state=%d\n", __func__, ring_ent->queue->qid,
> +                       ring_ent->state);
> +               return;
> +       }
> +
> +       list_move(&ring_ent->list, &queue->ent_avail_queue);
> +
> +       ring_ent->state = FRRS_WAIT;
> +}

AFAICS this function is essentially just one line, the rest is
debugging.   While it's good for initial development it's bad for
review because the of the bad signal to noise ratio (the debugging
part is irrelevant for code review).

Would it make sense to post the non-RFC version without most of the
pr_debug()/pr_warn() stuff and just keep the simple WARN_ON() lines
that signal if something has gone wrong.

Long term we could get rid of some of that too.   E.g ring_ent->state
seems to be there just for debugging, but if the code is clean enough
we don't need to have a separate state.

> +#if 0
> +       /* Does not work as sending over io-uring is async */
> +       err = -ETXTBSY;
> +       if (fc->initialized) {
> +               pr_info_ratelimited(
> +                       "Received FUSE_URING_REQ_FETCH after connection is initialized\n");
> +               return err;
> +       }
> +#endif

I fail to remember what's up with this.  Why is it important that
FETCH is sent before INIT reply?

> diff --git a/fs/fuse/fuse_dev_i.h b/fs/fuse/fuse_dev_i.h
> index 6c506f040d5fb57dae746880c657a95637ac50ce..e82cbf9c569af4f271ba0456cb49e0a5116bf36b 100644
> --- a/fs/fuse/fuse_dev_i.h
> +++ b/fs/fuse/fuse_dev_i.h
> @@ -8,6 +8,7 @@
>
>  #include <linux/types.h>
>
> +

Unneeded extra line.

Thanks,
Miklos