On Tue 23-10-12 14:56:36, Andrew Morton wrote: > On Tue, 23 Oct 2012 12:21:53 +0200 > Jan Kara <jack@xxxxxxx> wrote: > > > > That seems a fairly serious problem. To which kernel version(s) should > > > we apply the fix? > > Well, XFS will crash starting from 2.6.36 kernel where the assertion was > > added. Previously XFS just silently added buffers (as other filesystems do > > it) and wrote / redirtied the page (unnecessarily). So looking into > > maintained -stable branches I think pushing the patch to -stable from 3.0 > > on should be enough. > > OK, thanks, I made it so. > > > > > diff --git a/mm/rmap.c b/mm/rmap.c > > > > > > It's a bit surprising that none of the added comments mention the s390 > > > pte-dirtying oddity. I don't see an obvious place to mention this, but > > > I for one didn't know about this and it would be good if we could > > > capture the info _somewhere_? > > As Hugh says, the comment before page_test_and_clear_dirty() is somewhat > > updated. But do you mean recording somewhere the catch that s390 HW dirty > > bit gets set also whenever we write to a page from kernel? > > Yes, this. It's surprising behaviour which we may trip over again, so > how do we inform developers about it? > > > I guess we could > > add that also to the comment before page_test_and_clear_dirty() in > > page_remove_rmap() and also before definition of > > page_test_and_clear_dirty(). So most people that will add / remove these > > calls will be warned. OK? > > Sounds good, thanks. OK, the patch is attached. As Martin says, it may be obsolete soon but just in case Martin's patch set gets delayed... Honza -- Jan Kara <jack@xxxxxxx> SUSE Labs, CR
>From 55b91bcc0aff3e321d16a02c4381acfe3981f488 Mon Sep 17 00:00:00 2001 From: Jan Kara <jack@xxxxxxx> Date: Thu, 25 Oct 2012 21:57:12 +0200 Subject: [PATCH] mm: Comment on storage key dirty bit semantics Add coments that dirty bit in storage key gets set whenever page content is changed. Hopefully if someone will use this function, he'll have a look at one of the two places where we comment on this. Signed-off-by: Jan Kara <jack@xxxxxxx> --- arch/s390/include/asm/page.h | 3 +++ mm/rmap.c | 8 +++++--- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/arch/s390/include/asm/page.h b/arch/s390/include/asm/page.h index 27ab3c7..7787094 100644 --- a/arch/s390/include/asm/page.h +++ b/arch/s390/include/asm/page.h @@ -150,6 +150,9 @@ static inline int page_reset_referenced(unsigned long addr) * race against modification of the referenced bit. This function * should therefore only be called if it is not mapped in any * address space. + * + * Note that the bit gets set whenever page content is changed. That means + * also when the page is modified by DMA or from inside the kernel. */ #define __HAVE_ARCH_PAGE_TEST_AND_CLEAR_DIRTY static inline int page_test_and_clear_dirty(unsigned long pfn, int mapped) diff --git a/mm/rmap.c b/mm/rmap.c index 2ee1ef0..d359158 100644 --- a/mm/rmap.c +++ b/mm/rmap.c @@ -1139,9 +1139,11 @@ void page_remove_rmap(struct page *page) * containing the swap entry, but page not yet written to swap. * * And we can skip it on file pages, so long as the filesystem - * participates in dirty tracking; but need to catch shm and tmpfs - * and ramfs pages which have been modified since creation by read - * fault. + * participates in dirty tracking (note that this is not only an + * optimization but also solves problems caused by dirty flag in + * storage key getting set by a write from inside kernel); but need to + * catch shm and tmpfs and ramfs pages which have been modified since + * creation by read fault. * * Note that mapping must be decided above, before decrementing * mapcount (which luckily provides a barrier): once page is unmapped, -- 1.7.1