Btrfs devel - Feb 2010 - [PATCH 1/1] mm: invalidate_mapping_pages checks boundaries when lock fails

Not sure that I''m not missing something obvious. When
invalidate_mapping_pages
fails to lock the page, we continue to the next iteration, skipping the
next > end check. This can lead to a case where we invalidate a page that is
beyond the requested boundaries. Currently there are two callers that might be
affected, one is btrfs and the second one is the fadvice syscall.
Does that look right, or am I just missing something?

------

[PATCH 1/1] mm: invalidate_mapping_pages checks boundaries when lock fails

When we failed to lock the page, we continued to the next
iteration, skipping the next > end check. This might cause
throwing away a page that is beyond the requested boundaries.

Signed-off-by: Yehuda Sadeh <yehuda@hq.newdream.net>
---
 mm/truncate.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/mm/truncate.c b/mm/truncate.c
index 450cebd..abb67d4 100644
--- a/mm/truncate.c
+++ b/mm/truncate.c
@@ -345,11 +345,12 @@ unsigned long invalidate_mapping_pages(struct
address_space *mapping,
 				next = index;
 			next++;
 			if (lock_failed)
-				continue;
+				goto unlocked;
 
 			ret += invalidate_inode_page(page);
 
 			unlock_page(page);
+unlocked:
 			if (next > end)
 				break;
 		}
-- 
1.5.6.5

--
To unsubscribe, send a message with ''unsubscribe linux-mm'' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don''t email: <a href=mailto:"dont@kvack.org">
email@kvack.org </a>

Hi,

On Thu, Feb 18, 2010 at 05:22:17PM -0800, Yehuda Sadeh
wrote:
> Not sure that I''m not missing something obvious. When
invalidate_mapping_pages
> fails to lock the page, we continue to the next iteration, skipping the
> next > end check. This can lead to a case where we invalidate a page
that is
> beyond the requested boundaries. Currently there are two callers that might
be
> affected, one is btrfs and the second one is the fadvice syscall.
> Does that look right, or am I just missing something?
This can already happen with the first page being at an index above end
as the check only happens after we invalidated the page.

The damage is losing one cache-only (clean, unmapped) page.  It is a bit
ugly but not a huge problem I suppose.

How about checking page->index against end, like in the truncation case,
before the invalidation?  That should take care of both cases.

We already rely on a page->index when the page is pinned but locked by
somebody else.  And I think that''s fine.

Can we not just make that the default?  That could simplify the inner
loop to something like

	index = page->index;
	if (index > end)
		break;
	next = max(index, next) + 1;
	if (!trylock_page(page))
		continue;
	ret += invalidate_inode_page(page);
	unlock_page(page);

or something.

	Hannes

--
To unsubscribe, send a message with ''unsubscribe linux-mm'' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don''t email: <a href=mailto:"dont@kvack.org">
email@kvack.org </a>