invalidate_inode_pages2

invalidate_inode_pages2

[Andrea Arcangeli]

Something broke in invalidate_inode_pages2 between 2.4 and 2.6, this
causes malfunctions with mapped pages in 2.6.

I guess the below untested one liner should be enough to fix it. The
only single point of invalidate_inode_pages2, is to invalidate _mapped_
pages too. Otherwise we could as well use invalidate_inode_pages.
Clearly the dirty bit doesn't mean invalidate, invalidate primarly means
clearing the uptodate bitflag.

--- sles/mm/truncate.c.~1~	2004-05-18 19:24:40.000000000 +0200
+++ sles/mm/truncate.c	2004-05-19 02:09:28.311781864 +0200
@@ -260,9 +260,10 @@ void invalidate_inode_pages2(struct addr
 			if (page->mapping == mapping) {	/* truncate race? */
 				wait_on_page_writeback(page);
 				next = page->index + 1;
-				if (page_mapped(page))
+				if (page_mapped(page)) {
+					ClearPageUptodate(page);
 					clear_page_dirty(page);
-				else
+				} else
 					invalidate_complete_page(mapping, page);
 			}
 			unlock_page(page);

Re: invalidate_inode_pages2

[Andrew Morton]

Andrea Arcangeli <andrea@suse.de> wrote:
>
> Something broke in invalidate_inode_pages2 between 2.4 and 2.6, this
> causes malfunctions with mapped pages in 2.6.

What is the malfunction?

> I guess the below untested one liner should be enough to fix it. The
> only single point of invalidate_inode_pages2, is to invalidate _mapped_
> pages too. Otherwise we could as well use invalidate_inode_pages.
> Clearly the dirty bit doesn't mean invalidate, invalidate primarly means
> clearing the uptodate bitflag.
> 
> --- sles/mm/truncate.c.~1~	2004-05-18 19:24:40.000000000 +0200
> +++ sles/mm/truncate.c	2004-05-19 02:09:28.311781864 +0200
> @@ -260,9 +260,10 @@ void invalidate_inode_pages2(struct addr
>  			if (page->mapping == mapping) {	/* truncate race? */
>  				wait_on_page_writeback(page);
>  				next = page->index + 1;
> -				if (page_mapped(page))
> +				if (page_mapped(page)) {
> +					ClearPageUptodate(page);
>  					clear_page_dirty(page);
> -				else
> +				} else
>  					invalidate_complete_page(mapping, page);
>  			}
>  			unlock_page(page);

It's currently the case that pages which are mapped into process pagetables
are always up to date, which sounds like a good invariant to have.  This
changes that rule.  I dunno if it'll break anything though.

Re: invalidate_inode_pages2

[Andrea Arcangeli]

On Tue, May 18, 2004 at 05:27:18PM -0700, Andrew Morton wrote:
> Andrea Arcangeli <andrea@suse.de> wrote:
> >
> > Something broke in invalidate_inode_pages2 between 2.4 and 2.6, this
> > causes malfunctions with mapped pages in 2.6.
> 
> What is the malfunction?

>From Olaf Kirch

 -      single application on NFS client opens file and maps it.
        No-one else has this file open. File contains "zappa\n",
        and the test app stats it once a second and reports size and
        contents.
        len=6, data=7a 61 70 70 61 0a
 -      on the NFS server, I do "echo frobnorz > file"
 -      after a while, the test app on the client reports
        len=10, data=7a 61 70 70 61 0a
 -      I ctrl-C the app and restart it. We agree that this amounts to
        a munmap+mmap of the file, right?
        The test app now reports
        len=10, data=7a 61 70 70 61 0a 00 00 00 00

my fix is untested at this time (but I expect it to fix the above
problem).

> > I guess the below untested one liner should be enough to fix it. The
> > only single point of invalidate_inode_pages2, is to invalidate _mapped_
> > pages too. Otherwise we could as well use invalidate_inode_pages.
> > Clearly the dirty bit doesn't mean invalidate, invalidate primarly means
> > clearing the uptodate bitflag.
> > 
> > --- sles/mm/truncate.c.~1~	2004-05-18 19:24:40.000000000 +0200
> > +++ sles/mm/truncate.c	2004-05-19 02:09:28.311781864 +0200
> > @@ -260,9 +260,10 @@ void invalidate_inode_pages2(struct addr
> >  			if (page->mapping == mapping) {	/* truncate race? */
> >  				wait_on_page_writeback(page);
> >  				next = page->index + 1;
> > -				if (page_mapped(page))
> > +				if (page_mapped(page)) {
> > +					ClearPageUptodate(page);
> >  					clear_page_dirty(page);
> > -				else
> > +				} else
> >  					invalidate_complete_page(mapping, page);
> >  			}
> >  			unlock_page(page);
> 
> It's currently the case that pages which are mapped into process pagetables
> are always up to date, which sounds like a good invariant to have.  This

I already intentionally broke that invariant in 2.4 just to make exactly
this thing work safely, this is needed for correct O_DIRECT semantics
too.

All it matters is that the pages are re-read after munmap+mmap.

> changes that rule.  I dunno if it'll break anything though.

It didn't break anything in 2.4 AFIK.

Re: invalidate_inode_pages2

[Andrew Morton]

Andrea Arcangeli <andrea@suse.de> wrote:
>
> On Tue, May 18, 2004 at 05:27:18PM -0700, Andrew Morton wrote:
> > Andrea Arcangeli <andrea@suse.de> wrote:
> > >
> > > Something broke in invalidate_inode_pages2 between 2.4 and 2.6, this
> > > causes malfunctions with mapped pages in 2.6.
> > 
> > What is the malfunction?
> 
> >From Olaf Kirch
> 
>  -      single application on NFS client opens file and maps it.
>         No-one else has this file open. File contains "zappa\n",
>         and the test app stats it once a second and reports size and
>         contents.
>         len=6, data=7a 61 70 70 61 0a
>  -      on the NFS server, I do "echo frobnorz > file"
>  -      after a while, the test app on the client reports
>         len=10, data=7a 61 70 70 61 0a
>  -      I ctrl-C the app and restart it. We agree that this amounts to
>         a munmap+mmap of the file, right?
>         The test app now reports
>         len=10, data=7a 61 70 70 61 0a 00 00 00 00

OK.  Can we do a full pte invalidation and force a major fault?

> > It's currently the case that pages which are mapped into process pagetables
> > are always up to date, which sounds like a good invariant to have.  This
> 
> I already intentionally broke that invariant in 2.4 just to make exactly
> this thing work safely, this is needed for correct O_DIRECT semantics
> too.
> 
> All it matters is that the pages are re-read after munmap+mmap.
> 
> > changes that rule.  I dunno if it'll break anything though.
> 
> It didn't break anything in 2.4 AFIK.

It might have caused some of the debug checks in fs/buffer.c to get angry
when it's used by direct-IO.  But they're gone now anyway...

Re: invalidate_inode_pages2

[Andrea Arcangeli]

On Tue, May 18, 2004 at 06:00:28PM -0700, Andrew Morton wrote:
> OK.  Can we do a full pte invalidation and force a major fault?

we'd need to take the page_table_lock to do that from there, I find
safer to stay at the pagecache layer at that point inside the nfs
filesystem routines. the semantics of invalidate_inode_pages2 doesn't
require a synchronous invalidate with major fault, both O_DIRECT and nfs
cannot provide distributed shared memory anyways, all it matters is that
_future_ reads will trigger readpage again to provide inode invalidate
semantics.

> > > It's currently the case that pages which are mapped into process pagetables
> > > are always up to date, which sounds like a good invariant to have.  This
> > 
> > I already intentionally broke that invariant in 2.4 just to make exactly
> > this thing work safely, this is needed for correct O_DIRECT semantics
> > too.
> > 
> > All it matters is that the pages are re-read after munmap+mmap.
> > 
> > > changes that rule.  I dunno if it'll break anything though.
> > 
> > It didn't break anything in 2.4 AFIK.
> 
> It might have caused some of the debug checks in fs/buffer.c to get angry
> when it's used by direct-IO.  But they're gone now anyway...

sounds good then ;)

Re: invalidate_inode_pages2

[Jan Harkes]

On Wed, May 19, 2004 at 02:51:06AM +0200, Andrea Arcangeli wrote:
> On Tue, May 18, 2004 at 05:27:18PM -0700, Andrew Morton wrote:
> > Andrea Arcangeli <andrea@suse.de> wrote:
> > >
> > > Something broke in invalidate_inode_pages2 between 2.4 and 2.6, this
> > > causes malfunctions with mapped pages in 2.6.
> > 
> > What is the malfunction?
> 
> From Olaf Kirch
> 
>  -      single application on NFS client opens file and maps it.
>         No-one else has this file open. File contains "zappa\n",
>         and the test app stats it once a second and reports size and
>         contents.
>         len=6, data=7a 61 70 70 61 0a
>  -      on the NFS server, I do "echo frobnorz > file"
>  -      after a while, the test app on the client reports
>         len=10, data=7a 61 70 70 61 0a

I'm mostly just curious, what exactly happens when a second process
opens and mmaps the file at this point? Will it also see the new length
with the old data, or will that invalidate the mapping and pull the new
data off of the server?

Also what happens if the process had a shared mapping and dirtied the
page (f.i. it wrote a byte to to offset 0) but the update hasn't yet
been written back, will it end up committing the (stale) data from the
local copy of the page but with the updated length=10 back to the server?

Jan

Re: invalidate_inode_pages2

[Andrea Arcangeli]

On Wed, May 19, 2004 at 01:08:01AM -0400, Jan Harkes wrote:
> On Wed, May 19, 2004 at 02:51:06AM +0200, Andrea Arcangeli wrote:
> > On Tue, May 18, 2004 at 05:27:18PM -0700, Andrew Morton wrote:
> > > Andrea Arcangeli <andrea@suse.de> wrote:
> > > >
> > > > Something broke in invalidate_inode_pages2 between 2.4 and 2.6, this
> > > > causes malfunctions with mapped pages in 2.6.
> > > 
> > > What is the malfunction?
> > 
> > From Olaf Kirch
> > 
> >  -      single application on NFS client opens file and maps it.
> >         No-one else has this file open. File contains "zappa\n",
> >         and the test app stats it once a second and reports size and
> >         contents.
> >         len=6, data=7a 61 70 70 61 0a
> >  -      on the NFS server, I do "echo frobnorz > file"
> >  -      after a while, the test app on the client reports
> >         len=10, data=7a 61 70 70 61 0a
> 
> I'm mostly just curious, what exactly happens when a second process
> opens and mmaps the file at this point? Will it also see the new length
> with the old data, or will that invalidate the mapping and pull the new
> data off of the server?

depends if the new process will generate page faults or not. if it
generates page faults it will re-read the pages from the server while
the other task runs.

> Also what happens if the process had a shared mapping and dirtied the
> page (f.i. it wrote a byte to to offset 0) but the update hasn't yet
> been written back, will it end up committing the (stale) data from the
> local copy of the page but with the updated length=10 back to the server?

it will not commit the stale data because the invalidate clears the
dirty bit, again the new data will arrive from the server. Note that if
there's some VM paging in the client NFS side, the very same task can
pull the data again from the nfs server during pagein (the writeback
during pageout will not happen because the cache has been invalidated
and the dirty bit has been lost).