From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-it0-f65.google.com ([209.85.214.65]:40599 "EHLO mail-it0-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752983AbeFTOHJ (ORCPT ); Wed, 20 Jun 2018 10:07:09 -0400 Received: by mail-it0-f65.google.com with SMTP id 188-v6so5648799ita.5 for ; Wed, 20 Jun 2018 07:07:09 -0700 (PDT) Subject: Re: [PATCHSET v3 0/4] Submit ->readpages() IO as read-ahead To: Andrew Morton Cc: linux-fsdevel@vger.kernel.org, viro@zeniv.linux.org.uk References: <1527691329-2371-1-git-send-email-axboe@kernel.dk> <20180619165640.0c36473a33e20ea6c0fda463@linux-foundation.org> From: Jens Axboe Message-ID: <9b1d2225-104d-865f-d821-8670271d6d4a@kernel.dk> Date: Wed, 20 Jun 2018 08:07:06 -0600 MIME-Version: 1.0 In-Reply-To: <20180619165640.0c36473a33e20ea6c0fda463@linux-foundation.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On 6/19/18 5:56 PM, Andrew Morton wrote: > On Wed, 30 May 2018 08:42:05 -0600 Jens Axboe 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