[PATCHSET v5 0/9] Turn single segment imports into ITER_UBUF

[PATCHSET v5 0/9] Turn single segment imports into ITER_UBUF

[Jens Axboe]

Hi,

ather 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.

The main change since v4 is doing the overlay trick in the iov_iter,
so that the first segment of the iovec overlaps nicely with the ubuf
address and length. With that, we can drop any functional driver change,
as the normal iteration should just work out with ubuf having the single
segment. Replace he iter_is_iovec() check with a basic iter->user_backed
instead.

Unfortunately we can't make iov_iter_iovec() much prettier, as we have
to return a copy of the current iovec as the offset has to be added...

Include a block fix for a shoddy copy of an iterator, as that'll
otherwise cause issues.

Ran the liburing test cases and the previously indicated ltp cases
that barfed, and everything looks rosy. Comments welcome - with the
above approach, we don't ever return iter->__ubuf_iovec, it's really
just there as an overlay filler. Maybe we can make that a bit more
apparent.

-- 
Jens Axboe

[PATCH 1/9] 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 2/9] iov_iter: teach iov_iter_iovec() to deal with ITER_UBUF

[Jens Axboe]

Even if we're returning an iovec, we can trivially fill it in with the
details from an ITER_UBUF as well. This enables loops that assume
ITER_IOVEC to deal with ITER_UBUF transparently.

Suggested-by: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 include/linux/uio.h | 26 +++++++++++++++++++++-----
 1 file changed, 21 insertions(+), 5 deletions(-)

diff --git a/include/linux/uio.h b/include/linux/uio.h
index 27e3fd942960..3b4403efcce1 100644
--- a/include/linux/uio.h
+++ b/include/linux/uio.h
@@ -143,13 +143,29 @@ static inline size_t iov_length(const struct iovec *iov, unsigned long nr_segs)
 	return ret;
 }
 
+/*
+ * Don't assume we're called with ITER_IOVEC, enable usage of ITER_UBUF
+ * as well by simply filling in the iovec.
+ */
 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_len = min(iter->count,
-			       iter->iov->iov_len - iter->iov_offset),
-	};
+	if (WARN_ON_ONCE(!iter->user_backed)) {
+		return (struct iovec) {
+			.iov_base = NULL,
+			.iov_len = 0
+		};
+	} else if (iter_is_ubuf(iter)) {
+		return (struct iovec) {
+			.iov_base = iter->ubuf + iter->iov_offset,
+			.iov_len = iter->count
+		};
+	} else {
+		return (struct iovec) {
+			.iov_base = iter->iov->iov_base + iter->iov_offset,
+			.iov_len = min(iter->count,
+				       iter->iov->iov_len - iter->iov_offset),
+		};
+	}
 }
 
 size_t copy_page_from_iter_atomic(struct page *page, unsigned offset,
-- 
2.39.2

[PATCH 3/9] 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.

This allows doing:

struct iovec *vec = &iter->__ubuf_iovec;

interchangably with iter->iov for the first segment, enabling writing
code that deals with both without needing to check if we're dealing with
ITER_IOVEC or ITER_UBUF.

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

diff --git a/include/linux/uio.h b/include/linux/uio.h
index 3b4403efcce1..192831775d2b 100644
--- a/include/linux/uio.h
+++ b/include/linux/uio.h
@@ -49,14 +49,29 @@ 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 {
-		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 {
+				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;
-- 
2.39.2

[PATCH 4/9] 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 192831775d2b..953ccba94164 100644
--- a/include/linux/uio.h
+++ b/include/linux/uio.h
@@ -390,7 +390,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 5/9] 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 b1d6ca7e9708..fc2257c09b03 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 6/9] 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 80fe92a21f96..425de3bc3abf 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, from->iov, from->nr_segs);
-- 
2.39.2

[PATCH 7/9] 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 331380c2438b..8bfc40b358da 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -3530,7 +3530,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;
@@ -3567,7 +3567,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, from->iov->iov_len))
-- 
2.39.2

[PATCH 8/9] 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 274014e4eafe..fc82cc42ffe6 100644
--- a/lib/iov_iter.c
+++ b/lib/iov_iter.c
@@ -1866,9 +1866,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 9/9] 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 fc82cc42ffe6..63cf9997bd50 100644
--- a/lib/iov_iter.c
+++ b/lib/iov_iter.c
@@ -1780,6 +1780,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)
@@ -1788,6 +1812,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 3/9] iov_iter: overlay struct iovec and ubuf/len

[Linus Torvalds]

On Tue, Mar 28, 2023 at 2:58 PM Jens Axboe <axboe@kernel.dk> wrote:
>
> +               struct iovec __ubuf_iovec;

This really should be "const struct iovec".

