Received-SPF: pass (google.com: domain of linux-kernel+bounces-121283-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Wed, 27 Mar 2024 12:31:49 +0000
From: Matthew Wilcox <willy@infradead.org>
To: Zhaoyang Huang <huangzhaoyang@gmail.com>
Cc: =?utf-8?B?6buE5pyd6ZizIChaaGFveWFuZyBIdWFuZyk=?= <zhaoyang.huang@unisoc.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	"linux-mm@kvack.org" <linux-mm@kvack.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	=?utf-8?B?5bq357qq5ruoIChTdGV2ZSBLYW5nKQ==?= <Steve.Kang@unisoc.com>
Subject: Re: summarize all information again at bottom//reply: reply: [PATCH]
 mm: fix a race scenario in folio_isolate_lru
Message-ID: <ZgQRtQ60mrvOUKXo@casper.infradead.org>
References: <Zfj_-LbiIsAVWcPX@casper.infradead.org>
 <CAGWkznHOWVPpKHOuvvDF2zppkTT0_x4WyJ5S75j+EghhZmEDdg@mail.gmail.com>
 <ZfwpuRjAZV07_lc3@casper.infradead.org>
 <CAGWkznFtez1fj2L2CtFnA5k-Tn4WtxmDOw=fjOWPg-ZGJX=VWw@mail.gmail.com>
 <Zfz4_GJAHRInB8ul@casper.infradead.org>
 <CAGWkznEzpcWi4oXVn_WFahnQj3dHcwc_4VW6m1Ss-KJ8mD3F3Q@mail.gmail.com>
 <ZgDt9mwN-Py5Y-xr@casper.infradead.org>
 <CAGWkznHO27EpVVpFyKnv-SX-JTYCXQxb0MG+EW07gaupocR4RQ@mail.gmail.com>
 <ZgK91II_eSYY6D2F@casper.infradead.org>
 <CAGWkznGLySzLE17+rCe=UoA26vx=iM375o2zkruKM9ssG05QzA@mail.gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAGWkznGLySzLE17+rCe=UoA26vx=iM375o2zkruKM9ssG05QzA@mail.gmail.com>

On Wed, Mar 27, 2024 at 09:25:59AM +0800, Zhaoyang Huang wrote:
> > Ignoring any other thread, you're basically saying that there's a
> > refcount imbalance here.  Which means we'd hit an assert (that folio
> > refcount went below zero) in the normal case where another thread wasn't
> > simultaneously trying to do anything.
> Theoretically Yes but it is rare in practice as aops->readahead will
> launch all pages to IO under most scenarios.

Rare, but this path has been tested.

> read_pages
>     aops->readahead[1]
> ...
>     while (folio = readahead_folio)[2]
>         filemap_remove_folio
> 
> IMO, according to the comments of readahead_page, the refcnt
> represents page cache dropped in [1] makes sense for two reasons, '1.
> The folio is going to do IO and is locked until IO done;2. The refcnt
> will be added back when found again from the page cache and then serve
> for PTE or vfs' while it doesn't make sense in [2] as the refcnt of
> page cache will be dropped in filemap_remove_folio
> 
>  * Context: The page is locked and has an elevated refcount.  The caller
>  * should decreases the refcount once the page has been submitted for I/O
>  * and unlock the page once all I/O to that page has completed.
>  * Return: A pointer to the next page, or %NULL if we are done.

Follow the refcount through.

In page_cache_ra_unbounded():

                folio = filemap_alloc_folio(gfp_mask, 0);
(folio has refcount 1)
                ret = filemap_add_folio(mapping, folio, index + i, gfp_mask);
(folio has refcount 2)

Then we call read_pages()
First we call ->readahead() which for some reason stops early.
Then we call readahead_folio() which calls folio_put()
(folio has refcount 1)
Then we call folio_get()
(folio has refcount 2)
Then we call filemap_remove_folio()
(folio has refcount 1)
Then we call folio_unlock()
Then we call folio_put()
(folio has refcount 0 and is freed)

Yes, other things can happen in there to increment the refcount, so this
folio_put() might not be the last put, but we hold the folio locked the
entire time, so many things which might be attempted will block on the
folio lock.  In particular, nobody can remove it from the page cache,
so its refcount cannot reach 0 until the last folio_put() of the
sequence.