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

[Nitesh Shetty <nj.shetty@samsung.com>]

[-- Attachment #1: Type: text/plain, Size: 2224 bytes --]

On 17/04/25 03:53PM, Nitesh Shetty wrote:
>On 17/04/25 10:34AM, Pavel Begunkov wrote:
>>On 4/17/25 10:32, Pavel Begunkov wrote:
>>>From: Nitesh Shetty <nj.shetty@samsung.com>
>>...
>>>diff --git a/io_uring/rsrc.c b/io_uring/rsrc.c
>>>index 5cf854318b1d..4099b8225670 100644
>>>--- a/io_uring/rsrc.c
>>>+++ b/io_uring/rsrc.c
>>>@@ -1037,6 +1037,7 @@ static int io_import_fixed(int ddir, struct iov_iter *iter,
>>> 			   u64 buf_addr, size_t len)
>>> {
>>> 	const struct bio_vec *bvec;
>>>+	size_t folio_mask;
>>> 	unsigned nr_segs;
>>> 	size_t offset;
>>> 	int ret;
>>>@@ -1067,6 +1068,7 @@ static int io_import_fixed(int ddir, struct iov_iter *iter,
>>> 	 * 2) all bvecs are the same in size, except potentially the
>>> 	 *    first and last bvec
>>> 	 */
>>>+	folio_mask = (1UL << imu->folio_shift) - 1;
>>> 	bvec = imu->bvec;
>>> 	if (offset >= bvec->bv_len) {
>>> 		unsigned long seg_skip;
>>>@@ -1075,10 +1077,10 @@ static int io_import_fixed(int ddir, struct iov_iter *iter,
>>> 		offset -= bvec->bv_len;
>>> 		seg_skip = 1 + (offset >> imu->folio_shift);
>>> 		bvec += seg_skip;
>>>-		offset &= (1UL << imu->folio_shift) - 1;
>>>+		offset &= folio_mask;
>>> 	}
>>>-	nr_segs = imu->nr_bvecs - (bvec - imu->bvec);
>>>+	nr_segs = (offset + len + folio_mask) >> imu->folio_shift;
>>
>>Nitesh, let me know if you're happy with this version.
>>
>This looks great to me, I tested this series and see the
>improvement in IOPS from 7.15 to 7.65M here.
>

There is corner case where this might not work,
This happens when there is a first bvec has non zero offset.
Let's say bv_offset = 256, len = 512, iov_offset = 3584 (512*7, 8th IO),
here we expect IO to have 2 segments with present codebase, but this
patch set produces 1 segment.

So having a fix like this solves the issue,
+	nr_segs = (offset + len + bvec->bv_offset + folio_mask) >> imu->folio_shift;

Note:
I am investigating whether this is a valid case or not, because having a
512 byte IO with 256 byte alignment feel odd. So have sent one patch for
that as well[1]. If that patch[1] is upstreamed then above case is taken
care of, so we can use this series as it is.

Thanks,
Nitesh

[1]
https://lore.kernel.org/all/20250415181419.16305-1-nj.shetty@samsung.com/


[-- Attachment #2: Type: text/plain, Size: 0 bytes --]