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

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.

>
> > +
> > +     /* 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.

Thanks,
Joanne