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=-0.6 required=3.0 tests=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 A2AE3C34022 for ; Wed, 19 Feb 2020 05:35:50 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 685E324676 for ; Wed, 19 Feb 2020 05:35:50 +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="DuVHuymB" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 685E324676 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 0286D6B0003; Wed, 19 Feb 2020 00:35:50 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id EF3396B0006; Wed, 19 Feb 2020 00:35:49 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id DBACB6B0007; Wed, 19 Feb 2020 00:35:49 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0187.hostedemail.com [216.40.44.187]) by kanga.kvack.org (Postfix) with ESMTP id 97DAC6B0003 for ; Wed, 19 Feb 2020 00:35:49 -0500 (EST) Received: from smtpin20.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay02.hostedemail.com (Postfix) with ESMTP id 2757933CD for ; Wed, 19 Feb 2020 05:35:49 +0000 (UTC) X-FDA: 76505764818.20.back70_4fdc044c63218 X-HE-Tag: back70_4fdc044c63218 X-Filterd-Recvd-Size: 2898 Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) by imf11.hostedemail.com (Postfix) with ESMTP for ; Wed, 19 Feb 2020 05:35:48 +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=i5jjRMxpChRQy+2J/xRxaZpOqmFrFWQR+xd6cg6u2PI=; b=DuVHuymBPWn5wDST1aeEelhfNM /bXL88FT9MFQnHGtbK4rSNZ+luoUIXATKSnNmq0w0g+oFVWjyKpy66W6FFgRGnswoBOWQpscJbJQP 4VedcAPQbAlzmDR4kFU3J0Di3kgpR1We770va4VzU0v+3uU5Psu/vRRzwnfoiQqbwdrygPBoB0Acc Kj/viayLIFCgDZTYuGeW41k0nXnLmWVc48ZXm+FRjozkqV+sHubRee661ywX49PCOT/E1uqhfX2Ny D9csTE5YF0U7xa7/EP5Jtkli2RRKOHF+UMVo8N/d71C108g1DAP7O0iiHuaezDNJcoHL5lnGSyTJU QRfWManA==; Received: from willy by bombadil.infradead.org with local (Exim 4.92.3 #3 (Red Hat Linux)) id 1j4I1Q-0004Jt-TF; Wed, 19 Feb 2020 05:35:44 +0000 Date: Tue, 18 Feb 2020 21:35:44 -0800 From: Matthew Wilcox To: John Hubbard 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 v6 17/19] iomap: Restructure iomap_readpages_actor Message-ID: <20200219053544.GN24185@bombadil.infradead.org> References: <20200217184613.19668-1-willy@infradead.org> <20200217184613.19668-31-willy@infradead.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: 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, Feb 18, 2020 at 07:17:18PM -0800, John Hubbard wrote: > > - for (done = 0; done < length; done += ret) { > > nit: this "for" loop was perfect just the way it was. :) I'd vote here for reverting > the change to a "while" loop. Because with this change, now the code has to > separately initialize "done", separately increment "done", and the beauty of a > for loop is that the loop init and control is all clearly in one place. For things > that follow that model (as in this case!), that's a Good Thing. > > And I don't see any technical reason (even in the following patch) that requires > this change. It's doing the increment in the wrong place. We want the increment done in the middle of the loop, before we check whether we've got to the end of the page. Not at the end of the loop. > > + BUG_ON(ctx.cur_page); > > Is a full BUG_ON() definitely called for here? Seems like a WARN might suffice... Dave made a similar comment; I'll pick this up there.