Checking page_count(page) in invalidate_complete_page

Hi Andrew-

Steve Dickson and I have independently discovered some cache invalidation problems in 2.6.18's NFS client. Using git bisect, I was able to track it back to this commit:

Quoted 11 lines

commit 016eb4a0ed06a3677d67a584da901f0e9a63c666

Author: Andrew Morton <[email protected]>
Date:   Fri Sep 8 09:48:38 2006 -0700

[PATCH] invalidate_complete_page() race fix

If a CPU faults this page into pagetables after invalidate_mapping_pages() checked page_mapped(), invalidate_complete_page() will still proceed to remove the page from pagecache. This leaves the page-faulting process with a detached page. If it was MAP_SHARED then file data loss will ensue.

Quoted 7 lines

Fix that up by checking the page's refcount after taking tree_lock.

Cc Nick Piggin <[email protected]>

Cc Hugh Dickins <[email protected]>

Cc <[email protected]>

Signed-off-by Andrew Morton <[email protected]>

Signed-off-by Linus Torvalds <[email protected]>

Instrumenting get_page and put_page has shown that the page reclaim logic is temporarily bumping the page count on otherwise active but idle pages, racing with the new page_count check in invalidate_complete_page and preventing pages from being invalidated.

One problem for the NFS client is that invalidate_inode_pages2 is being used to invalidate the pages associated with a cached directory. If the directory pages can't be invalidated because of this race, the contents of the directory pages don't match the dentry cache, and all kinds of strange behavior results.

I haven't checked the data invalidation behavior for regular files, but the result will be file data corruption that comes and goes.

Would it be acceptable to revert that page_count(page) != 2 check in invalidate_complete_page ?

(Added linux-mm)

Quoted 22 lines · On Mon, 25 Sep 2006 15:51:26 -0400 Chuck Lever <[email protected]> wrote:

Hi Andrew-

Steve Dickson and I have independently discovered some cache invalidation problems in 2.6.18's NFS client. Using git bisect, I was able to track it back to this commit:

Quoted 18 lines

commit 016eb4a0ed06a3677d67a584da901f0e9a63c666

Author: Andrew Morton <[email protected]>
Date:   Fri Sep 8 09:48:38 2006 -0700

[PATCH] invalidate_complete_page() race fix

If a CPU faults this page into pagetables after invalidate_mapping_pages() checked page_mapped(), invalidate_complete_page() will still proceed to remove the page from pagecache. This leaves the page-faulting process with a detached page. If it was MAP_SHARED then file data loss will ensue.

Fix that up by checking the page's refcount after taking tree_lock.

Cc Nick Piggin <[email protected]>

Cc Hugh Dickins <[email protected]>

Cc <[email protected]>

Signed-off-by Andrew Morton <[email protected]>

Signed-off-by Linus Torvalds <[email protected]>

Quoted 3 lines

Instrumenting get_page and put_page has shown that the page reclaim logic is temporarily bumping the page count on otherwise active but idle pages, racing with the new page_count check in invalidate_complete_page and preventing pages from being invalidated.

One problem for the NFS client is that invalidate_inode_pages2 is being used to invalidate the pages associated with a cached directory. If the directory pages can't be invalidated because of this race, the contents of the directory pages don't match the dentry cache, and all kinds of strange behavior results.

NFS is presently ignoring the return value from invalidate_inode_pages2(), in two places. Could I suggest you fix that? Then we'd at least not be seeing "strange behaviour" and things will be easier to diagnose next time.

Quoted 1 line

Would it be acceptable to revert that page_count(page) != 2 check in invalidate_complete_page ?

Unfortunately not - that patch fixes cramfs failures and potential file corruption.

The way to keep memory reclaim away from that page is to take zone->lru_lock, but that's quite impractical for several reasons.

We could retry the invalidation a few times, but that stinks.

I think invalidate_inode_pages2() is sufficiently different from (ie:
stronger than) invalidate_inode_pages() to justify the addition of a new invalidate_complete_page2(), which skips the page refcount check.

It seems that the NFS client could now safely use a page cache invalidator that would wait for other page users to ensure that every page is invalidated properly, instead of skipping the pages that can't be immediately invalidated.

In my opinion that would be the correct fix here for NFS.

It seems that the NFS client could now safely use a page cache invalidator that would wait for other page users to ensure that every page is invalidated properly, instead of skipping the pages that can't be immediately invalidated.

In my opinion that would be the correct fix here for NFS.

Quoted 3 lines · Chuck Lever

It seems that the NFS client could now safely use a page cache invalidator that would wait for other page users to ensure that every page is invalidated properly, instead of skipping the pages that can't be immediately invalidated.

Quoted 1 line

In my opinion that would be the correct fix here for NFS.

I would have to agree with this... in debugging this I changed the invalidate_inode_pages2 in nfs_revalidate_mapping to truncate_inode_pages() for non-file inode which also seem to work... So it does beg the question as to why aren't we waiting for page to be invalidated? Is there some type of VM deadlock we are trying to avoid?

(Added linux-mm)

Quoted 17 lines · On Mon, 25 Sep 2006 15:51:26 -0400 Chuck Lever <[email protected]> wrote:

Hi Andrew-

Steve Dickson and I have independently discovered some cache invalidation problems in 2.6.18's NFS client. Using git bisect, I was able to track it back to this commit:

Quoted 13 lines

commit 016eb4a0ed06a3677d67a584da901f0e9a63c666

Author: Andrew Morton <[email protected]>
Date:   Fri Sep 8 09:48:38 2006 -0700

[PATCH] invalidate_complete_page() race fix

If a CPU faults this page into pagetables after invalidate_mapping_pages() checked page_mapped(), invalidate_complete_page() will still proceed to remove the page from pagecache. This leaves the page-faulting process with a detached page. If it was MAP_SHARED then file data loss will ensue.

Fix that up by checking the page's refcount after taking tree_lock.

Quoted 8 lines

Quoted 5 lines

Cc Nick Piggin <[email protected]>

Cc Hugh Dickins <[email protected]>

Cc <[email protected]>

Signed-off-by Andrew Morton <[email protected]>

Signed-off-by Linus Torvalds <[email protected]>

Instrumenting get_page and put_page has shown that the page reclaim logic is temporarily bumping the page count on otherwise active but idle pages, racing with the new page_count check in invalidate_complete_page and preventing pages from being invalidated.

One problem for the NFS client is that invalidate_inode_pages2 is being used to invalidate the pages associated with a cached directory. If the directory pages can't be invalidated because of this race, the contents of the directory pages don't match the dentry cache, and all kinds of strange behavior results.

