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=-3.5 required=3.0 tests=BAYES_00,DKIM_INVALID, DKIM_SIGNED,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS 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 886B6C2D0A3 for ; Tue, 3 Nov 2020 15:12:07 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id A61D3206DB for ; Tue, 3 Nov 2020 15:12:06 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=fail reason="signature verification failed" (2048-bit key) header.d=infradead.org header.i=@infradead.org header.b="rORpTKuE" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org A61D3206DB Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=infradead.org Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id CB59F6B006C; Tue, 3 Nov 2020 10:12:05 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id C65596B0072; Tue, 3 Nov 2020 10:12:05 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id B54B86B0073; Tue, 3 Nov 2020 10:12:05 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0022.hostedemail.com [216.40.44.22]) by kanga.kvack.org (Postfix) with ESMTP id 861286B006C for ; Tue, 3 Nov 2020 10:12:05 -0500 (EST) Received: from smtpin19.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay05.hostedemail.com (Postfix) with ESMTP id 12D72181AEF00 for ; Tue, 3 Nov 2020 15:12:05 +0000 (UTC) X-FDA: 77443447410.19.legs12_330f21e272b9 Received: from filter.hostedemail.com (10.5.16.251.rfc1918.com [10.5.16.251]) by smtpin19.hostedemail.com (Postfix) with ESMTP id DE85F1ACEA2 for ; Tue, 3 Nov 2020 15:12:04 +0000 (UTC) X-HE-Tag: legs12_330f21e272b9 X-Filterd-Recvd-Size: 3008 Received: from casper.infradead.org (casper.infradead.org [90.155.50.34]) by imf38.hostedemail.com (Postfix) with ESMTP for ; Tue, 3 Nov 2020 15:12:04 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=infradead.org; s=casper.20170209; h=In-Reply-To:Content-Type:MIME-Version: References:Message-ID:Subject:Cc:To:From:Date:Sender:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description; bh=aEj7VJo4jHCO8dIN63ejx7SuEQPe8te1s6UytFTRalo=; b=rORpTKuEzWg6zupj3sWK1SrP48 1v8oEFE+khacoJ5lLqShxy7KzdjtAyLpWN0223WSgiZKGVRY5kro6ao42kpqfsTtohk99K1uUfnbj aVhhK4DIAz04YXvCejvCsyCdvrKcZySnA5EWxMBfUNm0awmwZgjhJj6EF4g44Z+qIUE9iHddO3SmB inpMl9SXGpj3MuC1tRVWBHE/Uy4EOH6VNBExgO4BOrdkv1WYeuY8Im1m7ZRq35uMN/qHG5+a41K4I CtmHtqYfFlw1isSjn/+43aN88zNnxgG6/g0+2+ZHJUMeiufH9cy1zHoehw3HEfhQuYuu2GMIhMG/7 3kQbJjfg==; Received: from willy by casper.infradead.org with local (Exim 4.92.3 #3 (Red Hat Linux)) id 1kZxyZ-0005OX-HS; Tue, 03 Nov 2020 15:11:59 +0000 Date: Tue, 3 Nov 2020 15:11:59 +0000 From: Matthew Wilcox To: Christoph Hellwig Cc: linux-fsdevel@vger.kernel.org, linux-mm@kvack.org, kent.overstreet@gmail.com Subject: Re: [PATCH 07/17] mm/filemap: Change filemap_read_page calling conventions Message-ID: <20201103151159.GY27442@casper.infradead.org> References: <20201102184312.25926-1-willy@infradead.org> <20201102184312.25926-8-willy@infradead.org> <20201103073427.GG8389@lst.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20201103073427.GG8389@lst.de> 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 Tue, Nov 03, 2020 at 08:34:27AM +0100, Christoph Hellwig wrote: > > +static int filemap_read_page(struct file *file, struct address_space *mapping, > > + struct page *page) > > I still think we should drop the mapping argument as well. Feels a little weird to have it in the caller and not pass it in. > > + if (iocb->ki_flags & (IOCB_NOIO | IOCB_NOWAIT | IOCB_WAITQ)) { > > + unlock_page(page); > > + put_page(page); > > + return ERR_PTR(-EAGAIN); > > + } > > + error = filemap_read_page(iocb->ki_filp, mapping, page); > > + if (!error) > > + return page; > > I think a goto error for the error cases would be much more useful. > That would allow to also share the error put_page for the flag check > above and the truncated case below, but most importantly make the code > flow obvious vs the early return for the success case. In this patch, I'm trying to be obvious about the code motion between the two functions. This gets straightened out eventually: if (iocb->ki_flags & (IOCB_NOIO | IOCB_NOWAIT | IOCB_WAITQ)) { unlock_page(page); error = -EAGAIN; } else { error = filemap_read_page(iocb->ki_filp, mapping, page); if (!error) return 0; } error: put_page(page); return error;