lkml.org 
[lkml]   [2008]   [Jan]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH -v8 3/4] Enable the MS_ASYNC functionality in sys_msync()


On Wed, 23 Jan 2008, Miklos Szeredi wrote:
>
> Yeah, nasty.
>
> How about doing it in a separate pass, similarly to
> wait_on_page_writeback()? Just instead of waiting, clean the page
> tables for writeback pages.

That sounds like a good idea, but it doesn't work.

The thing is, we need to hold the page-table lock over the whole sequence
of

if (page_mkclean(page))
set_page_dirty(page);
if (TestClearPageDirty(page))
..

and there's a big comment about why in clear_page_dirty_for_io().

So if you split it up, so that the first phase is that

if (page_mkclean(page))
set_page_dirty(page);

and the second phase is the one that just does a

if (TestClearPageDirty(page))
writeback(..)

and having dropped the page lock in between, then you lose: because
another thread migth have faulted in and re-dirtied the page table entry,
and you MUST NOT do that "TestClearPageDirty()" in that case!

That dirty bit handling is really really important, and it's sadly also
really really easy to get wrong (usually in ways that are hard to even
notice: things still work 99% of the time, and you might just be leaking
memory slowly, and fsync/msync() might not write back memory mapped data
to disk at all etc).

> Sure, I would have though all of this stuff is 2.6.25, but it's your
> kernel... :)

Well, the plain added "file_update_time()" call addition looked like a
trivial fix, and if there are actually *customers* that have bad backups
due to this, then I think that part was worth doing. At least a "sync"
will then sync the file times...

Linus


\
 
 \ /
  Last update: 2008-01-23 22:41    [W:0.076 / U:0.040 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site