NFS is presently ignoring the return value from invalidate_inode_pages2(), in two places. Could I suggest you fix that? Then we'd at least not be seeing "strange behaviour" and things will be easier to diagnose next time.

Quoted 3 lines

I haven't checked the data invalidation behavior for regular files, but the result will be file data corruption that comes and goes.

Would it be acceptable to revert that page_count(page) != 2 check in invalidate_complete_page ?

Unfortunately not - that patch fixes cramfs failures and potential file corruption.

The way to keep memory reclaim away from that page is to take zone->lru_lock, but that's quite impractical for several reasons.

We could retry the invalidation a few times, but that stinks.

I think invalidate_inode_pages2() is sufficiently different from (ie:
stronger than) invalidate_inode_pages() to justify the addition of a new invalidate_complete_page2(), which skips the page refcount check.

invalidate_inode_pages2 appears to wait for locked pages, while invalidate_inode_pages skips them.

I think invalidate_inode_pages2() is sufficiently different from (ie:
stronger than) invalidate_inode_pages() to justify the addition of a new invalidate_complete_page2(), which skips the page refcount check.

Quoted 4 lines

I haven't checked the data invalidation behavior for regular files, but the result will be file data corruption that comes and goes.

Would it be acceptable to revert that page_count(page) != 2 check in invalidate_complete_page ?

Unfortunately not - that patch fixes cramfs failures and potential file corruption.

The way to keep memory reclaim away from that page is to take zone->lru_lock, but that's quite impractical for several reasons.

We could retry the invalidation a few times, but that stinks.

I think invalidate_inode_pages2() is sufficiently different from (ie:
stronger than) invalidate_inode_pages() to justify the addition of a new invalidate_complete_page2(), which skips the page refcount check.

Andrew Morton wrote:
Also, you can't guarantee anything much about its refcount even then (because it could be on a private reclaim list or pagevec somewhere).

Quoted 6 lines

We could retry the invalidation a few times, but that stinks.

I think invalidate_inode_pages2() is sufficiently different from (ie:
stronger than) invalidate_inode_pages() to justify the addition of a new invalidate_complete_page2(), which skips the page refcount check.

Yes, I think that would be possible using the lock_page in do_no_page trick. That would also enable you to invalidate pages that have direct IO going into them, and other weird and wonderful get_user_pages happenings.

I haven't thrown away those patches, and I am looking for a justification for them because they make the code look nicer ;)

For 2.6.18.stable, Andrew's idea of checking the return value and retry might be the only option.

Quoted 15 lines · Nick Piggin

Andrew Morton wrote:
Also, you can't guarantee anything much about its refcount even then (because it could be on a private reclaim list or pagevec somewhere).

Quoted 6 lines

We could retry the invalidation a few times, but that stinks.

I think invalidate_inode_pages2() is sufficiently different from (ie:
stronger than) invalidate_inode_pages() to justify the addition of a new invalidate_complete_page2(), which skips the page refcount check.

Yes, I think that would be possible using the lock_page in do_no_page trick. That would also enable you to invalidate pages that have direct IO going into them, and other weird and wonderful get_user_pages happenings.

Quoted 5 lines

I haven't thrown away those patches, and I am looking for a justification for them because they make the code look nicer ;)

For 2.6.18.stable, Andrew's idea of checking the return value and retry might be the only option.

I think allowing callers of invalidate_inode_pages2() to get the previous behavior is reasonable here. There are only 2 of them: v9fs and the NFS client.

Quoted 24 lines · Chuck Lever

Quoted 20 lines · Nick Piggin

Andrew Morton wrote:
Also, you can't guarantee anything much about its refcount even then (because it could be on a private reclaim list or pagevec somewhere).

Quoted 6 lines

We could retry the invalidation a few times, but that stinks.

I think invalidate_inode_pages2() is sufficiently different from (ie:
stronger than) invalidate_inode_pages() to justify the addition of a new invalidate_complete_page2(), which skips the page refcount check.

Yes, I think that would be possible using the lock_page in do_no_page trick. That would also enable you to invalidate pages that have direct IO going into them, and other weird and wonderful get_user_pages happenings.

I haven't thrown away those patches, and I am looking for a justification for them because they make the code look nicer ;)

For 2.6.18.stable, Andrew's idea of checking the return value and retry might be the only option.

Quoted 1 line

I think allowing callers of invalidate_inode_pages2() to get the previous behavior is reasonable here. There are only 2 of them: v9fs and the NFS client.

That still reintroduces the page fault race, but if the dumb check'n'retry is no good then it may be OK for 2.6.18.stable, considering the page fault race is much less common than the reclaim one. Not sure, not my call.

Upstream, it should be fixed properly without re-introducing bugs along the way.

Quoted 3 lines · Nick Piggin

That still reintroduces the page fault race, but if the dumb check'n'retry is no good then it may be OK for 2.6.18.stable, considering the page fault race is much less common than the reclaim one. Not sure, not my call.

The NFS client uses invalidate_inode_pages2 for files, symlinks, and directories. The latter two won't have the do_no_page race since you can't map those types of file objects.

