Re: [PATCH v6 3/6] fuse: add io-uring buffer pools

On Tue, Aug 11, 2026 at 2:28 PM Bernd Schubert <[email protected]> wrote:
>
>
> On 8/7/26 00:18, Joanne Koong wrote:
> > On Thu, Aug 6, 2026 at 11:36 AM Bernd Schubert <[email protected]> wrote:
> >>
> >>> On 7/16/26 19:59, Joanne Koong wrote:
> >>>
> >>> @@ -1397,14 +1578,17 @@ void fuse_uring_queue_fuse_req(struct fuse_iqueue *fiq, struct fuse_req *req)
> >>>       req->ring_queue = queue;
> >>>       ent = list_first_entry_or_null(&queue->ent_avail_queue,
> >>>                                      struct fuse_ring_ent, list);
> >>> -     if (ent)
> >>> -             fuse_uring_add_req_to_ring_ent(ent, req);
> >>> -     else
> >>> -             list_add_tail(&req->list, &queue->fuse_req_queue);
> >>> -     spin_unlock(&queue->lock);
> >>> +     if (ent) {
> >>> +             if (!fuse_uring_prep_buffer(ent, req)) {
> >>> +                     fuse_uring_add_req_to_ring_ent(ent, req);
> >>> +                     spin_unlock(&queue->lock);
> >>> +                     fuse_uring_dispatch_ent(ent);
> >>> +                     return;
> >>> +             }
> >>> +     }
> >>
> >> Pure style, somehow "if (!fuse_uring_prep_buffer(ent, req))" reads like an
> >> error case, *maybe*
> >>
> >>         ent = list_first_entry_or_null(&queue->ent_avail_queue,
> >>                                        struct fuse_ring_ent, list);
> >>         /* no idle entry, or no free pool buffer for this req */
> >>         if (!ent || fuse_uring_prep_buffer(ent, req)) {
> >>                 list_add_tail(&req->list, &queue->fuse_req_queue);
> >>                 spin_unlock(&queue->lock);
> >>                 return;
> >>         }
> >>
> >>         fuse_uring_add_req_to_ring_ent(ent, req);
> >>         spin_unlock(&queue->lock);
> >>         fuse_uring_dispatch_ent(ent);
> >>         return;
> >>
> >> err_unlock:
> >> ...
> >>
> >> and similar in fuse_uring_ent_assign_req()
> >>
> >>         req = list_first_entry_or_null(req_queue, struct fuse_req, list);
> >>         if (!req || fuse_uring_next_req_update_buffer(ent, req)) {
> >>                 fuse_uring_recycle_buffer(ent);
> >>                 return NULL;
> >>         }
> >>
> >>         fuse_uring_add_req_to_ring_ent(ent, req);
> >>         return req;
> >>
> >>
> >> Just a pure style suggestion, though.
> >>
> >
> > Happy to make this change. Thanks for reviewing the patches. I'll wait
> > until you have finished reviewing the other patches in this series
> > before sending this.
> >
> >>>
> >>> -     if (ent)
> >>> -             fuse_uring_dispatch_ent(ent);
> >>> +     list_add_tail(&req->list, &queue->fuse_req_queue);
> >>> +     spin_unlock(&queue->lock);
> >>>
> >>>       return;
> >>>
> >>> +struct fuse_bufpool {
> >>> +     /* starting uaddr of the bufpool */
> >>> +     uintptr_t base_uaddr;
> >>> +
> >>> +     /* size of each buffer in the pool */
> >>> +     size_t buf_size;
> >>> +
> >>> +     /* total number of buffers in the pool */
> >>> +     unsigned int nr_bufs;
> >>> +
> >>> +     /* bitmap tracking which buffers are free */
> >>> +     unsigned long free_map[];
> >>> +};
> >>> +
> >>>  /** A fuse ring entry, part of the ring queue */
> >>>  struct fuse_ring_ent {
> >>>       /* userspace buffer */
> >>>       struct fuse_uring_req_header __user *headers;
> >>> -     void __user *payload;
> >>> +     struct iovec payload;
> >>
> >> Here I have a real concern. What is if fuse server sets up
> >> really really large buffers, to thrash the CPUs?
> >>
> >> setup_fuse_copy_state() is now changed to
> >>
> >> -       err = import_ubuf(dir, ent->payload, ring->max_payload_sz, iter);
> >> +       err = import_ubuf(dir, ent->payload.iov_base, ent->payload.iov_len,
> >> +                         iter);
> >>
> >> And the iov is taken as it from the SQE. Is there actually a really good
> >> reason to have the payload as iov, at least in this series? I could imagine
> >> it might make sense to have it as iov once we make the buf pools more
> >> dynamically usable.
> >>
> >> Could we restrict setup_fuse_copy_state() to ring->max_payload_sz?
> >
> > The payload.iov fields are not taken directly from the bufpool sqe. It
> > is assigned in fuse_uring_select_buffer() to reflect one buffer within
> > the pool. ent->payload.iov_len is set to pool->buf_size, which in
> > fuse_uring_add_bufpool(), we set pool->buf_size to be
> > ring->max_payload_sz. Looking at import_ubuf() though, even if we did
> > pass in the entire buffer pool size here, it is harmless, as the bytes
> > copied are already limited/determined by the request (eg on the
> > send-side, copies are bound by in-arg sizes and on the reply side,
> > fuse_copy_out_args() rejects any payload_sz exceeding sum of the out
> > args before doing any copying). Right now pools hand out buffers of
> > the same size so iov_len tracking isn't needed, but pools will be
> > handing out differently-sized buffers.
>
> Yeah, I noticed later that it is actually not an issue, but had already
> disassembled my equipment and didn't manage to reply before now.
>
> >
> >>
> >>> +
> >>> +     /* buffer id in the pool, if bufpools are used. ignored otherwise */
> >>> +     unsigned int buf_id;
> >>>
> >>> diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
> >>> index cfb055c0c764..f7173b5d4321 100644
> >>> --- a/include/uapi/linux/fuse.h
> >>> +++ b/include/uapi/linux/fuse.h
> >>>  /**
> >>> @@ -1312,6 +1322,14 @@ struct fuse_uring_cmd_req {
> >>>       /* queue the command is for (queue index) */
> >>>       uint16_t qid;
> >>>       uint8_t padding[6];
> >>> +
> >>> +     union {
> >>> +             struct {
> >>> +                     /* base address of bufpool */
> >>> +                     uint64_t uaddr;
> >>> +                     uint32_t len;
> >>> +             } bufpool;
> >>
> >>
> >> Ok, if we ever want to add a new bufpool, I guess we could
> >> invent "struct { } bufpool2;". Personally I would have
> >> preferred to have an index in here, currently always set
> >> to 0, but not a strict requirement.
> >>
> >
> > If we ever want to add a new bufpool, I think it'd be cleaner for the
> > kernel to assign the bufpool id and pass it back in the cqe to the
> > FUSE_IO_URING_CMD_ADD_BUFPOOL cmd rather than the server assigning it.
> > With server-supplied indexes, we would have to also track / prevent
> > collisions, gaps, out-of-range values, etc.
>
> Dunno what your plans with that are, but I would take that number as
> opaque user-space identifier and not something to be used by fuse
> kernel. In my mind without coding, I would put the bufpools into a list
> without an index that fuse kernel would use. IMHO it only needs to check
> if any pool in the list has free bufs.
> Anyway, while I would prefer to have the userspace identifier in the
> struct, that is not a blocker and we can add it in bufpool2 when we have
> actual usage. Or userspace needs to use the identfier from io-uring and
> we use that, as you suggest.

I don't really have a preference here. I can add a zero-enforced
"uint32_t reserved" field in to keep this choice open-ended for now,
that could be repurposed to hold an id in the future, if that becomes
necessary. Even without this though, I don't think we would need a
separate bufpool2 - I think we could just use "flags" to indicate
multi-bufpool, and have an "id" field added to the struct.

Thanks,
Joanne