Skip to contentSkip to conversation

mm: fix find_get_page() for shmem exceptional entries

PATCH In review Konstantin Khlebnikov 2 participants 4 messages Feb 7, 2012 – Feb 8, 2012

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 mm: fix find_get_page() for shmem exceptional entries

It should return NULL, otherwise the caller will be very surprised.

---
 mm/filemap.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)
mm/filemap.c +1 -0
index 518223b..ca98cb5 100644
@@ -693,6 +693,7 @@ repeat:
 			 * here as an exceptional entry: so return it without
 			 * attempting to raise page count.
 			 */
+			page = NULL;
 			goto out;
 		}
 		if (!page_cache_get_speculative(page))

--
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 internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

#1 lore raw

Bug was added in commit v3.0-7291-g8079b1c (mm: clarify the radix_tree exceptional cases) So, v3.1 and v3.2 affected.

Quoted 13 lines · Konstantin Khlebnikov

It should return NULL, otherwise the caller will be very surprised.

---
  mm/filemap.c |    1 +
  1 files changed, 1 insertions(+), 0 deletions(-)
mm/filemap.c +0 -0
index 518223b..ca98cb5 100644

mm/filemap.c repeat: · line 699

Quoted 7 lines
mm/filemap.c +1 -0
@@ -693,6 +693,7 @@ repeat:
  			 * here as an exceptional entry: so return it without
  			 * attempting to raise page count.
  			 */
+			page = NULL;
  			goto out;
  		}
  		if (!page_cache_get_speculative(page))

--
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 internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

#2 lore raw
Quoted 8 lines · Konstantin Khlebnikov

Bug was added in commit v3.0-7291-g8079b1c (mm: clarify the radix_tree exceptional cases)
So, v3.1 and v3.2 affected.

Quoted 3 lines · Konstantin Khlebnikov

It should return NULL, otherwise the caller will be very surprised.

Thanks for worrying about it, but Nak to this patch.

If you have found somewhere that is surprised by an exceptional entry instead of a page, then indeed we shall need to fix that: I'm not aware of any.

There are several places that are prepared for the possibility:
find_lock_page() (and your patch would be breaking shmem.c's use of find_lock_page()), mincore_page(), memcontrol.c's mc_handle_file_pte().

Of the remaining calls to find_get_page(), my understanding is that either they are filesystems operating upon their own pagecache, or they involve using ->readpage() - that's one of the two reasons why I gave shmem its own ->splice_read() and removed its ->readpage() before switching over to use the exceptional entries.

Hugh

Quoted 4 lines
Quoted 4 lines
---
  mm/filemap.c |    1 +
  1 files changed, 1 insertions(+), 0 deletions(-)
Quoted 12 lines

mm/filemap.c repeat: · line 699

Quoted 12 lines
mm/filemap.c +1 -0
index 518223b..ca98cb5 100644
@@ -693,6 +693,7 @@ repeat:
  			 * here as an exceptional entry: so return it without
  			 * attempting to raise page count.
  			 */
+			page = NULL;
  			goto out;
  		}
  		if (!page_cache_get_speculative(page))

--
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 internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

#3 lore raw
Quoted 9 lines · Hugh Dickins
Quoted 7 lines · Konstantin Khlebnikov

Bug was added in commit v3.0-7291-g8079b1c (mm: clarify the radix_tree exceptional cases)
So, v3.1 and v3.2 affected.

Quoted 2 lines · Konstantin Khlebnikov

It should return NULL, otherwise the caller will be very surprised.

Quoted 7 lines
Quoted 1 line
Quoted 1 line

Thanks for worrying about it, but Nak to this patch.

If you have found somewhere that is surprised by an exceptional entry instead of a page, then indeed we shall need to fix that: I'm not aware of any.

Oh, this is very dangerous semantics, especially for function called "find-get-page" which sometimes returns not-getted not-a-page =)

Quoted 17 lines

There are several places that are prepared for the possibility:
find_lock_page() (and your patch would be breaking shmem.c's use of find_lock_page()), mincore_page(), memcontrol.c's mc_handle_file_pte().

Of the remaining calls to find_get_page(), my understanding is that either they are filesystems operating upon their own pagecache, or they involve using ->readpage() - that's one of the two reasons why I gave shmem its own ->splice_read() and removed its ->readpage() before switching over to use the exceptional entries.

Hugh

Quoted 4 lines
Quoted 4 lines
---
   mm/filemap.c |    1 +
   1 files changed, 1 insertions(+), 0 deletions(-)
Quoted 12 lines
Quoted 4 lines
Quoted 4 lines
mm/filemap.c +0 -0
index 518223b..ca98cb5 100644
Quoted 8 lines

mm/filemap.c repeat: · line 699

Quoted 8 lines
mm/filemap.c +1 -0
@@ -693,6 +693,7 @@ repeat:
   			 * here as an exceptional entry: so return it without
   			 * attempting to raise page count.
   			 */
+			page = NULL;
   			goto out;
   		}
   		if (!page_cache_get_speculative(page))

--
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 internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>