Re: Possible bug for ring-mapped provided buffer

[Jens Axboe <axboe@kernel.dk>]

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.

-- 
Jens Axboe