Re: [PATCH 2/7] mm/gup: check ref_count instead of lru before migration

[Hugh Dickins <hughd@google.com>]

On Mon, 8 Sep 2025, David Hildenbrand wrote:
> On 08.09.25 12:40, Hugh Dickins wrote:
> > On Mon, 1 Sep 2025, David Hildenbrand wrote:
> >> On 31.08.25 11:05, Hugh Dickins wrote:
> >>> diff --git a/mm/gup.c b/mm/gup.c
> >>> index adffe663594d..82aec6443c0a 100644
> >>> --- a/mm/gup.c
> >>> +++ b/mm/gup.c
> >>> @@ -2307,7 +2307,8 @@ static unsigned long
> >>> collect_longterm_unpinnable_folios(
> >>>      	continue;
> >>>      }
> >>>    -		if (!folio_test_lru(folio) && drain_allow) {
> >>> +		if (drain_allow && folio_ref_count(folio) !=
> >>> +				   folio_expected_ref_count(folio) + 1) {
> >>>       lru_add_drain_all();
> >>>       drain_allow = false;
> >>>      }
> >>
> >> In general, to the fix idea
> >>
> >>  Acked-by: David Hildenbrand <david@redhat.com>
> > 
> > Thanks, but I'd better not assume that in v2, even though code the same.
> > Will depend on how you feel about added paragraph in v2 commit message.
> > 
> >>
> >> But as raised in reply to patch #1, we have to be a bit careful about
> >> including private_2 in folio_expected_ref_count() at this point.
> >>
> >> If we cannot include it in folio_expected_ref_count(), it's all going to be
> >> a
> >> mess until PG_private_2 is removed for good.
> >>
> >> So that part still needs to be figured out.
> > 
> > Here's that added paragraph:
> > 
> > Note on PG_private_2: ceph and nfs are still using the deprecated
> > PG_private_2 flag, with the aid of netfs and filemap support functions.
> > Although it is consistently matched by an increment of folio ref_count,
> > folio_expected_ref_count() intentionally does not recognize it, and ceph
> > folio migration currently depends on that for PG_private_2 folios to be
> > rejected.  New references to the deprecated flag are discouraged, so do
> > not add it into the collect_longterm_unpinnable_folios() calculation:
> > but longterm pinning of transiently PG_private_2 ceph and nfs folios
> > (an uncommon case) may invoke a redundant lru_add_drain_all(). 
> 
> Would we also loop forever trying to migrate these folios if they reside on
> ZONE_MOVABLE? I would assume that is already the case, that migration will
> always fail due to the raised reference.

Loop around forever?  That would be unfortunate (but I presume killable).
But when I looked, it appeared that any failure of migrate_pages() there
gets reported as -ENOMEM, which would end up as an OOM?  But you know
mm/gup.c very much better than I do.

If it does loop around, it's not so bad in the PG_private_2 case, because
that's (nowadays always) a transient flag, much more like PG_writeback
than PG_private.

But whatever, yes, the move from testing lru to checking ref_count
makes no difference to that: the failure occurs in migration either way.

Hugh