From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-5.2 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS, USER_AGENT_SANE_1 autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 9F4C8C00A89 for ; Mon, 2 Nov 2020 08:18:51 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id CF882222B9 for ; Mon, 2 Nov 2020 08:18:50 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org CF882222B9 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=lst.de Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id 6D3DC6B005C; Mon, 2 Nov 2020 03:18:49 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 65DA96B005D; Mon, 2 Nov 2020 03:18:49 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 54C696B0068; Mon, 2 Nov 2020 03:18:49 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0015.hostedemail.com [216.40.44.15]) by kanga.kvack.org (Postfix) with ESMTP id 246646B005C for ; Mon, 2 Nov 2020 03:18:49 -0500 (EST) Received: from smtpin28.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay04.hostedemail.com (Postfix) with ESMTP id B8C761EE6 for ; Mon, 2 Nov 2020 08:18:48 +0000 (UTC) X-FDA: 77438777136.28.drop99_5901e62272ae Received: from filter.hostedemail.com (10.5.16.251.rfc1918.com [10.5.16.251]) by smtpin28.hostedemail.com (Postfix) with ESMTP id 96BC16C26 for ; Mon, 2 Nov 2020 08:18:48 +0000 (UTC) X-HE-Tag: drop99_5901e62272ae X-Filterd-Recvd-Size: 4039 Received: from verein.lst.de (verein.lst.de [213.95.11.211]) by imf10.hostedemail.com (Postfix) with ESMTP for ; Mon, 2 Nov 2020 08:18:47 +0000 (UTC) Received: by verein.lst.de (Postfix, from userid 2407) id 902F267373; Mon, 2 Nov 2020 09:18:44 +0100 (CET) Date: Mon, 2 Nov 2020 09:18:44 +0100 From: Christoph Hellwig To: Matthew Wilcox Cc: Christoph Hellwig , Andrew Morton , Kent Overstreet , linux-mm@kvack.org, linux-fsdevel@vger.kernel.org Subject: Re: [PATCH 04/13] mm: handle readahead in generic_file_buffered_read_pagenotuptodate Message-ID: <20201102081844.GA12752@lst.de> References: <20201031090004.452516-1-hch@lst.de> <20201031090004.452516-5-hch@lst.de> <20201031170646.GT27442@casper.infradead.org> <20201101103144.GC26447@lst.de> <20201101104958.GU27442@casper.infradead.org> <20201101105112.GA26860@lst.de> <20201101105158.GA26874@lst.de> <20201101110406.GV27442@casper.infradead.org> <20201101115217.GA27488@lst.de> <20201101145507.GZ27442@casper.infradead.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20201101145507.GZ27442@casper.infradead.org> User-Agent: Mutt/1.5.17 (2007-11-01) X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Sun, Nov 01, 2020 at 02:55:07PM +0000, Matthew Wilcox wrote: > Hm? I have this: Yes, this looks fine. Not sure if I saw an earlier version or was just confused. > > mm/filemap: Change calling convention for buffered read functions > > > > - please also drop the mapping argument to the various functions while > > you're at it > > Not sure I see the point to it. Sure, they _can_ retrieve it with > iocb->ki_filp->f_mapping, but usually we like to pass the mapping > argument to functions which do something with the mapping. There really isn't any point in passing an extra argument that can trivially be derived. > > Also I think the messy list of uptodate checks could now be simplified > > down to: > > > > if (!PageUptodate(page)) { > > if (inode->i_blkbits <= PAGE_SHIFT && > > I've been wondering about this test's usefulness in the presence > of THP. Do we want to make it 'if (inode->i_blkbits < (thp_order(page) > + PAGE_SHIFT)'? It doesn't make sense to leave it as it is because then > a 1kB and 4kB blocksize filesystem will behave differently. Yeah, the partially uptodate checks would make sense for huge pages. Just make sure that the iomap version does the right thing for this case first. > > > mapping->a_ops->is_partially_uptodate && > > !iov_iter_is_pipe(iter)) { > > if (!page->mapping) > > goto truncated; > > if (mapping->a_ops->is_partially_uptodate(page, > > pos & (thp_size(page) - 1), count)) > > goto uptodate; > > } > > Now that you've rearranged it like this, it's obvious that the truncated > check is in the wrong place. We don't want to call filemap_read_page() > if the page has been truncated either. True. > A later patch hoists the put_page to the caller, so I think you'll like > where it ends up. I still find the result in the callers a little weird as it doesn't follow the normal jump to the end for exceptions flow, but that is just another tiny nitpick. > > > mm/filemap: Restructure filemap_get_pages > > > > - I have to say I still like my little helper here for > > the two mapping_get_read_thps calls plus page_cache_sync_readahead > > I'll take a look at that. > > Looking at all this again, I think I want to pull the IOCB checks out > of filemap_read_page() and just pass a struct file to it. It'll make > it more clear that NOIO, NOWAIT and WAITQ can't get to calling ->readpage. filemap_update_page alread exits early for NOWAIT, so it would just need the NOIO check. filemap_create_page checks NOIO early, but allocating the page for NOWAIT also seems rather pointless. So yes, I think this would be an improvement.