[PATCHSET v6 0/11] Turn single segment imports into ITER_UBUF

[PATCHSET v6 0/11] Turn single segment imports into ITER_UBUF

[Jens Axboe]

Hi,

Rather than repeat the same blurb again, see the v2 posting here:

https://lore.kernel.org/linux-fsdevel/20230327180449.87382-1-axboe@kernel.dk/

tldr - turn single segment iovecs into ITER_UBUF rather than ITER_IOVEC,
because they are more efficient.

Attempt 2 at doing the overlay. The series starts by adding an
iter_iov() helper, which simply returns iter->iov. At the same time we
rename it, to catch anyone using it and to further signify that future
direct uses of it should be discouraged. There are a few manual bits
left, I'll clean those up if we agree this is moving in the right
direction.

Then we get rid of returning an iovec copy with iov_iter_iovec(), and
killing off that function. Two helpers are added to return the current
segment address and length.

Then the usual few iter_is_iovec() -> iter->user_backed changes. For
the alsa part, Takashi did say that single segments could be valid.
But with the rest of the changes, this is no longer interesting as we
don't have to deal with it separately.

Finally, do the last two patches that turn single iovec segments into
ITER_UBUF at import time.

Passes testing, and verified we do the right thing for 1 and multi
segments.

Also viewable here:

https://git.kernel.dk/cgit/linux-block/log/?h=iter-ubuf.2

-- 
Jens Axboe

[PATCH 01/11] block: ensure bio_alloc_map_data() deals with ITER_UBUF correctly

[Jens Axboe]

This helper blindly copies the iovec, even if we don't have one.
Make this case a bit smarter by only doing so if we have an iovec
array to copy.

Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 block/blk-map.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/block/blk-map.c b/block/blk-map.c
index 9137d16cecdc..3bfcad64d67c 100644
--- a/block/blk-map.c
+++ b/block/blk-map.c
@@ -29,10 +29,11 @@ static struct bio_map_data *bio_alloc_map_data(struct iov_iter *data,
 	bmd = kmalloc(struct_size(bmd, iov, data->nr_segs), gfp_mask);
 	if (!bmd)
 		return NULL;
-	memcpy(bmd->iov, data->iov, sizeof(struct iovec) * data->nr_segs);
 	bmd->iter = *data;
-	if (iter_is_iovec(data))
+	if (iter_is_iovec(data)) {
+		memcpy(bmd->iov, data->iov, sizeof(struct iovec) * data->nr_segs);
 		bmd->iter.iov = bmd->iov;
+	}
 	return bmd;
 }
 
-- 
2.39.2

[PATCH 02/11] iov_iter: add iter_iovec() helper

[Jens Axboe]

This returns a pointer to the current iovec entry in the iterator. Only
useful with ITER_IOVEC right now, but it prepares us to treat ITER_UBUF
and ITER_IOVEC identically for the first segment.

Rename struct iov_iter->iov to iov_iter->__iov to find any potentially
troublesome spots, and also to prevent anyone from adding new code that
accesses iter->iov directly.

Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 block/blk-map.c                          |  4 +-
 drivers/infiniband/hw/hfi1/file_ops.c    |  3 +-
 drivers/infiniband/hw/qib/qib_file_ops.c |  2 +-
 drivers/net/tun.c                        |  3 +-
 drivers/vhost/scsi.c                     |  2 +-
 fs/btrfs/file.c                          | 11 +++--
 fs/fuse/file.c                           |  2 +-
 include/linux/uio.h                      |  9 ++--
 io_uring/net.c                           |  4 +-
 io_uring/rw.c                            |  8 ++--
 lib/iov_iter.c                           | 56 +++++++++++++-----------
 sound/core/pcm_native.c                  | 22 ++++++----
 12 files changed, 73 insertions(+), 53 deletions(-)

diff --git a/block/blk-map.c b/block/blk-map.c
index 3bfcad64d67c..04c55f1c492e 100644
--- a/block/blk-map.c
+++ b/block/blk-map.c
@@ -31,8 +31,8 @@ static struct bio_map_data *bio_alloc_map_data(struct iov_iter *data,
 		return NULL;
 	bmd->iter = *data;
 	if (iter_is_iovec(data)) {
-		memcpy(bmd->iov, data->iov, sizeof(struct iovec) * data->nr_segs);
-		bmd->iter.iov = bmd->iov;
+		memcpy(bmd->iov, iter_iov(data), sizeof(struct iovec) * data->nr_segs);
+		bmd->iter.__iov = bmd->iov;
 	}
 	return bmd;
 }
