Re: [PATCHSET v3 0/4] Submit ->readpages() IO as read-ahead

[Jens Axboe <axboe@kernel.dk>]

On 6/20/18 5:45 PM, Andrew Morton wrote:
> On Wed, 20 Jun 2018 17:33:47 -0600 Jens Axboe <axboe@kernel.dk> wrote:
> 
>>> If we are to remove callers' ability to use readpages for non-readahead
>>> purposes we should rename the address_space field.
>>
>> Totally agree, and I'd be happy to do that. So how about we do that? I
>> rename it to ->readahead (or ->readaheadpages()?), and then it's
>> perfectly clear what is going on.
> 
> I'm not sure I have the heart to recommend that.
> 
> akpm3:/usr/src/linux-4.18-rc1> grep -r readpages .|wc -l
> 233
> 
> Really, every damn one will need an edit.  I'd understand if we left it
> at ->readpages and sprinkled some loud comments around the place.

That would be more convenient... Embrace and extend, I'll resend the
series with some comments added that'll hopefully get the point across.

>>>> Besides, this isn't going to be just about tracing. Yes, it'll be
>>>> awesome to actually get the right information from blktrace, since right
>>>> now nobody knows which parts are read-ahead and which ones are explicit
>>>> reads. Might be pretty darn useful to debug read-ahead issues.
>>>>
>>>> The read-ahead information must be reliable, otherwise it's impossible
>>>> to introduce other dependencies on top of that. We're having a lot of
>>>> issues with termination of tasks that are stuck in issuing read-ahead.
>>>> If we can rely on the read-ahead information being there AND being
>>>> correct, then we can terminate reads early. This might not sound like
>>>> huge win, but we're talking tens of minutes for some cases. That's the
>>>> planned functional change behind this, but can't be done before we make
>>>> progress on the topic at hand.
>>>
>>> Changelog material right there.
>>
>> I deliberately didn't want to include that, since it'd muddy the waters
>> on what is a 100% standalone change.
> 
> Omitting the info muddied the waters!

I claim no ill intent!

> Shrug, I give up.  "readpages is really only for readahead" can become
> another kernel wart, I guess.  Anyone who wants that capability in the
> future can sit there looping on ->readpage.

Or do the leg work, which is a rename and adding ->readpages(). As long
as folks don't get too confused, I think we're fine commandeering
->readpages() for read-ahead now.

-- 
Jens Axboe