mm: tweak page cache migration

First of all, I think this little slice of code is a bit under-documented. Perhaps this will help clarify things.

I'm pretty confident the page_count() check in the first patch is right, which is why I removed it outright. The xas_load() check is a bit murkier, so I just left a warning in for it.

From: Dave Hansen <[email protected]>

This is not a bug fix. It was found by inspection, but I believe that it is confusing as it stands.

First, page_ref_freeze() is implemented internally with:

	atomic_cmpxchg(&page->_refcount, expected, 0) == expected

The "cmp" part of cmpxchg is making sure that _refcount==expected which means that there's an implicit check here, equivalent to:

	page_count(page) == expected_count

This appears to have originated in "e286781: mm: speculative page references", which is pretty ancient. This check is also somewhat dangerous to have here because it might lead someone to think that page_ref_freeze() *doesn't* do its own page_count() checking.

Remove the unnecessary check.

---

 b/mm/migrate.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff -puN mm/migrate.c~remove_extra_page_count_check mm/migrate.c
@@ -425,11 +425,12 @@ int migrate_page_move_mapping(struct add
 	newzone = page_zone(newpage);
 
 	xas_lock_irq(&xas);
-	if (page_count(page) != expected_count || xas_load(&xas) != page) {
+	if (xas_load(&xas) != page) {
 		xas_unlock_irq(&xas);
 		return -EAGAIN;
 	}
 
+	/* Freezing will fail if page_count()!=expected_count */
 	if (!page_ref_freeze(page, expected_count)) {
 		xas_unlock_irq(&xas);
 		return -EAGAIN;

_

Make sense to me. Acked-by: Yang Shi <[email protected]>

From: Dave Hansen <[email protected]>

The xas_load() in question also originated in "e286781: mm:
speculative page references" as a radix_tree_deref_slot(), the only one in the tree at the time.

I'm thoroughly confused why it is needed, though. A page's slot in the page cache should be stabilized by lock_page() being held.

So, first of all, add a VM_BUG_ON_ONCE() to make it totally clear that the page is locked.

But, even if the page was truncated, we normally check:

	page_mapping(page) != mapping

to check for truncation. This would seem to imply that we are looking for some kind of state change that can happen to the xarray slot for a page, but without changing page->mapping.

I'm at a loss for that that might be. Stick a WARN_ON_ONCE() in there to see if we ever actually hit this.

---

 b/mm/migrate.c |    8 ++++++++
 1 file changed, 8 insertions(+)

diff -puN mm/migrate.c~remove_extra_xas_load_check mm/migrate.c
@@ -407,6 +407,8 @@ int migrate_page_move_mapping(struct add
 	int dirty;
 	int expected_count = expected_page_refs(mapping, page) + extra_count;
 
+	VM_WARN_ONCE(!PageLocked(page));
+
 	if (!mapping) {
 		/* Anonymous page without mapping */
 		if (page_count(page) != expected_count)

@@ -425,7 +427,13 @@ int migrate_page_move_mapping(struct add
 	newzone = page_zone(newpage);
 
 	xas_lock_irq(&xas);
+	/*
+	 * 'mapping' was established under the page lock, which
+	 * prevents the xarray slot for 'page' from being changed.
+	 * Thus, xas_load() failure here is unexpected.
+	 */
 	if (xas_load(&xas) != page) {
+		WARN_ON_ONCE(1);
 		xas_unlock_irq(&xas);
 		return -EAGAIN;
 	}

_