Re: [PATCH v9 10/17] fuse: Add io-uring sqe commit and fetch support

[Bernd Schubert <bernd@bsbernd.com>]

On 1/22/25 01:49, Bernd Schubert wrote:
> 
> 
> On 1/22/25 01:45, Joanne Koong wrote:
>> On Tue, Jan 21, 2025 at 4:18 PM Bernd Schubert <bernd@bsbernd.com> wrote:
>>>
>> ...
>>>>>
>>>>>>
>>>>>>> +
>>>>>>> +       err = fuse_ring_ent_set_commit(ring_ent);
>>>>>>> +       if (err != 0) {
>>>>>>> +               pr_info_ratelimited("qid=%d commit_id %llu state %d",
>>>>>>> +                                   queue->qid, commit_id, ring_ent->state);
>>>>>>> +               spin_unlock(&queue->lock);
>>>>>>> +               return err;
>>>>>>> +       }
>>>>>>> +
>>>>>>> +       ring_ent->cmd = cmd;
>>>>>>> +       spin_unlock(&queue->lock);
>>>>>>> +
>>>>>>> +       /* without the queue lock, as other locks are taken */
>>>>>>> +       fuse_uring_commit(ring_ent, issue_flags);
>>>>>>> +
>>>>>>> +       /*
>>>>>>> +        * Fetching the next request is absolutely required as queued
>>>>>>> +        * fuse requests would otherwise not get processed - committing
>>>>>>> +        * and fetching is done in one step vs legacy fuse, which has separated
>>>>>>> +        * read (fetch request) and write (commit result).
>>>>>>> +        */
>>>>>>> +       fuse_uring_next_fuse_req(ring_ent, queue, issue_flags);
>>>>>>
>>>>>> If there's no request ready to read next, then no request will be
>>>>>> fetched and this will return. However, as I understand it, once the
>>>>>> uring is registered, userspace should only be interacting with the
>>>>>> uring via FUSE_IO_URING_CMD_COMMIT_AND_FETCH. However for the case
>>>>>> where no request was ready to read, it seems like userspace would have
>>>>>> nothing to commit when it wants to fetch the next request?
>>>>>
>>>>> We have
>>>>>
>>>>> FUSE_IO_URING_CMD_REGISTER
>>>>> FUSE_IO_URING_CMD_COMMIT_AND_FETCH
>>>>>
>>>>>
>>>>> After _CMD_REGISTER the corresponding ring-entry is ready to get fuse
>>>>> requests and waiting. After it gets a request assigned and handles it
>>>>> by fuse server the _COMMIT_AND_FETCH scheme applies. Did you possibly
>>>>> miss that _CMD_REGISTER will already have it waiting?
>>>>>
>>>>
>>>> Sorry for the late reply. After _CMD_REGISTER and _COMMIT_AND_FETCH,
>>>> it seems possible that there is no fuse request waiting until a later
>>>> time? This is the scenario I'm envisioning:
>>>> a) uring registers successfully and fetches request through _CMD_REGISTER
>>>> b) server replies to request and fetches new request through _COMMIT_AND_FETCH
>>>> c) server replies to request, tries to fetch new request but no
>>>> request is ready, through _COMMIT_AND_FETCH
>>>>
>>>> maybe I'm missing something in my reading of the code, but how will
>>>> the server then fetch the next request once the request is ready? It
>>>> will have to commit something in order to fetch it since there's only
>>>> _COMMIT_AND_FETCH which requires a commit, no?
>>>>
>>>
>>> The right name would be '_COMMIT_AND_FETCH_OR_WAIT'. Please see
>>> fuse_uring_next_fuse_req().
>>>
>>> retry:
>>>         spin_lock(&queue->lock);
>>>         fuse_uring_ent_avail(ent, queue);           --> entry available
>>>         has_next = fuse_uring_ent_assign_req(ent);
>>>         spin_unlock(&queue->lock);
>>>
>>>         if (has_next) {
>>>                 err = fuse_uring_send_next_to_ring(ent, issue_flags);
>>>                 if (err)
>>>                         goto retry;
>>>         }
>>>
>>>
>>> If there is no available request, the io-uring cmd stored in ent->cmd is
>>> just queued/available.
>>
>> Could you point me to where the wait happens?  I think that's the part
>> I'm missing. In my reading of the code, if there's no available
>> request (eg queue->fuse_req_queue is empty), then I see that has_next
>> will return false and fuse_uring_next_fuse_req() /
>> fuse_uring_commit_fetch() returns without having fetched anything.
>> Where does the "if there is no available request, the io-uring cmd is
>> just queued/available" happen?
>>
> 
> You need to read it the other way around, without "has_next" the 
> avail/queued entry is not removed from the list - it is available 
> whenever a new request comes in. Looks like we either need refactoring 
> or at least a comment.

It also not the current task operation that waits - that happens in
io-uring with 'io_uring_submit_and_wait' and wait-nr > 0. In fuse is is
really just _not_ running io_uring_cmd_done() that make ent->cmd to be
available.

Does it help?


Thanks,
Bernd