diff --git a/drivers/infiniband/hw/hfi1/file_ops.c b/drivers/infiniband/hw/hfi1/file_ops.c
index b1d6ca7e9708..3065db9d6bb9 100644
--- a/drivers/infiniband/hw/hfi1/file_ops.c
+++ b/drivers/infiniband/hw/hfi1/file_ops.c
@@ -287,11 +287,12 @@ static ssize_t hfi1_write_iter(struct kiocb *kiocb, struct iov_iter *from)
 	}
 
 	while (dim) {
+		const struct iovec *iov = iter_iov(from);
 		int ret;
 		unsigned long count = 0;
 
 		ret = hfi1_user_sdma_process_request(
-			fd, (struct iovec *)(from->iov + done),
+			fd, (struct iovec *)(iov + done),
 			dim, &count);
 		if (ret) {
 			reqs = ret;
diff --git a/drivers/infiniband/hw/qib/qib_file_ops.c b/drivers/infiniband/hw/qib/qib_file_ops.c
index 80fe92a21f96..4cee39337866 100644
--- a/drivers/infiniband/hw/qib/qib_file_ops.c
+++ b/drivers/infiniband/hw/qib/qib_file_ops.c
@@ -2248,7 +2248,7 @@ static ssize_t qib_write_iter(struct kiocb *iocb, struct iov_iter *from)
 	if (!iter_is_iovec(from) || !from->nr_segs || !pq)
 		return -EINVAL;
 
-	return qib_user_sdma_writev(rcd, pq, from->iov, from->nr_segs);
+	return qib_user_sdma_writev(rcd, pq, iter_iov(from), from->nr_segs);
 }
 
 static struct class *qib_class;
diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index ad653b32b2f0..5df1eba7b30a 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -1486,7 +1486,8 @@ static struct sk_buff *tun_napi_alloc_frags(struct tun_file *tfile,
 	skb->truesize += skb->data_len;
 
 	for (i = 1; i < it->nr_segs; i++) {
-		size_t fragsz = it->iov[i].iov_len;
+		const struct iovec *iov = iter_iov(it);
+		size_t fragsz = iov->iov_len;
 		struct page *page;
 		void *frag;
 
diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
index b244e7c0f514..042caea64007 100644
--- a/drivers/vhost/scsi.c
+++ b/drivers/vhost/scsi.c
@@ -671,7 +671,7 @@ vhost_scsi_calc_sgls(struct iov_iter *iter, size_t bytes, int max_sgls)
 {
 	int sgl_count = 0;
 
-	if (!iter || !iter->iov) {
+	if (!iter || !iter_iov(iter)) {
 		pr_err("%s: iter->iov is NULL, but expected bytes: %zu"
 		       " present\n", __func__, bytes);
 		return -EINVAL;
diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index 5cc5a1faaef5..f649647392e0 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -3730,10 +3730,15 @@ static int check_direct_read(struct btrfs_fs_info *fs_info,
 	if (!iter_is_iovec(iter))
 		return 0;
 
-	for (seg = 0; seg < iter->nr_segs; seg++)
-		for (i = seg + 1; i < iter->nr_segs; i++)
-			if (iter->iov[seg].iov_base == iter->iov[i].iov_base)
+	for (seg = 0; seg < iter->nr_segs; seg++) {
+		for (i = seg + 1; i < iter->nr_segs; i++) {
+			const struct iovec *iov1 = iter_iov(iter) + seg;
+			const struct iovec *iov2 = iter_iov(iter) + i;
+
+			if (iov1->iov_base == iov2->iov_base)
 				return -EINVAL;
+		}
+	}
 	return 0;
 }
 
diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index de37a3a06a71..89d97f6188e0 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -1419,7 +1419,7 @@ static ssize_t fuse_cache_write_iter(struct kiocb *iocb, struct iov_iter *from)
 
 static inline unsigned long fuse_get_user_addr(const struct iov_iter *ii)
 {
-	return (unsigned long)ii->iov->iov_base + ii->iov_offset;
+	return (unsigned long)iter_iov(ii)->iov_base + ii->iov_offset;
 }
 
 static inline size_t fuse_get_frag_size(const struct iov_iter *ii,
diff --git a/include/linux/uio.h b/include/linux/uio.h
index 27e3fd942960..4218624b7f78 100644
--- a/include/linux/uio.h
+++ b/include/linux/uio.h
@@ -51,7 +51,8 @@ struct iov_iter {
 	};
 	size_t count;
 	union {
-		const struct iovec *iov;
+		/* use iter_iov() to get the current vec */
+		const struct iovec *__iov;
 		const struct kvec *kvec;
 		const struct bio_vec *bvec;
 		struct xarray *xarray;
@@ -68,6 +69,8 @@ struct iov_iter {
 	};
 };
 
+#define iter_iov(iter)	(iter)->__iov
+
 static inline enum iter_type iov_iter_type(const struct iov_iter *i)
 {
 	return i->iter_type;
@@ -146,9 +149,9 @@ static inline size_t iov_length(const struct iovec *iov, unsigned long nr_segs)
 static inline struct iovec iov_iter_iovec(const struct iov_iter *iter)
 {
 	return (struct iovec) {
-		.iov_base = iter->iov->iov_base + iter->iov_offset,
+		.iov_base = iter_iov(iter)->iov_base + iter->iov_offset,
 		.iov_len = min(iter->count,
-			       iter->iov->iov_len - iter->iov_offset),
+			       iter_iov(iter)->iov_len - iter->iov_offset),
 	};
 }
 
diff --git a/io_uring/net.c b/io_uring/net.c
index 4040cf093318..89e839013837 100644
--- a/io_uring/net.c
+++ b/io_uring/net.c
@@ -184,8 +184,8 @@ static int io_setup_async_msg(struct io_kiocb *req,
 		async_msg->msg.msg_name = &async_msg->addr;
 	/* if were using fast_iov, set it to the new one */
 	if (iter_is_iovec(&kmsg->msg.msg_iter) && !kmsg->free_iov) {
-		size_t fast_idx = kmsg->msg.msg_iter.iov - kmsg->fast_iov;
-		async_msg->msg.msg_iter.iov = &async_msg->fast_iov[fast_idx];
+		size_t fast_idx = iter_iov(&kmsg->msg.msg_iter) - kmsg->fast_iov;
+		async_msg->msg.msg_iter.__iov = &async_msg->fast_iov[fast_idx];
 	}
 
 	return -EAGAIN;
diff --git a/io_uring/rw.c b/io_uring/rw.c
index 4c233910e200..7573a34ea42a 100644
--- a/io_uring/rw.c
+++ b/io_uring/rw.c
@@ -503,10 +503,10 @@ static void io_req_map_rw(struct io_kiocb *req, const struct iovec *iovec,
 	if (!iovec) {
 		unsigned iov_off = 0;
 
-		io->s.iter.iov = io->s.fast_iov;
-		if (iter->iov != fast_iov) {
-			iov_off = iter->iov - fast_iov;
-			io->s.iter.iov += iov_off;
+		io->s.iter.__iov = io->s.fast_iov;
+		if (iter->__iov != fast_iov) {
+			iov_off = iter_iov(iter) - fast_iov;
+			io->s.iter.__iov += iov_off;
 		}
 		if (io->s.fast_iov != fast_iov)
 			memcpy(io->s.fast_iov + iov_off, fast_iov + iov_off,
diff --git a/lib/iov_iter.c b/lib/iov_iter.c
index 274014e4eafe..87488c4aad3f 100644
--- a/lib/iov_iter.c
+++ b/lib/iov_iter.c
@@ -126,13 +126,13 @@ __out:								\
 			iterate_buf(i, n, base, len, off,	\
 						i->ubuf, (I)) 	\
 		} else if (likely(iter_is_iovec(i))) {		\
-			const struct iovec *iov = i->iov;	\
+			const struct iovec *iov = iter_iov(i);	\
 			void __user *base;			\
 			size_t len;				\
 			iterate_iovec(i, n, base, len, off,	\
 						iov, (I))	\
-			i->nr_segs -= iov - i->iov;		\
-			i->iov = iov;				\
+			i->nr_segs -= iov - iter_iov(i);	\
+			i->__iov = iov;				\
 		} else if (iov_iter_is_bvec(i)) {		\
 			const struct bio_vec *bvec = i->bvec;	\
 			void *base;				\
@@ -355,7 +355,7 @@ size_t fault_in_iov_iter_readable(const struct iov_iter *i, size_t size)
 		size_t skip;
 
 		size -= count;
-		for (p = i->iov, skip = i->iov_offset; count; p++, skip = 0) {
+		for (p = iter_iov(i), skip = i->iov_offset; count; p++, skip = 0) {
 			size_t len = min(count, p->iov_len - skip);
 			size_t ret;
 
@@ -398,7 +398,7 @@ size_t fault_in_iov_iter_writeable(const struct iov_iter *i, size_t size)
 		size_t skip;
 
 		size -= count;
-		for (p = i->iov, skip = i->iov_offset; count; p++, skip = 0) {
+		for (p = iter_iov(i), skip = i->iov_offset; count; p++, skip = 0) {
 			size_t len = min(count, p->iov_len - skip);
 			size_t ret;
 
@@ -425,7 +425,7 @@ void iov_iter_init(struct iov_iter *i, unsigned int direction,
 		.nofault = false,
 		.user_backed = true,
 		.data_source = direction,
-		.iov = iov,
+		.__iov = iov,
 		.nr_segs = nr_segs,
 		.iov_offset = 0,
 		.count = count
@@ -876,14 +876,14 @@ static void iov_iter_iovec_advance(struct iov_iter *i, size_t size)
 	i->count -= size;
 
 	size += i->iov_offset; // from beginning of current segment
-	for (iov = i->iov, end = iov + i->nr_segs; iov < end; iov++) {
+	for (iov = iter_iov(i), end = iov + i->nr_segs; iov < end; iov++) {
 		if (likely(size < iov->iov_len))
 			break;
 		size -= iov->iov_len;
 	}
 	i->iov_offset = size;
-	i->nr_segs -= iov - i->iov;
-	i->iov = iov;
+	i->nr_segs -= iov - iter_iov(i);
+	i->__iov = iov;
 }
 
 void iov_iter_advance(struct iov_iter *i, size_t size)
@@ -958,12 +958,12 @@ void iov_iter_revert(struct iov_iter *i, size_t unroll)
 			unroll -= n;
 		}
 	} else { /* same logics for iovec and kvec */
-		const struct iovec *iov = i->iov;
+		const struct iovec *iov = iter_iov(i);
 		while (1) {
 			size_t n = (--iov)->iov_len;
 			i->nr_segs++;
 			if (unroll <= n) {
-				i->iov = iov;
+				i->__iov = iov;
 				i->iov_offset = n - unroll;
 				return;
 			}
@@ -980,7 +980,7 @@ size_t iov_iter_single_seg_count(const struct iov_iter *i)
 {
 	if (i->nr_segs > 1) {
 		if (likely(iter_is_iovec(i) || iov_iter_is_kvec(i)))
-			return min(i->count, i->iov->iov_len - i->iov_offset);
+			return min(i->count, iter_iov(i)->iov_len - i->iov_offset);
 		if (iov_iter_is_bvec(i))
 			return min(i->count, i->bvec->bv_len - i->iov_offset);
 	}
@@ -1095,13 +1095,14 @@ static bool iov_iter_aligned_iovec(const struct iov_iter *i, unsigned addr_mask,
 	unsigned k;
 
 	for (k = 0; k < i->nr_segs; k++, skip = 0) {
-		size_t len = i->iov[k].iov_len - skip;
+		const struct iovec *iov = iter_iov(i) + k;
+		size_t len = iov->iov_len - skip;
 
 		if (len > size)
 			len = size;
 		if (len & len_mask)
 			return false;
-		if ((unsigned long)(i->iov[k].iov_base + skip) & addr_mask)
+		if ((unsigned long)(iov->iov_base + skip) & addr_mask)
 			return false;
 
 		size -= len;
@@ -1194,9 +1195,10 @@ static unsigned long iov_iter_alignment_iovec(const struct iov_iter *i)
 	unsigned k;
 
 	for (k = 0; k < i->nr_segs; k++, skip = 0) {
-		size_t len = i->iov[k].iov_len - skip;
+		const struct iovec *iov = iter_iov(i) + k;
+		size_t len = iov->iov_len - skip;
 		if (len) {
-			res |= (unsigned long)i->iov[k].iov_base + skip;
+			res |= (unsigned long)iov->iov_base + skip;
 			if (len > size)
 				len = size;
 			res |= len;
@@ -1273,14 +1275,15 @@ unsigned long iov_iter_gap_alignment(const struct iov_iter *i)
 		return ~0U;
 
 	for (k = 0; k < i->nr_segs; k++) {
-		if (i->iov[k].iov_len) {
-			unsigned long base = (unsigned long)i->iov[k].iov_base;
+		const struct iovec *iov = iter_iov(i) + k;
+		if (iov->iov_len) {
+			unsigned long base = (unsigned long)iov->iov_base;
 			if (v) // if not the first one
 				res |= base | v; // this start | previous end
-			v = base + i->iov[k].iov_len;
-			if (size <= i->iov[k].iov_len)
+			v = base + iov->iov_len;
+			if (size <= iov->iov_len)
 				break;
-			size -= i->iov[k].iov_len;
+			size -= iov->iov_len;
 		}
 	}
 	return res;
@@ -1396,13 +1399,14 @@ static unsigned long first_iovec_segment(const struct iov_iter *i, size_t *size)
 		return (unsigned long)i->ubuf + i->iov_offset;
 
 	for (k = 0, skip = i->iov_offset; k < i->nr_segs; k++, skip = 0) {
-		size_t len = i->iov[k].iov_len - skip;
+		const struct iovec *iov = iter_iov(i) + k;
+		size_t len = iov->iov_len - skip;
 
 		if (unlikely(!len))
 			continue;
 		if (*size > len)
 			*size = len;
-		return (unsigned long)i->iov[k].iov_base + skip;
+		return (unsigned long)iov->iov_base + skip;
 	}
 	BUG(); // if it had been empty, we wouldn't get called
 }
@@ -1614,7 +1618,7 @@ static int iov_npages(const struct iov_iter *i, int maxpages)
 	const struct iovec *p;
 	int npages = 0;
 
-	for (p = i->iov; size; skip = 0, p++) {
+	for (p = iter_iov(i); size; skip = 0, p++) {
 		unsigned offs = offset_in_page(p->iov_base + skip);
 		size_t len = min(p->iov_len - skip, size);
 
@@ -1691,7 +1695,7 @@ const void *dup_iter(struct iov_iter *new, struct iov_iter *old, gfp_t flags)
 				    flags);
 	else if (iov_iter_is_kvec(new) || iter_is_iovec(new))
 		/* iovec and kvec have identical layout */
-		return new->iov = kmemdup(new->iov,
+		return new->__iov = kmemdup(new->__iov,
 				   new->nr_segs * sizeof(struct iovec),
 				   flags);
 	return NULL;
@@ -1918,7 +1922,7 @@ void iov_iter_restore(struct iov_iter *i, struct iov_iter_state *state)
 	if (iov_iter_is_bvec(i))
 		i->bvec -= state->nr_segs - i->nr_segs;
 	else
-		i->iov -= state->nr_segs - i->nr_segs;
+		i->__iov -= state->nr_segs - i->nr_segs;
 	i->nr_segs = state->nr_segs;
 }
 
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index 331380c2438b..8bb97ee6720d 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -3521,6 +3521,7 @@ static ssize_t snd_pcm_readv(struct kiocb *iocb, struct iov_iter *to)
 	unsigned long i;
 	void __user **bufs;
 	snd_pcm_uframes_t frames;
+	const struct iovec *iov = iter_iov(to);
 
 	pcm_file = iocb->ki_filp->private_data;
 	substream = pcm_file->substream;
@@ -3534,14 +3535,16 @@ static ssize_t snd_pcm_readv(struct kiocb *iocb, struct iov_iter *to)
 		return -EINVAL;
 	if (to->nr_segs > 1024 || to->nr_segs != runtime->channels)
 		return -EINVAL;
-	if (!frame_aligned(runtime, to->iov->iov_len))
+	if (!frame_aligned(runtime, iov->iov_len))
 		return -EINVAL;
-	frames = bytes_to_samples(runtime, to->iov->iov_len);
+	frames = bytes_to_samples(runtime, iov->iov_len);
 	bufs = kmalloc_array(to->nr_segs, sizeof(void *), GFP_KERNEL);
 	if (bufs == NULL)
 		return -ENOMEM;
-	for (i = 0; i < to->nr_segs; ++i)
-		bufs[i] = to->iov[i].iov_base;
+	for (i = 0; i < to->nr_segs; ++i) {
+		bufs[i] = iov->iov_base;
+		iov++;
+	}
 	result = snd_pcm_lib_readv(substream, bufs, frames);
 	if (result > 0)
 		result = frames_to_bytes(runtime, result);
@@ -3558,6 +3561,7 @@ static ssize_t snd_pcm_writev(struct kiocb *iocb, struct iov_iter *from)
 	unsigned long i;
 	void __user **bufs;
 	snd_pcm_uframes_t frames;
+	const struct iovec *iov = iter_iov(from);
 
 	pcm_file = iocb->ki_filp->private_data;
 	substream = pcm_file->substream;
@@ -3570,14 +3574,16 @@ static ssize_t snd_pcm_writev(struct kiocb *iocb, struct iov_iter *from)
 	if (!iter_is_iovec(from))
 		return -EINVAL;
 	if (from->nr_segs > 128 || from->nr_segs != runtime->channels ||
-	    !frame_aligned(runtime, from->iov->iov_len))
+	    !frame_aligned(runtime, iov->iov_len))
 		return -EINVAL;
-	frames = bytes_to_samples(runtime, from->iov->iov_len);
+	frames = bytes_to_samples(runtime, iov->iov_len);
 	bufs = kmalloc_array(from->nr_segs, sizeof(void *), GFP_KERNEL);
 	if (bufs == NULL)
 		return -ENOMEM;
-	for (i = 0; i < from->nr_segs; ++i)
-		bufs[i] = from->iov[i].iov_base;
+	for (i = 0; i < from->nr_segs; ++i) {
+		bufs[i] = iov->iov_base;
+		iov++;
+	}
 	result = snd_pcm_lib_writev(substream, bufs, frames);
 	if (result > 0)
 		result = frames_to_bytes(runtime, result);
-- 
2.39.2

[PATCH 03/11] iov_iter: add iter_iov_addr() and iter_iov_len() helpers

[Jens Axboe]

These just return the address and length of the current iovec segment
in the iterator. Convert existing iov_iter_iovec() users to use them
instead of getting a copy of the current vec.

Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 fs/read_write.c     | 11 +++++------
 include/linux/uio.h |  2 ++
 io_uring/rw.c       | 27 +++++++++++++--------------
 mm/madvise.c        |  9 ++++-----
 4 files changed, 24 insertions(+), 25 deletions(-)

diff --git a/fs/read_write.c b/fs/read_write.c
index 7a2ff6157eda..a21ba3be7dbe 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -749,15 +749,14 @@ static ssize_t do_loop_readv_writev(struct file *filp, struct iov_iter *iter,
 		return -EOPNOTSUPP;
 
 	while (iov_iter_count(iter)) {
-		struct iovec iovec = iov_iter_iovec(iter);
 		ssize_t nr;
 
 		if (type == READ) {
-			nr = filp->f_op->read(filp, iovec.iov_base,
-					      iovec.iov_len, ppos);
+			nr = filp->f_op->read(filp, iter_iov_addr(iter),
+						iter_iov_len(iter), ppos);
 		} else {
-			nr = filp->f_op->write(filp, iovec.iov_base,
-					       iovec.iov_len, ppos);
+			nr = filp->f_op->write(filp, iter_iov_addr(iter),
+						iter_iov_len(iter), ppos);
 		}
 
 		if (nr < 0) {
@@ -766,7 +765,7 @@ static ssize_t do_loop_readv_writev(struct file *filp, struct iov_iter *iter,
 			break;
 		}
 		ret += nr;
-		if (nr != iovec.iov_len)
+		if (nr != iter_iov_len(iter))
 			break;
 		iov_iter_advance(iter, nr);
 	}
diff --git a/include/linux/uio.h b/include/linux/uio.h
index 4218624b7f78..b7fce87b720e 100644
--- a/include/linux/uio.h
+++ b/include/linux/uio.h
@@ -70,6 +70,8 @@ struct iov_iter {
 };
 
 #define iter_iov(iter)	(iter)->__iov
+#define iter_iov_addr(iter)	(iter_iov(iter)->iov_base + (iter)->iov_offset)
+#define iter_iov_len(iter)	(iter_iov(iter)->iov_len - (iter)->iov_offset)
 
 static inline enum iter_type iov_iter_type(const struct iov_iter *i)
 {
diff --git a/io_uring/rw.c b/io_uring/rw.c
index 7573a34ea42a..f33ba6f28247 100644
--- a/io_uring/rw.c
+++ b/io_uring/rw.c
@@ -447,26 +447,25 @@ static ssize_t loop_rw_iter(int ddir, struct io_rw *rw, struct iov_iter *iter)
 	ppos = io_kiocb_ppos(kiocb);
 
 	while (iov_iter_count(iter)) {
-		struct iovec iovec;
+		void __user *addr;
+		size_t len;
 		ssize_t nr;
 
 		if (iter_is_ubuf(iter)) {
-			iovec.iov_base = iter->ubuf + iter->iov_offset;
-			iovec.iov_len = iov_iter_count(iter);
+			addr = iter->ubuf + iter->iov_offset;
+			len = iov_iter_count(iter);
 		} else if (!iov_iter_is_bvec(iter)) {
-			iovec = iov_iter_iovec(iter);
+			addr = iter_iov_addr(iter);
+			len = iter_iov_len(iter);
 		} else {
-			iovec.iov_base = u64_to_user_ptr(rw->addr);
-			iovec.iov_len = rw->len;
+			addr = u64_to_user_ptr(rw->addr);
+			len = rw->len;
 		}
 
-		if (ddir == READ) {
-			nr = file->f_op->read(file, iovec.iov_base,
-					      iovec.iov_len, ppos);
-		} else {
-			nr = file->f_op->write(file, iovec.iov_base,
-					       iovec.iov_len, ppos);
-		}
+		if (ddir == READ)
+			nr = file->f_op->read(file, addr, len, ppos);
+		else
+			nr = file->f_op->write(file, addr, len, ppos);
 
 		if (nr < 0) {
 			if (!ret)
@@ -482,7 +481,7 @@ static ssize_t loop_rw_iter(int ddir, struct io_rw *rw, struct iov_iter *iter)
 			if (!rw->len)
 				break;
 		}
-		if (nr != iovec.iov_len)
+		if (nr != len)
 			break;
 	}
 
diff --git a/mm/madvise.c b/mm/madvise.c
index 340125d08c03..9f389c5304d2 100644
--- a/mm/madvise.c
+++ b/mm/madvise.c
@@ -1456,7 +1456,7 @@ SYSCALL_DEFINE5(process_madvise, int, pidfd, const struct iovec __user *, vec,
 		size_t, vlen, int, behavior, unsigned int, flags)
 {
 	ssize_t ret;
-	struct iovec iovstack[UIO_FASTIOV], iovec;
+	struct iovec iovstack[UIO_FASTIOV];
 	struct iovec *iov = iovstack;
 	struct iov_iter iter;
 	struct task_struct *task;
@@ -1503,12 +1503,11 @@ SYSCALL_DEFINE5(process_madvise, int, pidfd, const struct iovec __user *, vec,
 	total_len = iov_iter_count(&iter);
 
 	while (iov_iter_count(&iter)) {
-		iovec = iov_iter_iovec(&iter);
-		ret = do_madvise(mm, (unsigned long)iovec.iov_base,
-					iovec.iov_len, behavior);
+		ret = do_madvise(mm, (unsigned long)iter_iov_addr(&iter),
+					iter_iov_len(&iter), behavior);
 		if (ret < 0)
 			break;
-		iov_iter_advance(&iter, iovec.iov_len);
+		iov_iter_advance(&iter, iter_iov_len(&iter));
 	}
 
 	ret = (total_len - iov_iter_count(&iter)) ? : ret;
-- 
2.39.2

[PATCH 04/11] iov_iter: remove iov_iter_iovec()

[Jens Axboe]

No more users are left of this function.

Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 include/linux/uio.h | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/include/linux/uio.h b/include/linux/uio.h
index b7fce87b720e..7f585ceedcb2 100644
--- a/include/linux/uio.h
+++ b/include/linux/uio.h
@@ -148,15 +148,6 @@ static inline size_t iov_length(const struct iovec *iov, unsigned long nr_segs)
 	return ret;
 }
 
-static inline struct iovec iov_iter_iovec(const struct iov_iter *iter)
-{
-	return (struct iovec) {
-		.iov_base = iter_iov(iter)->iov_base + iter->iov_offset,
-		.iov_len = min(iter->count,
-			       iter_iov(iter)->iov_len - iter->iov_offset),
-	};
-}
-
 size_t copy_page_from_iter_atomic(struct page *page, unsigned offset,
 				  size_t bytes, struct iov_iter *i);
 void iov_iter_advance(struct iov_iter *i, size_t bytes);
-- 
2.39.2

[PATCH 05/11] iov_iter: set nr_segs = 1 for ITER_UBUF

[Jens Axboe]

To avoid needing to check if a given user backed iov_iter is of type
ITER_IOVEC or ITER_UBUF, set the number of segments for the ITER_UBUF
case to 1 as we're carrying a single segment.

Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 include/linux/uio.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/linux/uio.h b/include/linux/uio.h
index 7f585ceedcb2..5dbd2dcab35c 100644
--- a/include/linux/uio.h
+++ b/include/linux/uio.h
@@ -355,7 +355,8 @@ static inline void iov_iter_ubuf(struct iov_iter *i, unsigned int direction,
 		.user_backed = true,
 		.data_source = direction,
 		.ubuf = buf,
-		.count = count
+		.count = count,
+		.nr_segs = 1
 	};
 }
 /* Flags for iov_iter_get/extract_pages*() */
-- 
2.39.2

[PATCH 06/11] iov_iter: overlay struct iovec and ubuf/len

[Jens Axboe]

Add an internal struct iovec that we can return as a pointer, with the
fields of the iovec overlapping with the ITER_UBUF ubuf and length
fields.

Then we can have iter_iov() check for the appropriate type, and return
&iter->__ubuf_iovec for ITER_UBUF and iter->__iov for ITER_IOVEC and
things will magically work out for a single segment request regardless
of either type.

Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 include/linux/uio.h | 39 ++++++++++++++++++++++++++++++---------
 1 file changed, 30 insertions(+), 9 deletions(-)

diff --git a/include/linux/uio.h b/include/linux/uio.h
index 5dbd2dcab35c..361688b86291 100644
--- a/include/linux/uio.h
+++ b/include/linux/uio.h
@@ -49,15 +49,30 @@ struct iov_iter {
 		size_t iov_offset;
 		int last_offset;
 	};
-	size_t count;
+	/*
+	 * Hack alert: overlay ubuf_iovec with iovec + count, so
+	 * that the members resolve correctly regardless of the type
+	 * of iterator used. This means that you can use:
+	 *
+	 * &iter->ubuf or iter->iov
+	 *
+	 * interchangably for the user_backed cases, hence simplifying
+	 * some of the cases that need to deal with both.
+	 */
 	union {
-		/* use iter_iov() to get the current vec */
-		const struct iovec *__iov;
-		const struct kvec *kvec;
-		const struct bio_vec *bvec;
-		struct xarray *xarray;
-		struct pipe_inode_info *pipe;
-		void __user *ubuf;
+		struct iovec __ubuf_iovec;
+		struct {
+			union {
+				/* use iter_iov() to get the current vec */
+				const struct iovec *__iov;
+				const struct kvec *kvec;
+				const struct bio_vec *bvec;
+				struct xarray *xarray;
+				struct pipe_inode_info *pipe;
+				void __user *ubuf;
+			};
+			size_t count;
+		};
 	};
 	union {
 		unsigned long nr_segs;
@@ -69,7 +84,13 @@ struct iov_iter {
 	};
 };
 
-#define iter_iov(iter)	(iter)->__iov
+static inline const struct iovec *iter_iov(const struct iov_iter *iter)
+{
+	if (iter->iter_type == ITER_UBUF)
+		return (const struct iovec *) &iter->__ubuf_iovec;
+	return iter->__iov;
+}
+
 #define iter_iov_addr(iter)	(iter_iov(iter)->iov_base + (iter)->iov_offset)
 #define iter_iov_len(iter)	(iter_iov(iter)->iov_len - (iter)->iov_offset)
 
-- 
2.39.2

[PATCH 07/11] IB/hfi1: check for user backed iterator, not specific iterator type

[Jens Axboe]

In preparation for switching single segment iterators to using ITER_UBUF,
swap the check for whether we are user backed or not. While at it, move
it outside the srcu locking area to clean up the code a bit.

Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 drivers/infiniband/hw/hfi1/file_ops.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/infiniband/hw/hfi1/file_ops.c b/drivers/infiniband/hw/hfi1/file_ops.c
index 3065db9d6bb9..f3d6ce45c397 100644
--- a/drivers/infiniband/hw/hfi1/file_ops.c
+++ b/drivers/infiniband/hw/hfi1/file_ops.c
@@ -267,6 +267,8 @@ static ssize_t hfi1_write_iter(struct kiocb *kiocb, struct iov_iter *from)
 
 	if (!HFI1_CAP_IS_KSET(SDMA))
 		return -EINVAL;
+	if (!from->user_backed)
+		return -EINVAL;
 	idx = srcu_read_lock(&fd->pq_srcu);
 	pq = srcu_dereference(fd->pq, &fd->pq_srcu);
 	if (!cq || !pq) {
@@ -274,11 +276,6 @@ static ssize_t hfi1_write_iter(struct kiocb *kiocb, struct iov_iter *from)
 		return -EIO;
 	}
 
-	if (!iter_is_iovec(from) || !dim) {
-		srcu_read_unlock(&fd->pq_srcu, idx);
-		return -EINVAL;
-	}
-
 	trace_hfi1_sdma_request(fd->dd, fd->uctxt->ctxt, fd->subctxt, dim);
 
 	if (atomic_read(&pq->n_reqs) == pq->n_max_reqs) {
-- 
2.39.2

[PATCH 08/11] IB/qib: check for user backed iterator, not specific iterator type

[Jens Axboe]

In preparation for switching single segment iterators to using ITER_UBUF,
swap the check for whether we are user backed or not.

Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 drivers/infiniband/hw/qib/qib_file_ops.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/infiniband/hw/qib/qib_file_ops.c b/drivers/infiniband/hw/qib/qib_file_ops.c
index 4cee39337866..815ea72ad473 100644
--- a/drivers/infiniband/hw/qib/qib_file_ops.c
+++ b/drivers/infiniband/hw/qib/qib_file_ops.c
@@ -2245,7 +2245,7 @@ static ssize_t qib_write_iter(struct kiocb *iocb, struct iov_iter *from)
 	struct qib_ctxtdata *rcd = ctxt_fp(iocb->ki_filp);
 	struct qib_user_sdma_queue *pq = fp->pq;
 
-	if (!iter_is_iovec(from) || !from->nr_segs || !pq)
+	if (!from->user_backed || !from->nr_segs || !pq)
 		return -EINVAL;
 
 	return qib_user_sdma_writev(rcd, pq, iter_iov(from), from->nr_segs);
-- 
2.39.2

[PATCH 09/11] ALSA: pcm: check for user backed iterator, not specific iterator type

[Jens Axboe]

In preparation for switching single segment iterators to using ITER_UBUF,
swap the check for whether we are user backed or not.

Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 sound/core/pcm_native.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index 8bb97ee6720d..5868661d461b 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -3531,7 +3531,7 @@ static ssize_t snd_pcm_readv(struct kiocb *iocb, struct iov_iter *to)
 	if (runtime->state == SNDRV_PCM_STATE_OPEN ||
 	    runtime->state == SNDRV_PCM_STATE_DISCONNECTED)
 		return -EBADFD;
-	if (!iter_is_iovec(to))
+	if (!to->user_backed)
 		return -EINVAL;
 	if (to->nr_segs > 1024 || to->nr_segs != runtime->channels)
 		return -EINVAL;
@@ -3571,7 +3571,7 @@ static ssize_t snd_pcm_writev(struct kiocb *iocb, struct iov_iter *from)
 	if (runtime->state == SNDRV_PCM_STATE_OPEN ||
 	    runtime->state == SNDRV_PCM_STATE_DISCONNECTED)
 		return -EBADFD;
-	if (!iter_is_iovec(from))
+	if (!from->user_backed)
 		return -EINVAL;
 	if (from->nr_segs > 128 || from->nr_segs != runtime->channels ||
 	    !frame_aligned(runtime, iov->iov_len))
-- 
2.39.2

[PATCH 10/11] iov_iter: convert import_single_range() to ITER_UBUF

[Jens Axboe]

Since we're just importing a single vector, we don't have to turn it
into an ITER_IOVEC. Instead turn it into an ITER_UBUF, which is cheaper
to iterate.

Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 lib/iov_iter.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/lib/iov_iter.c b/lib/iov_iter.c
index 87488c4aad3f..f411bda1171f 100644
--- a/lib/iov_iter.c
+++ b/lib/iov_iter.c
@@ -1870,9 +1870,7 @@ int import_single_range(int rw, void __user *buf, size_t len,
 	if (unlikely(!access_ok(buf, len)))
 		return -EFAULT;
 
-	iov->iov_base = buf;
-	iov->iov_len = len;
-	iov_iter_init(i, rw, iov, 1, len);
+	iov_iter_ubuf(i, rw, buf, len);
 	return 0;
 }
 EXPORT_SYMBOL(import_single_range);
-- 
2.39.2

[PATCH 11/11] iov_iter: import single vector iovecs as ITER_UBUF

[Jens Axboe]

Add a special case to __import_iovec(), which imports a single segment
iovec as an ITER_UBUF rather than an ITER_IOVEC. ITER_UBUF is cheaper
to iterate than ITER_IOVEC, and for a single segment iovec, there's no
point in using a segmented iterator.

Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 lib/iov_iter.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/lib/iov_iter.c b/lib/iov_iter.c
index f411bda1171f..3e6c9bcfa612 100644
--- a/lib/iov_iter.c
+++ b/lib/iov_iter.c
@@ -1784,6 +1784,30 @@ struct iovec *iovec_from_user(const struct iovec __user *uvec,
 	return iov;
 }
 
+/*
+ * Single segment iovec supplied by the user, import it as ITER_UBUF.
+ */
+static ssize_t __import_iovec_ubuf(int type, const struct iovec __user *uvec,
+				   struct iovec **iovp, struct iov_iter *i,
+				   bool compat)
+{
+	struct iovec *iov = *iovp;
+	ssize_t ret;
+
+	if (compat)
+		ret = copy_compat_iovec_from_user(iov, uvec, 1);
+	else
+		ret = copy_iovec_from_user(iov, uvec, 1);
+	if (unlikely(ret))
+		return ret;
+
+	ret = import_ubuf(type, iov->iov_base, iov->iov_len, i);
+	if (unlikely(ret))
+		return ret;
+	*iovp = NULL;
+	return i->count;
+}
+
 ssize_t __import_iovec(int type, const struct iovec __user *uvec,
 		 unsigned nr_segs, unsigned fast_segs, struct iovec **iovp,
 		 struct iov_iter *i, bool compat)
@@ -1792,6 +1816,9 @@ ssize_t __import_iovec(int type, const struct iovec __user *uvec,
 	unsigned long seg;
 	struct iovec *iov;
 
+	if (nr_segs == 1)
+		return __import_iovec_ubuf(type, uvec, iovp, i, compat);
+
 	iov = iovec_from_user(uvec, nr_segs, fast_segs, *iovp, compat);
 	if (IS_ERR(iov)) {
 		*iovp = NULL;
-- 
2.39.2

Re: [PATCH 06/11] iov_iter: overlay struct iovec and ubuf/len

[Linus Torvalds]

On Wed, Mar 29, 2023 at 11:41 AM Jens Axboe <axboe@kernel.dk> wrote:
>
> +               struct iovec __ubuf_iovec;

I think this is the third time I say this: this should be "const struct iovec".

No other use is ever valid, and this cast:

> +static inline const struct iovec *iter_iov(const struct iov_iter *iter)
> +{
> +       if (iter->iter_type == ITER_UBUF)
> +               return (const struct iovec *) &iter->__ubuf_iovec;

should simply not exist.

The first rule of cast club is that casting one pointer to another is
generally a sign that something is wrong.

Casting a pointer to an integer? That's a valid way to get at the bit
representation for things like tagged pointers or for virtual address
arithmetic etc (ok, and by "valid" I mean "valid in kernel contexts -
not necessarily in all other contexts).

Casting an integer to a pointer? Same thing - some things just need to
do bit operations on what will become a pointer (allocators etc).

But casting a pointer to another one - that should basically always
raise eyebrows. You should try really hard to avoid it.

Yes, we do it in the kernel, and yes, it *can* be valid. But most of
the time it's really a bad sign.

             Linus

Re: [PATCH 06/11] iov_iter: overlay struct iovec and ubuf/len

[Jens Axboe]

On 3/29/23 1:30 PM, Linus Torvalds wrote:
> On Wed, Mar 29, 2023 at 11:41 AM Jens Axboe <axboe@kernel.dk> wrote:
>>
>> +               struct iovec __ubuf_iovec;
> 
> I think this is the third time I say this: this should be "const struct iovec".

Doh sorry, not sure why I keep missing that... But yes, it should, I'll make
the edit and actually amend it.

> No other use is ever valid, and this cast:
> 
>> +static inline const struct iovec *iter_iov(const struct iov_iter *iter)
>> +{
>> +       if (iter->iter_type == ITER_UBUF)
>> +               return (const struct iovec *) &iter->__ubuf_iovec;
> 
> should simply not exist.

Yep. Fixed both up.

-- 
Jens Axboe

Re: [PATCH 06/11] iov_iter: overlay struct iovec and ubuf/len

[Jens Axboe]

On 3/29/23 1:38 PM, Jens Axboe wrote:
> On 3/29/23 1:30 PM, Linus Torvalds wrote:
>> On Wed, Mar 29, 2023 at 11:41 AM Jens Axboe <axboe@kernel.dk> wrote:
>>>
>>> +               struct iovec __ubuf_iovec;
>>
>> I think this is the third time I say this: this should be "const struct iovec".
> 
> Doh sorry, not sure why I keep missing that... But yes, it should, I'll make
> the edit and actually amend it.

Now I recall why that ended up like that again, during the initial fiddling
with this. If we leave it const, we get:

  CC      arch/arm64/kernel/asm-offsets.s
In file included from ./include/linux/socket.h:8,
                 from ./include/linux/compat.h:15,
                 from ./arch/arm64/include/asm/ftrace.h:52,
                 from ./include/linux/ftrace.h:23,
                 from arch/arm64/kernel/asm-offsets.c:12:
./include/linux/uio.h: In function ‘iov_iter_ubuf’:
./include/linux/uio.h:374:12: error: assignment of read-only location ‘*i’
  374 |         *i = (struct iov_iter) {
      |            ^
make[1]: *** [scripts/Makefile.build:114: arch/arm64/kernel/asm-offsets.s] Error 1
make: *** [Makefile:1286: prepare0] Error 2

Let me take a closer look at that...

-- 
Jens Axboe

Re: [PATCHSET v6 0/11] Turn single segment imports into ITER_UBUF

[Linus Torvalds]

On Wed, Mar 29, 2023 at 11:41 AM Jens Axboe <axboe@kernel.dk> wrote:
>
> Passes testing, and verified we do the right thing for 1 and multi
> segments.

Apart from the pointer casting rant, this looks sane to me.

I feel like 02/11 has a few potential cleanups:

 (a) it feels like a few too many "iter.__iov" uses remaining, but
they mostly (all?) look like assignments.

I do get the feeling that any time you assign __iov, you should also
assign "nr_segs", and it worries me a bit that I see one without the
other. Maybe room for another helper that enforces a "if you set the
__iov pointer, you must be setting nr_segs too"?

And maybe I'm just being difficult.

 (b) I see at least one "iov = iter_iov(from)" that precedes a later
check for "iter_is_iovec()", which again means that *if* we add some
debug sanity test to "iter_iov()", it might trigger when it shouldn't?

The one I see is in snd_pcm_writev(), but I th ink the same thing
happens in snd_pcm_readv() but just isn't visible in the patch due to
not having the context lines.

            Linus

Re: [PATCH 06/11] iov_iter: overlay struct iovec and ubuf/len

[Jens Axboe]

On 3/29/23 1:42 PM, Jens Axboe wrote:
> On 3/29/23 1:38 PM, Jens Axboe wrote:
>> On 3/29/23 1:30 PM, Linus Torvalds wrote:
>>> On Wed, Mar 29, 2023 at 11:41 AM Jens Axboe <axboe@kernel.dk> wrote:
>>>>
>>>> +               struct iovec __ubuf_iovec;
>>>
>>> I think this is the third time I say this: this should be "const struct iovec".
>>
>> Doh sorry, not sure why I keep missing that... But yes, it should, I'll make
>> the edit and actually amend it.
> 
> Now I recall why that ended up like that again, during the initial fiddling
> with this. If we leave it const, we get:
> 
>   CC      arch/arm64/kernel/asm-offsets.s
> In file included from ./include/linux/socket.h:8,
>                  from ./include/linux/compat.h:15,
>                  from ./arch/arm64/include/asm/ftrace.h:52,
>                  from ./include/linux/ftrace.h:23,
>                  from arch/arm64/kernel/asm-offsets.c:12:
> ./include/linux/uio.h: In function ‘iov_iter_ubuf’:
> ./include/linux/uio.h:374:12: error: assignment of read-only location ‘*i’
>   374 |         *i = (struct iov_iter) {
>       |            ^
> make[1]: *** [scripts/Makefile.build:114: arch/arm64/kernel/asm-offsets.s] Error 1
> make: *** [Makefile:1286: prepare0] Error 2
> 
> Let me take a closer look at that...

We can get rid of these if we convert the iov_iter initializers to
just assign the members rather than the copy+zero fill. The automatic
zero fill is nice though, in terms of sanity.

-- 
Jens Axboe

Re: [PATCH 06/11] iov_iter: overlay struct iovec and ubuf/len

[Linus Torvalds]

On Wed, Mar 29, 2023 at 12:49 PM Jens Axboe <axboe@kernel.dk> wrote:
>
> We can get rid of these if we convert the iov_iter initializers to
> just assign the members rather than the copy+zero fill. The automatic
> zero fill is nice though, in terms of sanity.

The automatic zero fill is good, but I think it should be fixed by
just not making that

        const struct iovec __ubuf_iovec;

member be the first member of a union.

The way union initializers work is that if they aren't named, they are
for the first member.

So I *think* the reason you get that warning is literally just because
the __ubuf_iovec member is first in that union, and moving it down to
below the other struct will just fix things.

                   Linus

Re: [PATCHSET v6 0/11] Turn single segment imports into ITER_UBUF

[Jens Axboe]

On 3/29/23 1:44 PM, Linus Torvalds wrote:
> On Wed, Mar 29, 2023 at 11:41 AM Jens Axboe <axboe@kernel.dk> wrote:
>>
>> Passes testing, and verified we do the right thing for 1 and multi
>> segments.
> 
> Apart from the pointer casting rant, this looks sane to me.
> 
> I feel like 02/11 has a few potential cleanups:
> 
>  (a) it feels like a few too many "iter.__iov" uses remaining, but
> they mostly (all?) look like assignments.
> 
> I do get the feeling that any time you assign __iov, you should also
> assign "nr_segs", and it worries me a bit that I see one without the
> other. Maybe room for another helper that enforces a "if you set the
> __iov pointer, you must be setting nr_segs too"?
> 
> And maybe I'm just being difficult.

No, I think that's valid, and the cover letter does touch upon that.
The thought of doing an iov assign helper has occurred to me as well.
I just wanted to get general feelings on the direction first, then
do a round of polish when prudent rather than prematurely.

>  (b) I see at least one "iov = iter_iov(from)" that precedes a later
> check for "iter_is_iovec()", which again means that *if* we add some
> debug sanity test to "iter_iov()", it might trigger when it shouldn't?
> 
> The one I see is in snd_pcm_writev(), but I th ink the same thing
> happens in snd_pcm_readv() but just isn't visible in the patch due to
> not having the context lines.

I think that's mostly a patch ordering issue. Should probably just
push the sound and IB patches to the front of the series.

-- 
Jens Axboe

Re: [PATCH 06/11] iov_iter: overlay struct iovec and ubuf/len

[Jens Axboe]

On 3/29/23 1:52 PM, Linus Torvalds wrote:
> On Wed, Mar 29, 2023 at 12:49 PM Jens Axboe <axboe@kernel.dk> wrote:
>>
>> We can get rid of these if we convert the iov_iter initializers to
>> just assign the members rather than the copy+zero fill. The automatic
>> zero fill is nice though, in terms of sanity.
> 
> The automatic zero fill is good, but I think it should be fixed by
> just not making that
> 
>         const struct iovec __ubuf_iovec;
> 
> member be the first member of a union.
> 
> The way union initializers work is that if they aren't named, they are
> for the first member.
> 
> So I *think* the reason you get that warning is literally just because
> the __ubuf_iovec member is first in that union, and moving it down to
> below the other struct will just fix things.

Nope, still fails with it moved below.

-- 
Jens Axboe

Re: [PATCH 06/11] iov_iter: overlay struct iovec and ubuf/len

[Linus Torvalds]

On Wed, Mar 29, 2023 at 12:56 PM Jens Axboe <axboe@kernel.dk> wrote:
>
> Nope, still fails with it moved below.

Ouch. I guess the 'const' cast may be the only way then.  It sounds
like gcc may warn whenever it sees an assignment to any structure that
has a const member, rather than when it sees an assignment to that
particular member.

Sad.

           Linus

Re: [PATCH 06/11] iov_iter: overlay struct iovec and ubuf/len

[Jens Axboe]

On 3/29/23 1:59 PM, Linus Torvalds wrote:
> On Wed, Mar 29, 2023 at 12:56 PM Jens Axboe <axboe@kernel.dk> wrote:
>>
>> Nope, still fails with it moved below.
> 
> Ouch. I guess the 'const' cast may be the only way then.  It sounds
> like gcc may warn whenever it sees an assignment to any structure that
> has a const member, rather than when it sees an assignment to that
> particular member.
> 
> Sad.

Yeah, I tried a bunch of variants to trick it, including having
it in a union with another non-const iovec first. But it cannot be
tricked, so I think we're stuck with that. I'll add a comment.

-- 
Jens Axboe

[PATCH 11/11] iov_iter: import single vector iovecs as ITER_UBUF

[Jens Axboe]

Add a special case to __import_iovec(), which imports a single segment
iovec as an ITER_UBUF rather than an ITER_IOVEC. ITER_UBUF is cheaper
to iterate than ITER_IOVEC, and for a single segment iovec, there's no
point in using a segmented iterator.

Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 lib/iov_iter.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/lib/iov_iter.c b/lib/iov_iter.c
index f411bda1171f..3e6c9bcfa612 100644
--- a/lib/iov_iter.c
+++ b/lib/iov_iter.c
@@ -1784,6 +1784,30 @@ struct iovec *iovec_from_user(const struct iovec __user *uvec,
 	return iov;
 }
 
+/*
+ * Single segment iovec supplied by the user, import it as ITER_UBUF.
+ */
+static ssize_t __import_iovec_ubuf(int type, const struct iovec __user *uvec,
+				   struct iovec **iovp, struct iov_iter *i,
+				   bool compat)
+{
+	struct iovec *iov = *iovp;
+	ssize_t ret;
+
+	if (compat)
+		ret = copy_compat_iovec_from_user(iov, uvec, 1);
+	else
+		ret = copy_iovec_from_user(iov, uvec, 1);
+	if (unlikely(ret))
+		return ret;
+
+	ret = import_ubuf(type, iov->iov_base, iov->iov_len, i);
+	if (unlikely(ret))
+		return ret;
+	*iovp = NULL;
+	return i->count;
+}
+
 ssize_t __import_iovec(int type, const struct iovec __user *uvec,
 		 unsigned nr_segs, unsigned fast_segs, struct iovec **iovp,
 		 struct iov_iter *i, bool compat)
@@ -1792,6 +1816,9 @@ ssize_t __import_iovec(int type, const struct iovec __user *uvec,
 	unsigned long seg;
 	struct iovec *iov;
 
+	if (nr_segs == 1)
+		return __import_iovec_ubuf(type, uvec, iovp, i, compat);
+
 	iov = iovec_from_user(uvec, nr_segs, fast_segs, *iovp, compat);
 	if (IS_ERR(iov)) {
 		*iovp = NULL;
-- 
2.39.2