Review: fix mapping invalidation callouts

Review: fix mapping invalidation callouts

[David Chinner]

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)
 void
 fs_tosspages(
 	bhv_desc_t	*bdp,
@@ -32,7 +34,9 @@ fs_tosspages(
 	struct inode	*ip = vn_to_inode(vp);
 
 	if (VN_CACHED(vp))
-		truncate_inode_pages(ip->i_mapping, first);
+		invalidate_inode_pages2_range(ip->i_mapping,
+					XFS_OFF_TO_PCSIZE(first),
+					XFS_OFF_TO_PCSIZE(last));
 }
 
 void
@@ -49,7 +53,9 @@ fs_flushinval_pages(
 		if (VN_TRUNC(vp))
 			VUNTRUNCATE(vp);
 		filemap_write_and_wait(ip->i_mapping);
-		truncate_inode_pages(ip->i_mapping, first);
+		invalidate_inode_pages2_range(ip->i_mapping,
+					XFS_OFF_TO_PCSIZE(first),
+					XFS_OFF_TO_PCSIZE(last));
 	}
 }
 

Re: Review: fix mapping invalidation callouts

[Christoph Hellwig]

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?

Generally looks good.  But I feel a little cautios about changes in this
area, so we should throw all possible test loads at this before commiting
it.

Re: Review: fix mapping invalidation callouts

[David Chinner]

On Mon, Jan 08, 2007 at 09:09:16AM +0000, Christoph Hellwig 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?
> 
> Generally looks good.  But I feel a little cautios about changes in this
> area, so we should throw all possible test loads at this before commiting
> it.

Yup - fsx is one test that I really want to hit with this. The guy that
reported the initial problem has replied saying this patch fixes the
warnings (good start ;), but I'll hold off pushing it for a little
while to test it more. This (or something like it) will need to go
into 2.6.20 before it is released so we've got limited time to
test this one out....

Cheers,

Dave.
-- 
Dave Chinner
Principal Engineer
SGI Australian Software Group

Re: Review: fix mapping invalidation callouts

[Lachlan McIlroy]

David Chinner wrote:
> On Mon, Jan 08, 2007 at 09:09:16AM +0000, Christoph Hellwig 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?
>>
>>Generally looks good.  But I feel a little cautios about changes in this
>>area, so we should throw all possible test loads at this before commiting
>>it.
> 
> 
> Yup - fsx is one test that I really want to hit with this. The guy that
> reported the initial problem has replied saying this patch fixes the
> warnings (good start ;), but I'll hold off pushing it for a little
> while to test it more. This (or something like it) will need to go
> into 2.6.20 before it is released so we've got limited time to
> test this one out....
> 

This patch fixes fs_tosspages() and fs_flushinval_pages() but will a
call to fs_flush_pages() with flags including B_INVAL work correctly?
I can't see any code that passes B_INVAL into fs_flush_pages() but it
should probably support it.

Re: Review: fix mapping invalidation callouts

[David Chinner]

On Tue, Jan 09, 2007 at 11:57:38AM +0000, Lachlan McIlroy wrote:
> 
> This patch fixes fs_tosspages() and fs_flushinval_pages() but will a
> call to fs_flush_pages() with flags including B_INVAL work correctly?

By definition fs_flush_pages() only flushes pages. If you need
to flush and invalidate pages, you use fs_flushinval_pages().

Passing B_INVAL to fs_flush_pages() is broken code.

> I can't see any code that passes B_INVAL into fs_flush_pages()

good ;)

Cheers,

Dave.
-- 
Dave Chinner
Principal Engineer
SGI Australian Software Group

Re: Review: fix mapping invalidation callouts

[David Chinner]

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?

Cheers,

Dave.
-- 
Dave Chinner
Principal Engineer
SGI Australian Software Group

Re: Review: fix mapping invalidation callouts

[Lachlan McIlroy]

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.

Re: Review: fix mapping invalidation callouts

[David Chinner]

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....

Cheers,

Dave.
-- 
Dave Chinner
Principal Engineer
SGI Australian Software Group

Re: Review: fix mapping invalidation callouts

[David Chinner]

On Thu, Jan 11, 2007 at 05:49:58PM +1100, David Chinner wrote:
> On Wed, Jan 10, 2007 at 08:39:33AM +0000, Lachlan McIlroy wrote:
> > David Chinner wrote:
> > >>+#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.

Of course, I just realised that this is 2.6.19 that I'm testing
on (fmeh) and so the code is different - cancel-dirty_page()
doesn't exist in this tree, and the warning is coming from
invalidate_inode_pages2_range() because invalidate_complete_page2()
is returning an error for some reason.....

Looks like it's a partial page truncation problem.
invalidate_inode_pages2_range() fails on partial page truncation
when part of the page (i.e. a bufferhead) is dirty.

This looks like a _big_ mess.

Cheers,

Dave.
-- 
Dave Chinner
Principal Engineer
SGI Australian Software Group

Re: Review: fix mapping invalidation callouts

[David Chatterton]

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