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 5E531C352A3 for ; Tue, 11 Feb 2020 12:54:22 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 251BE20675 for ; Tue, 11 Feb 2020 12:54:21 +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="eWC7poTU" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 251BE20675 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 8F7F16B02C9; Tue, 11 Feb 2020 07:54:21 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 881686B02CB; Tue, 11 Feb 2020 07:54:21 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 74A006B02CC; Tue, 11 Feb 2020 07:54:21 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0045.hostedemail.com [216.40.44.45]) by kanga.kvack.org (Postfix) with ESMTP id 590D16B02C9 for ; Tue, 11 Feb 2020 07:54:21 -0500 (EST) Received: from smtpin15.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay05.hostedemail.com (Postfix) with ESMTP id ECDD3181AC9CC for ; Tue, 11 Feb 2020 12:54:20 +0000 (UTC) X-FDA: 76477839480.15.wound25_601e898e99b1a X-HE-Tag: wound25_601e898e99b1a X-Filterd-Recvd-Size: 4298 Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) by imf47.hostedemail.com (Postfix) with ESMTP for ; Tue, 11 Feb 2020 12:54:20 +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=+uQN30cS5Bj87iEEBzdOLx8lg9+i0ExEu8VOZjq8Ndo=; b=eWC7poTUmbgHCYbKut5tUcDZwL w3YYTT854/6orjDXWt0o8myOrLaqYxmNildHwtqHwB9P6sAmRSzUVkC2ndTa5MBo6vPhEIM38iBQg 3QFH9rbgbTMx5jUAauOSubj91u3I4QqJCftUeJ3u2BRZFY7uw1LRQwqgp3/dZBByXH8F5OzZAb0t+ miBihdnvmVJGXqnRBr1rG3atRoSZFunPwyyK3rI6Tg1RqDm7wVtET7WbIVm0KlJr9sAYInPEKF3Lu GTYeC4TiyWPK4e73vfdDf6T1j5MFZMYQQejC42VttS5bt+wx+jfOtOIGuI60RBaTCQF1YD4hRLaX6 1eYOefYA==; Received: from willy by bombadil.infradead.org with local (Exim 4.92.3 #3 (Red Hat Linux)) id 1j1V3O-0006U6-1C; Tue, 11 Feb 2020 12:54:14 +0000 Date: Tue, 11 Feb 2020 04:54:13 -0800 From: Matthew Wilcox To: Dave Chinner 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 v5 04/13] mm: Add readahead address space operation Message-ID: <20200211125413.GU8731@bombadil.infradead.org> References: <20200211010348.6872-1-willy@infradead.org> <20200211010348.6872-5-willy@infradead.org> <20200211045230.GD10776@dread.disaster.area> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20200211045230.GD10776@dread.disaster.area> 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 11, 2020 at 03:52:30PM +1100, Dave Chinner wrote: > > +struct readahead_control { > > + struct file *file; > > + struct address_space *mapping; > > +/* private: use the readahead_* accessors instead */ > > + pgoff_t start; > > + unsigned int nr_pages; > > + unsigned int batch_count; > > +}; > > + > > +static inline struct page *readahead_page(struct readahead_control *rac) > > +{ > > + struct page *page; > > + > > + if (!rac->nr_pages) > > + return NULL; > > + > > + page = xa_load(&rac->mapping->i_pages, rac->start); > > + VM_BUG_ON_PAGE(!PageLocked(page), page); > > + rac->batch_count = hpage_nr_pages(page); > > + rac->start += rac->batch_count; > > There's no mention of large page support in the patch description > and I don't recall this sort of large page batching in previous > iterations. > > This seems like new functionality to me, not directly related to > the initial ->readahead API change? What have I missed? I had a crisis of confidence when I was working on this -- the loop originally looked like this: #define readahead_for_each(rac, page) \ for (; (page = readahead_page(rac)); rac->nr_pages--) and then I started thinking about what I'd need to do to support large pages, and that turned into #define readahead_for_each(rac, page) \ for (; (page = readahead_page(rac)); \ rac->nr_pages -= hpage_nr_pages(page)) but I realised that was potentially a use-after-free because 'page' has certainly had put_page() called on it by then. I had a brief period where I looked at moving put_page() away from being the filesystem's responsibility and into the iterator, but that would introduce more changes into the patchset, as well as causing problems for filesystems that want to break out of the loop. By this point, I was also looking at the readahead_for_each_batch() iterator that btrfs uses, and so we have the batch count anyway, and we might as well use it to store the number of subpages of the large page. And so it became easier to just put the whole ball of wax into the initial patch set, rather than introduce the iterator now and then fix it up in the patch set that I'm basing on this. So yes, there's a certain amount of excess functionality in this patch set ... I can remove it for the next release.