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

[David Hildenbrand <david@redhat.com>]

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.

-- 
Cheers

David / dhildenb