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

[Oscar Salvador <osalvador@suse.de>]

On Fri, Jun 13, 2025 at 03:56:15PM +0200, David Hildenbrand wrote:
> On 12.06.25 15:46, Oscar Salvador wrote:
> > -	/* hugetlb_wp() requires page locks of pte_page(vmf.orig_pte) */
> > +	/*
> > +	 * We need to lock the folio before calling hugetlb_wp().
> > +	 * Either the folio is in the pagecache and we need to copy it over
> > +	 * to another file, so it must remain stable throughout the operation,
> 
> But as discussed, why is that the case? We don't need that for ordinary
> pages, and existing folio mappings can already concurrently modify the page?

Normal faulting path takes the lock when we fault in a file read-only or to
to map it privately.
That is done via __do_fault or cow_fault, in __do_fault()->vma->vm_ops_>fault().
E.g. filemap_fault() will locate the page and lock it.
And it will hold it during the entire operation, note that we unlock it
after we have called finish_fault().

The page can't go away because filemap_fault also gets a reference on
it, so I guess it's to hold it stable.

Now, when we get a wp fault for an already private mapping, via do_wp_page(), we
only take the lock to check whether we can re-use the page for us.
We don't hold it during the copy.

The only reason we hold it in hugetlb for the entire hugetlb_wp() is because making
this distinction on the lock's timespan depending on what we are dealing with,
would be tricky with the current code.
We would need to do some dancing of release-and-take-it depending on
what we want to do, and frankly it seems overcomplicated at this point.

I plan to do some more work in that area, and potentially glue it with
the generic faulting path (I have some ideas but I need some other works
to land first)

 

-- 
Oscar Salvador
SUSE Labs