Re: Improve page refcount scalability

[Kiryl Shutsemau <kirill@shutemov.name>]

On Tue, Oct 13, 2026 at 04:39:24PM -0700, Linus Torvalds wrote:
> On Tue, 13 Oct 2026 at 15:43, Kiryl Shutsemau <kirill@shutemov.name> wrote:
> >
> > There's a race in filemap_read_rcu_batch(). If the folio is replaced
> > under us with a new one and then i_size is extended, we can read data
> > that never got written there:
> 
> Hmm. Wouldn't it be better to move the i_size check up, and then have
> one single check for that
> 
>                 if (xas_reload(&xas) != folio)
>                         break;
>
> case?

I was not sure it is possible. i_size has to be read after we know the
folio is uptodate, and the uptodate bit only means something after the
reload.

But it is fine: PG_rcu_free is only set after the folio was seen
uptodate, so reading i_size after the flag test and smp_rmb() gives the
same ordering. And i_size_read() is an acquire, so it is ordered before
the reload.

The uptodate check in filemap_read_rcu_can_copy() has to stay, though.
The flag only says the folio was uptodate when it was marked. Some
filesystems clear PG_uptodate on a folio that stays in the page cache,
and that can happen after the folio got the flag.

See the patch below.

> Otherwise we end up with two of them, one inside that "folio != prev" case...
> 
> IOW, I wonder if we could just make the whole
> 
>                         if (!test_bit(PG_rcu_free, &folio->flags.f))
>                                 break;
>                         smp_rmb();
>                         if (xas_reload(&xas) != folio)
>                                 break;
> 
> be unconditional, and move the i_size read up there too (I notice that
> the comment about uptodate got in there too - PG_rcu_free indirectly
> does imply that, so it's not *wrong*, but it's perhaps a bit
> surprising).
> 
> That said:
> 
> > My branch contains a few more changes that address downstream effects of
> > RCU-freeing the page cache. But we can come back to that later once we get
> > agreement on the core mechanism.
> 
> Would you be willing to take this whole series over?

Yes, can do.

diff --git a/mm/filemap.c b/mm/filemap.c
index b85de94a1b6e..1836da9eee5e 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -2879,9 +2879,9 @@ static bool filemap_read_rcu_can_copy(struct folio *folio, pgoff_t index,
  * Copy page by page, each page looked up in i_pages.  Without a reference
  * a split can change the folio's size under us, so only the page at the
  * index xas_load() returned the folio for is known to be part of it.
- * The folio's flags are checked once per folio: prev is the folio of the
- * previous page.  *last_index is the page the previous copy ended in, and
- * is set to each page copied, as the slow path sets last_pos.
+ * filemap_read_rcu_can_copy() is called once per folio: prev is the folio
+ * of the previous page.  *last_index is the page the previous copy ended
+ * in, and is set to each page copied, as the slow path sets last_pos.
  *
  * Copy at most FOLIO_BATCH_SIZE pages under one rcu_read_lock().  Return
  * true if that many were copied and the read is not done.  prev is not
@@ -2928,34 +2928,25 @@ static bool filemap_read_rcu_batch(struct kiocb *iocb, struct iov_iter *iter,
 		if (xas_retry(&xas, folio) || !folio || xa_is_value(folio))
 			break;
 
+		/* Not folio_test_rcu_free(): it may not be a folio any more */
+		if (!test_bit(PG_rcu_free, &folio->flags.f))
+			break;
+		/* Read i_size and slot after the flag: it implies uptodate */
+		smp_rmb();
+		isize = i_size_read(inode);
+		if (pos >= isize)
+			break;
+		/* A write fills the slot before it extends i_size */
+		if (xas_reload(&xas) != folio)
+			break;
+		/* Read the rest of the folio only once the slot holds it */
+		smp_rmb();
 		if (folio != prev) {
-			/*
-			 * The memory may be anything by now, a tail page or
-			 * no folio at all, so test the bit and not the folio
-			 * flag.  The loads must happen in this order: the
-			 * flag, then the slot, then everything else.
-			 */
-			if (!test_bit(PG_rcu_free, &folio->flags.f))
-				break;
-			/*
-			 * The flag before the slot: a flag set on a later
-			 * folio in this memory was set after this folio left
-			 * the slot (see above)
-			 */
-			smp_rmb();
-			if (xas_reload(&xas) != folio)
-				break;
-			/* The slot before anything else of the folio */
-			smp_rmb();
 			if (!filemap_read_rcu_can_copy(folio, index, *last_index))
 				break;
 			prev = folio;
 		}
 
-		/* i_size must be checked after we know the folio is uptodate */
-		isize = i_size_read(inode);
-		if (pos >= isize)
-			break;
 		bytes = min_t(loff_t, isize - pos,
 			      min_t(size_t, iov_iter_count(iter),
 				    PAGE_SIZE - offset));
-- 
  Kiryl Shutsemau / Kirill A. Shutemov