Re: [patch] mm: find_get_pages_contig fixlet

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Nov 12, 2010 at 3:05 PM, Andrew Morton
<akpm@xxxxxxxxxxxxxxxxxxxx> wrote:
> On Thu, 11 Nov 2010 18:54:55 +1100 Nick Piggin <npiggin@xxxxxxxxx> 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@xxxxxxxxx>
>> ---
>>  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@xxxxxxxxxx  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@xxxxxxxxx";> email@xxxxxxxxx </a>
>



-- 
Kind regards,
Minchan Kim

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxxx  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


[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]