Re: [PATCH] mm/huge_memory: transfer the pmd dirty bit to the folio on zap

[Kiryl Shutsemau <kas@kernel.org>]

On Thu, Aug 20, 2026 at 01:12:18PM +0100, Pedro Falcato wrote:
> +CC willy
> 
> On Thu, Aug 20, 2026 at 02:13:37PM +0800, Lance Yang wrote:
> > 
> > On Wed, Aug 19, 2026 at 05:35:41PM +0100, Pedro Falcato wrote:
> > >On Wed, Aug 19, 2026 at 03:31:40PM +0100, Kiryl Shutsemau wrote:
> > >> On Wed, Aug 19, 2026 at 03:12:22AM -0700, Usama Arif wrote:
> > >> > zap_huge_pmd_folio() propagates the pmd young bit to the folio for the
> > >> > file case, but not the dirty bit.  The pte path does propagate it, in
> > >> > zap_present_folio_ptes() and so does the pmd split path, in
> > >> > __split_huge_pmd_locked().
> > >> > 
> > >> > For most file mappings the omission is harmless, because writing to a
> > >> > shared file mapping goes through page_mkwrite(), which dirties the
> > >> > folio.  tmpfs is different: it has no page_mkwrite(), and
> > >> > vma_wants_writenotify() is false for it, so a *read* fault on a
> > >> > MAP_SHARED tmpfs mapping installs a writable pmd via do_read_fault().
> > >> > do_read_fault() does not call fault_dirty_shared_page(), so subsequent
> > >> > stores through that mapping set only the hardware dirty bit in the pmd
> > >> > and never call folio_mark_dirty().
> > >> >
> > >> > A shmem folio allocated by a fault
> > >> > is marked uptodate but not dirty (see the clear: block in
> > >> > shmem_get_folio_gfp()), so PG_dirty is never set at all.
> > >> > 
> > >> > Unmapping such a folio - munmap(), or exit_mmap() when the process dies
> > >> > - then loses the only record that it was written, because zap_huge_pmd()
> > >> > drops the pmd without transferring the dirty bit.  Reclaim afterwards
> > >> > sees a clean shmem folio: the whole swap-out block in
> > >> > shrink_folio_list() is inside "if (folio_test_dirty(folio))", so
> > >> > pageout() is skipped and the folio falls into __remove_mapping().
> > >> > There, folio_is_file_lru() is false for a swapbacked folio, so no shadow
> > >> > entry is created and __filemap_remove_folio(folio, NULL) simply empties
> > >> > the i_pages slot.  The data is freed without ever being written to swap,
> > >> > and the next fault on that index returns a freshly zeroed folio.
> > >> > 
> > >> > This is silent data loss for any process that keeps state in a
> > >> > MAP_SHARED tmpfs segment across an unmap - for example a cache handed
> > >> > from one process generation to the next through /dev/shm.  It requires
> > >> > the folio to be PMD-mapped, so it only shows up once shmem THP is
> > >> > enabled (which is what we did in Meta fleet and started noticing crashes);
> > >> > with THP off the pte path transfers the dirty bit correctly.
> > >> > It also only becomes visible when swap is enabled, because with no swap
> > >> > device shmem folios (which are on the anon LRU) are not scanned by
> > >> > reclaim at all, so the clean folio is never dropped.
> > >> > 
> > >> > Reproduced on x86_64 with a tmpfs mounted huge=within_size: read-fault a
> > >> > 2MB-backed region, write a known pattern through the resulting mapping,
> > >> > munmap, force reclaim of the cgroup, then re-map and read back.  Without
> > >> > this patch the region reads back as zeros and vmstat shows zswpout 0 -
> > >> > the data was discarded rather than swapped.  With this patch the region
> > >> > reads back correctly and the pages are swapped out as expected.  With
> > >> > huge=never, or when the first touch is a write, the test passes either
> > >> > way.
> > >> 
> > >> +Hugh.
> > >> 
> > >> Oopsie.
> > >> 
> > >> I'm confused why it took a decade to discover the bug...
> > >> Maybe read ahead of write for shmem is too rare, I donno.
> > >> 
> > >> > 
> > >> > Fixes: 800d8c63b2e9 ("shmem: add huge pages support")
> > >> 
> > >> This would be more precise: b5072380eb61 ("thp: support file pages in zap_huge_pmd()")
> > >> 
> > >> Reviewed-by: Kiryl Shutsemau <kas@kernel.org>
> > >> 
> > >> > Cc: <stable@vger.kernel.org>
> > >> > Signed-off-by: Usama Arif <usama.arif@linux.dev>
> > >> > ---
> > >> >  mm/huge_memory.c | 2 ++
> > >> >  1 file changed, 2 insertions(+)
> > >> > 
> > >> > diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> > >> > index ced400f72d43a..afbb5974bd225 100644
> > >> > --- a/mm/huge_memory.c
> > >> > +++ b/mm/huge_memory.c
> > >> > @@ -2449,6 +2449,8 @@ static void zap_huge_pmd_folio(struct mm_struct *mm, struct vm_area_struct *vma,
> > >> >  		add_mm_counter(mm, mm_counter_file(folio),
> > >> >  			       -HPAGE_PMD_NR);
> > >> >  
> > >> > +		if (is_present && pmd_dirty(pmdval))
> > >> > +			folio_mark_dirty(folio);
> > >> 
> > >> Unrelated to your patch, but noticed while looking at it: we drop the rmap
> > >> here under the pmd lock, while the TLB flush is deferred to
> > >> tlb_finish_mmu(). The pte path handles this with
> > >> tlb_delay_rmap()/force_flush (5df397dec7c4), but there's no pmd equivalent:
> > >> tlb_flush_rmap_batch() only knows folio_remove_rmap_ptes(), and
> > >> zap_huge_pmd() uses tlb_remove_page_size(), which takes no delay_rmap.
> > >> 
> > >> Doesn't matter for shmem, but xfs & friends do get PMD-order folios, and
> > >> do_set_pmd() makes the pmd dirty+writable once page_mkwrite() has run. So
> > >> folio_mkclean() can clean the folio while another CPU still stores through a
> > >> stale TLB entry -- silently lost write, no PG_dirty left behind.
> > >
> > >Where do you see page_mkwrite being called in the same path as do_set_pmd()?
> > >Per my understanding of the code, this Should Not Happen, and it really Should
> > >Not Happen for many, many reasons (write amplification being the main one).
> > 
> > Hmm.. that happens on an initial shared write fault. For non-DAX XFS, the
> > path starts with an empty PMD.
> > 
> > TL;DR
> > 
> > With an empty PMD and PMD-order THP allowed, __handle_mm_fault() first
> > tries create_huge_pmd(). VM_FAULT_FALLBACK sends the fault to
> > handle_pte_fault():
> > 
> > static vm_fault_t __handle_mm_fault(struct vm_area_struct *vma,
> > 		unsigned long address, unsigned int flags)
> > {
> > 	...
> > 	if (pmd_none(*vmf.pmd) &&
> > 	    thp_vma_allowable_order(vma, vm_flags, TVA_PAGEFAULT, PMD_ORDER)) {
> > 		ret = create_huge_pmd(&vmf);
> > 		if (ret & VM_FAULT_FALLBACK)
> > 			goto fallback;
> > 		else
> > 			return ret;
> > 	}
> > 	...
> > fallback:
> > 	return handle_pte_fault(&vmf);
> > }
> > 
> > create_huge_pmd() dispatches to the filesystem's huge_fault callback:
> > 
> > static inline vm_fault_t create_huge_pmd(struct vm_fault *vmf)
> > {
> > 	struct vm_area_struct *vma = vmf->vma;
> > 	...
> > 	if (vma->vm_ops->huge_fault)
> > 		return vma->vm_ops->huge_fault(vmf, PMD_ORDER);
> > 	return VM_FAULT_FALLBACK;
> > }
> > 
> > For non-DAX XFS, that callback returns VM_FAULT_FALLBACK:
> > 
> > static vm_fault_t
> > xfs_filemap_huge_fault(
> > 	struct vm_fault		*vmf,
> > 	unsigned int		order)
> > {
> > 	if (!IS_DAX(file_inode(vmf->vma->vm_file)))
> > 		return VM_FAULT_FALLBACK;
> > 	...
> > }
> > 
> > XFS installs the huge-fault, regular-fault, and page_mkwrite callbacks
> > in the same vm_ops:
> > 
> > static const struct vm_operations_struct xfs_file_vm_ops = {
> > 	.fault		= xfs_filemap_fault,
> > 	.huge_fault	= xfs_filemap_huge_fault,
> > 	...
> > 	.page_mkwrite	= xfs_filemap_page_mkwrite,
> > 	...
> > };
> > 
> > On the fallback path, handle_pte_fault() leaves an empty PMD without a
> > PTE and calls do_pte_missing():
> > 
> > static vm_fault_t handle_pte_fault(struct vm_fault *vmf)
> > {
> > 	...
> > 	if (unlikely(pmd_none(*vmf->pmd))) {
> > 		/*
> > 		 * Leave __pte_alloc() until later: because vm_ops->fault may
> > 		 * want to allocate huge page, and if we expose page table
> > 		 * for an instant, it will be difficult to retract from
> > 		 * concurrent faults and from rmap lookups.
> > 		 */
> > 		vmf->pte = NULL;
> > 		vmf->flags &= ~FAULT_FLAG_ORIG_PTE_VALID;
> > 	...
> > 	}
> > 
> > 	if (!vmf->pte)
> > 		return do_pte_missing(vmf);
> > 	...
> > }
> > 
> > For a file VMA, do_pte_missing() calls do_fault():
> > 
> > static vm_fault_t do_pte_missing(struct vm_fault *vmf)
> > {
> > 	if (vma_is_anonymous(vmf->vma))
> > 		return do_anonymous_page(vmf);
> > 	else
> > 		return do_fault(vmf);
> > }
> > 
> > do_fault() sends FAULT_FLAG_WRITE + VM_SHARED to do_shared_fault():
> > 
> > static vm_fault_t do_fault(struct vm_fault *vmf)
> > {
> > 	struct vm_area_struct *vma = vmf->vma;
> > 	...
> > 	if (!vma->vm_ops->fault) {
> > 		...
> > 	} else if (!(vmf->flags & FAULT_FLAG_WRITE))
> > 		ret = do_read_fault(vmf);
> > 	else if (!(vma->vm_flags & VM_SHARED))
> > 		ret = do_cow_fault(vmf);
> > 	else
> > 		ret = do_shared_fault(vmf);
> > 	...
> > }
> > 
> > do_shared_fault() first calls __do_fault():
> > 
> > static vm_fault_t do_shared_fault(struct vm_fault *vmf)
> > {
> > 	struct vm_area_struct *vma = vmf->vma;
> > 	vm_fault_t ret, tmp;
> > 	struct folio *folio;
> > 	...
> > 	ret = __do_fault(vmf);
> > 	...
> > }
> > 
> > __do_fault() invokes the regular fault callback:
> > 
> > static vm_fault_t __do_fault(struct vm_fault *vmf)
> > {
> > 	struct vm_area_struct *vma = vmf->vma;
> > 	struct folio *folio;
> > 	vm_fault_t ret;
> > 	...
> > 	ret = vma->vm_ops->fault(vmf);
> > 	...
> > 	return ret;
> > }
> > 
> > For non-DAX XFS, xfs_filemap_fault() reaches filemap_fault():
> > 
> > static vm_fault_t
> > xfs_filemap_fault(
> > 	struct vm_fault		*vmf)
> > {
> > 	struct inode		*inode = file_inode(vmf->vma->vm_file);
> > 	...
> > 	return filemap_fault(vmf);
> > }
> > 
> > Once that returns the folio, do_shared_fault() calls do_page_mkwrite()
> > and then finish_fault():
> > 
> > static vm_fault_t do_shared_fault(struct vm_fault *vmf)
> > {
> > 	struct vm_area_struct *vma = vmf->vma;
> > 	vm_fault_t ret, tmp;
> > 	struct folio *folio;
> > 	...
> > 	folio = page_folio(vmf->page);
> > 	...
> > 	if (vma->vm_ops->page_mkwrite) {
> > 		folio_unlock(folio);
> > 		tmp = do_page_mkwrite(vmf, folio);
> > 		...
> > 	}
> > 
> > 	ret |= finish_fault(vmf);
> > 	...
> > }
> > 
> > do_page_mkwrite() calls the XFS callback installed above and restores
> > the original fault flags:
> > 
> > static vm_fault_t do_page_mkwrite(struct vm_fault *vmf, struct folio *folio)
> > {
> > 	vm_fault_t ret;
> > 	unsigned int old_flags = vmf->flags;
> > 
> > 	vmf->flags = FAULT_FLAG_WRITE|FAULT_FLAG_MKWRITE;
> > 	...
> > 	ret = vmf->vma->vm_ops->page_mkwrite(vmf);
> > 	/* Restore original flags so that caller is not surprised */
> > 	vmf->flags = old_flags;
> > 	...
> > }
> > 
> > So finish_fault() still sees FAULT_FLAG_WRITE. With an empty PMD, no
> > fallback requirement, and a PMD-mappable folio, it tries do_set_pmd():
> > 
> > vm_fault_t finish_fault(struct vm_fault *vmf)
> > {
> > 	...
> > 	if (pmd_none(*vmf->pmd)) {
> > 		if (!needs_fallback && folio_test_pmd_mappable(folio)) {
> > 			ret = do_set_pmd(vmf, folio, page);
> > 			if (ret != VM_FAULT_FALLBACK)
> > 				return ret;
> > 		}
> > 		...
> > 	}
> > 	...
> > }
> > 
> > After its checks pass, do_set_pmd() takes FAULT_FLAG_WRITE from vmf and
> > installs a dirty+writable PMD:
> > 
> > vm_fault_t do_set_pmd(struct vm_fault *vmf, struct folio *folio, struct page *page)
> > {
> > 	struct vm_area_struct *vma = vmf->vma;
> > 	bool write = vmf->flags & FAULT_FLAG_WRITE;
> > 	unsigned long haddr = vmf->address & HPAGE_PMD_MASK;
> > 	pmd_t entry;
> > 	...
> > 	entry = folio_mk_pmd(folio, vma->vm_page_prot);
> > 	if (write)
> > 		entry = maybe_pmd_mkwrite(pmd_mkdirty(entry), vma);
> > 	...
> > 	set_pmd_at(vma->vm_mm, haddr, vmf->pmd, entry);
> > 	...
> > }
> > 
> > maybe_pmd_mkwrite() sets write permission for VM_WRITE:
> > 
> > pmd_t maybe_pmd_mkwrite(pmd_t pmd, struct vm_area_struct *vma)
> > {
> > 	if (likely(vma->vm_flags & VM_WRITE))
> > 		pmd = pmd_mkwrite(pmd, vma);
> > 	return pmd;
> > }
> 
> Thanks, this makes sense!
> 
> > 
> > >Namely, see the comment in wp_huge_pmd():
> > >	/* COW or write-notify handled on pte level: split pmd. */
> > >
> > >if file huge pages get mapped writable, that's a bug.
> > 
> > That comment is about a different path. __handle_mm_fault() calls
> > wp_huge_pmd() only when a write/unshare fault hits an existing PMD THP
> > which is not writable:
> 
> No. That is simply a bug. There's little reason you wouldn't try do un-WP
> a huge PMD if the idea would be to do PMD granularity for write notifications.
> It isn't, naturally, because that results in horrible write amplification.

Write notification is already folio-granular. Installing PTE instead of
PMD changes nothing.

> The fix IMO is to make it so write faults on shared mappings with page_mkwrite
> never create a PMD.

Why? Write batching from large folios is a win.

> I don't think it makes sense to add rmap flushing hacks
> for PMDs, when the common case (read + write) instantly and purposefully
> breaks down to the PTE level (such that you really aren't supposed to get
> the above; you'll notice that as soon as the folio gets cleaned, it will
> get broken by the next write fault, period).

If you consider delayed rmap a hack (I don't), it has to fixed on PTE
level too.

> See the attached patch. I know willy has been working on related stuff, so
> perhaps he might want to pick it up.

As I said the patch doesn't do what you expect it to do. PG_dirty is on
folio and we writeback folios, not PTEs.

Also, needs_fallback is not just "no PMD": finish_fault() then forces
nr_pages = 1, so it is 512 faults and 512 ->page_mkwrite calls per 2M folio
instead of one.

-- 
  Kiryl Shutsemau / Kirill A. Shutemov