Re: [PATCH v1 23/30] io_uring/rsrc: split io_buffer_register_request() logic

[Caleb Sander Mateos <csander@purestorage.com>]

On Fri, Dec 12, 2025 at 9:24 PM Joanne Koong <joannelkoong@gmail.com> wrote:
>
> On Sun, Dec 7, 2025 at 5:42 PM Caleb Sander Mateos
> <csander@purestorage.com> wrote:
> >
> > On Tue, Dec 2, 2025 at 4:37 PM Joanne Koong <joannelkoong@gmail.com> wrote:
> > >
> > > Split the main initialization logic in io_buffer_register_request() into
> > > a helper function.
> > >
> > > This is a preparatory patch for supporting kernel-populated buffers in
> > > fuse io-uring, which will be reusing this logic.
> > >
> > > Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
> > > ---
> > >  io_uring/rsrc.c | 80 +++++++++++++++++++++++++++++--------------------
> > >  1 file changed, 48 insertions(+), 32 deletions(-)
> > >
> > > diff --git a/io_uring/rsrc.c b/io_uring/rsrc.c
> > > index 59cafe63d187..18abba6f6b86 100644
> > > --- a/io_uring/rsrc.c
> > > +++ b/io_uring/rsrc.c
> > > @@ -941,63 +941,79 @@ int io_sqe_buffers_register(struct io_ring_ctx *ctx, void __user *arg,
> > >         return ret;
> > >  }
> > >
> > > -int io_buffer_register_request(struct io_ring_ctx *ctx, struct request *rq,
> > > -                              void (*release)(void *), unsigned int index,
> > > -                              unsigned int issue_flags)
> > > +static int io_buffer_init(struct io_ring_ctx *ctx, unsigned int nr_bvecs,
> >
> > Consider adding "kernel" somewhere in the name to distinguish this
> > from the userspace registered buffer initialization
>
> I can rename this to io_kernel_buffer_init().

Sounds good.

Thanks,
Caleb

>
> >
> > > +                         unsigned int total_bytes, u8 dir,
> > > +                         void (*release)(void *), void *priv,
> > > +                         unsigned int index)
> > >  {
> > >         struct io_rsrc_data *data = &ctx->buf_table;
> > > -       struct req_iterator rq_iter;
> > >         struct io_mapped_ubuf *imu;
> > >         struct io_rsrc_node *node;
> > > -       struct bio_vec bv;
> > > -       unsigned int nr_bvecs = 0;
> > > -       int ret = 0;
> > >
> > > -       io_ring_submit_lock(ctx, issue_flags);
> > > -       if (index >= data->nr) {
> > > -               ret = -EINVAL;
> > > -               goto unlock;
> > > -       }
> > > +       if (index >= data->nr)
> > > +               return -EINVAL;
> > >         index = array_index_nospec(index, data->nr);
> > >
> > > -       if (data->nodes[index]) {
> > > -               ret = -EBUSY;
> > > -               goto unlock;
> > > -       }
> > > +       if (data->nodes[index])
> > > +               return -EBUSY;
> > >
> > >         node = io_rsrc_node_alloc(ctx, IORING_RSRC_BUFFER);
> > > -       if (!node) {
> > > -               ret = -ENOMEM;
> > > -               goto unlock;
> > > -       }
> > > +       if (!node)
> > > +               return -ENOMEM;
> > >
> > > -       /*
> > > -        * blk_rq_nr_phys_segments() may overestimate the number of bvecs
> > > -        * but avoids needing to iterate over the bvecs
> > > -        */
> > > -       imu = io_alloc_imu(ctx, blk_rq_nr_phys_segments(rq));
> > > +       imu = io_alloc_imu(ctx, nr_bvecs);
> > >         if (!imu) {
> > >                 kfree(node);
> > > -               ret = -ENOMEM;
> > > -               goto unlock;
> > > +               return -ENOMEM;
> > >         }
> > >
> > >         imu->ubuf = 0;
> > > -       imu->len = blk_rq_bytes(rq);
> > > +       imu->len = total_bytes;
> > >         imu->acct_pages = 0;
> > >         imu->folio_shift = PAGE_SHIFT;
> > > +       imu->nr_bvecs = nr_bvecs;
> > >         refcount_set(&imu->refs, 1);
> > >         imu->release = release;
> > > -       imu->priv = rq;
> > > +       imu->priv = priv;
> > >         imu->is_kbuf = true;
> > > -       imu->dir = 1 << rq_data_dir(rq);
> > > +       imu->dir = 1 << dir;
> > > +
> > > +       node->buf = imu;
> > > +       data->nodes[index] = node;
> > > +
> > > +       return 0;
> > > +}
> > > +
> > > +int io_buffer_register_request(struct io_ring_ctx *ctx, struct request *rq,
> > > +                              void (*release)(void *), unsigned int index,
> > > +                              unsigned int issue_flags)
> > > +{
> > > +       struct req_iterator rq_iter;
> > > +       struct io_mapped_ubuf *imu;
> > > +       struct bio_vec bv;
> > > +       unsigned int nr_bvecs;
> > > +       unsigned int total_bytes;
> > > +       int ret;
> > > +
> > > +       io_ring_submit_lock(ctx, issue_flags);
> > > +
> > > +       /*
> > > +        * blk_rq_nr_phys_segments() may overestimate the number of bvecs
> > > +        * but avoids needing to iterate over the bvecs
> > > +        */
> > > +       nr_bvecs = blk_rq_nr_phys_segments(rq);
> > > +       total_bytes = blk_rq_bytes(rq);
> >
> > These could be initialized before io_ring_submit_lock()
> >
> > > +       ret = io_buffer_init(ctx, nr_bvecs, total_bytes, rq_data_dir(rq), release, rq,
> > > +                            index);
> > > +       if (ret)
> > > +               goto unlock;
> > >
> > > +       imu = ctx->buf_table.nodes[index]->buf;
> >
> > It would be nice to avoid all these additional dereferences. Could
> > io_buffer_init() return the struct io_mapped_ubuf *, using ERR_PTR()
> > to return any error code?
>
> I'll make this change for v2.
>
> Thanks,
> Joanne
> >
> > Best,
> > Caleb