On Wed, 20 Jun 2018 16:28:54 -0600 Jens Axboe <axboe@xxxxxxxxx> wrote: > >> 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. > > The assumption has been true for a decade or more. The only problem with > it is that we have this ->readpages() interface that is only used for > readahead. Hence it should be ->readahead() or similar, that avoid the > fragility argument. There's no reason why readpages shouldn't be used for non-readahead purposes. > >>> 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. > > Maybe we aren't talking about the same thing. Which individual > ->readpage() exactly? I forget, never mind ;) > >>> 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. > > We have a perfectly fine established interface for telling the block > layer what kind of IO we're submitting, and that's setting it in the > bio. Introducing a secondary and orthogonal interface for this is a > horrible idea, imho. ->readpages() callers don't have access to the bio(s). > >> 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. > > I totally agree, there's no reason why it is, and everybody is surprised > that that is the case. But the fact of the matter is that that is the > case, and always has been so. So I'll repeat what I said before, we need > to embrace that and make it EXPLICIT instead of side stepping the issue. My point is that f2fs (for example) *should* be using mpage_readpages() right now, for its non-readahead purposes. If we are to remove callers' ability to use readpages for non-readahead purposes we should rename the address_space field. > > Can we change blktrace instead? Rename REQ_RAHEAD to REQ_READPAGES? > > Then everything will fit together OK. > > Why do you insist on working around it?! That's just creating another > assumption in another place, not fixing the actual issue. All I've been told about is blktrace. > 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.
