Re: [PATCH v2 8/8] fuse: support io-uring registered buffers

[Joanne Koong <joannelkoong@gmail.com>]

On Thu, Nov 6, 2025 at 11:48 AM Bernd Schubert <bernd@bsbernd.com> wrote:
>
> On 10/27/25 23:28, Joanne Koong wrote:
> > Add support for io-uring registered buffers for fuse daemons
> > communicating through the io-uring interface. Daemons may register
> > buffers ahead of time, which will eliminate the overhead of
> > pinning/unpinning user pages and translating virtual addresses for every
> > server-kernel interaction.
> >
> > To support page-aligned payloads, the buffer is structured such that the
> > payload is at the front of the buffer and the fuse_uring_req_header is
> > offset from the end of the buffer.
> >
> > To be backwards compatible, fuse uring still needs to support non-registered
> > buffers as well.
> >
> > Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
> > ---
> >  fs/fuse/dev_uring.c   | 200 +++++++++++++++++++++++++++++++++---------
> >  fs/fuse/dev_uring_i.h |  27 +++++-
> >  2 files changed, 183 insertions(+), 44 deletions(-)
> >
> > diff --git a/fs/fuse/dev_uring.c b/fs/fuse/dev_uring.c
> > index c6b22b14b354..f501bc81f331 100644
> > --- a/fs/fuse/dev_uring.c
> > +++ b/fs/fuse/dev_uring.c
> >
> > +/*
> > + * Prepare fixed buffer for access. Sets up the payload iter and kmaps the
> > + * header.
> > + *
> > + * Callers must call fuse_uring_unmap_buffer() in the same scope to release the
> > + * header mapping.
> > + *
> > + * For non-fixed buffers, this is a no-op.
> > + */
> > +static int fuse_uring_map_buffer(struct fuse_ring_ent *ent)
> > +{
> > +     size_t header_size = sizeof(struct fuse_uring_req_header);
> > +     struct iov_iter iter;
> > +     struct page *header_page;
> > +     size_t count, start;
> > +     ssize_t copied;
> > +     int err;
> > +
> > +     if (!ent->fixed_buffer)
> > +             return 0;
> > +
> > +     err = io_uring_cmd_import_fixed_full(ITER_DEST, &iter, ent->cmd, 0);
>
> This seems to be a rather expensive call, especially as it gets
> called twice (during submit and fetch).
> Wouldn't be there be a possibility to check if the user buffer changed
> and then keep the existing iter? I think Caleb had a similar idea
> in patch 1/8.

I think the best approach is to get rid of the call entirely by
returning -EBUSY to the server if it tries unregistering the buffers
while a connection is still alive. Then we would just have to set this
up once at registration time, and use that for the lifetime of the
connection. The discussion about this with Pavel is in [1] - I'm
planning to do this as a separate follow-up.

[1] https://lore.kernel.org/linux-fsdevel/9f0debb1-ce0e-4085-a3fe-0da7a8fd76a6@gmail.com/

>
> > +     if (err)
> > +             return err;
> > +
> > +     count = iov_iter_count(&iter);
> > +     if (count < header_size || count & (PAGE_SIZE - 1))
> > +             return -EINVAL;
>
> || !PAGE_ALIGNED(count)) ?

Nice, I didn't realize this macro existed. Thanks.

>
> > +
> > +     /* Adjust the payload iter to protect the header from any overwrites */
> > +     ent->payload_iter = iter;
> > +     iov_iter_truncate(&ent->payload_iter, count - header_size);
> > +
> > +     /* Set up the headers */
> > +     iov_iter_advance(&iter, count - header_size);
> > +     copied = iov_iter_get_pages2(&iter, &header_page, header_size, 1, &start);
>
> The iter is later used for the payload, but I miss a reset? iov_iter_revert()?

This iter is separate from the payload iter and doesn't affect the
payload iter's values because the "ent->payload_iter = iter;"
assignment above shallow copies that out first.

>
> > +     if (copied < header_size)
> > +             return -EFAULT;
> > +     ent->headers = kmap_local_page(header_page) + start;
>
> My plan for the alternative pinning patch (with io-uring) was to let the
> header be shared by multiple entries. Current libfuse master handles
> a fixed page size buffer for the payload (prepared page pinning - I
> didn't expect I was blocked for 9 months on other work), missing is to
> share it between ring entries.
> I think this wouldn't work with registered buffer approach - it
> always needs one full page?

I've been working on the patches for zero-copy and that has required
the design for registered buffers in this patch to change, namely that
the payload and the headers must be separated out. For v3, I have them
separate now.
>
> I would also like to discuss dynamic multiple payload sizes per queue.
> For example to have something like
>
> 256 x 4K
> 8 x 128K
> 4 x 1M

I think zero-copy might obviate the need for this. The way I have it
right now, it uses sparse buffers for payloads, which prevents the
server from needing to allocate the 1M buffer per ent. I'm hoping to
send out the patches for this as part of v3 at the end of next week or
next next week.

Thanks,
joanne

>
> I think there are currently two ways to do that
>
> 1) Sort entries into pools
> 2) Sort buffers into pools and let entries use these. Here the header
> would be fixed and payload would come from a pool.
>
> With the appraoch to have payload and header in one buffer we couldn't
> use 2). Using 1) should be fine, though.
>
> >
> >  /*
> > @@ -1249,20 +1358,29 @@ static void fuse_uring_send_in_task(struct io_uring_cmd *cmd,
> >  {
> >       struct fuse_ring_ent *ent = uring_cmd_to_ring_ent(cmd);
> >       struct fuse_ring_queue *queue = ent->queue;
> > +     bool send_ent = true;
> >       int err;
> >
> > -     if (!(issue_flags & IO_URING_F_TASK_DEAD)) {
> > -             err = fuse_uring_prepare_send(ent, ent->fuse_req);
> > -             if (err) {
> > -                     if (!fuse_uring_get_next_fuse_req(ent, queue))
> > -                             return;
> > -                     err = 0;
> > -             }
> > -     } else {
> > -             err = -ECANCELED;
> > +     if (issue_flags & IO_URING_F_TASK_DEAD) {
> > +             fuse_uring_send(ent, cmd, -ECANCELED, issue_flags);
> > +             return;
> > +     }
> > +
> > +     err = fuse_uring_map_buffer(ent);
> > +     if (err) {
> > +             fuse_uring_req_end(ent, ent->fuse_req, err);
> > +             return;
>
> I think this needs to abort the connection now. There could be multiple
> commands on the queue and they would be stuck now and there is no
> notification to fuse server either.

This approach makes sense to me and makes things a bit simpler. I'll
add this to v3.

Thanks,
Joanne