Re: [PATCH 2/5] mm,hugetlb: Document the reason to lock the folio in the faulting path

[David Hildenbrand <david@redhat.com>]

On 16.06.25 16:10, Oscar Salvador wrote:
> On Mon, Jun 16, 2025 at 11:22:43AM +0200, David Hildenbrand wrote:
>   
>> hugetlb_fault->hugetlb_no_page->hugetlb_wp
>>
>> already *mapped* the pagecache page into the page table.
>>
>> See
>> 	if (anon_rmap)
>> 		hugetlb_add_new_anon_rmap(folio, vma, vmf->address);
>> 	else
>> 		hugetlb_add_file_rmap(folio);
>>
>> So at that point it would be "stable" unless I am missing something?
>>
>> So once we are in hugetlb_wp(), that path much rather corresponds to
>> do_wp_page()->wp_page_copy.
> 
> Yes, that's right.
> That's something I've been thinking over the weekend.
> 
> E.g: do_cow_fault, first copies the page from the pagecache to a new one
> and __then__ maps the that page into the page tables.
> While in hugetlb_no_page->hugetlb_wp, the workflow is a bit different.
> 
> We first map it and then we copy it if we need to.
> 
> What do you mean by stable?

The same "stable" you used in the doc, that I complained about ;)

> In the generic faulting path, we're not worried about the page going away
> because we hold a reference, so I guess the lock must be to keep content stable?

What you want to avoid is IIRC, is someone doing a truncation/reclaim on 
the folio while you are mapping it.

Take a look at truncate_inode_pages_range() where we do a folio_lock() 
around truncate_inode_folio().

In other words, while you hold the folio lock (and verified that the 
folio was not truncated yet: for example, that folio->mapping is still 
set), you know that it cannot get truncated concurrently -- without 
holding other expensive locks.

Observe how truncate_cleanup_folio() calls

	if (folio_mapped(folio))
		unmap_mapping_folio(folio);

To remove all page table mappings.

So while holding the folio lock, new page table mappings are not 
expected to appear (IIRC).

> 
> I mean, yes, after we have mapped the page privately into the pagetables,
> we don't have business about content-integrity anymore, so given this rule, yes,
> I guess hugetlb_wp() wouldn't need the lock (for !anonymous) because we already
> have mapped it privately at that point.

That's my understanding. And while holding the PTL it cannot get 
unmapped. Whenever you temporarily drop the PTL, you have to do a 
pte_same() check to make sure concurrent truncation didn't happen.

So far my understanding at least of common filemap code.

> 
> But there's something I don't fully understand and makes me feel uneasy.
> If the lock in the generic faultin path is to keep content stable till we
> have mapped it privately, wouldn't be more correct to also hold it
> during the copy in hugetlb_wp, to kinda emulate that?
As long there us a page table mapping, it cannot get truncated. So if 
you find a PTE under PTL that maps that folio, truncation could not have 
happened.

-- 
Cheers,

David / dhildenb