Re: [PATCH v6 4/6] fuse: support registered buffer pools in io-uring

On 8/13/26 00:56, Joanne Koong wrote:
> On Tue, Aug 11, 2026 at 3:45 PM Bernd Schubert <[email protected]> wrote:
>>
>>
>>
>> On 7/16/26 19:59, Joanne Koong wrote:
>>> Allow servers to use a buffer pool that is also registered through
>>> io-uring. When the server registers a buffer pool with io-uring, the
>>> pages backing the pool are pinned upfront. This eliminates the overhead
>>> of pinning/unpinning user pages and translating virtual addresses per
>>> i/o request. This also allows servers to use the same registered memory
>>> for subsequent backing store I/O (eg read_fixed/write_fixed), keeping
>>> data in the same pinned pages without additional pinning or mapping
>>> overhead required.
>>>
>>> To use this, the server needs to set the FUSE_URING_REGISTERED_BUFPOOL
>>> flag when adding a bufpool through the FUSE_IO_URING_CMD_ADD_BUFPOOL
>>> cmd. For every sqe submitted (including the one for adding the bufpool),
>>> it should set sqe->uring_cmd_flags to include IORING_URING_CMD_FIXED,
>>> and pass in the index where the registered bufpool resides to
>>> sqe->buf_index.
>>>
>>> Benchmarked with passthrough_hp (--nopassthrough, q_depth=8) on a
>>> 2-socket Intel Xeon Gold 6138 (40 cores / 80 threads), using fio (sync
>>> engine, bs=1M, O_DIRECT, numjobs=2, 30s run + 10s ramp, 3 runs) where
>>> direct-I/O throughput is against a RAM-backed (tmpfs) source (backing
>>> I/O is not the bottleneck):
>>>
>>>                   baseline      registered buffers
>>>   direct read       ~5.1 GB/s     ~5.4 GB/s   (+~5%)
>>>   direct write      ~3.4 GB/s     ~4.8 GB/s   (+~45%)
>>>
>>> Registered buffers bring up the write path speed up closer to speed of
>>> reads. There isn't much improvement for reads because it is already fast
>>> enough where it's at the copy-bound ceiling (surpassing that requires
>>> doing zero-copy). On a device-bound NVMe though, the differences are
>>> within noise, as backing I/O dominates per-request latency.
>>>
>>> Signed-off-by: Joanne Koong <[email protected]>
>>> ---
>>>  fs/fuse/dev_uring.c       | 122 +++++++++++++++++++++++++++++++-------
>>>  fs/fuse/dev_uring_i.h     |   8 +++
>>>  include/uapi/linux/fuse.h |   4 ++
>>>  3 files changed, 112 insertions(+), 22 deletions(-)
>>>
>>> diff --git a/fs/fuse/dev_uring.c b/fs/fuse/dev_uring.c
>>> index e9da92b46090..ed7e783bd68d 100644
>>> --- a/fs/fuse/dev_uring.c
>>> +++ b/fs/fuse/dev_uring.c
>>> @@ -22,6 +22,8 @@ MODULE_PARM_DESC(enable_uring,
>>>  #define FUSE_URING_IOV_HEADERS 0
>>>  #define FUSE_URING_IOV_PAYLOAD 1
>>>
>>> +#define FUSE_URING_ADD_BUFPOOL_FLAGS (FUSE_URING_REGISTERED_BUFPOOL)
>>> +
>>>  bool fuse_uring_enabled(void)
>>>  {
>>>       return enable_uring;
>>> @@ -47,6 +49,27 @@ static inline bool bufpool_enabled(struct fuse_ring_queue *queue)
>>>       return queue->payload_mode == FUSE_PAYLOAD_BUFPOOL;
>>>  }
>>>
>>> +static inline bool bufpool_registered(struct fuse_ring_queue *queue)
>>> +{
>>> +     return queue->bufpool && queue->bufpool->registered;
>>> +}
>>> +
>>> +/*
>>> + * For a registered bufpool, every sqe that drives a payload import (REGISTER,
>>> + * COMMIT_AND_FETCH) must carry the registered buffer index of the pool.
>>> + * This also must be called from the command's issue handler, where cmd->sqe is
>>> + * still valid
>>> + */
>>> +static inline bool fuse_uring_cmd_index_ok(struct io_uring_cmd *cmd,
>>> +                                        struct fuse_ring_queue *queue)
>>> +{
>>> +     if (!bufpool_registered(queue))
>>> +             return true;
>>> +
>>> +     return (cmd->flags & IORING_URING_CMD_FIXED) &&
>>> +            READ_ONCE(cmd->sqe->buf_index) == queue->bufpool->registered_index;
>>> +}
>>> +
>>>  static void uring_cmd_set_ring_ent(struct io_uring_cmd *cmd,
>>>                                  struct fuse_ring_ent *ring_ent)
>>>  {
>>> @@ -653,19 +676,42 @@ static int copy_header_from_ring(struct fuse_ring_ent *ent,
>>>       return 0;
>>>  }
>>>
>>> +static int fuse_uring_import_payload(struct fuse_ring_ent *ent, int dir,
>>> +                                  struct iov_iter *iter,
>>> +                                  unsigned int issue_flags)
>>> +{
>>> +     void __user *base = ent->payload.iov_base;
>>> +     size_t len = ent->payload.iov_len;
>>> +     int err = 0;
>>> +
>>> +     if (!base) {
>>> +             memset(iter, 0, sizeof(*iter));
>>> +             return 0;
>>> +     }
>>> +
>>> +     if (bufpool_registered(ent->queue))
>>> +             err = io_uring_cmd_import_fixed((u64)(uintptr_t)base, len, dir,
>>> +                                             iter, ent->cmd, issue_flags);
>>> +     else
>>> +             err = import_ubuf(dir, base, len, iter);
>>
>> Hmm ok, just an annotation, I have to admit that I don't like
>> issue_flags too much. Especially for the optimization that DDN has by
>> avoiding io_uring_cmd_complete_in_task(), IO_URING_F_UNLOCKED is rather
>> painful. Additionaly with that optimization the lock might get
>> contended. The new addition here doesn't introduce it, but also doesn't
>> make it any better.
> 
> For what it's worth, the locks aren't taken on these paths (eg sends
> go through fuse_uring_dispatch_ent() ->
> io_uring_cmd_complete_in_task() and task work runs with
> ctx->uring_lock held, and commits run in the command's issue handler,
> where the lock is likewise held).

Yeah, except for the optimization that avoids
io_uring_cmd_complete_in_task/fuse_uring_send_in_task
and submits the request directly from the caller.
Anyway, it was just an annotation, as this lock is not new.

> 
> Threading the issue_flags through gets a bit annoying, but I
> personally don't see a better way for core io-uring infrastructure to
> determine whether a given call path already holds the lock or not.
> 

The flag doesn't work that well with the optimization when the caller is
in io-uring context, we need to figure out that with Jens/Pavel later on.

>>
>>> +
>>> +     if (err)
>>> +             pr_info_ratelimited("fuse: Import of user buffer failed\n");
>>> +
>>> +     return err;
>>> +}
>>> +
>>> @@ -1362,8 +1421,21 @@ static int fuse_uring_add_bufpool(struct io_uring_cmd *cmd,
>>>       uintptr_t pool_uaddr;
>>>       unsigned int pool_len, nr_bufs;
>>>       size_t pool_size, buf_size;
>>> +     bool registered = flags & FUSE_URING_REGISTERED_BUFPOOL;
>>> +
>>> +     if (!ring || qid >= ring->nr_queues)
>>> +             return -EINVAL;
>>>
>>> -     if (!ring || qid >= ring->nr_queues || flags)
>>> +     if (flags & ~FUSE_URING_ADD_BUFPOOL_FLAGS)
>>> +             return -EINVAL;
>>> +
>>> +     /*
>>> +      * A registered bufpool is reached through an io_uring fixed buffer, so
>>> +      * this command must be submitted with IORING_URING_CMD_FIXED and the
>>> +      * registered buffer index in sqe->buf_index. That index is recorded
>>> +      * below and enforced on every REGISTER / COMMIT_AND_FETCH sqe.
>>> +      */
>>> +     if (registered && !(cmd->flags & IORING_URING_CMD_FIXED))
>>>               return -EINVAL;
>>
>> Hmm, why do we actually need FUSE_URING_REGISTERED_BUFPOOL if the
>> decision can be made by IORING_URING_CMD_FIXED?
> 
> It's not strictly needed, but imo it makes uapi more explicit and the
> contract more clear. I don't feel strongly about this though, I'm fine
> with either approach.

I don't have a strong opinion, it just looks a bit weird to have a flag
that could be used and then to introduce another one and give userspace
even more choices and possible failure points.
Would there ever be a reason to register a buffer, but not to use that
in fuse?

> 
>>
>>>
>>> diff --git a/fs/fuse/dev_uring_i.h b/fs/fuse/dev_uring_i.h
>>> index cdf56f8b38b5..e142cae43022 100644
>>> --- a/fs/fuse/dev_uring_i.h
>>> +++ b/fs/fuse/dev_uring_i.h
>>> @@ -49,6 +49,14 @@ enum fuse_queue_payload_mode {
>>>  };
>>>
>>>  struct fuse_bufpool {
>>> +     bool registered;
>>> +
>>> +     /*
>>> +      * io_uring registered buffer table index for this pool, bound at
>>> +      * ADD_BUFPOOL time. Only valid if the bufpool is registered
>>> +      */
>>> +     u16 registered_index;
>>
>> Dunno, could this go into the previous commit or be reserved in the
>> previous commit? This addition make bisect hard due to ABI change?
> 
> This is an addition to a kernel-internal struct, not an uapi-defined
> one, so I don't think this affects ABI. The previous commit doesn't
> have any notion of registered buffers, so I think moving these
> definitions to the previous commit would be confusing.

Ah yes sorry, my fault, I shouldn't do reviews past midnight. Sorry again.


Thanks,
Bernd