Re: [PATCH v4 19/25] fuse: add io-uring kernel-managed buffer ring

[Bernd Schubert <bernd@bsbernd.com>]

On 2/26/26 00:42, Joanne Koong wrote:
> On Wed, Feb 25, 2026 at 9:55 AM Bernd Schubert <bernd@bsbernd.com> wrote:
>> On 1/28/26 22:44, Bernd Schubert wrote:
>>> On 1/17/26 00:30, Joanne Koong wrote:
>>>> diff --git a/fs/fuse/dev_uring.c b/fs/fuse/dev_uring.c
>>>> @@ -940,6 +1188,7 @@ static int fuse_uring_commit_fetch(struct io_uring_cmd *cmd, int issue_flags,
>>>>      unsigned int qid = READ_ONCE(cmd_req->qid);
>>>>      struct fuse_pqueue *fpq;
>>>>      struct fuse_req *req;
>>>> +    bool send;
>>>>
>>>>      err = -ENOTCONN;
>>>>      if (!ring)
>>>> @@ -990,7 +1239,12 @@ static int fuse_uring_commit_fetch(struct io_uring_cmd *cmd, int issue_flags,
>>>>
>>>>      /* without the queue lock, as other locks are taken */
>>>>      fuse_uring_prepare_cancel(cmd, issue_flags, ent);
>>>> -    fuse_uring_commit(ent, req, issue_flags);
>>>> +
>>>> +    err = fuse_uring_headers_prep(ent, ITER_SOURCE, issue_flags);
>>>> +    if (err)
>>>> +            fuse_uring_req_end(ent, req, err);
>>>> +    else
>>>> +            fuse_uring_commit(ent, req, issue_flags);
>>>>
>>>>      /*
>>>>       * Fetching the next request is absolutely required as queued
>>>> @@ -998,7 +1252,9 @@ static int fuse_uring_commit_fetch(struct io_uring_cmd *cmd, int issue_flags,
>>>>       * and fetching is done in one step vs legacy fuse, which has separated
>>>>       * read (fetch request) and write (commit result).
>>>>       */
>>>> -    if (fuse_uring_get_next_fuse_req(ent, queue))
>>>> +    send = fuse_uring_get_next_fuse_req(ent, queue, issue_flags);
>>>> +    fuse_uring_headers_cleanup(ent, issue_flags);
>>>> +    if (send)
>>>>              fuse_uring_send(ent, cmd, 0, issue_flags);
>>>>      return 0;
>>
>>
>> Hello Joanne,
>>
>> couldn't it call fuse_uring_headers_cleanup() before the
>> fuse_uring_get_next_fuse_req()? I find it a bit confusing that it firsts
>> gets the next request and then cleans up the buffer from the previous
>> request.
> 
> Hi Bernd,
> 
> Thanks for taking a look.
> 
> The fuse_uring_headers_cleanup() call has to happen after the
> fuse_uring_get_next_fuse_req() call because
> fuse_uring_get_next_fuse_req() copies payload to the header, so we
> can't yet relinquish the refcount on the headers buffer / clean it up
> yet. I can add a comment about this to make this more clear.

I only found time right now and already super late (or early) here.

I guess that is fuse_uring_copy_to_ring -> copy_header_to_ring, but why
can it then call fuse_uring_headers_cleanup() ->
io_uring_fixed_index_put(). I.e. doesn't it put buffer it just copied
to? Why not the sequence of

err = fuse_uring_headers_prep(ent, ITER_SOURCE, issue_flags);
fuse_uring_commit(ent, req, issue_flags);
fuse_uring_headers_cleanup(ent, issue_flags);

And then fuse_uring_get_next_fuse_req() does another
fuse_uring_headers_prep() with ITER_DEST?


> 
>>
>> As I understand it, the the patch basically adds the feature of 0-byte
>> payloads. Maybe worth mentioning in the commit message?
> 
> Hmm I'm not really sure I am seeing where the 0-byte payload gets
> added. On the server side, they don't receive payloads that are
> 0-bytes. If there is no next fuse request to send, then nothing gets
> sent. But maybe I'm not interpreting your comment about 0-byte
> payloads correctly?

There is fuse_uring_req_has_payload() and
fuse_uring_select_buffer()/fuse_uring_next_req_update_buffer() using
that function. When a request doesn't have a payload the ring entries
runs without a payload - effectively that introduces 0-byte payloads,
doesn't it?

> 
>> I also wonder if it would be worth to document as code comment that
>> fuse_uring_ent_assign_req / fuse_uring_next_req_update_buffer are
>> allowed to fail for a buffer upgrade (i.e. 0 to max-payload). At least
> 
> Good idea, I'll add a comment about this.
> 
>> the current comment of  "Fetching the next request is absolutely
>> required" is actually not entirely true anymore.
>>
> 
> I don't think this patch introduces new behavior on this front.
> fuse_uring_get_next_fuse_req() is still called to fetch the next
> request AFAICS.
> 

It still does, but if the request didn't have a payload it might not
have a buffer and if it didn't have a buffer and doesn't manage to get a
buffer, it doesn't handle a request - that a bit change of
'commit-and-fetch always fetches a new request if there is any request
queued'.


Thanks,
Bernd