| Message ID | 20190110030838.84446-1-yuzhao@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm/shmem: make find_get_pages_range() work for huge page | expand |
> On Jan 9, 2019, at 8:08 PM, Yu Zhao <yuzhao@google.com> wrote: > > 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 <yuzhao@google.com> > --- > mm/filemap.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/mm/filemap.c b/mm/filemap.c > index 81adec8ee02c..cf5fd773314a 100644 > --- a/mm/filemap.c > +++ b/mm/filemap.c > @@ -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; > -- 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.
On Thu, Jan 10, 2019 at 04:43:57AM -0700, William Kucharski wrote: > > > > On Jan 9, 2019, at 8:08 PM, Yu Zhao <yuzhao@google.com> wrote: > > > > 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 <yuzhao@google.com> > > --- > > mm/filemap.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/mm/filemap.c b/mm/filemap.c > > index 81adec8ee02c..cf5fd773314a 100644 > > --- a/mm/filemap.c > > +++ b/mm/filemap.c > > @@ -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; > > -- > > 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. > 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. > Does anyone else have a feeling one way or the other? I could be persuaded either way.
> On Feb 12, 2019, at 4:57 PM, Yu Zhao <yuzhao@google.com> wrote: > > It seems to me it's pefectly fine to use fields of xas directly, > and it's being done this way throughout the file. Fair enough. Reviewed-by: William Kucharski <william.kucharski@oracle.com>
diff --git a/mm/filemap.c b/mm/filemap.c index 81adec8ee02c..cf5fd773314a 100644 --- a/mm/filemap.c +++ b/mm/filemap.c @@ -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;
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 <yuzhao@google.com> --- mm/filemap.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)