Message-ID: <2fae27fe-6e2e-3587-4b68-072118d80cf8@google.com>
Date: Tue, 4 Mar 2025 03:49:32 -0800 (PST)
From: Hugh Dickins <hughd@...gle.com>
To: Zi Yan <ziy@...dia.com>
cc: linux-mm@...ck.org, Andrew Morton <akpm@...ux-foundation.org>, 
    "Kirill A . Shutemov" <kirill.shutemov@...ux.intel.com>, 
    "Matthew Wilcox (Oracle)" <willy@...radead.org>, 
    Ryan Roberts <ryan.roberts@....com>, Hugh Dickins <hughd@...gle.com>, 
    David Hildenbrand <david@...hat.com>, 
    Yang Shi <yang@...amperecomputing.com>, Miaohe Lin <linmiaohe@...wei.com>, 
    Kefeng Wang <wangkefeng.wang@...wei.com>, Yu Zhao <yuzhao@...gle.com>, 
    John Hubbard <jhubbard@...dia.com>, 
    Baolin Wang <baolin.wang@...ux.alibaba.com>, 
    linux-kselftest@...r.kernel.org, linux-kernel@...r.kernel.org, 
    Kairui Song <kasong@...cent.com>, Liu Shixin <liushixin2@...wei.com>
Subject: Re: [PATCH v9 2/8] mm/huge_memory: add two new (not yet used)
 functions for folio_split()

On Wed, 26 Feb 2025, Zi Yan wrote:

> This is a preparation patch, both added functions are not used yet.
> 
> The added __split_unmapped_folio() is able to split a folio with its
> mapping removed in two manners: 1) uniform split (the existing way), and
> 2) buddy allocator like split.
> 
> The added __split_folio_to_order() can split a folio into any lower order.
> For uniform split, __split_unmapped_folio() calls it once to split the
> given folio to the new order.  For buddy allocator split,
> __split_unmapped_folio() calls it (folio_order - new_order) times and each
> time splits the folio containing the given page to one lower order.
> 
> Signed-off-by: Zi Yan <ziy@...dia.com>

Sorry, I'm tired and don't really want to be writing this yet, but the
migrate "hotfix" has tipped my hand, and I need to get this out to you
before more days pass.

I'd been unable to complete even a single iteration of my "kernel builds
on huge tmpfs while swapping to SSD" testing during this current 6.14-rc
mm.git cycle (6.14-rc itself fine) - until the last week, when some
important fixes have come in, so I'm no longer getting I/O errors from
ext4-on-loop0-on-huge-tmpfs, and "Huh VM_FAULT_OOM leaked" warnings: good.

But I still can't get beyond a few iterations, a few minutes: there's
some corruption of user data, which usually manifests as a kernel build
failing because fixdep couldn't find some truncated-on-the-left pathname.

While it definitely bisected to your folio_split() series, it's quite
possible that you're merely exposing an existing bug to wider use.

I've spent the last few days trying to track this down, but still not
succeeded: I'm still getting much the same corruption.  But have been
folding in various fixes as I found them, even though they have not
solved the main problem at all.  I'll return to trying to debug the
corruption "tomorrow".

I think (might be wrong, I'm in a rush) my mods are all to this
"add two new (not yet used) functions for folio_split()" patch:
please merge them in if you agree.

1. From source inspection, it looks like a folio_set_order() was missed.

2. Why is swapcache only checked when folio_test_anon? I can see that
   you've just copied that over from the old __split_huge_page(), but
   it seems wrong to me here and there - I guess a relic from before
   shmem could swap out a huge page.

3. Doing folio_next() inside the for(;;) is unsafe in those configs
   which have to look up zone etc, I got an oops from the "new_folio"
   loop; didn't hit an oops from the "release" loop but fixed that too.

4. While correcting anon versus mapping versus swap_cache, shortened
   the lines by avoiding origin_folio->mapping and &release->page.

Signed-off-by: Hugh Dickins <hughd@...gle.com>
---
 mm/huge_memory.c | 39 ++++++++++++++++++++-------------------
 1 file changed, 20 insertions(+), 19 deletions(-)

diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index 0e45937c0d91..9ce3906672b9 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -3612,7 +3612,9 @@ static void __split_folio_to_order(struct folio *folio, int new_order)
 		folio_xchg_last_cpupid(new_folio, folio_last_cpupid(folio));
 	}
 