The only valid use for this is as an alternative to "iter->iov", and
that one is a 'const' pointer too:

> +                               const struct iovec *iov;

and any code that tries to use it as a non-const iovec entry really is
very very wrong.

And yes, the current infiniband/hw/hfi1/* code does indeed do all of
this wrong and cast the pointer to a non-const one, but that's
actually just because somebody didn't do the const conversion right.

That cast should just go away, and hfi1_user_sdma_process_request()
should just take a 'const struct iovec *iovec' argument.  It doesn't
actually want to write to it anyway, so it's literally just a "change
the prototype of the function" change.

              Linus

Re: [PATCH 3/9] iov_iter: overlay struct iovec and ubuf/len

[Jens Axboe]

On 3/28/23 4:16 PM, Linus Torvalds wrote:
> On Tue, Mar 28, 2023 at 2:58 PM Jens Axboe <axboe@kernel.dk> wrote:
>>
>> +               struct iovec __ubuf_iovec;
> 
> This really should be "const struct iovec".
> 
> The only valid use for this is as an alternative to "iter->iov", and
> that one is a 'const' pointer too:

True, it should. But as per the cover letter, this only really
serves as a space filler, none of the code actually uses it. But
let's make it const, because that is the right thing to do.

>> +                               const struct iovec *iov;
> 
> and any code that tries to use it as a non-const iovec entry really is
> very very wrong.

Nobody should use it, though. The one case where I thought we'd use
it was iov_iter_iovec(), but that doesn't work...

> And yes, the current infiniband/hw/hfi1/* code does indeed do all of
> this wrong and cast the pointer to a non-const one, but that's
> actually just because somebody didn't do the const conversion right.
> 
> That cast should just go away, and hfi1_user_sdma_process_request()
> should just take a 'const struct iovec *iovec' argument.  It doesn't
> actually want to write to it anyway, so it's literally just a "change
> the prototype of the function" change.

Let's leave that for the IB people!

-- 
Jens Axboe

Re: [PATCH 3/9] iov_iter: overlay struct iovec and ubuf/len

[Linus Torvalds]

 thing

On Tue, Mar 28, 2023 at 3:19 PM Jens Axboe <axboe@kernel.dk> wrote:
>
> Nobody should use it, though. The one case where I thought we'd use
> it was iov_iter_iovec(), but that doesn't work...

You only think that because all your conversions are bogus and wrong.

Your latest "[PATCH 7/9] ALSA: pcm.. " patch is just wrong.

Doing this:

-       if (!iter_is_iovec(from))
+       if (!from->user_backed)

does not work AT ALL. You also need to switch all the uses of
"from->iov" to use the "iter_iov()" helper (that you didn't use).

Because "from->iov" is _only_ valid as a iov pointer for an ITER_IOV.

For an ITER_UBUF, that will be the user pointer in the union, and to
get the iov, you need to do that

    iov = &from->ubuf_iov

thing.

The "overlay ubuf as an iov" does *not* make "from->iov" work at all.
Really. You still need to *generate* that pointer from the overlaid
data.

So your latest patches may build, but they most definitely won't work.

I would in fact suggest renaming the "iov" member entirely, to make
sure that nobody uses the "direct access" model ever.

           Linus

Re: [PATCH 3/9] iov_iter: overlay struct iovec and ubuf/len

[Jens Axboe]

On 3/28/23 4:30?PM, Linus Torvalds wrote:
>  thing
> 
> On Tue, Mar 28, 2023 at 3:19?PM Jens Axboe <axboe@kernel.dk> wrote:
>>
>> Nobody should use it, though. The one case where I thought we'd use
>> it was iov_iter_iovec(), but that doesn't work...
> 
> You only think that because all your conversions are bogus and wrong.
> 
> Your latest "[PATCH 7/9] ALSA: pcm.. " patch is just wrong.
> 
> Doing this:
> 
> -       if (!iter_is_iovec(from))
> +       if (!from->user_backed)
> 
> does not work AT ALL. You also need to switch all the uses of
> "from->iov" to use the "iter_iov()" helper (that you didn't use).
> 
> Because "from->iov" is _only_ valid as a iov pointer for an ITER_IOV.
> 
> For an ITER_UBUF, that will be the user pointer in the union, and to
> get the iov, you need to do that
> 
>     iov = &from->ubuf_iov
> 
> thing.
> 
> The "overlay ubuf as an iov" does *not* make "from->iov" work at all.
> Really. You still need to *generate* that pointer from the overlaid
> data.
> 
> So your latest patches may build, but they most definitely won't work.
> 
> I would in fact suggest renaming the "iov" member entirely, to make
> sure that nobody uses the "direct access" model ever.

Yeah ok, I see what you mean now. I'll rework it and post something
tomorrow when it looks sane.

-- 
Jens Axboe