Re: [S390] page_mkclean data corruption.

[Linus Torvalds]

On Wed, 4 Apr 2007, Martin Schwidefsky wrote:
>
> the attached patch fixes a data corruption problem that has been
> introduced with the page_mkclean/clear_page_dirty_for_io change
> (the "Yes, Virginia, this is indeed insane." problem :-/)

Ok. I'm a bit worried about something like this, this late in the release 
cycle, but since I guess page_test_and_clear_dirty() is always 0 for any 
architecture but S390, I guess there are no possible downsides except for 
that architecture.

So I'll apply it, but:

> The effect of the two changes is that for every call to
> clear_page_dirty_for_io a page_test_and_clear_dirty is done. If
> the per page dirty bit is set set_page_dirty is called. Strangly
> clear_page_dirty_for_io is called for not-uptodate pages, e.g.
> over this call-chain:
> 
>  [<000000000007c0f2>] clear_page_dirty_for_io+0x12a/0x130
>  [<000000000007c494>] generic_writepages+0x258/0x3e0 
>  [<000000000007c692>] do_writepages+0x76/0x7c 
>  [<00000000000c7a26>] __writeback_single_inode+0xba/0x3e4
>  [<00000000000c831a>] sync_sb_inodes+0x23e/0x398 
>  [<00000000000c8802>] writeback_inodes+0x12e/0x140 
>  [<000000000007b9ee>] wb_kupdate+0xd2/0x178 
>  [<000000000007cca2>] pdflush+0x162/0x23c 
> 
> The bad news now is that page_test_and_clear_dirty might claim
> that a not-uptodate page is dirty since SetPageUptodate which
> resets the per page dirty bit has not yet been called. The page
> writeback that follows clobbers the data on disk.

Wouldn't it be best if S390 tried to avoid this by clearing the dirty bit 
whenever a new page is allocated? 

This is a very subtle and very surprising problem with the whole 
"page_test_and_clear_dirty()" thing - where a new page can be marked dirty 
for no obvious reason.

If S390 marked it clean at *allocation* time instead of at 
SetPageUptodate() time, that would also mean that the whole strange 
special case for S390 in SetPageUptodate() would go away.

Hmm? Or is marking things clean so expensive that you generally don't want 
to do it in the allocation path?

Anyway, I'll apply the patch, since for 2.6.21 this is clearly the 
simplest solution, but 
 (a) I think it might be ugly
and
 (b) are you sure that it doesn't introduce a new bug on S390, where some 
     page has been *removed* from the mappings, and should still trigger 
     the "page_test_and_clear_dirty()" test, but now, because it's done 
     inside the "if (page_mapped())" case, we miss it?

That said, in many ways, moving the whole "page_test_and_clear_dirty()" 
thing inside the "page_mapped()" thing does seem to make conceptual sense 
(since the only way it would become dirty in that way is if it's mapped), 
so I don't mind the patch, I just worry about (b) a bit, and if we got rid 
of the strange special code in S390 to SetPageUptodate() that would also 
be nice.

		Linus
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/