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

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.

Thanks,
Bernd