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,URIBL_BLOCKED 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 AF79FC43461 for ; Thu, 10 Sep 2020 14:27:45 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id CFCB520855 for ; Thu, 10 Sep 2020 14:27:44 +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="ojX3SnxT" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org CFCB520855 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 1552F900010; Thu, 10 Sep 2020 10:27:44 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 105B190000D; Thu, 10 Sep 2020 10:27:44 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 04147900010; Thu, 10 Sep 2020 10:27:43 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0251.hostedemail.com [216.40.44.251]) by kanga.kvack.org (Postfix) with ESMTP id E311190000D for ; Thu, 10 Sep 2020 10:27:43 -0400 (EDT) Received: from smtpin07.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay05.hostedemail.com (Postfix) with ESMTP id A37F6181AEF2A for ; Thu, 10 Sep 2020 14:27:43 +0000 (UTC) X-FDA: 77247380406.07.hope02_461736d270e6 Received: from filter.hostedemail.com (10.5.16.251.rfc1918.com [10.5.16.251]) by smtpin07.hostedemail.com (Postfix) with ESMTP id 63F291803F9B3 for ; Thu, 10 Sep 2020 14:27:41 +0000 (UTC) X-HE-Tag: hope02_461736d270e6 X-Filterd-Recvd-Size: 4656 Received: from casper.infradead.org (casper.infradead.org [90.155.50.34]) by imf43.hostedemail.com (Postfix) with ESMTP for ; Thu, 10 Sep 2020 14:27:40 +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=IQvlxYYO8RQkrV5Dd72B8Ltqj5iMm8Fq8cYsZbNrhqc=; b=ojX3SnxTCZ95lrerSsPgDX5ANv wtG3BenO9qUCHVzdLenux98vl48WBXCwx6hdpfqck2PDHOKL7/h0FWpfPGvaRpFbZhB+7XNCNg6p4 chw5GO5bduqkB8tHlaq4OwsAL2mH+NA9sZqvQqRMiTub7glL0pqe1ycWB5loLWCUNtppxXHHz8aZa 7o/4ujqhAcpN62zPchBmVfYHAdn/gwJptG385RE/WZpo1UaPj2yCVMwbLqfUK8ALIzrdLZShvjflH aZ1gPYmjK8P6cLGdzFCg5bQcNSHsMpC1REF3ahmG+AeYoywttTjfXmN+dBvkKGsyHwtYsHKnsCYxW FXGI7H7w==; Received: from willy by casper.infradead.org with local (Exim 4.92.3 #3 (Red Hat Linux)) id 1kGNY2-0005Ou-3y; Thu, 10 Sep 2020 14:27:38 +0000 Date: Thu, 10 Sep 2020 15:27:38 +0100 From: Matthew Wilcox To: Hugh Dickins Cc: linux-mm@kvack.org Subject: Re: When is page->index stable? Message-ID: <20200910142738.GS6583@casper.infradead.org> References: <20200827191407.GM14765@casper.infradead.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Rspamd-Queue-Id: 63F291803F9B3 X-Spamd-Result: default: False [0.00 / 100.00] X-Rspamd-Server: rspam02 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, Aug 27, 2020 at 01:52:47PM -0700, Hugh Dickins wrote: > On Thu, 27 Aug 2020, Matthew Wilcox wrote: > > We have a number of places where we look up a page in the page cache, > > lock it, then have some kind of assertion that we got back the page we > > asked for, eg filemap_fault(): > > > > page = find_get_page(mapping, offset); > > ... > > if (!lock_page_maybe_drop_mmap(vmf, page, &fpin)) > > goto out_retry; > > ... > > VM_BUG_ON_PAGE(page_to_pgoff(page) != offset, page); > > > > but today I noticed this in shmem_undo_range(): > > > > pvec.nr = find_get_entries(mapping, index, > > min(end - index, (pgoff_t)PAGEVEC_SIZE), > > pvec.pages, indices); > > ... > > VM_BUG_ON_PAGE(page_to_pgoff(page) != index, page); > > ... > > if (!trylock_page(page)) > > continue; > > > > So is page->index stable if we have a refcount on the page, > > Yes (once it has been found in the page cache - > obviously not stable before it has been put into the page cache). > > > or is a lock on the page required? > > No. A lock on the page is required for page cache page->mapping > to be stable, but not required for its page->index to remain stable. > > > A refcount on the page prevents it from being > > split or freed. And there's plenty of comments along the lines of: > > > > mm/filemap.c: /* Leave page->index set: truncation lookup relies on it */ > > > > which indicates that once a page is removed from the page cache, its > > index remains reliable (until it's freed). > > > > It might be nice to remove all these assertions from the callers and > > bury them down in find_get_(entry,page,entries,...), but we can't do > > that if we need the lock to check the index. If we don't need the lock, > > then it should be safe to check as soon as we've checked that > > page == xas_reload(). > > Yes. > > But you might then discover something violating the principle. > I have an indistinct memory of spotting an instance once, maybe > just in a prospective patchset that didn't reach the kernel; perhaps > someone resetting page->index to 0 "for tidiness" before freeing; > maybe page migration did that once upon a time, then got fixed. > > And of course beware of hugetlbfs, defining page->index differently > (unless you have fixed that already). The other thing to beware of is swapcache, which also defines page->index differently. We went through this last year ... https://lore.kernel.org/linux-mm/20190323033852.GC10344@bombadil.infradead.org/T/#u As can be seen from that thread, even calling page_index() instead of directly looking at page->index is insufficient because having a reference on a page is insufficient to keep ClearPageSwapCache from being cleared. What you're doing is safe, because you know mapping isn't a swapcache mapping; it's a shmem mapping. So I'm going to back out a good chunk of the work I did yesterday :-(