On 6/19/18 5:56 PM, Andrew Morton wrote: > On Wed, 30 May 2018 08:42:05 -0600 Jens Axboe <axboe@xxxxxxxxx> 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. > For example, f2fs_mpage_readpages(). Which, being f2fs, has decided to > copy-paste-modify mpage_readpages() and to then use it for > non-readahead purposes. If that code ever gets its act together then > we have a problem. > > A proper approach might be to add a new > address_space_operations.readahead which sets REQ_RAHEAD but after > doing all that, nothing would use address_space_operations.readpages, > which is a bit weird. I did consider that, but as you also found out, that would leave us with a readpages() that's never used. Alternatively, we can just rename it to make it more explicit. > And what's happening with the individual ->readpage() calls which > read_pages() does? Do they still not get REQ_RAHEAD treatment? They should, yes. > 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. 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. -- Jens Axboe
