Re: Review: fix mapping invalidation callouts

[David Chatterton <chatz@melbourne.sgi.com>]

David Chinner wrote:
> On Wed, Jan 10, 2007 at 08:39:33AM +0000, Lachlan McIlroy wrote:
>> 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.
> 
> Yeah, you'd think so. The first xfsqa run I do -after- checking it in
> (been running for 24 hours) I get a stack dump with the warning
> in cancel_dirty_page(), so clearly this isn't right either. I'm
> not sure WTF is going on here.
> 
> Chatz, don't push that mod yet....
> 

Ack.

Lets get Donald to pull 2.6.20-rc into 2.6.x-xfs, or do we need to wait
until you have this fixed?

David

-- 
David Chatterton
XFS Engineering Manager
SGI Australia