Re: Review: fix mapping invalidation callouts

[Lachlan McIlroy <lachlan@sgi.com>]

David Chinner wrote:
> On Mon, Jan 08, 2007 at 03:03:09PM +1100, David Chinner wrote:
> 
>>With the recent cancel_dirty_page() changes, a warning was
>>added if we cancel a dirty page that is still mapped into
>>the page tables.
>>This happens in XFS from fs_tosspages() and fs_flushinval_pages()
>>because they call truncate_inode_pages().
>>
>>truncate_inode_pages() does not invalidate existing page mappings;
>>it is expected taht this is called only when truncating the file
>>or destroying the inode and on both these cases there can be
>>no mapped ptes. However, we call this when doing direct I/O writes
>>to remove pages from the page cache. As a result, we can rip
>>a page from the page cache that still has mappings attached.
>>
>>The correct fix is to use invalidate_inode_pages2_range() instead
>>of truncate_inode_pages(). They essentially do the same thing, but
>>the former also removes any pte mappings before removing the page
>>from the page cache.
>>
>>Comments?
>>
>>Cheers,
>>
>>Dave.
>>-- 
>>Dave Chinner
>>Principal Engineer
>>SGI Australian Software Group
>>
>>
>>---
>> fs/xfs/linux-2.6/xfs_fs_subr.c |   10 ++++++++--
>> 1 file changed, 8 insertions(+), 2 deletions(-)
>>
>>Index: 2.6.x-xfs-new/fs/xfs/linux-2.6/xfs_fs_subr.c
>>===================================================================
>>--- 2.6.x-xfs-new.orig/fs/xfs/linux-2.6/xfs_fs_subr.c	2006-12-12 12:05:17.000000000 +1100
>>+++ 2.6.x-xfs-new/fs/xfs/linux-2.6/xfs_fs_subr.c	2007-01-08 09:30:22.056571711 +1100
>>@@ -21,6 +21,8 @@ int  fs_noerr(void) { return 0; }
>> int  fs_nosys(void) { return ENOSYS; }
>> void fs_noval(void) { return; }
>> 
>>+#define XFS_OFF_TO_PCSIZE(off)	\
>>+	(((off) + PAGE_CACHE_SIZE - 1) >> PAGE_CACHE_SHIFT)
> 
> 
> I don't think this is right.
> 
> Assuming 4k page size, first = 2k, last = 6k will result in
> invalidating page indexes 1 and 2 i.e. offset 4k -> 12k. In fact,
> we want to invalidate pages 0 and 1.
> 
> IOWs, I think it should be:
> 
> +#define XFS_OFF_TO_PCINDEX(off)	((off) >> PAGE_CACHE_SHIFT)
> 
> Comments?
> 

Makes sense to me.