Re: [patch] mm: find_get_pages_contig fixlet

[Minchan Kim <minchan.kim@gmail.com>]

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>