Re: [PATCH 2/8] io_uring: add infra for importing vectored reg buffers

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

On Mon, Mar 3, 2025 at 7:51 AM Pavel Begunkov <asml.silence@gmail.com> wrote:
>
> Add io_import_reg_vec(), which will be responsible for importing
> vectored registered buffers. iovecs are overlapped with the resulting
> bvec in memory, which is why the iovec is expected to be padded in
> iou_vec.
>
> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
> ---
>  include/linux/io_uring_types.h |   5 +-
>  io_uring/rsrc.c                | 124 +++++++++++++++++++++++++++++++++
>  io_uring/rsrc.h                |   5 ++
>  3 files changed, 133 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/io_uring_types.h b/include/linux/io_uring_types.h
> index 9101f12d21ef..b770a2b12da6 100644
> --- a/include/linux/io_uring_types.h
> +++ b/include/linux/io_uring_types.h
> @@ -111,7 +111,10 @@ struct io_uring_task {
>  };
>
>  struct iou_vec {
> -       struct iovec            *iovec;
> +       union {
> +               struct iovec    *iovec;
> +               struct bio_vec  *bvec;
> +       };

If I understand correctly, io_import_reg_vec() converts the iovecs to
bio_vecs in place. If an iovec expands to more than one bio_vec (i.e.
crosses a folio boundary), wouldn't the bio_vecs overwrite iovecs that
hadn't been processed yet?

>         unsigned                nr;
>  };
>
> diff --git a/io_uring/rsrc.c b/io_uring/rsrc.c
> index 9b05e614819e..1ec1f5b3e385 100644
> --- a/io_uring/rsrc.c
> +++ b/io_uring/rsrc.c
> @@ -1267,9 +1267,133 @@ int io_register_clone_buffers(struct io_ring_ctx *ctx, void __user *arg)
>
>  void io_vec_free(struct iou_vec *iv)
>  {
> +       BUILD_BUG_ON(sizeof(struct bio_vec) > sizeof(struct iovec));
> +
>         if (!iv->iovec)
>                 return;
>         kfree(iv->iovec);
>         iv->iovec = NULL;
>         iv->nr = 0;
>  }
> +
> +int io_vec_realloc(struct iou_vec *iv, unsigned nr_entries)
> +{
> +       struct iovec *iov;
> +
> +       WARN_ON_ONCE(nr_entries <= 0);
> +
> +       iov = kmalloc_array(nr_entries, sizeof(iov[0]), GFP_KERNEL);
> +       if (!iov)
> +               return -ENOMEM;
> +
> +       io_vec_free(iv);
> +       iv->iovec = iov;
> +       iv->nr = nr_entries;
> +       return 0;
> +}
> +
> +static int io_vec_fill_bvec(int ddir, struct iov_iter *iter,
> +                               struct io_mapped_ubuf *imu,
> +                               struct iovec *iovec, int nr_iovs,
> +                               struct iou_vec *vec)
> +{
> +       unsigned long folio_size = (1 << imu->folio_shift);
> +       unsigned long folio_mask = folio_size - 1;
> +       struct bio_vec *res_bvec = vec->bvec;
> +       size_t total_len = 0;
> +       int bvec_idx = 0;
> +       int iov_idx;
> +
> +       for (iov_idx = 0; iov_idx < nr_iovs; iov_idx++) {
> +               size_t iov_len = iovec[iov_idx].iov_len;
> +               u64 buf_addr = (u64)iovec[iov_idx].iov_base;
> +               u64 folio_addr = imu->ubuf & ~folio_mask;

The computation of folio_addr could be moved out of the loop.

> +               struct bio_vec *src_bvec;
> +               size_t offset;
> +               u64 buf_end;
> +
> +               if (unlikely(check_add_overflow(buf_addr, (u64)iov_len, &buf_end)))
> +                       return -EFAULT;
> +               if (unlikely(buf_addr < imu->ubuf || buf_end > (imu->ubuf + imu->len)))
> +                       return -EFAULT;
> +
> +               total_len += iov_len;
> +               /* by using folio address it also accounts for bvec offset */
> +               offset = buf_addr - folio_addr;
> +               src_bvec = imu->bvec + (offset >> imu->folio_shift);
> +               offset &= folio_mask;
> +
> +               for (; iov_len; offset = 0, bvec_idx++, src_bvec++) {
> +                       size_t seg_size = min_t(size_t, iov_len,
> +                                               folio_size - offset);
> +
> +                       res_bvec[bvec_idx].bv_page = src_bvec->bv_page;
> +                       res_bvec[bvec_idx].bv_offset = offset;
> +                       res_bvec[bvec_idx].bv_len = seg_size;

Could just increment res_bvec to avoid the variable bvec_idx?

> +                       iov_len -= seg_size;
> +               }
> +       }
> +       if (total_len > MAX_RW_COUNT)
> +               return -EINVAL;
> +
> +       iov_iter_bvec(iter, ddir, res_bvec, bvec_idx, total_len);
> +       return 0;
> +}
> +
> +static int io_estimate_bvec_size(struct iovec *iov, unsigned nr_iovs,
> +                                struct io_mapped_ubuf *imu)
> +{
> +       unsigned shift = imu->folio_shift;
> +       size_t max_segs = 0;
> +       unsigned i;
> +
> +       for (i = 0; i < nr_iovs; i++)
> +               max_segs += (iov[i].iov_len >> shift) + 2;

Sees like this may overestimate a bit. I think something like this
would give the exact number of segments for each iovec?
(((u64)iov_base & folio_mask) + iov_len + folio_mask) >> folio_shift

> +       return max_segs;
> +}
> +
> +int io_import_reg_vec(int ddir, struct iov_iter *iter,
> +                       struct io_kiocb *req, struct iou_vec *vec,
> +                       int nr_iovs, unsigned iovec_off,
> +                       unsigned issue_flags)
> +{
> +       struct io_rsrc_node *node;
> +       struct io_mapped_ubuf *imu;
> +       unsigned cache_nr;
> +       struct iovec *iov;
> +       unsigned nr_segs;
> +       int ret;
> +
> +       node = io_find_buf_node(req, issue_flags);
> +       if (!node)
> +               return -EFAULT;
> +       imu = node->buf;
> +       if (imu->is_kbuf)
> +               return -EOPNOTSUPP;
> +
> +       iov = vec->iovec + iovec_off;
> +       ret = io_estimate_bvec_size(iov, nr_iovs, imu);
> +       if (ret < 0)
> +               return ret;

io_estimate_bvec_size() doesn't (intentionally) return an error code,
just an unsigned value cast to an int.

Best,
Caleb

> +       nr_segs = ret;
> +       cache_nr = vec->nr;
> +
> +       if (WARN_ON_ONCE(iovec_off + nr_iovs != cache_nr) ||
> +           nr_segs > cache_nr) {
> +               struct iou_vec tmp_vec = {};
> +
> +               ret = io_vec_realloc(&tmp_vec, nr_segs);
> +               if (ret)
> +                       return ret;
> +
> +               iovec_off = tmp_vec.nr - nr_iovs;
> +               memcpy(tmp_vec.iovec + iovec_off, iov, sizeof(*iov) * nr_iovs);
> +               io_vec_free(vec);
> +
> +               *vec = tmp_vec;
> +               iov = vec->iovec + iovec_off;
> +               req->flags |= REQ_F_NEED_CLEANUP;
> +       }
> +
> +       return io_vec_fill_bvec(ddir, iter, imu, iov, nr_iovs, vec);
> +}
> diff --git a/io_uring/rsrc.h b/io_uring/rsrc.h
> index e3f1cfb2ff7b..769ef5d76a4b 100644
> --- a/io_uring/rsrc.h
> +++ b/io_uring/rsrc.h
> @@ -61,6 +61,10 @@ struct io_rsrc_node *io_find_buf_node(struct io_kiocb *req,
>  int io_import_reg_buf(struct io_kiocb *req, struct iov_iter *iter,
>                         u64 buf_addr, size_t len, int ddir,
>                         unsigned issue_flags);
> +int io_import_reg_vec(int ddir, struct iov_iter *iter,
> +                       struct io_kiocb *req, struct iou_vec *vec,
> +                       int nr_iovs, unsigned iovec_off,
> +                       unsigned issue_flags);
>
>  int io_register_clone_buffers(struct io_ring_ctx *ctx, void __user *arg);
>  int io_sqe_buffers_unregister(struct io_ring_ctx *ctx);
> @@ -146,6 +150,7 @@ static inline void __io_unaccount_mem(struct user_struct *user,
>  }
>
>  void io_vec_free(struct iou_vec *iv);
> +int io_vec_realloc(struct iou_vec *iv, unsigned nr_entries);
>
>  static inline void io_vec_reset_iovec(struct iou_vec *iv,
>                                       struct iovec *iovec, unsigned nr)
> --
> 2.48.1
>
>