On Tue, Jan 21, 2025 at 4:55 PM Bernd Schubert <bernd@xxxxxxxxxxx> wrote:
>
>
>
> 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@xxxxxxxxxxx> 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.
Oh I see, the io_uring_cmd_done handles it internally. It's the
.send_req = fuse_uring_queue_fuse_req -> fuse_uring_send_req_in_task()
-> io_uring_cmd_done() that gets triggered and signals to userspace
that a fetch is ready when a new request is available later on. It
makes sense to me now, thanks.
>
> Does it help?
>
>
> Thanks,
> Bernd
