diff mbox series

mm/khugepaged: fix regression in collapse_file()

Message ID df5819aa-c79-6b10-4ce1-a45af118c0f3@google.com
State New
Headers
Series mm/khugepaged: fix regression in collapse_file() |

Commit Message

Hugh Dickins June 25, 2023, 7:06 p.m. UTC 
  There is no xas_pause(&xas) in collapse_file()'s main loop, at the points
where it does xas_unlock_irq(&xas) and then continues.

That would explain why, once two weeks ago and twice yesterday, I have
hit the VM_BUG_ON_PAGE(page != xas_load(&xas), page) since "mm/khugepaged:
fix iteration in collapse_file" removed the xas_set(&xas, index) just
before it: xas.xa_node could be left pointing to a stale node, if there
was concurrent activity on the file which transformed its xarray.

I tried inserting xas_pause()s, but then even bootup crashed on that
VM_BUG_ON_PAGE(): there appears to be a subtle "nextness" implicit in
xas_pause().

xas_next() and xas_pause() are good for use in simple loops, but not in
this one: xas_set() worked well until now, so use xas_set(&xas, index)
explicitly at the head of the loop; and change that VM_BUG_ON_PAGE() not
to need its own xas_set(), and not to interfere with the xa_state (which
would probably stop the crashes from xas_pause(), but I trust that less).

Link: https://lore.kernel.org/linux-mm/f18e4b64-3f88-a8ab-56cc-d1f5f9c58d4@google.com/
Fixes: c8a8f3b4a95a ("mm/khugepaged: fix iteration in collapse_file")
Signed-off-by: Hugh Dickins <hughd@google.com>
---
Linus, I'm rushing this directly to you, but not really expecting you
to put it in at this stage, unless you're very comfortable with it,
or perhaps it catches Matthew's eye and gets a quick Ack from him.

The commit being fixed only got in after -rc7, after being held up by my
initial report of this crash; but I had to rescind that when I couldn't
reproduce it at all. Then yesterday morning it hit again on two machines,
and reading XArray Doc reminded me of xas_pause() - seems obvious now.
Patch ran for 14 hours overnight on those two machines without a problem.

 mm/khugepaged.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

Comments

Linus Torvalds June 25, 2023, 7:42 p.m. UTC | #1 
On Sun, 25 Jun 2023 at 12:06, Hugh Dickins <hughd@google.com> wrote:
>
> Linus, I'm rushing this directly to you, but not really expecting you
> to put it in at this stage, unless you're very comfortable with it,
> or perhaps it catches Matthew's eye and gets a quick Ack from him.

Yeah, the fix and explanation sound fine to me, but the maple tree
code confuses me enough and has been subtle enough (as exemplified by
this bug and many others) that there's no way I'll apply this just
before the final 6.4 without some "Ack, that's obviously correct" from
Willy or Liam or somebody.

Willy?

                    Linus
diff mbox series

Patch

diff --git a/mm/khugepaged.c b/mm/khugepaged.c
index 2d0d58fb4e7f..47b59f2843f6 100644
--- a/mm/khugepaged.c
+++ b/mm/khugepaged.c
@@ -1918,9 +1918,9 @@  static int collapse_file(struct mm_struct *mm, unsigned long addr,
 		}
 	} while (1);
 
-	xas_set(&xas, start);
 	for (index = start; index < end; index++) {
-		page = xas_next(&xas);
+		xas_set(&xas, index);
+		page = xas_load(&xas);
 
 		VM_BUG_ON(index != xas.xa_index);
 		if (is_shmem) {
@@ -1935,7 +1935,6 @@  static int collapse_file(struct mm_struct *mm, unsigned long addr,
 						result = SCAN_TRUNCATED;
 						goto xa_locked;
 					}
-					xas_set(&xas, index + 1);
 				}
 				if (!shmem_charge(mapping->host, 1)) {
 					result = SCAN_FAIL;
@@ -2071,7 +2070,7 @@  static int collapse_file(struct mm_struct *mm, unsigned long addr,
 
 		xas_lock_irq(&xas);
 
-		VM_BUG_ON_PAGE(page != xas_load(&xas), page);
+		VM_BUG_ON_PAGE(page != xa_load(xas.xa, index), page);
 
 		/*
 		 * We control three references to the page: