Hi Andrew,
we just tracked down a severe bug in the memory management
code of s390. There is a race window where s390 can loose
a dirty bit. I never expected that SetPageUptodate is called
on an already up to date page...
blue skies,
Martin.
---
[PATCH] s390: lost dirty bits.
The SetPageUptodate function is called for pages that are already
up to date. The arch_set_page_uptodate function of s390 may not
clear the dirty bit in that case otherwise a dirty bit which is set
between the start of an i/o for a writeback and a following call
to SetPageUptodate is lost.
Signed-off-by: Martin Schwidefsky <[email protected]>
diffstat:
--- linux-2.5/include/asm-s390/pgtable.h 24 Mar 2004 18:18:22 -0000 1.23
+++ linux-2.5/include/asm-s390/pgtable.h 15 Jun 2004 16:43:35 -0000 1.23.2.1
@@ -652,7 +652,8 @@
#define arch_set_page_uptodate(__page) \
do { \
- asm volatile ("sske %0,%1" : : "d" (0), \
+ if (!PageUptodate(__page)) \
+ asm volatile ("sske %0,%1" : : "d" (0), \
"a" (__pa((__page-mem_map) << PAGE_SHIFT)));\
} while (0)
Martin Schwidefsky <[email protected]> wrote:
>
> The SetPageUptodate function is called for pages that are already
> up to date. The arch_set_page_uptodate function of s390 may not
> clear the dirty bit in that case otherwise a dirty bit which is set
> between the start of an i/o for a writeback and a following call
> to SetPageUptodate is lost.
>
> Signed-off-by: Martin Schwidefsky <[email protected]>
>
> diffstat:
>
> --- linux-2.5/include/asm-s390/pgtable.h 24 Mar 2004 18:18:22 -0000 1.23
> +++ linux-2.5/include/asm-s390/pgtable.h 15 Jun 2004 16:43:35 -0000 1.23.2.1
> @@ -652,7 +652,8 @@
>
> #define arch_set_page_uptodate(__page) \
> do { \
> - asm volatile ("sske %0,%1" : : "d" (0), \
> + if (!PageUptodate(__page)) \
> + asm volatile ("sske %0,%1" : : "d" (0), \
> "a" (__pa((__page-mem_map) << PAGE_SHIFT)));\
> } while (0)
Do you know what the call path for the redundant SetpageUptodate() is?
This patch still has a little race - it'd be better to override _all_ of
SetPageUptodate() in page-flags.h and do:
if (!test_and_set_bit(PG_uptodate, &page->flags))
...
--- 25/include/asm-s390/pgtable.h~s390-lost-dirty-bits 2004-06-15 21:02:00.621441504 -0700
+++ 25-akpm/include/asm-s390/pgtable.h 2004-06-15 21:06:43.391453928 -0700
@@ -656,7 +656,8 @@ static inline pte_t mk_pte_phys(unsigned
#define arch_set_page_uptodate(__page) \
do { \
- asm volatile ("sske %0,%1" : : "d" (0), \
+ if (!test_and_set_bit(PG_uptodate, __page)) \
+ asm volatile ("sske %0,%1" : : "d" (0), \
"a" (__pa((__page-mem_map) << PAGE_SHIFT)));\
} while (0)
diff -puN include/linux/page-flags.h~s390-lost-dirty-bits include/linux/page-flags.h
--- 25/include/linux/page-flags.h~s390-lost-dirty-bits 2004-06-15 21:04:58.982326528 -0700
+++ 25-akpm/include/linux/page-flags.h 2004-06-15 21:08:30.493171992 -0700
@@ -194,16 +194,12 @@ extern unsigned long __read_page_state(u
#define ClearPageReferenced(page) clear_bit(PG_referenced, &(page)->flags)
#define TestClearPageReferenced(page) test_and_clear_bit(PG_referenced, &(page)->flags)
-#ifndef arch_set_page_uptodate
-#define arch_set_page_uptodate(page) do { } while (0)
+#ifdef arch_set_page_uptodate
+#define SetPageUptodate(page) arch_set_page_uptodate(page)
+#else
+#define SetPageUptodate(page) set_bit(PG_uptodate, &(page)->flags)
#endif
-
#define PageUptodate(page) test_bit(PG_uptodate, &(page)->flags)
-#define SetPageUptodate(page) \
- do { \
- arch_set_page_uptodate(page); \
- set_bit(PG_uptodate, &(page)->flags); \
- } while (0)
#define ClearPageUptodate(page) clear_bit(PG_uptodate, &(page)->flags)
#define PageDirty(page) test_bit(PG_dirty, &(page)->flags)
_
On Tue, Jun 15, 2004 at 09:09:19PM -0700, Andrew Morton wrote:
> #define ClearPageReferenced(page) clear_bit(PG_referenced, &(page)->flags)
> #define TestClearPageReferenced(page) test_and_clear_bit(PG_referenced, &(page)->flags)
>
> -#ifndef arch_set_page_uptodate
> -#define arch_set_page_uptodate(page) do { } while (0)
> +#ifdef arch_set_page_uptodate
> +#define SetPageUptodate(page) arch_set_page_uptodate(page)
> +#else
> +#define SetPageUptodate(page) set_bit(PG_uptodate, &(page)->flags)
> #endif
Eek. It looks like SetPageUptodate, it smells like SetPageUptodate, why
do you give it another name? Just put a
#ifndef SetPageUptodate /* S390 wants to override this */
#define SetPageUptodate set_bit(PG_uptodate, &(page)->flags)
#endif
in mm.h
> This patch still has a little race - it'd be better to override _all_ of
> SetPageUptodate() in page-flags.h and do:
This is even better because it really makes a race impossible. I think it
is correct with the simple if as well because a page which isn't up to date
isn't mapped anywhere and while a page is read from the backing store it
is locked. The end io function first does SetPageUptodate and then unlocks
the page.
Combining the test_and_set_bit idea with Christophs valid objection I
created a new patch.
blue skies,
Martin
---
[PATCH] s390: lost dirty bits.
The SetPageUptodate function is called for pages that are already
up to date. The arch_set_page_uptodate function of s390 may not
clear the dirty bit in that case otherwise a dirty bit which is set
between the start of an i/o for a writeback and a following call
to SetPageUptodate is lost.
Signed-off-by: Martin Schwidefsky <[email protected]>
diffstat:
include/asm-s390/pgtable.h | 6 ++++--
include/linux/page-flags.h | 12 +++---------
2 files changed, 7 insertions(+), 11 deletions(-)
diff -urN linux-2.6/include/asm-s390/pgtable.h linux-2.6-s390/include/asm-s390/pgtable.h
--- linux-2.6/include/asm-s390/pgtable.h Wed Jun 16 10:39:37 2004
+++ linux-2.6-s390/include/asm-s390/pgtable.h Wed Jun 16 10:39:49 2004
@@ -654,9 +654,11 @@
__pte; \
})
-#define arch_set_page_uptodate(__page) \
+#define SetPageUptodate(_page) \
do { \
- asm volatile ("sske %0,%1" : : "d" (0), \
+ struct page *__page = (_page); \
+ if (!test_and_set_bit(PG_uptodate, &__page->flags)) \
+ asm volatile ("sske %0,%1" : : "d" (0), \
"a" (__pa((__page-mem_map) << PAGE_SHIFT)));\
} while (0)
diff -urN linux-2.6/include/linux/page-flags.h linux-2.6-s390/include/linux/page-flags.h
--- linux-2.6/include/linux/page-flags.h Wed Jun 16 10:39:37 2004
+++ linux-2.6-s390/include/linux/page-flags.h Wed Jun 16 10:39:49 2004
@@ -194,16 +194,10 @@
#define ClearPageReferenced(page) clear_bit(PG_referenced, &(page)->flags)
#define TestClearPageReferenced(page) test_and_clear_bit(PG_referenced, &(page)->flags)
-#ifndef arch_set_page_uptodate
-#define arch_set_page_uptodate(page) do { } while (0)
-#endif
-
#define PageUptodate(page) test_bit(PG_uptodate, &(page)->flags)
-#define SetPageUptodate(page) \
- do { \
- arch_set_page_uptodate(page); \
- set_bit(PG_uptodate, &(page)->flags); \
- } while (0)
+#ifndef SetPageUptodate
+#define SetPageUptodate(page) set_bit(PG_uptodate, &(page)->flags)
+#endif
#define ClearPageUptodate(page) clear_bit(PG_uptodate, &(page)->flags)
#define PageDirty(page) test_bit(PG_dirty, &(page)->flags)