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.6 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 5CC17C4320A for ; Sat, 21 Aug 2021 18:48:30 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 876DF61220 for ; Sat, 21 Aug 2021 18:48:29 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 876DF61220 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=infradead.org Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=kvack.org Received: by kanga.kvack.org (Postfix) id 972AD8D0001; Sat, 21 Aug 2021 14:48:28 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 922166B0072; Sat, 21 Aug 2021 14:48:28 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 80F518D0001; Sat, 21 Aug 2021 14:48:28 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0235.hostedemail.com [216.40.44.235]) by kanga.kvack.org (Postfix) with ESMTP id 62F426B006C for ; Sat, 21 Aug 2021 14:48:28 -0400 (EDT) Received: from smtpin37.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay04.hostedemail.com (Postfix) with ESMTP id E284C2BA64 for ; Sat, 21 Aug 2021 18:48:27 +0000 (UTC) X-FDA: 78499973454.37.E355731 Received: from casper.infradead.org (casper.infradead.org [90.155.50.34]) by imf22.hostedemail.com (Postfix) with ESMTP id 3003A1900 for ; Sat, 21 Aug 2021 18:48:27 +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=Y+SHGRYZ6q5sk+kjn9pjpKk095saNNvdcL2W/RGfeXU=; b=FloXxzzbcRyIpFAcyyAaxejdh7 p5Fv/EOhyEFdbmXN21jBgGrYM6Mb88zAXoreW12qJ4LNuHiUOAbPWk0WuRhViuMZTEi/QJA5UDsq6 CiF8IylI3eFPmDDU6UPH/PTvTJrG7/SM61dYhDh9cXZkGkw8I6KTTHDi5PGgiQ/zAXuvvYEB4itKD cBCWgsO9WONjG3Lv71u9kRm+VMZTbyIc3a0nobaQjCXNC0ozA8Gxty2d40gOUN8NxJwezXeD5GuNs 9yh6b7ehDDiWgXXa002B9xEp8XcrQa9luk5KCnlgNT3uwTQuY0cormIGsEYtjfDBb+PzOYTCmpq/S AelJ8XLg==; Received: from willy by casper.infradead.org with local (Exim 4.94.2 #2 (Red Hat Linux)) id 1mHW2L-007q6p-WD; Sat, 21 Aug 2021 18:48:19 +0000 Date: Sat, 21 Aug 2021 19:48:09 +0100 From: Matthew Wilcox To: linux-kernel@vger.kernel.org Cc: linux-mm@kvack.org, linux-fsdevel@vger.kernel.org Subject: Re: [PATCH v14 112/138] mm/filemap: Convert filemap_get_read_batch to use folios Message-ID: References: <20210715033704.692967-1-willy@infradead.org> <20210715033704.692967-113-willy@infradead.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20210715033704.692967-113-willy@infradead.org> Authentication-Results: imf22.hostedemail.com; dkim=pass header.d=infradead.org header.s=casper.20170209 header.b=FloXxzzb; spf=none (imf22.hostedemail.com: domain of willy@infradead.org has no SPF policy when checking 90.155.50.34) smtp.mailfrom=willy@infradead.org; dmarc=none X-Rspamd-Server: rspam05 X-Rspamd-Queue-Id: 3003A1900 X-Stat-Signature: jyjsmruu15jna9bufmohc4bp8q55tdro X-HE-Tag: 1629571707-823790 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 Thu, Jul 15, 2021 at 04:36:38AM +0100, Matthew Wilcox (Oracle) wrote: > rcu_read_lock(); > - for (head = xas_load(&xas); head; head = xas_next(&xas)) { > - if (xas_retry(&xas, head)) > + for (folio = xas_load(&xas); folio; folio = xas_next(&xas)) { > + if (xas_retry(&xas, folio)) > continue; > - if (xas.xa_index > max || xa_is_value(head)) > + if (xas.xa_index > max || xa_is_value(folio)) > break; > - if (!page_cache_get_speculative(head)) > + if (!folio_try_get_rcu(folio)) > goto retry; > > - /* Has the page moved or been split? */ > - if (unlikely(head != xas_reload(&xas))) > + if (unlikely(folio != xas_reload(&xas))) > goto put_page; > > - if (!pagevec_add(pvec, head)) > + if (!pagevec_add(pvec, &folio->page)) > break; > - if (!PageUptodate(head)) > + if (!folio_test_uptodate(folio)) > break; > - if (PageReadahead(head)) > + if (folio_test_readahead(folio)) > break; > - xas.xa_index = head->index + thp_nr_pages(head) - 1; > + xas.xa_index = folio->index + folio_nr_pages(folio) - 1; > xas.xa_offset = (xas.xa_index >> xas.xa_shift) & XA_CHUNK_MASK; > continue; It's not a bug in _this_ patch, but these last two lines become a bug once the page cache is converted to store folios as multi-index entries (as opposed to now when it replicates an order-N entry 2^N times). I should not have used xas.xa_shift (which is the shift of the entry we're looking for and is always 0), but xas.xa_node->shift (which is the shift of the entry that we found). If you have an order-7 page, occupying (say) indices 128-255, we set xa_index to 255, but instead of setting xa_offset to 3, we set it to 63. That tricks __xas_next() into going up to the parent node, and then back down, which might mean that we terminate the scan early, or that we skip over all the other entries in the node. What I actually noticed was a crash where we ended up loading an internal entry out of the XArray. It's all a bit complicated really. That calls for a helper, and this is my current candidate: +static inline void xas_advance(struct xa_state *xas, unsigned long index) +{ + unsigned char shift = xas_is_node(xas) ? xas->xa_node->shift : 0; + + xas->xa_index = index; + xas->xa_offset = (index >> shift) & XA_CHUNK_MASK; +} ... - xas.xa_index = folio->index + folio_nr_pages(folio) - 1; - xas.xa_offset = (xas.xa_index >> xas.xa_shift) & XA_CHUNK_MASK; + xas_advance(&xas, folio->index + folio_nr_pages(folio) - 1); This is coming up on 4 hours of continuous testing using generic/559. Without it, it would usually crash in about 40 minutes.