Re: [PATCH] io_uring/rsrc: send exact nr_segs for fixed buffer

[Jens Axboe <axboe@kernel.dk>]

>>> Should we just make it saner first? Sth like these 3 completely
>>> untested commits
>>>
>>> https://github.com/isilence/linux/commits/rsrc-import-cleanup/
>>>
>>> And then it'll become
>>>
>>> nr_segs = ALIGN(offset + len, 1UL << folio_shift);
>>
>> Let's please do that, certainly an improvement. Care to send this out? I
>> can toss them at the testing. And we'd still need that last patch to
> 
> I need to test it first, perhaps tomorrow

Sounds good, I'll run it through testing here too. Would be nice to
stuff in for -rc3, it's pretty minimal and honestly makes the code much
easier to read and reason about.

>> ensure the segment count is correct. Honestly somewhat surprised that
> 
> Right, I can pick up the Nitesh's patch to that.

Sounds good.

>> the only odd fallout of that is (needlessly) hitting the bio split path.
> 
> It's perfectly correct from the iter standpoint, AFAIK, length
> and nr of segments don't have to match. Though I am surprised
> it causes perf issues in the split path.

Theoretically it is, but it always makes me a bit nervous as there are
some _really_ odd iov_iter use cases out there. And passing down known
wrong segment counts is pretty wonky.

> Btw, where exactly does it stumble in there? I'd assume we don't

Because segments != 1, and then that hits the slower path.

> need to do the segment correction for kbuf as the bio splitting
> can do it (and probably does) in exactly the same way?

It doesn't strictly need to, but we should handle that case too. That'd
basically just be the loop addition I already did, something ala the
below on top for both of them:

diff --git a/io_uring/rsrc.c b/io_uring/rsrc.c
index d8fa7158e598..767ac89c8426 100644
--- a/io_uring/rsrc.c
+++ b/io_uring/rsrc.c
@@ -1032,6 +1032,25 @@ static int validate_fixed_range(u64 buf_addr, size_t len,
 	return 0;
 }
 
+static int io_import_kbuf(int ddir, struct iov_iter *iter,
+			  struct io_mapped_ubuf *imu, size_t len, size_t offset)
+{
+	iov_iter_bvec(iter, ddir, iter->bvec, imu->nr_bvecs, len + offset);
+	iov_iter_advance(iter, offset);
+
+	if (len + offset < imu->len) {
+		const struct bio_vec *bvec = iter->bvec;
+
+		while (len > bvec->bv_len) {
+			len -= bvec->bv_len;
+			bvec++;
+		}
+		iter->nr_segs = bvec - iter->bvec;
+	}
+
+	return 0;
+}
+
 static int io_import_fixed(int ddir, struct iov_iter *iter,
 			   struct io_mapped_ubuf *imu,
 			   u64 buf_addr, size_t len)
@@ -1054,13 +1073,9 @@ static int io_import_fixed(int ddir, struct iov_iter *iter,
 	 * and advance us to the beginning.
 	 */
 	offset = buf_addr - imu->ubuf;
-	bvec = imu->bvec;
 
-	if (imu->is_kbuf) {
-		iov_iter_bvec(iter, ddir, bvec, imu->nr_bvecs, offset + len);
-		iov_iter_advance(iter, offset);
-		return 0;
-	}
+	if (imu->is_kbuf)
+		return io_import_kbuf(ddir, iter, imu, len, offset);
 
 	/*
 	 * Don't use iov_iter_advance() here, as it's really slow for
@@ -1083,7 +1098,7 @@ static int io_import_fixed(int ddir, struct iov_iter *iter,
 	 * have the size property of user registered ones, so we have
 	 * to use the slow iter advance.
 	 */
-
+	bvec = imu->bvec;
 	if (offset >= bvec->bv_len) {
 		unsigned long seg_skip;
 
@@ -1094,7 +1109,7 @@ static int io_import_fixed(int ddir, struct iov_iter *iter,
 		offset &= (1UL << imu->folio_shift) - 1;
 	}
 
-	nr_segs = imu->nr_bvecs - (bvec - imu->bvec);
+	nr_segs = ALIGN(offset + len, 1UL << imu->folio_shift) >> imu->folio_shift;
 	iov_iter_bvec(iter, ddir, bvec, nr_segs, len);
 	iter->iov_offset = offset;
 	return 0;

-- 
Jens Axboe