Re: [PATCH] mm/vmscan: fix hwpoisoned large folio handling in shrink_folio_list

[David Hildenbrand <david@redhat.com>]

On 11.06.25 19:52, Zi Yan wrote:
> On 11 Jun 2025, at 13:34, David Hildenbrand wrote:
> 
>>> So __folio_split() has an implicit rule that:
>>> 1. if the given list is not NULL, the folio cannot be on LRU;
>>> 2. if the given list is NULL, the folio is on LRU.
>>>
>>> And the rule is buried deeply in lru_add_split_folio().
>>>
>>> Should we add some checks in __folio_split()?
>>>
>>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
>>> index d3e66136e41a..8ce2734c9ca0 100644
>>> --- a/mm/huge_memory.c
>>> +++ b/mm/huge_memory.c
>>> @@ -3732,6 +3732,11 @@ static int __folio_split(struct folio *folio, unsigned int new_order,
>>>    	VM_BUG_ON_FOLIO(!folio_test_locked(folio), folio);
>>>    	VM_BUG_ON_FOLIO(!folio_test_large(folio), folio);
>>>
>>> +	if (list && folio_test_lru(folio))
>>> +		return -EINVAL;
>>> +	if (!list && !folio_test_lru(folio))
>>> +		return -EINVAL;
>>> +
>>
>> I guess we currently don't run into that, because whenever a folio is otherwise isolated, there is an additional reference or a page table mapping, so it cannot get split either way (e.g., freezing the refcount fails).
>>
>> So maybe these checks would be too early and they should happen after we froze the refcount?
> 
> But if the caller does the isolation, the additional refcount is OK and
> can_split_folio() will return true. In addition, __folio_split() does not
> change folio LRU state, so these two checks are orthogonal to refcount
> check, right? The placement of them does not matter, but earlier the better
> to avoid unnecessary work. I see these are sanity checks for callers.

In light of the discussion in this thread, if you have someone that 
takes the folio off the LRU concurrently, I think we could still run 
into a race here. Because that could happen just after we passed the 
test in __folio_split().

That's why I think the test would have to happen when there are no such 
races possible anymore.

But the real question is if it is okay to remove the folio from the LRU 
as done in the patch discussed here ...

-- 
Cheers,

David / dhildenb