Re: [PATCH 03/13] mm: cachestat: restart the walk after each entry

[Lorenzo Stoakes (ARM)]

On Wed, Nov 11, 2026 at 11:38:39PM +0000, Usama Arif wrote:
>
>
> On 11/11/2026 18:13, Lorenzo Stoakes (ARM) wrote:
> > From: Chris Mason <mason@xxxxxxxxxx>
> >
> > cachestat() can leave cached pages out of its counts. This happens if
> > the entry that the call has just counted is replaced by a larger folio
> > that starts in the same XArray slot.
> >
> > The issue is that filemap_cachestat() walks the page cache under the RCU
> > read lock, so racing concurrent collapse/split is possible.
> >
> > Thus, an entry can be replaced immediately after the walk has returned
> > it. When that happens, the next xas_find() skips the sibling entries that
> > point to the slot of that entry, since ordinarily they belong to that
> > entry.
> >
> > However a concurrent collapse_file() or __filemap_add_folio() (when
> > replacing a shadow entry) can store a larger folio in the slot, in which
> > case the sibling entries belong to the new folio and the walk still skips
> > them.
> >
> > Consider:
> >
> >     cachestat                         collapse
> >     =========                         ========
> >     count a 64-page folio in slot 8
> >                                       store a 512-page folio in slot 8,
> >                                       with sibling entries in slots 9-15
> >     xas_find() starts at slot 8
> >     and skips slots 9-15
> >       /* 448 pages are never counted */
> >
> > Also consider:
> >
> >     filemap_cachestat()                  __filemap_add_folio()
> >     ===================                  =====================
> >     counts the shadow entry in slot 0
> >                                          slot 0: a folio of 32 pages
> >                                          slots 1-31: siblings of slot 0
> >     xas_find() skips slots 1-31
> >       /* 31 pages are never counted */
> >
> > Essentially the fact that large folios might be present means we cannot
> > safely continue the XArray walk, and must reset the iterator each time.
> >
> > Fixes: cf264e1329fb ("cachestat: implement cachestat syscall")
> > Signed-off-by: Chris Mason <mason@xxxxxxxxxx>
> > [ ljs: squashed 2 commits, fixed up comment in code, and commit message ]
> > Signed-off-by: Lorenzo Stoakes (ARM) <ljs@xxxxxxxxxx>

Oops I didn't even include a 'fix this by' even...

David/Andrew - as per Usama, could you add this to the end of the commit msg:

	Fix this by resetting the walk in each loop. This incurs a performance
	penalty, but given that only the RCU lock is held here, it can't be
	avoided.

> > ---
> >  mm/filemap.c | 7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/mm/filemap.c b/mm/filemap.c
> > index 7c05e4fad081..c4e82891767b 100644
> > --- a/mm/filemap.c
> > +++ b/mm/filemap.c
> > @@ -4731,10 +4731,11 @@ static void filemap_cachestat(struct address_space *mapping,
> >  		if (folio_last_index >= last_index)
> >  			break;
> >
> > -		if (need_resched()) {
> > -			xas_set(&xas, curr_index);
> > +		/* Collapse or new folio spanning shadow entries needs reset. */
> > +		xas_set(&xas, curr_index);
> > +
>
> I imagine this will be a performance hit for the call, as each entry now
> needs a traversal from the xarray root. Might be good to callout in commit
> message. But is the right thing to do.

Yeah good point and I did try to find a way not to do that. But it's apparently
(says AI overlords) less bad than an xa_for_each() implementation :P

> Acked-by: Usama Arif <usama.arif@xxxxxxxxx>

Thanks!

--
Cheers, Lorenzo