On 8/17/20 8:29 PM, Jens Axboe wrote:
> On 8/17/20 2:25 AM, Stefan Metzmacher wrote:
>> Hi Jens,
>>
>>> Since we've had a few cases of applications not dealing with this
>>> appopriately, I believe the safest course of action is to ensure that
>>> we don't return short reads when we really don't have to.
>>>
>>> The first patch is just a prep patch that retains iov_iter state over
>>> retries, while the second one actually enables just doing retries if
>>> we get a short read back.
>>>
>>> This passes all my testing, both liburing regression tests but also
>>> tests that explicitly trigger internal short reads and hence retry
>>> based on current state. No short reads are passed back to the
>>> application.
>>
>> Thanks! I was going to ask about exactly that :-)
>>
>> It wasn't clear why returning short reads were justified by resulting
>> in better performance... As it means the application needs to do
>> a lot more work and syscalls.
> 
> It mostly boils down to figuring out a good way to do it. With the
> task_work based retry, the async buffered reads, we're almost there and
> the prep patch adds the last remaining bits to retain the iov_iter state
> across issues.
> 
>> Will this be backported?
> 
> I can, but not really in an efficient manner. It depends on the async
> buffered work to make progress, and the task_work handling retry. The
> latter means it's 5.7+, while the former is only in 5.9+...
> 
> We can make it work for earlier kernels by just using the thread offload
> for that, and that may be worth doing. That would enable it in
> 5.7-stable and 5.8-stable. For that, you just need these two patches.
> Patch 1 would work as-is, while patch 2 would need a small bit of
> massaging since io_read() doesn't have the retry parts.
> 
> I'll give it a whirl just out of curiosity, then we can debate it after
> that.

Here are the two patches against latest 5.7-stable (the rc branch, as
we had quite a few queued up after 5.9-rc1). Totally untested, just
wanted to see if it was doable.

First patch is mostly just applied, with various bits removed that we
don't have in 5.7. The second patch just does -EAGAIN punt for the
short read case, which will queue the remainder with io-wq for
async execution.

Obviously needs quite a bit of testing before it can go anywhere else,
but wanted to throw this out there in case you were interested in
giving it a go...

-- 
Jens Axboe