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

[Andrew Morton <akpm@linux-foundation.org>]

On Wed, 20 Jun 2018 08:07:06 -0600 Jens Axboe <axboe@kernel.dk> wrote:

> On 6/19/18 5:56 PM, Andrew Morton wrote:
> > On Wed, 30 May 2018 08:42:05 -0600 Jens Axboe <axboe@kernel.dk> wrote:
> > 
> >> The only caller of ->readpages() is from read-ahead, yet we don't
> >> submit IO flagged with REQ_RAHEAD. This means we don't see it in
> >> blktrace, for instance, which is a shame. We already make assumptions
> >> about ->readpages() just being for read-ahead in the mpage
> >> implementation, using readahead_gfp_mask(mapping) as out GFP mask of
> >> choice.
> >>
> >> This small series fixes up mpage_readpages() to submit with
> >> REQ_RAHEAD, which takes care of file systems using mpage_readpages().
> >> The first patch is a prep patch, that makes do_mpage_readpage() take
> >> an argument structure.
> > 
> > Well gee.  That sounds like a total fluke and I don't see anything
> > which prevents future code from using readpages for something other
> > than readahead.
> 
> Nothing prevents it, but it's always been the case. So we need to
> embrace the fact one way or another.

All it will take is one simple (and desirable) cleanup to f2fs and this
will all break.  Take that as an indication of the fragility of this
assumption.

> > And what's happening with the individual ->readpage() calls which
> > read_pages() does?  Do they still not get REQ_RAHEAD treatment?
> 
> They should, yes.

hm, how does that happen.

> > This is kinda dirty, but we could add a readahead flag to blk_plug, set
> > it in read_pages(), test it down in the block layer somewhere....
> > That's grubby, but at least it wouldn't increase sizeof(task_struct)?
> 
> That sounds way too dirty for my liking, I'd really not want to mix that
> in with plugging.

That's the wrong way of thinking about it ;) Rename blk_plug to
blk_state or something and view it as a communication package between
the VFS level and the block layer and it all fits together pretty well.

> Why not just treat it like it is - readpages() is clearly just
> read-ahead. This isn't something new I'm inventing, it's always been so.
> Seems to me that we just need to make it explicit.

There's no reason at all why ->readpages() is exclusively for use by
readahead!  I'm rather surprised that fsf2 is the only fs which is
(should be) using ->readpages internally.

Can we change blktrace instead?  Rename REQ_RAHEAD to REQ_READPAGES? 
Then everything will fit together OK.