Support readv/writev with fixed buffers, and introduce IOSQE_FIXED_BUFFER,
consistent with fixed files.
I don't like it at all, see issues below. The actual implementation would
be much uglier.
I propose you to split the series and push separately. Your first 6 patches
first, I don't have conceptual objections to them. Then registration sharing
(I still need to look it up). And then we can return to this, if you're not
yet convinced.
Ok. The sharing patch is actually the highest priority for us so it'd
be great to know if you think it's in the right direction.
Should I submit them as they are or address your fixed_file_ref comments
in Patch 4/8 as well? Would I need your prep patch beforehand?
+static ssize_t io_import_iovec_fixed(int rw, struct io_kiocb *req, void *buf,
+ unsigned segs, unsigned fast_segs,
+ struct iovec **iovec,
+ struct iov_iter *iter)
+{
+ struct io_ring_ctx *ctx = req->ctx;
+ struct io_mapped_ubuf *imu;
+ struct iovec *iov;
+ u16 index, buf_index;
+ ssize_t ret;
+ unsigned long seg;
+
+ if (unlikely(!ctx->buf_data))
+ return -EFAULT;
+
+ ret = import_iovec(rw, buf, segs, fast_segs, iovec, iter);
Did you test it? import_iovec() does access_ok() against each iov_base,
which in your case are an index.
I used liburing:test/file-{register,update} as models for the equivalent
buffer tests and they seem to work. I can send out the tests and the
liburing changes if you want.
The fixed io test registers an empty iov table first:
ret = io_uring_register_buffers(ring, iovs, UIO_MAXIOV);
It next updates the table with two actual buffers at offset 768:
ret = io_uring_register_buffers_update(ring, 768, ups, 2);
It later uses the buffer at index 768 for writev similar to the
file-register test (IOSQE_FIXED_BUFFER instead of IOSQE_FIXED_FILE):
iovs[768].iov_base = (void *)768;
iovs[768].iov_len = pagesize;
io_uring_prep_writev(sqe, fd, iovs, 1, 0);
sqe->flags |= IOSQE_FIXED_BUFFER;
ret = io_uring_submit(ring);
Below is the iovec returned from
io_import_iovec_fixed()
-> io_import_vec()
{iov_base = 0x300 <dm_early_create+412>, iov_len = 4096}
+ if (ret < 0)
+ return ret;
+
+ iov = (struct iovec *)iter->iov;
+
+ for (seg = 0; seg < iter->nr_segs; seg++) {
+ buf_index = *(u16 *)(&iov[seg].iov_base);
That's ugly, and also not consistent with rw_fixed, because iov_base is
used to calculate offset.
Would offset be applicable when using readv/writev?
My thinkig was that for those cases, each iovec should be used exactly
as registered.
+ if (unlikely(buf_index < 0 || buf_index >= ctx->nr_user_bufs))
+ return -EFAULT;
+
+ index = array_index_nospec(buf_index, ctx->nr_user_bufs);
+ imu = io_buf_from_index(ctx, index);
+ if (!imu->ubuf || !imu->len)
+ return -EFAULT;
+ if (iov[seg].iov_len > imu->len)
+ return -EFAULT;
+
+ iov[seg].iov_base = (void *)imu->ubuf;
Nope, that's not different from non registered version.
What import_fixed actually do is setting up the iter argument to point
to a bvec (a vector of struct page *).
So in fact, the buffers end up being pinned again because they are not
being as bvecs?
So it either would need to keep a vector of bvecs, that's a vector of vectors,
that's not supported by iter, etc., so you'll also need to iterate over them
in io_read/write and so on. Or flat 2D structure into 1D, but that's still ugly.
So you're saying there's no clean way to create a readv/writev + fixed
buffers API? It would've been nice to have a consistent API between
files and buffers.
@@ -5692,7 +5743,7 @@ static int io_timeout_remove_prep(struct io_kiocb *req,
{
if (unlikely(req->ctx->flags & IORING_SETUP_IOPOLL))
return -EINVAL;
- if (unlikely(req->flags & (REQ_F_FIXED_FILE | REQ_F_BUFFER_SELECT)))
+ if (unlikely(req->flags & (REQ_F_FIXED_RSRC | REQ_F_BUFFER_SELECT)))
Why it's here?
#define REQ_F_FIXED_RSRC (REQ_F_FIXED_FILE | REQ_F_FIXED_BUFFER)
So, why do you | with REQ_F_BUFFER_SELECT again here?
I thought to group fixed files/buffers but distinguish them from
selected buffers.
@@ -87,6 +88,8 @@ enum {
#define IOSQE_ASYNC (1U << IOSQE_ASYNC_BIT)
/* select buffer from sqe->buf_group */
#define IOSQE_BUFFER_SELECT (1U << IOSQE_BUFFER_SELECT_BIT)
+/* use fixed buffer set */
+#define IOSQE_FIXED_BUFFER (1U << IOSQE_FIXED_BUFFER_BIT)
Unfortenatuly, we're almost out of flags bits -- it's a 1 byte
field and 6 bits are already taken. Let's not use it.