For NFS files, the do_no_page race does exist (at least theoretically
-- I've never seen a report of such a problem). Most people are not brave enough to use shared mapped files on NFS... so that race may be very rare indeed.

Quoted 2 lines

Upstream, it should be fixed properly without re-introducing bugs along the way.

Of course... thanks for sending the history.

I'm wondering aloud here, because I'm a VM neophyte. I'm not sure how common the reclaim race is in real environments. For instance, the test I'm running is pretty simple, and I run it just after the client has rebooted. Why is try_to_free_pages being called here? If I knew that I could probably make a better guess about how common this race is.

Also, the last get_page() call is from pagevec_strip(). Why do we need to try to strip buffers off of a page that is guaranteed not to have any buffers?

Quoted 6 lines · Chuck Lever

Quoted 3 lines · Nick Piggin

That still reintroduces the page fault race, but if the dumb check'n'retry is no good then it may be OK for 2.6.18.stable, considering the page fault race is much less common than the reclaim one. Not sure, not my call.

Quoted 1 line

The NFS client uses invalidate_inode_pages2 for files, symlinks, and directories. The latter two won't have the do_no_page race since you can't map those types of file objects.

But they're present on the LRU? That's unusual (I guess NFS doesn't have a buffer cache for a backing block device).

Quoted 1 line

Also, the last get_page() call is from pagevec_strip(). Why do we need to try to strip buffers off of a page that is guaranteed not to have any buffers?

I don't see where pagevec_strip calls get_page()?

Quoted 1 line · Nick Piggin

I don't see where pagevec_strip calls get_page()?

Ah, you're right, I was off by one symbol. The get_page call is in pagevec_lookup() (via find_get_pages). That could mean there isn't a reclaim race at all.

The page references I'm noting are:

1. add_to_page_cache_lru 1 -> 2

2. do_generic_mapping_read, released in nfs_do_filldir

3. read_cache_page, released in nfs_readdir

4. pagevec_lookup (from invalidate_inode_pages2_range) 2 -> 3

The odd thing is some of the pages for the directory are released, so they must have a ref count of 2 *after* the pagevec_lookup. Looks like more investigation is needed.

Quoted 10 lines

The page references I'm noting are:

1. add_to_page_cache_lru 1 -> 2

2. do_generic_mapping_read, released in nfs_do_filldir

3. read_cache_page, released in nfs_readdir

4. pagevec_lookup (from invalidate_inode_pages2_range) 2 -> 3

Quoted 1 line

The odd thing is some of the pages for the directory are released, so they must have a ref count of 2 *after* the pagevec_lookup. Looks like more investigation is needed.

Well they could get flushed out of the lru add pagevecs which drops their count. Just to test that theory you could run lru_add_drain_all() at the start of the invalidate function. That's not going to actually fix the problem, but it might help 2.6.18 limp along when combined with some other work.

Hi Nick-

Quoted 15 lines · Nick Piggin

Quoted 6 lines

The page references I'm noting are:

1. add_to_page_cache_lru 1 -> 2

2. do_generic_mapping_read, released in nfs_do_filldir

Quoted 7 lines

3. read_cache_page, released in nfs_readdir

4. pagevec_lookup (from invalidate_inode_pages2_range) 2 -> 3

The odd thing is some of the pages for the directory are released, so they must have a ref count of 2 *after* the pagevec_lookup. Looks like more investigation is needed.

Quoted 7 lines

Well they could get flushed out of the lru add pagevecs which drops their count. Just to
test that theory you could run lru_add_drain_all() at the start of the invalidate function.
That's not going to actually fix the problem, but it might help 2.6.18 limp along when
combined with some other work.

I've verified that the pages that fail to be invalidated are indeed languishing in the per-CPU lru-add pagevec. Adding a call to lru_add_drain_all() to the invalidate_inode_pages2() path does fix the NFS readdir problem.

I think a call to lru_add_drain_all() belongs in both the invalidate_inode_pages() and the invalidate_inode_pages2() path. Do you agree?

Quoted 3 lines

I think a call to lru_add_drain_all() belongs in both the invalidate_inode_pages() and the invalidate_inode_pages2() path. Do you agree?

Yes.

Quoted 2 lines · On Thu, 28 Sep 2006 09:36:40 -0700 Andrew Morton <[email protected]> wrote:

Quoted 1 line

I think a call to lru_add_drain_all() belongs in both the invalidate_inode_pages() and the invalidate_inode_pages2() path. Do you agree?

Yes.

Or maybe not. lru_add_drain() will only drain the local CPU's buffer. If the page is sitting in another CPU's buffer, the same problem will occur.

IOW, you got lucky.

Quoted 6 lines · Andrew Morton

Quoted 3 lines · On Thu, 28 Sep 2006 09:36:40 -0700 Andrew Morton <[email protected]> wrote:

Quoted 3 lines

I think a call to lru_add_drain_all() belongs in both the invalidate_inode_pages() and the invalidate_inode_pages2() path. Do you agree?

Quoted 6 lines

Quoted 1 line

Yes.

Or maybe not. lru_add_drain() will only drain the local CPU's buffer. If the page is sitting in another CPU's buffer, the same problem will occur.

IOW, you got lucky.

I used lru_add_drain_all(), so it hit all the per-CPU pagevecs.

Quoted 11 lines · On Thu, 28 Sep 2006 12:42:44 -0400 Chuck Lever <[email protected]> wrote:

Quoted 9 lines · Andrew Morton

Quoted 2 lines · On Thu, 28 Sep 2006 09:36:40 -0700 Andrew Morton <[email protected]> wrote:

Quoted 1 line

I think a call to lru_add_drain_all() belongs in both the invalidate_inode_pages() and the invalidate_inode_pages2() path. Do you agree?

Yes.

Or maybe not. lru_add_drain() will only drain the local CPU's buffer. If the page is sitting in another CPU's buffer, the same problem will occur.

IOW, you got lucky.

I used lru_add_drain_all(), so it hit all the per-CPU pagevecs.

lru_add_drain_all() is a nasty, hacky, not-exported-to-modules thing. It equates to lru_add_drain() if !CONFIG_NUMA.

Sigh, we're not getting there, are we?

I'm still thinking we add invalidate_complete_page2() to get us out of trouble and park the problem :(. That'd be a good approach for 2.6.18.x, which I assume is fairly urgent.

Quoted 2 lines · Andrew Morton

lru_add_drain_all() is a nasty, hacky, not-exported-to-modules thing. It equates to lru_add_drain() if !CONFIG_NUMA.

Quoted 6 lines

Sigh, we're not getting there, are we?

I'm still thinking we add invalidate_complete_page2() to get us out of trouble and park the problem :(. That'd be a good approach for 2.6.18.x, which I assume is fairly urgent.

Choosing which fix to include is above my pay grade. Both of these proposals address the NFS readdir cache invalidation problem.

But it seems like there is a real problem here -- the pages that are waiting to be added the LRU will always have a page count that is too high for invalidate_inode_pages to work on them.

Quoted 5 lines · Chuck Lever

Quoted 2 lines · Andrew Morton

lru_add_drain_all() is a nasty, hacky, not-exported-to-modules thing. It equates to lru_add_drain() if !CONFIG_NUMA.

It should drain on all CPUs though, I can't remember why it doesn't. Not that I disagree that throwing IPIs around is a hack ;)

Quoted 10 lines

Quoted 5 lines

Sigh, we're not getting there, are we?

I'm still thinking we add invalidate_complete_page2() to get us out of trouble and park the problem :(. That'd be a good approach for 2.6.18.x, which I assume is fairly urgent.

Choosing which fix to include is above my pay grade. Both of these proposals address the NFS readdir cache invalidation problem.

But it seems like there is a real problem here -- the pages that are waiting to be added the LRU will always have a page count that is too high for invalidate_inode_pages to work on them.

If you do the lru_add_drain_all, then the vmscan problem should be probably mostly fixable by detecting failure, waiting, and retrying a few times.

After that, making an invalidate_complete_page2 ignore the page count or dirty status would only save you from a very small number of cases, and they would be likely to be a data loss / corruption case.

OTOH, we haven't had many complaints before, so for 2.6.18, an invalidate_complete_page2 may indeed be the best option?

One option that hasn't been entertained is to remove the "batched LRU add" logic all together. Just gut lru_cache_add -- it should send pages immediately to be added to the LRU list. This is a bit slower, but it fixes the invalidation problems, and makes the icky lru_add_drain_all() a no-op.

Quoted 3 lines · Chuck Lever

One option that hasn't been entertained is to remove the "batched LRU
add" logic all together. Just gut lru_cache_add -- it should send
pages immediately to be added to the LRU list. This is a bit slower, but it fixes the invalidation problems, and makes the icky lru_add_drain_all() a no-op.

That would bring some larger machines to their knees contending on zone->lock.

Quoted 8 lines · Peter Zijlstra

Quoted 5 lines · Chuck Lever

One option that hasn't been entertained is to remove the "batched LRU add" logic all together. Just gut lru_cache_add -- it should send pages immediately to be added to the LRU list. This is a bit slower, but it fixes the invalidation problems, and makes the icky lru_add_drain_all() a no-op.

Quoted 3 lines

That would bring some larger machines to their knees contending on zone->lock.

Versus calling lru_add_drain_all() every time a file system tries to invalidate an inode's cached pages?

buggerit, let's do this. It'll fix NFS, yes?

From: Andrew Morton <[email protected]>

The recent fix to invalidate_inode_pages() (git commit 016eb4a) managed to unfix invalidate_inode_pages2().

The problem is that various bits of code in the kernel can take transient refs on pages: the page scanner will do this when inspecting a batch of pages, and the lru_cache_add() batching pagevecs also hold a ref.

Net result is transient failures in invalidate_inode_pages2(). This affects NFS directory invalidation (observed) and presumably also block-backed direct-io (not yet reported).

Fix it by reverting invalidate_inode_pages2() back to the old version which ignores the page refcounts.

We may come up with something more clever later, but for now we need a 2.6.18 fix for NFS.

Cc Chuck Lever <[email protected]>

Cc Nick Piggin <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc <[email protected]>

Signed-off-by Andrew Morton <[email protected]>

---

 mm/truncate.c |   34 ++++++++++++++++++++++++++++++++--
 1 files changed, 32 insertions(+), 2 deletions(-)

diff -puN mm/truncate.c~invalidate_inode_pages2-ignore-page-refcounts mm/truncate.c

mm/truncate.c renamed from mm/truncate.c~invalidate_inode_pages2-ignore-page-refcounts +32 -2

@@ -261,9 +261,39 @@ unsigned long invalidate_inode_pages(str
 {
 	return invalidate_mapping_pages(mapping, 0, ~0UL);
 }
-
 EXPORT_SYMBOL(invalidate_inode_pages);
 
+/*
+ * This is like invalidate_complete_page(), except it ignores the page's
+ * refcount.  We do this because invalidate_indoe_pages2() needs stronger
+ * invalidation guarantees, and cannot afford to leave pages behind because
+ * shrink_list() has a temp ref on them, or because they're transiently sitting
+ * in the lru_cache_add() pagevecs.
+ */
+static int
+invalidate_complete_page2(struct address_space *mapping, struct page *page)
+{
+	if (page->mapping != mapping)
+		return 0;
+
+	if (PagePrivate(page) && !try_to_release_page(page, 0))
+		return 0;
+
+	write_lock_irq(&mapping->tree_lock);
+	if (PageDirty(page))
+		goto failed;
+
+	BUG_ON(PagePrivate(page));
+	__remove_from_page_cache(page);
+	write_unlock_irq(&mapping->tree_lock);
+	ClearPageUptodate(page);
+	page_cache_release(page);	/* pagecache ref */
+	return 1;
+failed:
+	write_unlock_irq(&mapping->tree_lock);
+	return 0;
+}
+
 /**
  * invalidate_inode_pages2_range - remove range of pages from an address_space
  * @mapping: the address_space

@@ -330,7 +360,7 @@ int invalidate_inode_pages2_range(struct
 				}
 			}
 			was_dirty = test_clear_page_dirty(page);
-			if (!invalidate_complete_page(mapping, page)) {
+			if (!invalidate_complete_page2(mapping, page)) {
 				if (was_dirty)
 					set_page_dirty(page);
 				ret = -EIO;

_

Quoted 1 line · Andrew Morton

buggerit, let's do this. It'll fix NFS, yes?

It looks right to me.

I'll discuss a patch with Trond that adds a
warning in nfs_revalidate_mapping() and perhaps a performance counter to see how many times we hit this in practice.

Quoted 3 lines · On Fri, 29 Sep 2006 17:48:23 -0400 Chuck Lever <[email protected]> wrote:

Quoted 1 line · Andrew Morton

buggerit, let's do this. It'll fix NFS, yes?

It looks right to me.

s/right/less incorrect/ ;)

Please double-check that it passes testing.

Quoted 3 lines · On Fri, 29 Sep 2006 17:48:23 -0400 Chuck Lever <[email protected]> wrote:

Quoted 1 line · Andrew Morton

buggerit, let's do this. It'll fix NFS, yes?

It looks right to me.

s/right/less incorrect/ ;)

Please double-check that it passes testing.

Quoted 4 lines · Andrew Morton

Quoted 1 line · On Fri, 29 Sep 2006 17:48:23 -0400 Chuck Lever <[email protected]> wrote:

Andrew Morton wrote:

Quoted 6 lines

Quoted 2 lines

Quoted 1 line

buggerit, let's do this. It'll fix NFS, yes?

It looks right to me.

s/right/less incorrect/ ;)

Please double-check that it passes testing.

I've run this for 24 hours worth of continuous NFS testing. I haven't seen a problem with it.

Question: Maybe I missed this... but what is NFS suppose to do when invalidate_inode_pages2() fails on non-file inodes? Noting it with as metric as Chuck suggested is a good way to detect its happening, but does it make sense for NFS to keep calling invalidate_inode_pages2() until it does not fail when trying to flush the readdir cache?

Quoted 6 lines · Steve Dickson

Question: Maybe I missed this... but what is NFS suppose to do when invalidate_inode_pages2() fails on non-file inodes? Noting it with as metric as Chuck suggested is a good way to detect its happening, but does it make sense for NFS to keep calling invalidate_inode_pages2() until it does not fail when trying to flush the readdir cache?

Definitely not. There is not much we can do at the filesystem level if the VM fails, and that is why we haven't bothered checking the return value previously.

Quoted 7 lines · On Mon, 02 Oct 2006 09:25:22 -0400 Trond Myklebust…

Quoted 6 lines · Steve Dickson

Question: Maybe I missed this... but what is NFS suppose to do when invalidate_inode_pages2() fails on non-file inodes? Noting it with as metric as Chuck suggested is a good way to detect its happening, but does it make sense for NFS to keep calling invalidate_inode_pages2() until it does not fail when trying to flush the readdir cache?

Quoted 4 lines

Definitely not. There is not much we can do at the filesystem level if the VM fails, and that is why we haven't bothered checking the return value previously.

Please add a printk.

Quoted 10 lines · Andrew Morton

Quoted 7 lines · On Mon, 02 Oct 2006 09:25:22 -0400 Trond Myklebust…

Quoted 6 lines · Steve Dickson

Question: Maybe I missed this... but what is NFS suppose to do when invalidate_inode_pages2() fails on non-file inodes? Noting it with as metric as Chuck suggested is a good way to detect its happening, but does it make sense for NFS to keep calling invalidate_inode_pages2() until it does not fail when trying to flush the readdir cache?

Quoted 6 lines

Quoted 4 lines

Definitely not. There is not much we can do at the filesystem level if the VM fails, and that is why we haven't bothered checking the return value previously.

Please add a printk.

Please add a printk that can be turned off....

This is our user's data we're talking about here.

If that printk comes out then we need to fix the kernel so that it no longer wants to print that printk. We don't want to just hide it.

Quoted 2 lines · Andrew Morton

This is our user's data we're talking about here.

Point...

Quoted 3 lines

If that printk comes out then we need to fix the kernel so that it no longer wants to print that printk. We don't want to just hide it.

I'm concern about the printk popping when we are flushing the readdir cache (i.e. stale data) and either flooding the console to a ton a messages (basically bring a system to its knees for no good reason) or scaring the hell out people by saying we have a major problem when in reality we are just flushing stale data...

So I definitely agree the printk should be there and be on by default, but I so think it would be a good idea to have way to turn it off if need be...

Quoted 8 lines · Steve Dickson

Quoted 2 lines · Andrew Morton

This is our user's data we're talking about here.

Point...

Quoted 3 lines

If that printk comes out then we need to fix the kernel so that it no longer wants to print that printk. We don't want to just hide it.

Quoted 9 lines

I'm concern about the printk popping when we are flushing the readdir cache (i.e. stale data) and either flooding the console to a ton a messages (basically bring a system to its knees for no good reason) or scaring the hell out people by saying we have a major problem when in reality we are just flushing stale data...

So I definitely agree the printk should be there and be on by default, but I so think it would be a good idea to have way to turn it off if need be...

[ Sorry for the attachment... anyone know how to include a diff inline with Thunderbird? ]

The attached patch is my suggestion for reporting the cache invalidation failure from within the NFS client. Please review and comment. My testing with this patch applied has not triggered a single message, but I haven't tried any memory exhaustion scenarios.

I honestly doubt that we will see log floods. The original problem that was causing stale data to remain cached has been addressed. The reclaim race will almost certainly be rare.
plain text document attachment (nfs-check-return-codes.diff)
NFS: Add return code checks for page invalidation

Print a warning if the page invalidation functions don't behave as expected. A BUG is probably overkill here since the client's internal data structures will remain consistent.

We're trying to catch cases where invaliding an inode's page cache races with vmscan or direct I/O, resulting in stale data remaining in the page cache.

Signed-off-by Chuck Lever <[email protected]>

---

 fs/nfs/dir.c           |   34 +++++++++++++++++++++++++++++-----
 fs/nfs/direct.c        |    2 +-
 fs/nfs/inode.c         |   25 +++++++++++++++++++++++--
 fs/nfs/iostat.h        |    1 +
 include/linux/nfs_fs.h |    1 +
 5 files changed, 55 insertions(+), 8 deletions(-)

fs/nfs/dir.c +13 -0

index 7432f1a..0bb1a42 100644
@@ -156,6 +156,32 @@ typedef struct {
 	int		error;
 } nfs_readdir_descriptor_t;
 
+/*
+ * Trim off all pages past page zero.  This ensures consistent page
+ * alignment of cached data.
+ *
+ * NB: This assumes we have exclusive access to this mapping either
+ *     through inode->i_mutex or some other mechanism.
+ */
+static void nfs_truncate_directory_cache(struct inode *inode)
+{
+	int result;
+
+	dfprintk(DIRCACHE, "NFS: %s: truncating directory (%s/%Ld)\n",
+			__FUNCTION__, inode->i_sb->s_id,

fs/nfs/dir.c +6 -5

+	}
+}
+
 /* Now we cache directories properly, by stuffing the dirent
  * data directly in the page cache.
  *

@@ -199,12 +225,10 @@ int nfs_readdir_filler(nfs_readdir_descr
 	spin_lock(&inode->i_lock);
 	NFS_I(inode)->cache_validity |= NFS_INO_INVALID_ATIME;
 	spin_unlock(&inode->i_lock);
-	/* Ensure consistent page alignment of the data.
-	 * Note: assumes we have exclusive access to this mapping either
-	 *	 through inode->i_mutex or some other mechanism.
-	 */
+
 	if (page->index == 0)
-		invalidate_inode_pages2_range(inode->i_mapping, PAGE_CACHE_SIZE, -1);
+		nfs_truncate_directory_cache(inode);
+
 	unlock_page(page);
 	return 0;
  error:

fs/nfs/direct.c +0 -0

index 377839b..fe69c39 100644

fs/nfs/direct.c +0 -0

 	if (retval > 0)
 		iocb->ki_pos = pos + retval;

fs/nfs/inode.c +7 -0

index bc9376c..e1cf978 100644
@@ -657,6 +657,27 @@ int nfs_revalidate_inode(struct nfs_serv
 }
 
 /**
+ * nfs_invalidate_mapping - Invalidate the inode's page cache
+ * @inode - pointer to host inode
+ * @mapping - pointer to mapping
+ */
+void nfs_invalidate_mapping(struct inode *inode, struct address_space *mapping)
+{
+	int result;

fs/nfs/inode.c +6 -2

+	}
+}
+
+/**
  * nfs_revalidate_mapping - Revalidate the pagecache
  * @inode - pointer to host inode
  * @mapping - pointer to mapping

@@ -673,10 +694,10 @@ int nfs_revalidate_mapping(struct inode 
 		ret = __nfs_revalidate_inode(NFS_SERVER(inode), inode);
 
 	if (nfsi->cache_validity & NFS_INO_INVALID_DATA) {
-		nfs_inc_stats(inode, NFSIOS_DATAINVALIDATE);
 		if (S_ISREG(inode->i_mode))
 			nfs_sync_mapping(mapping);
-		invalidate_inode_pages2(mapping);
+
+		nfs_invalidate_mapping(inode, mapping);
 
 		spin_lock(&inode->i_lock);
 		nfsi->cache_validity &= ~NFS_INO_INVALID_DATA;

fs/nfs/iostat.h +1 -0

index 6350ecb..df41150 100644
@@ -104,6 +104,7 @@ enum nfs_stat_eventcounters {
 	NFSIOS_SHORTREAD,
 	NFSIOS_SHORTWRITE,
 	NFSIOS_DELAY,
+	NFSIOS_INVALIDATEFAILED,
 	__NFSIOS_COUNTSMAX,
 };
 

include/linux/nfs_fs.h +0 -0

index 98c9b9f..dc3cac3 100644

Quoted 4 lines · Chuck Lever

Quoted 3 lines · Steve Dickson

Quoted 2 lines · Andrew Morton

This is our user's data we're talking about here.

Quoted 5 lines

Quoted 5 lines

Point...

Quoted 3 lines

If that printk comes out then we need to fix the kernel so that it no longer wants to print that printk. We don't want to just hide it.

So what _is_ stopping us from fixing it right now? Are we missing an audit of the possible errors? That can be arranged...

Quoted 9 lines

Quoted 9 lines

I'm concern about the printk popping when we are flushing the readdir cache (i.e. stale data) and either flooding the console to a ton a messages (basically bring a system to its knees for no good reason) or scaring the hell out people by saying we have a major problem when in reality we are just flushing stale data...

So I definitely agree the printk should be there and be on by default, but I so think it would be a good idea to have way to turn it off if need be...

Why? If we know there is a problem, then why wait to fix it?

Quoted 21 lines

fs/nfs/dir.c +13 -0

index 7432f1a..0bb1a42 100644
@@ -156,6 +156,32 @@ typedef struct {
 	int		error;
 } nfs_readdir_descriptor_t;
 
+/*
+ * Trim off all pages past page zero.  This ensures consistent page
+ * alignment of cached data.
+ *
+ * NB: This assumes we have exclusive access to this mapping either
+ *     through inode->i_mutex or some other mechanism.
+ */
+static void nfs_truncate_directory_cache(struct inode *inode)
+{
+	int result;
+
+	dfprintk(DIRCACHE, "NFS: %s: truncating directory (%s/%Ld)\n",
+			__FUNCTION__, inode->i_sb->s_id,

fs/nfs/dir.c typedef struct { · line 181

Quoted 10 lines

fs/nfs/dir.c +10 -0

+			(long long)NFS_FILEID(inode));
+
+	result = invalidate_inode_pages2_range(inode->i_mapping,
+							PAGE_CACHE_SIZE, -1);
+	if (unlikely(result < 0)) {
+		nfs_inc_stats(inode, NFSIOS_INVALIDATEFAILED);
+		printk(KERN_ERR
+			"NFS: error %d invalidating cache for dir (%s/%Ld)\n",
+				result, inode->i_sb->s_id,
+				(long long)NFS_FILEID(inode));

See gripe below.

Quoted 28 lines

fs/nfs/dir.c +6 -5

+	}
+}
+
 /* Now we cache directories properly, by stuffing the dirent
  * data directly in the page cache.
  *

@@ -199,12 +225,10 @@ int nfs_readdir_filler(nfs_readdir_descr
 	spin_lock(&inode->i_lock);
 	NFS_I(inode)->cache_validity |= NFS_INO_INVALID_ATIME;
 	spin_unlock(&inode->i_lock);
-	/* Ensure consistent page alignment of the data.
-	 * Note: assumes we have exclusive access to this mapping either
-	 *	 through inode->i_mutex or some other mechanism.
-	 */
+
 	if (page->index == 0)
-		invalidate_inode_pages2_range(inode->i_mapping, PAGE_CACHE_SIZE, -1);
+		nfs_truncate_directory_cache(inode);
+
 	unlock_page(page);
 	return 0;
  error:

fs/nfs/direct.c +0 -0

index 377839b..fe69c39 100644
@@ -823,7 +823,7 @@ ssize_t nfs_file_direct_write(struct kio
 	 *      occur before the writes complete.  Kind of racey.

fs/nfs/direct.c ssize_t nfs_file_direct_write(struct kio · line 826

Quoted 4 lines

fs/nfs/direct.c +1 -1

 	 */
 	if (mapping->nrpages)
-		invalidate_inode_pages2(mapping);
+		nfs_invalidate_mapping(mapping->host, mapping);

This looks wrong. Why are we bumping the NFSIOS_DATAINVALIDATE counter on a direct write? We're not registering a cache consistency problem here.

Quoted 23 lines

fs/nfs/direct.c +0 -0

 	if (retval > 0)
 		iocb->ki_pos = pos + retval;

fs/nfs/inode.c +12 -0

index bc9376c..e1cf978 100644
@@ -657,6 +657,27 @@ int nfs_revalidate_inode(struct nfs_serv
 }
 
 /**
+ * nfs_invalidate_mapping - Invalidate the inode's page cache
+ * @inode - pointer to host inode
+ * @mapping - pointer to mapping
+ */
+void nfs_invalidate_mapping(struct inode *inode, struct address_space *mapping)
+{
+	int result;
+
+	nfs_inc_stats(inode, NFSIOS_DATAINVALIDATE);
+
+	result = invalidate_inode_pages2(mapping);
+	if (unlikely(result) < 0) {

fs/nfs/inode.c int nfs_revalidate_inode(struct nfs_serv · line 676

Quoted 5 lines

fs/nfs/inode.c +5 -0

+		nfs_inc_stats(inode, NFSIOS_INVALIDATEFAILED);
+		printk(KERN_ERR
+			"NFS: error %d invalidating pages for inode (%s/%Ld)\n",
+				result, inode->i_sb->s_id,
+				(long long)NFS_FILEID(inode));

So what _are_ users expected to do about this? Sue us? Complain bitterly to lkml, and then get told that the VM is broken?

Quoted 6 lines

Quoted 6 lines

fs/nfs/direct.c +0 -0

index 377839b..fe69c39 100644
@@ -823,7 +823,7 @@ ssize_t nfs_file_direct_write(struct kio
 	 *      occur before the writes complete.  Kind of racey.

Quoted 8 lines

fs/nfs/direct.c ssize_t nfs_file_direct_write(struct kio · line 826

Quoted 4 lines

fs/nfs/direct.c +1 -1

 	 */
 	if (mapping->nrpages)
-		invalidate_inode_pages2(mapping);
+		nfs_invalidate_mapping(mapping->host, mapping);

This looks wrong. Why are we bumping the NFSIOS_DATAINVALIDATE counter on a direct write? We're not registering a cache consistency problem here.

We're looking for potential races between direct I/O and cache invalidation, among others. Is your concern that this may report false positives?

I'm not sure this invalidation is useful in any event. Direct writes are treated like some other client has modified the file, so cached pages will get invalidated eventually anyway. Maybe we should just remove this one?

Quoted 10 lines

fs/nfs/direct.c ssize_t nfs_file_direct_write(struct kio

Quoted 7 lines

fs/nfs/direct.c +7 -0

+	result = invalidate_inode_pages2(mapping);
+	if (unlikely(result) < 0) {
+		nfs_inc_stats(inode, NFSIOS_INVALIDATEFAILED);
+		printk(KERN_ERR
+			"NFS: error %d invalidating pages for inode (%s/%Ld)\n",
+				result, inode->i_sb->s_id,
+				(long long)NFS_FILEID(inode));

So what _are_ users expected to do about this? Sue us? Complain bitterly to lkml, and then get told that the VM is broken?

Such a message will be reported to distributors or lkml, and we will be able to collect data about the scenario where there is a problem.

Another option for customers is to run application-level data consistency checks when this error is reported.

Quoted 13 lines · Chuck Lever

Quoted 13 lines

fs/nfs/direct.c ssize_t nfs_file_direct_write(struct kio · line 826

Quoted 10 lines

fs/nfs/direct.c +1 -1

index 377839b..fe69c39 100644
@@ -823,7 +823,7 @@ ssize_t nfs_file_direct_write(struct kio
 	 *      occur before the writes complete.  Kind of racey.
 	 */
 	if (mapping->nrpages)
-		invalidate_inode_pages2(mapping);
+		nfs_invalidate_mapping(mapping->host, mapping);

This looks wrong. Why are we bumping the NFSIOS_DATAINVALIDATE counter on a direct write? We're not registering a cache consistency problem here.

Quoted 1 line

We're looking for potential races between direct I/O and cache invalidation, among others. Is your concern that this may report false positives?

No. I simply don't see what the use case is for this statistic. AFAICS it is purely a debugging tool for _developers_. That would have absolutely no place at all in /proc/self/mountstats, which is supposed to provide useful statistics for _administrators_.

Quoted 1 line

I'm not sure this invalidation is useful in any event. Direct writes are treated like some other client has modified the file, so cached pages will get invalidated eventually anyway. Maybe we should just remove this one?

That would break the principle that if one process modifies the file, then all processes on the same client will immediately see those changes.

Quoted 11 lines

Quoted 10 lines

fs/nfs/inode.c int nfs_revalidate_inode(struct nfs_serv · line 676

Quoted 7 lines

fs/nfs/inode.c +7 -0

+	result = invalidate_inode_pages2(mapping);
+	if (unlikely(result) < 0) {
+		nfs_inc_stats(inode, NFSIOS_INVALIDATEFAILED);
+		printk(KERN_ERR
+			"NFS: error %d invalidating pages for inode (%s/%Ld)\n",
+				result, inode->i_sb->s_id,
+				(long long)NFS_FILEID(inode));

So what _are_ users expected to do about this? Sue us? Complain bitterly to lkml, and then get told that the VM is broken?

Quoted 2 lines

Such a message will be reported to distributors or lkml, and we will be able to collect data about the scenario where there is a problem.

i.e. we're throwing our hands up into the air, and saying "we don't understand what is going on here"?

Sorry, but that is not an option. If we can't design a clearly defined API for cache invalidation, with well defined errors, then we should all go home and start learning Solaris programming now.

Quoted 13 lines · On Tue, 03 Oct 2006 15:10:01 -0400 Trond Myklebust…

Quoted 10 lines

Quoted 9 lines

fs/nfs/inode.c int nfs_revalidate_inode(struct nfs_serv · line 676

Quoted 7 lines

fs/nfs/inode.c +7 -0

+	result = invalidate_inode_pages2(mapping);
+	if (unlikely(result) < 0) {
+		nfs_inc_stats(inode, NFSIOS_INVALIDATEFAILED);
+		printk(KERN_ERR
+			"NFS: error %d invalidating pages for inode (%s/%Ld)\n",
+				result, inode->i_sb->s_id,
+				(long long)NFS_FILEID(inode));

So what _are_ users expected to do about this? Sue us? Complain bitterly to lkml, and then get told that the VM is broken?

Such a message will be reported to distributors or lkml, and we will be able to collect data about the scenario where there is a problem.

i.e. we're throwing our hands up into the air, and saying "we don't understand what is going on here"?

Quoted 3 lines

Sorry, but that is not an option. If we can't design a clearly defined API for cache invalidation, with well defined errors, then we should all go home and start learning Solaris programming now.

I don't think we need the stats increment in there. We could make it a plain WARN_ON() or BUG_ON(). Or just whack a printk in there. It's hardly a big deal.

Quoted 21 lines · Andrew Morton

Quoted 17 lines · On Tue, 03 Oct 2006 15:10:01 -0400 Trond Myklebust…

Quoted 11 lines

Quoted 9 lines

fs/nfs/inode.c int nfs_revalidate_inode(struct nfs_serv · line 676

Quoted 7 lines

fs/nfs/inode.c +7 -0

+	result = invalidate_inode_pages2(mapping);
+	if (unlikely(result) < 0) {
+		nfs_inc_stats(inode, NFSIOS_INVALIDATEFAILED);
+		printk(KERN_ERR
+			"NFS: error %d invalidating pages for inode (%s/%Ld)\n",
+				result, inode->i_sb->s_id,
+				(long long)NFS_FILEID(inode));

So what _are_ users expected to do about this? Sue us? Complain bitterly to lkml, and then get told that the VM is broken?

Such a message will be reported to distributors or lkml, and we will be able to collect data about the scenario where there is a problem.

i.e. we're throwing our hands up into the air, and saying "we don't understand what is going on here"?

Sorry, but that is not an option. If we can't design a clearly defined API for cache invalidation, with well defined errors, then we should all go home and start learning Solaris programming now.

Quoted 3 lines

I don't think we need the stats increment in there. We could make it a plain WARN_ON() or BUG_ON(). Or just whack a printk in there. It's hardly a big deal.

A "WARN_ON(ret != 0);" placed at the end of invalidate_inode_pages2_range() should be sufficient and harmless.

Quoted 1 line

A "WARN_ON(ret != 0);" placed at the end of invalidate_inode_pages2_range() should be sufficient and harmless.

true. I think. I'll take a look at adding a WARN_ON_ONCE().

Quoted 19 lines · On Thu, 28 Sep 2006 12:26:36 -0400 Chuck Lever <[email protected]> wrote:

Hi Nick-

Quoted 16 lines · Nick Piggin

Quoted 11 lines

The page references I'm noting are:

1. add_to_page_cache_lru 1 -> 2

2. do_generic_mapping_read, released in nfs_do_filldir

3. read_cache_page, released in nfs_readdir

4. pagevec_lookup (from invalidate_inode_pages2_range) 2 -> 3

The odd thing is some of the pages for the directory are released, so they must have a ref count of 2 *after* the pagevec_lookup. Looks like more investigation is needed.

Well they could get flushed out of the lru add pagevecs which drops their count. Just to test that theory you could run lru_add_drain_all() at the start of the invalidate function. That's not going to actually fix the problem, but it might help 2.6.18 limp along when combined with some other work.

Quoted 3 lines

I've verified that the pages that fail to be invalidated are indeed languishing in the per-CPU lru-add pagevec. Adding a call to lru_add_drain_all() to the invalidate_inode_pages2() path does fix the NFS readdir problem.

I think a call to lru_add_drain_all() belongs in both the invalidate_inode_pages() and the invalidate_inode_pages2() path. Do you agree?

Yes. But the page-pinned-by-vmscan scenario which you earlier identified can still happen, I believe.

Quoted 2 lines · Nick Piggin

But they're present on the LRU? That's unusual (I guess NFS doesn't have a buffer cache for a backing block device).

That is correct -- NFS doesn't use the buffer cache.

Quoted 6 lines · Chuck Lever

Quoted 2 lines · Nick Piggin

But they're present on the LRU? That's unusual (I guess NFS doesn't have a buffer cache for a backing block device).

Quoted 1 line

That is correct -- NFS doesn't use the buffer cache.

So that raises another question: how do they get to invalidate_inode_pages2 if they are not part of the buffer or pagecache?

Quoted 2 lines · Nick Piggin

So that raises another question: how do they get to invalidate_inode_pages2 if they are not part of the buffer or pagecache?

It does use the page cache to cache data pages for files, directories, and symlinks. It does not use buffers, however, since incoming file system data is read from a socket, not from a block device. I believe the client provides a dummy backing device for the few things in the VFS that require it.

Invalidate_inode_pages2() is used to remove page cache data that the client has determined is stale. The client detects that the file has changed on the server, and it is not responsible for those changes, by examining the file's attributes and noticing mtime or size changes. When such a change is detected, all pages cached for a file are invalidated, and the page cache is gradually repopulated from the server as applications access parts of the file.

I'd like to understand the difference between invalidate_inode_pages2() and truncate_inode_pages() in this scenario.

truncate_inode_pages will a) throw away everything including dirty pages, and b) probably be fairly
racy unless the inode's i_size (for normal files) is modified.

We really want to make an invalidation that works properly for you.

If you can guarantee that a pagecache page can never get mapped to a user mapping (eg. perhaps for
directories and symlinks) and also ensure that you don't dirty it via the filesystem, then you don't
have to worry about it becoming dirty, so we can skip the checks Andrew has added and maybe add a
WARN_ON(PageDirty()).

Quoted 5 lines · On Wed, 27 Sep 2006 14:47:26 +1000 Nick Piggin <[email protected]> wrote:

truncate_inode_pages will a) throw away everything including dirty pages, and b) probably be fairly racy unless the inode's i_size (for normal files) is modified.

We really want to make an invalidation that works properly for you.

Quoted 5 lines

If you can guarantee that a pagecache page can never get mapped to a user mapping (eg. perhaps for directories and symlinks) and also ensure that you don't dirty it via the filesystem, then you don't have to worry about it becoming dirty, so we can skip the checks Andrew has added and maybe add a WARN_ON(PageDirty()).

None of that is true for when invalidate_inode_pages2() is used by block-backed direct-io.

We should fix it for that application..

Quoted 19 lines · Andrew Morton

Quoted 6 lines · On Wed, 27 Sep 2006 14:47:26 +1000 Nick Piggin <[email protected]> wrote:

truncate_inode_pages will a) throw away everything including dirty pages, and b) probably be fairly
racy unless the inode's i_size (for normal files) is modified.

We really want to make an invalidation that works properly for you.

Quoted 8 lines

If you can guarantee that a pagecache page can never get mapped to a user mapping (eg. perhaps for
directories and symlinks) and also ensure that you don't dirty it via the filesystem, then you don't
have to worry about it becoming dirty, so we can skip the checks Andrew has added and maybe add a
WARN_ON(PageDirty()).

Quoted 3 lines

None of that is true for when invalidate_inode_pages2() is used by block-backed direct-io.

Sure, you wouldn't be able to skip those checks for direct IO, just for the NFS usage.

Quoted 2 lines

We should fix it for that application..

I thought direct IO fell back to buffered IO when the teardown failed.

I
was surprised that it didn't and gave me errors when I was testing. Do you see a better way to handle the direct IO case?

Quoted 2 lines · Chuck Lever

Invalidate_inode_pages2() is used to remove page cache data that the client has determined is stale. The client detects that the file has changed on the server, and it is not responsible for those changes, by examining the file's attributes and noticing mtime or size changes. When such a change is detected, all pages cached for a file are invalidated, and the page cache is gradually repopulated from the server as applications access parts of the file.

If you can guarantee that a pagecache page can never get mapped to a user mapping (eg. perhaps for directories and symlinks) and also ensure that you don't dirty it via the filesystem, then you don't have to worry about it becoming dirty, so we can skip the checks Andrew has added and maybe add a WARN_ON(PageDirty()).

Now that won't help you for regular file pages that can be mmapped. For those, we need to ensure that the page isn't mapped, and the page will not be dirtied via a get_user_pages user, and you need to ensure that it can't get dirtied via the filesystem. The former two will require VM changes of the scale that aren't going to get into 2.6.19, but I'm working on them.

For now, can make do with flushing lru pagevecs, and also testing and retrying in the caller?

I think invalidate_inode_pages2() is sufficiently different from (ie:
stronger than) invalidate_inode_pages() to justify the addition of a new invalidate_complete_page2(), which skips the page refcount check.