[patch] mm: fix lockless pagecache reordering bug (was Re: BUG: soft lockup - is this XFS problem?)

Nick Piggin
2009-01-05T16:41:50Z

Hi,

This patch should be applied to 2.6.29 and 27/28 stable kernels, please.
--

Peter Klotz and Roman Kononov both reported a bug where in XFS workloads where
they were seeing softlockups in find_get_pages
(http://oss.sgi.com/bugzilla/show_bug.cgi?id=805).

Basically it would go into an "infinite" loop, although it would sometimes be
able to break out of the loop depending on the phase of the moon.

This turns out to be a bug in the lockless pagecache patch. There is a missing
compiler barrier in the "increment reference count unless it was zero" failure
case of the lockless pagecache protocol in the gang lookup functions.

This would cause the compiler to use a cached value of struct page pointer to
retry the operation with, rather than reload it. So the page might have been
removed from pagecache and freed (refcount==0) but the lookup would not correctly
notice the page is no longer in pagecache, and keep attempting to increment the
refcount and failing, until the page gets reallocated for something else. This
isn't a data corruption because the condition will be properly handled if the
page does get reallocated. However it can result in a lockup.

Add a the required compiler barrier and comment to fix this.

Assembly snippet from find_get_pages, before:
.L220:
movq (%rbx), %rax #* ivtmp.1162, tmp82
movq (%rax), %rdi #, prephitmp.1149
.L218:
testb $1, %dil #, prephitmp.1149
jne .L217 #,
testq %rdi, %rdi # prephitmp.1149
je .L203 #,
cmpq $-1, %rdi #, prephitmp.1149
je .L217 #,
movl 8(%rdi), %esi # <variable>._count.counter, c
testl %esi, %esi # c
je .L218 #,

after:
.L212:
movq (%rbx), %rax #* ivtmp.1109, tmp81
movq (%rax), %rdi #, ret
testb $1, %dil #, ret
jne .L211 #,
testq %rdi, %rdi # ret
je .L197 #,
cmpq $-1, %rdi #, ret
je .L211 #,
movl 8(%rdi), %esi # <variable>._count.counter, c
testl %esi, %esi # c
je .L212 #,

(notice the obvious infinite loop in the first example, if page->count remains 0)

The problem was noticed and resolved on 2.6.27 stable kernels, and also applies
upstream (where I was able to reproduce it and verify the fix).

Reported-by: Peter Klotz <peter.klotz@aon.at>
Reported-by: Roman Kononov <kononov@ftml.net>
Tested-by: Peter Klotz <peter.klotz@aon.at>
Tested-by: Roman Kononov <kononov@ftml.net>
Signed-off-by: Nick Piggin <npiggin@suse.de>
---
Index: linux-2.6/mm/filemap.c
===================================================================
--- linux-2.6.orig/mm/filemap.c 2009-01-05 17:22:57.000000000 +1100
+++ linux-2.6/mm/filemap.c 2009-01-05 17:28:40.000000000 +1100
@@ -794,8 +794,19 @@ repeat:
if (unlikely(page == RADIX_TREE_RETRY))
goto restart;

- if (!page_cache_get_speculative(page))
+ if (!page_cache_get_speculative(page)) {
+ /*
+ * A failed page_cache_get_speculative operation does
+ * not imply any barriers (Documentation/atomic_ops.txt),
+ * and as such, we must force the compiler to deref the
+ * radix-tree slot again rather than using the cached
+ * value (because we need to give up if the page has been
+ * removed from the radix-tree, rather than looping until
+ * it gets reused for something else).
+ */
+ barrier();
goto repeat;
+ }

/* Has the page moved? */
if (unlikely(page != *((void **)pages[i]))) {
@@ -850,8 +861,11 @@ repeat:
if (page->mapping == NULL || page->index != index)
break;

- if (!page_cache_get_speculative(page))
+ if (!page_cache_get_speculative(page)) {
+ /* barrier: see find_get_pages() */
+ barrier();
goto repeat;
+ }

/* Has the page moved? */
if (unlikely(page != *((void **)pages[i]))) {
@@ -904,8 +918,11 @@ repeat:
if (unlikely(page == RADIX_TREE_RETRY))
goto restart;

- if (!page_cache_get_speculative(page))
+ if (!page_cache_get_speculative(page)) {
+ /* barrier: see find_get_pages() */
+ barrier();
goto repeat;
+ }

/* Has the page moved? */
if (unlikely(page != *((void **)pages[i]))) {