Re: Possible bug for ring-mapped provided buffer

[Hao Xu <hao.xu@linux.dev>]

On 6/9/22 23:06, Jens Axboe wrote:
> On 6/9/22 4:32 AM, Hao Xu wrote:
>> On 6/9/22 18:19, Jens Axboe wrote:
>>> On 6/9/22 4:14 AM, Hao Xu wrote:
>>>> On 6/9/22 18:06, Jens Axboe wrote:
>>>>> On 6/9/22 1:53 AM, Hao Xu wrote:
>>>>>> Hi all,
>>>>>> I haven't done tests to demonstrate it. It is for partial io case, we
>>>>>> don't consume/release the buffer before arm_poll in ring-mapped mode.
>>>>>> But seems we should? Otherwise ring head isn't moved and other requests
>>>>>> may take that buffer. What do I miss?
>>>>>
>>>>> On vacation this week, so can't take a look at the code. But the
>>>>> principle is precisely not to consume the buffer if we arm poll, because
>>>>> then the next one can grab it instead. We don't want to consume a buffer
>>>>> over poll, as that defeats the purpose of a provided buffer. It should
>>>>> be grabbed and consumed only if we can use it right now.
>>>>>
>>>>> Hence the way it should work is that we DON'T consume the buffer in this
>>>>> case, and that someone else can just use it. At the same time, we should
>>>>> ensure that we grab a NEW buffer for this case, whenever the poll
>>>>
>>>> If we grab a new buffer for it, then we have to copy the data since we
>>>> have done partial io...this also defeats the purpose of this feature.
>>>
>>> For partial IO, we never drop the buffer. See the logic in
>>> io_kbuf_recycle(). It should be as follows:
>>
>> Yea, in io_kbuf_recycle(), if it's partial io, we just return. For
>> legacy mode, this means we keep the buffer. For ring-mapped mode, this
>> means we then release the uring_lock without moving the ring->head,
>> and then other requests may take that buffer which is in use..
>> And next time we do (for example) recv(), we lost the data which we got
>> at the previous time.
>> Do I miss something?
> 
> If we don't commit for ring mapped buffers, then yeah that's definitely
> a bug. Please send a fix :-)
> 
> Pavel can take care of it this week.
> 

I'll send a patch tomorrow.

Thanks,
Hao