Skip to contentSkip to conversation

mm/shmem: make find_get_pages_range() work for huge page

PATCH In review Yu Zhao 2 participants 4 messages Jan 10, 2019 – Feb 14, 2019

Subsystems (2)
Memory management Page cache
lore
Share & tools
lore

Yu Zhao

author
#0 lore raw

PATCH 1/1 mm/shmem: make find_get_pages_range() work for huge page

find_get_pages_range() and find_get_pages_range_tag() already correctly increment reference count on head when seeing compound page, but they may still use page index from tail. Page index from tail is always zero, so these functions don't work on huge shmem. This hasn't been a problem because, AFAIK, nobody calls these functions on (huge) shmem. Fix them anyway just in case.

Signed-off-by Yu Zhao <[email protected]>
---
 mm/filemap.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
mm/filemap.c +2 -2
index 81adec8ee02c..cf5fd773314a 100644
@@ -1704,7 +1704,7 @@ unsigned find_get_pages_range(struct address_space *mapping, pgoff_t *start,
 
 		pages[ret] = page;
 		if (++ret == nr_pages) {
-			*start = page->index + 1;
+			*start = xas.xa_index + 1;
 			goto out;
 		}
 		continue;

@@ -1850,7 +1850,7 @@ unsigned find_get_pages_range_tag(struct address_space *mapping, pgoff_t *index,
 
 		pages[ret] = page;
 		if (++ret == nr_pages) {
-			*index = page->index + 1;
+			*index = xas.xa_index + 1;
 			goto out;
 		}
 		continue;
Signature · 1 line
2.20.1.97.g81188d93c3-goog
#1 lore raw
Quoted 31 lines · Yu Zhao

find_get_pages_range() and find_get_pages_range_tag() already correctly increment reference count on head when seeing compound page, but they may still use page index from tail. Page index from tail is always zero, so these functions don't work on huge shmem. This hasn't been a problem because, AFAIK, nobody calls these functions on (huge) shmem. Fix them anyway just in case.

Signed-off-by Yu Zhao <[email protected]>

---
mm/filemap.c | 4 ++--

1 file changed, 2 insertions(+), 2 deletions(-)
mm/filemap.c +1 -1
index 81adec8ee02c..cf5fd773314a 100644
@@ -1704,7 +1704,7 @@ unsigned find_get_pages_range(struct address_space *mapping, pgoff_t *start,
 
 		pages[ret] = page;
 		if (++ret == nr_pages) {
-			*start = page->index + 1;
+			*start = xas.xa_index + 1;
 			goto out;
 		}
 		continue;

mm/filemap.c unsigned find_get_pages_range_tag(struct address_space *mapping, pgoff_t *index, · line 1856

Quoted 8 lines
mm/filemap.c +1 -1
@@ -1850,7 +1850,7 @@ unsigned find_get_pages_range_tag(struct address_space *mapping, pgoff_t *index,
 
 		pages[ret] = page;
 		if (++ret == nr_pages) {
-			*index = page->index + 1;
+			*index = xas.xa_index + 1;
 			goto out;
 		}
 		continue;

While this works, it seems like this would be more readable for future maintainers were it to instead squirrel away the value for *start/*index when ret was zero on the first iteration through the loop.

Though xa_index is designed to hold the first index of the entry, it seems inappropriate to have these routines deference elements of xas directly; I guess it depends on how opaque we want to keep xas and struct xa_state.

Does anyone else have a feeling one way or the other? I could be persuaded either way.

author
#2 lore raw
Quoted 31 lines · William Kucharski
Quoted 31 lines · Yu Zhao

find_get_pages_range() and find_get_pages_range_tag() already correctly increment reference count on head when seeing compound page, but they may still use page index from tail. Page index from tail is always zero, so these functions don't work on huge shmem. This hasn't been a problem because, AFAIK, nobody calls these functions on (huge) shmem. Fix them anyway just in case.

Signed-off-by Yu Zhao <[email protected]>

---
mm/filemap.c | 4 ++--

1 file changed, 2 insertions(+), 2 deletions(-)
mm/filemap.c +1 -1
index 81adec8ee02c..cf5fd773314a 100644
@@ -1704,7 +1704,7 @@ unsigned find_get_pages_range(struct address_space *mapping, pgoff_t *start,
 
 		pages[ret] = page;
 		if (++ret == nr_pages) {
-			*start = page->index + 1;
+			*start = xas.xa_index + 1;
 			goto out;
 		}
 		continue;

@@ -1850,7 +1850,7 @@ unsigned find_get_pages_range_tag(struct address_space *mapping, pgoff_t *index,
Quoted 12 lines

mm/filemap.c unsigned find_get_pages_range_tag(struct address_space *mapping, pgoff_t *index, · line 1856

Quoted 8 lines
mm/filemap.c +1 -1
 		pages[ret] = page;
 		if (++ret == nr_pages) {
-			*index = page->index + 1;
+			*index = xas.xa_index + 1;
 			goto out;
 		}
 		continue;

While this works, it seems like this would be more readable for future maintainers were it to instead squirrel away the value for *start/*index when ret was zero on the first iteration through the loop.

I'm not sure how this could be more readable, and it sounds independent from the problem the patch fixes.

Quoted 3 lines

Though xa_index is designed to hold the first index of the entry, it seems inappropriate to have these routines deference elements of xas directly; I guess it depends on how opaque we want to keep xas and struct xa_state.

It seems to me it's pefectly fine to use fields of xas directly, and it's being done this way throughout the file.

Quoted 1 line

Does anyone else have a feeling one way or the other? I could be persuaded either way.