mm: find_get_pages_contig fixlet
Subsystems (2)
PATCH 1/1 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.
--- mm/filemap.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-)
mm/filemap.c +10 -3
@@ -835,9 +835,6 @@ unsigned find_get_pages_contig(struct adif (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 [email protected]. 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:"[email protected]"> [email protected] </a>
Quoted 4 lines · 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 <[email protected]>
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 [email protected]. 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:"[email protected]"> [email protected] </a>
Quoted 9 lines · Wu Fengguang
Quoted 4 lines · 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 <[email protected]>Reviewed-by Wu Fengguang <[email protected]>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 [email protected]. 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:"[email protected]"> [email protected] </a>
Quoted 4 lines · 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 <[email protected]>
Nice catch.
Signature · 9 lines
Kind regards, Minchan Kim -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. 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:"[email protected]"> [email protected] </a>
Andrew Morton
maintainerQuoted 26 lines · On Thu, 11 Nov 2010 18:54:55 +1100 Nick Piggin <[email protected]> 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 <[email protected]>--- mm/filemap.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-)Index:
linux-2.6/mm/filemap.c
===================================================================mm/filemap.c +0
-3@@ -835,9 +835,6 @@ unsigned find_get_pages_contig(struct adif (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; }
mm/filemap.c unsigned find_get_pages_contig(struct ad · line 856
Quoted 10 lines
mm/filemap.c +10
-0+/* +* 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 [email protected]. 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:"[email protected]"> [email protected] </a>
Quoted 28 lines · Andrew Morton
Quoted 26 lines · On Thu, 11 Nov 2010 18:54:55 +1100 Nick Piggin <[email protected]> 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 <[email protected]>---
mm/filemap.c | 13 ++++++++++---1 file changed, 10 insertions(+), 3 deletions(-)Index:
linux-2.6/mm/filemap.c
===================================================================mm/filemap.c 2010-11-11 18:51:52.000000000 +1100 renamed from mm/filemap.c 2010-11-11 18:51:51.000000000 +1100 +0
-3@@ -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; }
Quoted 13 lines
mm/filemap.c 2010-11-11 18:51:52.000000000 +1100 unsigned find_get_pages_contig(struct ad · line 856
Quoted 10 lines
mm/filemap.c 2010-11-11 18:51:52.000000000 +1100 renamed from mm/filemap.c 2010-11-11 18:51:51.000000000 +1100 +10
-0+ /* + * 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.
Quoted 8 lines
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. 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:"[email protected]"> [email protected] </a>
Signature · 9 lines
Kind regards, Minchan Kim -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. 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:"[email protected]"> [email protected] </a>
Quoted 28 lines · Andrew Morton
Quoted 26 lines · On Thu, 11 Nov 2010 18:54:55 +1100 Nick Piggin <[email protected]> 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 <[email protected]>--- mm/filemap.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-)Index:
linux-2.6/mm/filemap.c
===================================================================mm/filemap.c +0
-3@@ -835,9 +835,6 @@ unsigned find_get_pages_contig(struct adif (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; }
Quoted 13 lines
mm/filemap.c unsigned find_get_pages_contig(struct ad · line 856
Quoted 10 lines
mm/filemap.c +10
-0+/* +* 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 [email protected]. 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:"[email protected]"> [email protected] </a>