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

[Jens Axboe <axboe@kernel.dk>]

On 4/17/25 6:56 AM, Pavel Begunkov wrote:
> 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.

I just tested it, and yes it certainly blows up... Can also confirm that
the corrected nr_segs calculation does the right thing, doesn't end up
underestimating the segment count by 1 in that case.

I'll turn the test case into something we can add to liburing, and fold
in that change.

-- 
Jens Axboe