Re: [PATCH v4 00/16] Overhaul multi-page lookups for THP

[Hugh Dickins <hughd@google.com>]

On Thu, 12 Nov 2020, Matthew Wilcox (Oracle) wrote:

> This THP prep patchset changes several page cache iteration APIs to only
> return head pages.
> 
>  - It's only possible to tag head pages in the page cache, so only
>    return head pages, not all their subpages.
>  - Factor a lot of common code out of the various batch lookup routines
>  - Add mapping_seek_hole_data()
>  - Unify find_get_entries() and pagevec_lookup_entries()
>  - Make find_get_entries only return head pages, like find_get_entry().
> 
> These are only loosely connected, but they seem to make sense together
> as a series.
> 
> v4:
>  - Add FGP_ENTRY, remove find_get_entry and find_lock_entry
>  - Rename xas_find_get_entry to find_get_entry
>  - Add "Optimise get_shadow_from_swap_cache"
>  - Move "iomap: Use mapping_seek_hole_data" to this patch series
>  - Rebase against next-20201112

I hope next-20201112 had nothing vital for this series, I applied
it to v5.10-rc3, and have been busy testing huge tmpfs on that.

Several fixes necessary.  It was only a couple of hours ago that I
finally saw what was wrong in shmem_undo_range(), I'm tired now and
sending these off somewhat hastily, may have got something wrong, but
best to get them to you soonest, to rework and fold in as you see fit.
Please allow me to gather them here below, instead of replying to
individual patches.

Fix to [PATCH v4 06/16] mm/filemap: Add helper for finding pages.
I hit that VM_BUG_ON_PAGE(!thp_contains) when swapping, it is not
safe without page lock, during the interval when shmem is moving a
page between page cache and swap cache.  It could be tightened by
passing in a new FGP to distinguish whether searching page or swap
cache, but I think never tight enough in the swap case - because there
is no rule for persisting page->private as there is for page->index.
The best I could do is:

--- 5103w/mm/filemap.c	2020-11-12 15:46:23.191275470 -0800
+++ 5103wh/mm/filemap.c	2020-11-16 01:09:35.427677277 -0800
@@ -1858,7 +1858,20 @@ retry:
 		put_page(page);
 		goto reset;
 	}
-	VM_BUG_ON_PAGE(!thp_contains(page, xas->xa_index), page);
+
+#ifdef CONFIG_DEBUG_VM
+	/*
+	 * thp_contains() is unreliable when shmem is swizzling between page
+	 * and swap cache, when both PageSwapCache and page->mapping are set.
+	 */
+	if (!thp_contains(page, xas->xa_index)) {
+		VM_BUG_ON_PAGE(!PageSwapBacked(page), page);
+		if (trylock_page(page)) {
+			VM_BUG_ON_PAGE(!thp_contains(page, xas->xa_index), page);
+			unlock_page(page);
+		}
+	}
+#endif
 
 	return page;
 reset:

Fix to [PATCH v4 07/16] mm/filemap: Add mapping_seek_hole_data.
Crashed on a swap entry 0x2ff09, fairly obvious...

--- 5103w/mm/filemap.c	2020-11-12 15:46:23.191275470 -0800
+++ 5103wh/mm/filemap.c	2020-11-16 01:09:35.427677277 -0800
@@ -2632,7 +2632,8 @@ loff_t mapping_seek_hole_data(struct add
 				seek_data);
 		if (start < pos)
 			goto unlock;
-		put_page(page);
+		if (!xa_is_value(page))
+			put_page(page);
 	}
 	rcu_read_unlock();
 
Fix to [PATCH v4 15/16] mm/truncate,shmem: Handle truncates that split THPs.
One machine ran fine, swapping and building in ext4 on loop0 on huge tmpfs;
one machine got occasional pages of zeros in its .os; one machine couldn't
get started because of ext4_find_dest_de errors on the newly mkfs'ed fs.
The partial_end case was decided by PAGE_SIZE, when there might be a THP
there.  The below patch has run well (for not very long), but I could
easily have got it slightly wrong, off-by-one or whatever; and I have
not looked into the similar code in mm/truncate.c, maybe that will need
a similar fix or maybe not.

--- 5103w/mm/shmem.c	2020-11-12 15:46:21.075254036 -0800
+++ 5103wh/mm/shmem.c	2020-11-16 01:09:35.431677308 -0800
@@ -874,7 +874,7 @@ static void shmem_undo_range(struct inod
 	long nr_swaps_freed = 0;
 	pgoff_t index;
 	int i;
-	bool partial_end;
+	bool same_page;
 
 	if (lend == -1)
 		end = -1;	/* unsigned, so actually very big */
@@ -907,16 +907,12 @@ static void shmem_undo_range(struct inod
 		index++;
 	}
 
-	partial_end = ((lend + 1) % PAGE_SIZE) > 0;
+	same_page = (lstart >> PAGE_SHIFT) == end;
 	page = NULL;
 	shmem_getpage(inode, lstart >> PAGE_SHIFT, &page, SGP_READ);
 	if (page) {
-		bool same_page;
-
 		page = thp_head(page);
 		same_page = lend < page_offset(page) + thp_size(page);
-		if (same_page)
-			partial_end = false;
 		set_page_dirty(page);
 		if (!truncate_inode_partial_page(page, lstart, lend)) {
 			start = page->index + thp_nr_pages(page);
@@ -928,7 +924,7 @@ static void shmem_undo_range(struct inod
 		page = NULL;
 	}
 
-	if (partial_end)
+	if (!same_page)
 		shmem_getpage(inode, end, &page, SGP_READ);
 	if (page) {
 		page = thp_head(page);

Fix to [PATCH v4 15/16] mm/truncate,shmem: Handle truncates that split THPs.
xfstests generic/012 on huge tmpfs hit this every time (when checking
xfs_io commands available: later decides "not run" because no "fiemap").
I grabbed this line unthinkingly from one of your later series, it fixes
the crash; but once I actually thought about it when trying to track down
weirder behaviours, realize that the kmap_atomic() and flush_dcache_page()
in zero_user_segments() are not prepared for a THP - so highmem and
flush_dcache_page architectures will be disappointed. If I searched
through your other series, I might find the complete fix; or perhaps
it's already there in linux-next, I haven't looked.

--- 5103w/include/linux/highmem.h	2020-10-11 14:15:50.000000000 -0700
+++ 5103wh/include/linux/highmem.h	2020-11-16 01:09:35.423677246 -0800
@@ -290,7 +290,7 @@ static inline void zero_user_segments(st
 {
 	void *kaddr = kmap_atomic(page);
 
-	BUG_ON(end1 > PAGE_SIZE || end2 > PAGE_SIZE);
+	BUG_ON(end1 > page_size(page) || end2 > page_size(page));
 
 	if (end1 > start1)
 		memset(kaddr + start1, 0, end1 - start1);

I also had noise from the WARN_ON(page_to_index(page) != index)
in invalidate_inode_pages2_range(): but that's my problem, since
for testing I add a dummy shmem_direct_IO() (return 0): for that
I've now added a shmem_mapping() check at the head of pages2_range().

That's all for now: I'll fire off more overnight testing.

Hugh