-	if (!new_order)
+	if (new_order)
+		folio_set_order(folio, new_order);
+	else
 		ClearPageCompound(&folio->page);
 }
 
@@ -3682,7 +3684,9 @@ static int __split_unmapped_folio(struct folio *folio, int new_order,
 	int ret = 0;
 	bool stop_split = false;
 
-	if (folio_test_anon(folio) && folio_test_swapcache(folio)) {
+	if (folio_test_swapcache(folio)) {
+		VM_BUG_ON(mapping);
+
 		/* a swapcache folio can only be uniformly split to order-0 */
 		if (!uniform_split || new_order != 0)
 			return -EINVAL;
@@ -3750,9 +3754,8 @@ static int __split_unmapped_folio(struct folio *folio, int new_order,
 		 * is new_order, since the folio will be worked on in next
 		 * iteration.
 		 */
-		for (release = folio, next = folio_next(folio);
-		     release != end_folio;
-		     release = next, next = folio_next(next)) {
+		for (release = folio; release != end_folio; release = next) {
+			next = folio_next(release);
 			/*
 			 * for buddy allocator like split, the folio containing
 			 * page will be split next and should not be released,
@@ -3784,32 +3787,31 @@ static int __split_unmapped_folio(struct folio *folio, int new_order,
 			lru_add_page_tail(origin_folio, &release->page,
 						lruvec, list);
 
-			/* Some pages can be beyond EOF: drop them from page cache */
+			/* Some pages can be beyond EOF: drop them from cache */
 			if (release->index >= end) {
-				if (shmem_mapping(origin_folio->mapping))
+				if (shmem_mapping(mapping))
 					nr_dropped += folio_nr_pages(release);
 				else if (folio_test_clear_dirty(release))
 					folio_account_cleaned(release,
-						inode_to_wb(origin_folio->mapping->host));
+						inode_to_wb(mapping->host));
 				__filemap_remove_folio(release, NULL);
 				folio_put(release);
-			} else if (!folio_test_anon(release)) {
-				__xa_store(&origin_folio->mapping->i_pages,
-						release->index, &release->page, 0);
+			} else if (mapping) {
+				__xa_store(&mapping->i_pages,
+						release->index, release, 0);
 			} else if (swap_cache) {
 				__xa_store(&swap_cache->i_pages,
 						swap_cache_index(release->swap),
-						&release->page, 0);
+						release, 0);
 			}
 		}
 	}
 
 	unlock_page_lruvec(lruvec);
 
-	if (folio_test_anon(origin_folio)) {
-		if (folio_test_swapcache(origin_folio))
-			xa_unlock(&swap_cache->i_pages);
-	} else
+	if (swap_cache)
+		xa_unlock(&swap_cache->i_pages);
+	if (mapping)
 		xa_unlock(&mapping->i_pages);
 
 	/* Caller disabled irqs, so they are still disabled here */
@@ -3828,9 +3830,8 @@ static int __split_unmapped_folio(struct folio *folio, int new_order,
 	 * For buddy allocator like split, the first after-split folio is left
 	 * for caller to unlock.
 	 */
-	for (new_folio = origin_folio, next = folio_next(origin_folio);
-	     new_folio != next_folio;
-	     new_folio = next, next = folio_next(next)) {
+	for (new_folio = origin_folio; new_folio != next_folio; new_folio = next) {
+		next = folio_next(new_folio);
 		if (new_folio == page_folio(lock_at))
 			continue;
 
-- 
2.43.0

Powered by blists - more mailing lists