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

[Pavel Begunkov <asml.silence@gmail.com>]

On 4/17/25 12:50, Nitesh Shetty wrote:
> 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;

Ah yes, looks like the right fix up. We can make it nicer, but
that's for later. It'd also be great to have a test for it.


> 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

Block might filter it out, but for example net/ doesn't care,
fs as well. IIUC what you mean, either way we definitely should
correct that.

> 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/
> 
> 

-- 
Pavel Begunkov