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=-6.6 required=3.0 tests=DKIM_INVALID,DKIM_SIGNED, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY, SPF_HELO_NONE,SPF_PASS autolearn=unavailable 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 E8138C35670 for ; Sat, 22 Feb 2020 01:54:39 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id A9EF420656 for ; Sat, 22 Feb 2020 01:54:39 +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="okpmlLDU" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org A9EF420656 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 563216B0003; Fri, 21 Feb 2020 20:54:39 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 5126F6B0006; Fri, 21 Feb 2020 20:54:39 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 428456B0007; Fri, 21 Feb 2020 20:54:39 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0053.hostedemail.com [216.40.44.53]) by kanga.kvack.org (Postfix) with ESMTP id 288026B0003 for ; Fri, 21 Feb 2020 20:54:39 -0500 (EST) Received: from smtpin30.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay04.hostedemail.com (Postfix) with ESMTP id D55FE1F06 for ; Sat, 22 Feb 2020 01:54:38 +0000 (UTC) X-FDA: 76516093836.30.event50_459fae39a3e3d X-HE-Tag: event50_459fae39a3e3d X-Filterd-Recvd-Size: 5426 Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) by imf32.hostedemail.com (Postfix) with ESMTP for ; Sat, 22 Feb 2020 01:54:38 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=infradead.org; s=bombadil.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=3z9OxyAbSn1SdH2kkHfL31c9BGogsy9OLW6r1AiETwA=; b=okpmlLDUcn2WJSLgCMGAQ/y3ea wUx7sYNdOzqYPRD79lRmEtjSkrPrcJfVsD9xPGx/OEj4opTESHh6CYwZzOmuJEsyumYBwdnBSPv7g 16GkwPCUQhbT1ezTB0jognGyz8oIm6SUil1aybwZf5s5+OhrDRLxuYqnAi5mVLeaU2RxOwOxA/zNs KYbdl3XUppUek8iRi+5uW/I3hpcLPqxH+AmdzbPTNFg7qp4grnRwvEiCEJmAo3/C8soB6NZZdaPwo 6XP9UHgU7AUBNbe3o0CA3AZUkjmFriX3wxHNG3GcDmgcTse+CtXbmuDxLU2MsixE0H2J7aWT8SIfZ PnUpa2Fw==; Received: from willy by bombadil.infradead.org with local (Exim 4.92.3 #3 (Red Hat Linux)) id 1j5K03-0000tY-FT; Sat, 22 Feb 2020 01:54:35 +0000 Date: Fri, 21 Feb 2020 17:54:35 -0800 From: Matthew Wilcox To: "Darrick J. Wong" Cc: linux-fsdevel@vger.kernel.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org, linux-btrfs@vger.kernel.org, linux-erofs@lists.ozlabs.org, linux-ext4@vger.kernel.org, linux-f2fs-devel@lists.sourceforge.net, cluster-devel@redhat.com, ocfs2-devel@oss.oracle.com, linux-xfs@vger.kernel.org Subject: Re: [PATCH v7 21/24] iomap: Restructure iomap_readpages_actor Message-ID: <20200222015435.GH24185@bombadil.infradead.org> References: <20200219210103.32400-1-willy@infradead.org> <20200219210103.32400-22-willy@infradead.org> <20200222004425.GG9506@magnolia> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20200222004425.GG9506@magnolia> 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 Fri, Feb 21, 2020 at 04:44:25PM -0800, Darrick J. Wong wrote: > On Wed, Feb 19, 2020 at 01:01:00PM -0800, Matthew Wilcox wrote: > > From: "Matthew Wilcox (Oracle)" > > > > By putting the 'have we reached the end of the page' condition at the end > > of the loop instead of the beginning, we can remove the 'submit the last > > page' code from iomap_readpages(). Also check that iomap_readpage_actor() > > didn't return 0, which would lead to an endless loop. > > > > Signed-off-by: Matthew Wilcox (Oracle) > > --- > > fs/iomap/buffered-io.c | 32 ++++++++++++++++++-------------- > > 1 file changed, 18 insertions(+), 14 deletions(-) > > > > diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c > > index cb3511eb152a..31899e6cb0f8 100644 > > --- a/fs/iomap/buffered-io.c > > +++ b/fs/iomap/buffered-io.c > > @@ -400,15 +400,9 @@ iomap_readpages_actor(struct inode *inode, loff_t pos, loff_t length, > > void *data, struct iomap *iomap, struct iomap *srcmap) > > { > > struct iomap_readpage_ctx *ctx = data; > > - loff_t done, ret; > > - > > - for (done = 0; done < length; done += ret) { > > - if (ctx->cur_page && offset_in_page(pos + done) == 0) { > > - if (!ctx->cur_page_in_bio) > > - unlock_page(ctx->cur_page); > > - put_page(ctx->cur_page); > > - ctx->cur_page = NULL; > > - } > > + loff_t ret, done = 0; > > + > > + while (done < length) { > > if (!ctx->cur_page) { > > ctx->cur_page = iomap_next_page(inode, ctx->pages, > > pos, length, &done); > > @@ -418,6 +412,20 @@ iomap_readpages_actor(struct inode *inode, loff_t pos, loff_t length, > > } > > ret = iomap_readpage_actor(inode, pos + done, length - done, > > ctx, iomap, srcmap); > > + done += ret; > > + > > + /* Keep working on a partial page */ > > + if (ret && offset_in_page(pos + done)) > > + continue; > > + > > + if (!ctx->cur_page_in_bio) > > + unlock_page(ctx->cur_page); > > + put_page(ctx->cur_page); > > + ctx->cur_page = NULL; > > + > > + /* Don't loop forever if we made no progress */ > > + if (WARN_ON(!ret)) > > + break; > > } > > > > return done; > > @@ -451,11 +459,7 @@ iomap_readpages(struct address_space *mapping, struct list_head *pages, > > done: > > if (ctx.bio) > > submit_bio(ctx.bio); > > - if (ctx.cur_page) { > > - if (!ctx.cur_page_in_bio) > > - unlock_page(ctx.cur_page); > > - put_page(ctx.cur_page); > > - } > > + BUG_ON(ctx.cur_page); > > Whoah, is the system totally unrecoverably hosed at this point? > > I get that this /shouldn't/ happen, but should we somehow end up with a > page here, are we unable either to release it or even just leak it? I'd > have thought a WARN_ON would be just fine here. If we do find a page here, we don't actually know what to do with it. It might be (currently) locked, it might have the wrong refcount. Whatever is going on, it's probably better that we stop everything right here rather than allow things to go further and possibly present bad data to the application. I mean, we could even be leaking the previous contents of this page to userspace. Or maybe the future contents of a page which shouldn't be in the page cache any more, but userspace gets a mapping to it. I'm not enthusiastic about putting in some code here to try to handle a "can't happen" case, since it's never going to be tested, and might end up causing more problems than it tries to solve. Let's just stop.