Re: [PATCH 1/2] mm: always call rmap_walk() on locked folios

[David Hildenbrand <david@redhat.com>]

On 02.10.25 08:46, Lokesh Gidra wrote:
> On Thu, Sep 25, 2025 at 4:07 AM David Hildenbrand <david@redhat.com> wrote:
>>
>> On 24.09.25 21:17, Lokesh Gidra wrote:
>>> On Wed, Sep 24, 2025 at 3:00 AM David Hildenbrand <david@redhat.com> wrote:
>>>>
>>>>>>
>>>>>> Is mf_generic_kill_procs()->collect_procs() problematic?
>>>>>>
>>>>>> The dax_lock_folio/dax_unlock_folio part confuses me: I think it only
>>>>>> locks an entry in the page cache but not the actual folio ("Lock the DAX
>>>>>> entry corresponding to a folio")?
>>>>> Yeah. The name dax_lock_folio() gives an impression as if the folio is
>>>>> locked but it isn't :)
>>>>
>>>> Sorry for the late reply, I saw you posted v2 in the meantime.
>>>>
>>>>>
>>>>> IIUC, a dax folio can't have an anon_vma (folio->mapping is actually
>>>>> an address_space instead of anon_vma), right?
>>>>
>>>> We have these weird device-private dax folios that are anonymous and
>>>> should have the anon_vma set up.
>>>>
>>>>> So, I thought it wasn't
>>>>> required to actually lock the folio in this case. Please let me know
>>>>> if you want me to still lock the folio around collect_procs(), or add
>>>>> a comment?
>>>>
>>>> I think we can end up reaching memory_failure_dev_pagemap() with an
>>>> anonymous dax folio.
>>>>
>>>> Not sure if anything would prevent us into calling
>>>>
>>>> mf_generic_kill_procs()->collect_procs()->collect_procs_anon()->folio_lock_anon_vma_read()
>>>>
>>> I must be missing something but dax_lock_folio() dereferences
>>
>> Maybe the code is missing something :)
>>
>>> folio->mapping (to get to host) without checking for
>>> FOLIO_MAPPING_FLAGS presence. If it were an anon folio, wouldn't that
>>> be a problem? And then in collect_procs() we obviously check for
>>> folio_test_anon() on the same folio before calling
>>> collect_procs_anon().
>>
>> Right, if we reach dax_lock_folio() with an anon folio we are probably
>> already messing things up? Not sure if the "!dax_mapping(mapping)" would
>> save us, likely not, because it would be an anon_vma.
>>
>> Hopefully Dan+Alistair have a clue how this works and if it already
>> works as expected with device-private anon folios.
>>
> Hi David,
> 
> Any suggestion how to make progress? Should I upload v3 with
> mf_generic_kill_procs()->collect_procs() in folio-lock critical
> section?

Staring at mf_generic_kill_procs() once again, we have this

switch (pgmap->type) {
case MEMORY_DEVICE_PRIVATE:
case MEMORY_DEVICE_COHERENT:
	rc = -ENXIO;
	goto unlock;
...
}

IIRC, all anon device folios are MEMORY_DEVICE_PRIVATE, so we should 
actually not succeed in this function and just abort.

We still call dax_lock_folio() earlier, which likely doesn't do the 
right thing for anon device folios, but that's an existing issue.

So regarding your patch I think we're good!

-- 
Cheers

David / dhildenb