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

[Bernd Schubert <bernd.schubert@fastmail.fm>]

On 11/23/24 10:52, Miklos Szeredi wrote:
> 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?

Why? This struct holds all the queues? We could also put into fc, but
it would take additional memory, even if uring is not used.

> 
>> +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.

I can remove the pr_debug/pr_warn lines for the non-RFC version, that
will come early next week.

> 
> 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.

Almost, please see 

[PATCH RFC v6 15/16] fuse: {io-uring} Prevent mount point hang on fuse-server termination

there you really need a ring state, because access is outside of lists.
Unless you want to iterate over the lists, if the the entry is still
in there. Please see the discussion with Joanne in RFC v5.
I have also added in v6 15/16 comments about non-list access.

> 
>> +#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?

That is basically what I try to explain with the comment,
it does not work. I had left it in the code, so that you would
run over it, looks like that worked  :)

Even though libfuse sends the SQEs before
setting up /dev/fuse threads, handling the SQEs takes longer.
So what happens is that while IORING_OP_URING_CMD/FUSE_URING_REQ_FETCH
are coming in, FUSE_INIT reply gets through. In userspace we do not
know at all, when these SQEs are registered, because we don't get
a reply. Even worse, we don't even know if io-uring works at all and
cannot adjust number of /dev/fuse handling threads. Here setup with
ioctls had a clear advantage - there was a clear reply.

The other issue is, that we will probably first need handle FUSE_INIT
in userspace before sending SQEs at all, in order to know the payload
buffer size.

> 
>> 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.

Yeah, I didn't review these patches on my own yet, there are probably
more tiny things like that all over the place - will be done by next
with in the non-RFC version.


Thanks,
Bernd