[patch] mm: find_get_pages_contig fixlet

[patch] mm: find_get_pages_contig fixlet

[Nick Piggin]

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>
---
 mm/filemap.c |   13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

Index: linux-2.6/mm/filemap.c
===================================================================
--- linux-2.6.orig/mm/filemap.c	2010-11-11 18:51:51.000000000 +1100
+++ linux-2.6/mm/filemap.c	2010-11-11 18:51:52.000000000 +1100
@@ -835,9 +835,6 @@ unsigned find_get_pages_contig(struct ad
 		if (radix_tree_deref_retry(page))
 			goto restart;
 
-		if (page->mapping == NULL || page->index != index)
-			break;
-
 		if (!page_cache_get_speculative(page))
 			goto repeat;
 
@@ -847,6 +844,16 @@ unsigned find_get_pages_contig(struct ad
 			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++;

--
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/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch] mm: find_get_pages_contig fixlet

[Wu Fengguang]

On Thu, Nov 11, 2010 at 06:54:55PM +1100, Nick Piggin wrote:
> 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>

Just out of curious, did you catch it by code review or tests?

Thanks,
Fengguang

--
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/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch] mm: find_get_pages_contig fixlet

[Nick Piggin]

On Thu, Nov 11, 2010 at 08:02:55PM +0800, Wu Fengguang wrote:
> On Thu, Nov 11, 2010 at 06:54:55PM +1100, Nick Piggin wrote:
> > 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>
> 
> Just out of curious, did you catch it by code review or tests?

It was just review.

--
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/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch] mm: find_get_pages_contig fixlet

[Minchan Kim]

On Thu, Nov 11, 2010 at 4:54 PM, Nick Piggin <npiggin@kernel.dk> wrote:
> 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: Minchan Kim <minchan.kim@gmail.com>

Nice catch.


-- 
Kind regards,
Minchan Kim

--
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/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch] mm: find_get_pages_contig fixlet

[Andrew Morton]

On Thu, 11 Nov 2010 18:54:55 +1100 Nick Piggin <npiggin@kernel.dk> wrote:

> 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>
> ---
>  mm/filemap.c |   13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> Index: linux-2.6/mm/filemap.c
> ===================================================================
> --- linux-2.6.orig/mm/filemap.c	2010-11-11 18:51:51.000000000 +1100
> +++ linux-2.6/mm/filemap.c	2010-11-11 18:51:52.000000000 +1100
> @@ -835,9 +835,6 @@ unsigned find_get_pages_contig(struct ad
>  		if (radix_tree_deref_retry(page))
>  			goto restart;
>  
> -		if (page->mapping == NULL || page->index != index)
> -			break;
> -
>  		if (!page_cache_get_speculative(page))
>  			goto repeat;
>  
> @@ -847,6 +844,16 @@ unsigned find_get_pages_contig(struct ad
>  			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;
> +		}
> +

Dumb question: if it's been "reused" then what prevents the page from
having a non-NULL ->mapping and a matching index?

--
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/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch] mm: find_get_pages_contig fixlet

[Minchan Kim]

On Fri, Nov 12, 2010 at 3:05 PM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Thu, 11 Nov 2010 18:54:55 +1100 Nick Piggin <npiggin@kernel.dk> wrote:
>
>> 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>
>> ---
>>  mm/filemap.c |   13 ++++++++++---
>>  1 file changed, 10 insertions(+), 3 deletions(-)
>>
>> Index: linux-2.6/mm/filemap.c
>> ===================================================================
>> --- linux-2.6.orig/mm/filemap.c       2010-11-11 18:51:51.000000000 +1100
>> +++ linux-2.6/mm/filemap.c    2010-11-11 18:51:52.000000000 +1100
>> @@ -835,9 +835,6 @@ unsigned find_get_pages_contig(struct ad
>>               if (radix_tree_deref_retry(page))
>>                       goto restart;
>>
>> -             if (page->mapping == NULL || page->index != index)
>> -                     break;
>> -
>>               if (!page_cache_get_speculative(page))
>>                       goto repeat;
>>
>> @@ -847,6 +844,16 @@ unsigned find_get_pages_contig(struct ad
>>                       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;
>> +             }
>> +
>
> Dumb question: if it's been "reused" then what prevents the page from
> having a non-NULL ->mapping and a matching index?

Maybe

                /* Has the page moved? */
                if (unlikely(page != *((void **)pages[i]))) {
                        page_cache_release(page);
                        goto repeat;
                }

If the page have been reused for other mapping and same index, the
page would be removed from radix tree slot of current mapping.
So radix tree slot of current mapping doesn't have a same page pointer
any more.
If I am wrong, Please correct me.

>
> --
> 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/ .
> Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
>



-- 
Kind regards,
Minchan Kim

--
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/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch] mm: find_get_pages_contig fixlet

[Nick Piggin]

On Thu, Nov 11, 2010 at 10:05:53PM -0800, Andrew Morton wrote:
> On Thu, 11 Nov 2010 18:54:55 +1100 Nick Piggin <npiggin@kernel.dk> wrote:
> 
> > 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>
> > ---
> >  mm/filemap.c |   13 ++++++++++---
> >  1 file changed, 10 insertions(+), 3 deletions(-)
> > 
> > Index: linux-2.6/mm/filemap.c
> > ===================================================================
> > --- linux-2.6.orig/mm/filemap.c	2010-11-11 18:51:51.000000000 +1100
> > +++ linux-2.6/mm/filemap.c	2010-11-11 18:51:52.000000000 +1100
> > @@ -835,9 +835,6 @@ unsigned find_get_pages_contig(struct ad
> >  		if (radix_tree_deref_retry(page))
> >  			goto restart;
> >  
> > -		if (page->mapping == NULL || page->index != index)
> > -			break;
> > -
> >  		if (!page_cache_get_speculative(page))
> >  			goto repeat;
> >  
> > @@ -847,6 +844,16 @@ unsigned find_get_pages_contig(struct ad
> >  			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;
> > +		}
> > +
> 
> Dumb question: if it's been "reused" then what prevents the page from
> having a non-NULL ->mapping and a matching index?

Nothing, but the following check will catch that it has moved. If it has
been removed then inserted back to the _same_ place, then it doesn't
matter does it? It is, in fact, "the page we are looking for " :).

In the previous sequence of checking mapping and index _before_ taking
the ref, it is possible with a small window that they had changed to
some values we expected to see to satisfy a contiguous range, but the
lack of a ref means that they may subsequently change after that.

--
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/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>