Re: [PATCH v3 1/9] mm: limit pagecache folio refcount when mapping pages

[Pedro Falcato]

On Thu, Nov 12, 2026 at 03:16:00PM +0100, David Hildenbrand (Arm) wrote:
> On 11/12/26 15:06, Pedro Falcato wrote:
> > On Thu, Nov 12, 2026 at 02:16:33PM +0100, David Hildenbrand (Arm) wrote:
> >>> Mostly LGTM, with some comments:
> >>>
> >>> Reviewed-by: Pedro Falcato <pfalcato@xxxxxxx>
> >>
> >> Thanks for the review, Pedro!
> >>
> >>>
> >>>
> >>> I see you use this pattern often. Would it make sense to have a
> >>> bool folio_get_unless_overflow()?
> >>
> >> I commented somewhere (in v2 I think) that I played with using
> >> folio_get_checked() from #2, but didn't quite like it so I discarded it as
> >>
> >> (a) it cannot be used everywhere here, as I'd need more functions to add
> >> multiple references. next_uptodate_folio() also could use it as I would need yet
> >> another temporary helper for a try_get().
> >>
> >> (b) these changes here are going to get reverted fairly soon once I change the
> >> way how mappings are refcounted (which is, unfortunately, not suitable for
> >> stable, otherwise I wouldn't be throwing in this temporary change in the first
> >> place)
> >>
> >> So everything that is expected to stay in later patches uses the proper helper.
> > 
> > Alright.
> > 
> >>
> >> [...]
> >>
> >>>
> >>> A comment as to what's going on here with VM_MIXEDMAP would be useful. I guess
> >>> the idea is that faulting on a mixedmap later will not necessarily hand you the
> >>> exact same page fault result?
> >>
> >> I can throw in a comment, yes.
> >>
> >> [...]
> >>
> >>>
> >>> In case this function ever returns over limit, we probably want to log this
> >>> in the kernel log. If this function gets the folio you could even print
> >>> the problematic file offset/inode/filesystem/etc. Ratelimited of course.
> >>
> >> You mean, if this function returns "true", right? Is printing always fine?
> >> (e.g., irqs disabled?)
> > 
> > Yes. As far as I know, it's always fine, even under NMI. But definitely fine
> > with IRQs disabled.
> > 
> >>
> >>>
> >>>
> >>> I'm fairly sure this will break the build for a number of non-GPL drivers
> >>> when calling folio_get() with CONFIG_DEBUG_VM=y.
> >>
> >> You mean, breaking OOT drivers, right? I can switch to EXPORT_SYMBOL if that's a
> >> concern.
> > 
> > I had the impression we had some stuff in-tree that isn't marked as GPL. But a quick
> > grep shows that everything is either GPL or dual-licensed.
> > 
> > In any case, yes, this will quite possibly break debug builds for people OOT.
> > I think folio_get() might be a bit too ubiquitous to do this to.
> 
> Thinking again, folio_get() should not be affected, it rightfully only checks
> for a refcount underflow (when we exceed the limit + headroom). So only
> try_get_page() is affected, which OOT very likely doesn't use.

Aha, yes, you're right. I mixed up your folio_get() change. Would it make sense
to VM_WARN_ON() on this as well, or does the <= 0 check cover us already?
> 
> -- 
> Cheers,
> 
> David

-- 
Pedro