[PATCH] mm: Remove redundant test from find_get_pages_contig

[PATCH] mm: Remove redundant test from find_get_pages_contig

[Matthew Wilcox]

After we establish a reference on the page, we check the pointer continues
to be in the correct position in i_pages.  There's no need to check the
page->mapping or page->index afterwards; if those can change after we've
got the reference, they can change after we return the page to the caller.

Signed-off-by: Matthew Wilcox <willy@infradead.org>
---
 mm/filemap.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/mm/filemap.c b/mm/filemap.c
index 9f5e323e883e6..935fbc29aeb13 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -1837,16 +1837,6 @@ unsigned find_get_pages_contig(struct address_space *mapping, pgoff_t index,
 		if (unlikely(page != xas_reload(&xas)))
 			goto put_page;
 
-		/*
-		 * must check mapping and index after taking the ref.
-		 * otherwise we can get both false positives and false
-		 * negatives, which is just confusing to the caller.
-		 */
-		if (!page->mapping || page_to_pgoff(page) != xas.xa_index) {
-			put_page(page);
-			break;
-		}
-
 		pages[ret] = page;
 		if (++ret == nr_pages)
 			break;
-- 
2.20.1

Re: [PATCH] mm: Remove redundant test from find_get_pages_contig

[Andrew Morton]

On Mon,  7 Jan 2019 12:02:24 -0800 Matthew Wilcox <willy@infradead.org> wrote:

> After we establish a reference on the page, we check the pointer continues
> to be in the correct position in i_pages.  There's no need to check the
> page->mapping or page->index afterwards; if those can change after we've
> got the reference, they can change after we return the page to the caller.

But that isn't what the comment says.

> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -1837,16 +1837,6 @@ unsigned find_get_pages_contig(struct address_space *mapping, pgoff_t index,
>  		if (unlikely(page != xas_reload(&xas)))
>  			goto put_page;
>  
> -		/*
> -		 * must check mapping and index after taking the ref.
> -		 * otherwise we can get both false positives and false
> -		 * negatives, which is just confusing to the caller.
> -		 */
> -		if (!page->mapping || page_to_pgoff(page) != xas.xa_index) {
> -			put_page(page);
> -			break;
> -		}

The assertion here is that the page's state can alter before we take
the ref but not afterwards.  Which is contrary to your assertion that
"they can change after we return the page to the caller".

This:

commit 9cbb4cb21b19fff46cf1174d0ed699ef710e641c
Author:     Nick Piggin <npiggin@kernel.dk>
AuthorDate: Thu Jan 13 15:45:51 2011 -0800
Commit:     Linus Torvalds <torvalds@linux-foundation.org>
CommitDate: Thu Jan 13 17:32:32 2011 -0800

    mm: find_get_pages_contig fixlet
    
    Testing ->mapping and ->index without a ref is not stable as the page
    may have been reused at this point.
    
    Signed-off-by: Nick Piggin <npiggin@kernel.dk>
    Reviewed-by: Wu Fengguang <fengguang.wu@intel.com>
    Reviewed-by: Minchan Kim <minchan.kim@gmail.com>
    Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
    Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>

diff --git a/mm/filemap.c b/mm/filemap.c
index ca389394fa2a..1a3dd5914726 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -837,9 +837,6 @@ unsigned find_get_pages_contig(struct address_space *mapping, pgoff_t index,
 		if (radix_tree_deref_retry(page))
 			goto restart;
 
-		if (page->mapping == NULL || page->index != index)
-			break;
-
 		if (!page_cache_get_speculative(page))
 			goto repeat;
 
@@ -849,6 +846,16 @@ unsigned find_get_pages_contig(struct address_space *mapping, pgoff_t index,
 			goto repeat;
 		}
 
+		/*
+		 * must check mapping and index after taking the ref.
+		 * otherwise we can get both false positives and false
+		 * negatives, which is just confusing to the caller.
+		 */
+		if (page->mapping == NULL || page->index != index) {
+			page_cache_release(page);
+			break;
+		}
+
 		pages[ret] = page;
 		ret++;
 		index++;

Re: [PATCH] mm: Remove redundant test from find_get_pages_contig

[Matthew Wilcox]

On Mon, Jan 07, 2019 at 02:33:19PM -0800, Andrew Morton wrote:
> On Mon,  7 Jan 2019 12:02:24 -0800 Matthew Wilcox <willy@infradead.org> wrote:
> 
> > After we establish a reference on the page, we check the pointer continues
> > to be in the correct position in i_pages.  There's no need to check the
> > page->mapping or page->index afterwards; if those can change after we've
> > got the reference, they can change after we return the page to the caller.
> 
> But that isn't what the comment says.

Right.  That patch from Nick moved the check from before taking the
ref to after taking the ref.  It was racy to have it before.  But it's
unnecessary to have it afterwards -- pages can't move once there's a
ref on them.  Or if they can move, they can move after the ref is taken.

> > --- a/mm/filemap.c
> > +++ b/mm/filemap.c
> > @@ -1837,16 +1837,6 @@ unsigned find_get_pages_contig(struct address_space *mapping, pgoff_t index,
> >  		if (unlikely(page != xas_reload(&xas)))
> >  			goto put_page;
> >  
> > -		/*
> > -		 * must check mapping and index after taking the ref.
> > -		 * otherwise we can get both false positives and false
> > -		 * negatives, which is just confusing to the caller.
> > -		 */
> > -		if (!page->mapping || page_to_pgoff(page) != xas.xa_index) {
> > -			put_page(page);
> > -			break;
> > -		}
> 
> The assertion here is that the page's state can alter before we take
> the ref but not afterwards.  Which is contrary to your assertion that
> "they can change after we return the page to the caller".
> 
> This:
> 
> commit 9cbb4cb21b19fff46cf1174d0ed699ef710e641c
> Author:     Nick Piggin <npiggin@kernel.dk>
> AuthorDate: Thu Jan 13 15:45:51 2011 -0800
> Commit:     Linus Torvalds <torvalds@linux-foundation.org>
> CommitDate: Thu Jan 13 17:32:32 2011 -0800
> 
>     mm: find_get_pages_contig fixlet
>     
>     Testing ->mapping and ->index without a ref is not stable as the page
>     may have been reused at this point.
>     
>     Signed-off-by: Nick Piggin <npiggin@kernel.dk>
>     Reviewed-by: Wu Fengguang <fengguang.wu@intel.com>
>     Reviewed-by: Minchan Kim <minchan.kim@gmail.com>
>     Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
>     Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
> 
> diff --git a/mm/filemap.c b/mm/filemap.c
> index ca389394fa2a..1a3dd5914726 100644
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -837,9 +837,6 @@ unsigned find_get_pages_contig(struct address_space *mapping, pgoff_t index,
>  		if (radix_tree_deref_retry(page))
>  			goto restart;
>  
> -		if (page->mapping == NULL || page->index != index)
> -			break;
> -
>  		if (!page_cache_get_speculative(page))
>  			goto repeat;
>  
> @@ -849,6 +846,16 @@ unsigned find_get_pages_contig(struct address_space *mapping, pgoff_t index,
>  			goto repeat;
>  		}
>  
> +		/*
> +		 * must check mapping and index after taking the ref.
> +		 * otherwise we can get both false positives and false
> +		 * negatives, which is just confusing to the caller.
> +		 */
> +		if (page->mapping == NULL || page->index != index) {
> +			page_cache_release(page);
> +			break;
> +		}
> +
>  		pages[ret] = page;
>  		ret++;
>  		index++;
> 
> 

Re: [PATCH] mm: Remove redundant test from find_get_pages_contig

[Andrew Morton]

On Mon, 7 Jan 2019 14:39:35 -0800 Matthew Wilcox <willy@infradead.org> wrote:

> On Mon, Jan 07, 2019 at 02:33:19PM -0800, Andrew Morton wrote:
> > On Mon,  7 Jan 2019 12:02:24 -0800 Matthew Wilcox <willy@infradead.org> wrote:
> > 
> > > After we establish a reference on the page, we check the pointer continues
> > > to be in the correct position in i_pages.  There's no need to check the
> > > page->mapping or page->index afterwards; if those can change after we've
> > > got the reference, they can change after we return the page to the caller.
> > 
> > But that isn't what the comment says.
> 
> Right.  That patch from Nick moved the check from before taking the
> ref to after taking the ref.  It was racy to have it before.  But it's
> unnecessary to have it afterwards -- pages can't move once there's a
> ref on them.  Or if they can move, they can move after the ref is taken.

So Nick's patch was never necessary?  I wonder what inspired it.

Would it be excessively cautious to put a WARN_ON_ONCE() in there for a
while?

Re: [PATCH] mm: Remove redundant test from find_get_pages_contig

[Matthew Wilcox]

On Mon, Jan 07, 2019 at 03:09:04PM -0800, Andrew Morton wrote:
> On Mon, 7 Jan 2019 14:39:35 -0800 Matthew Wilcox <willy@infradead.org> wrote:
> 
> > On Mon, Jan 07, 2019 at 02:33:19PM -0800, Andrew Morton wrote:
> > > On Mon,  7 Jan 2019 12:02:24 -0800 Matthew Wilcox <willy@infradead.org> wrote:
> > > 
> > > > After we establish a reference on the page, we check the pointer continues
> > > > to be in the correct position in i_pages.  There's no need to check the
> > > > page->mapping or page->index afterwards; if those can change after we've
> > > > got the reference, they can change after we return the page to the caller.
> > > 
> > > But that isn't what the comment says.
> > 
> > Right.  That patch from Nick moved the check from before taking the
> > ref to after taking the ref.  It was racy to have it before.  But it's
> > unnecessary to have it afterwards -- pages can't move once there's a
> > ref on them.  Or if they can move, they can move after the ref is taken.
> 
> So Nick's patch was never necessary?  I wonder what inspired it.

It was necessary to not check before the pin; that was clearly correct.
Checking after the pin, even with the code the way it was in 2006, was
unnecessary.  Look with a bit more context:

-               if (page->mapping == NULL || page->index != index)
-                       break;
-
                if (!page_cache_get_speculative(page))
                        goto repeat;
 
                /* Has the page moved? */
                if (unlikely(page != *((void **)pages[i]))) {
                        page_cache_release(page);
                        goto repeat;
                }
 
+               /*
+                * must check mapping and index after taking the ref.
+                * otherwise we can get both false positives and false
+                * negatives, which is just confusing to the caller.
+                */
+               if (page->mapping == NULL || page->index != index) {
+                       page_cache_release(page);
+                       break;
+               }
+

It's not immediately obvious that those added lines merely re-check the
condition checked by the 'page != *((void **)pages[i])', but if you think
about it, if page->index changes, then page must necessarily move within
the radix tree / xarray.

> Would it be excessively cautious to put a WARN_ON_ONCE() in there for a
> while?

I think it would ... it'd get in the way of a subsequent patch to store
only head pages in the page cache.

Re: [PATCH] mm: Remove redundant test from find_get_pages_contig

[Andrew Morton]

On Tue, 8 Jan 2019 12:26:35 -0800 Matthew Wilcox <willy@infradead.org> wrote:

> > Would it be excessively cautious to put a WARN_ON_ONCE() in there for a
> > while?
> 
> I think it would ... it'd get in the way of a subsequent patch to store
> only head pages in the page cache.

OK, shall grab.  Perhaps the changelog could gain a few words
explaining the history, etc.

Re: [PATCH] mm: Remove redundant test from find_get_pages_contig

[Matthew Wilcox]

On Tue, Jan 08, 2019 at 01:26:49PM -0800, Andrew Morton wrote:
> On Tue, 8 Jan 2019 12:26:35 -0800 Matthew Wilcox <willy@infradead.org> wrote:
> 
> > > Would it be excessively cautious to put a WARN_ON_ONCE() in there for a
> > > while?
> > 
> > I think it would ... it'd get in the way of a subsequent patch to store
> > only head pages in the page cache.
> 
> OK, shall grab.  Perhaps the changelog could gain a few words
> explaining the history, etc.

Yeah, I suck at changelogs.  Particularly when I've encountered something
that's distracting me from the thing I was trying to do.  How about this:

mm: Remove redundant test from find_get_pages_contig

After we establish a reference on the page, we check the pointer
continues to be in the correct position in i_pages.  Checking page->index
afterwards is unnecessary; if it were to change, then the pointer to it
from the page cache would also move.  The check used to be done before
grabbing a reference on the page which was racy (see 9cbb4cb21b19f
("mm: find_get_pages_contig fixlet")), but nobody noticed that moving
the check after grabbing the reference was redundant.

Signed-off-by: Matthew Wilcox <willy@infradead.org>