| Message ID | 20181122213224.12793-2-willy@infradead.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Better support for THP in page cache | expand |
On Thu, Nov 22, 2018 at 01:32:23PM -0800, Matthew Wilcox wrote: > After we establish a reference on the page, we check the pointer continues > to be in the correct position in i_pages. There's no need to check the > page->mapping or page->index afterwards; if those can change after we've > got the reference, they can change after we return the page to the caller. Hm. IIRC, page->mapping can be set to NULL due truncation, but what about index? When it can be changed? Truncation doesn't touch it.
On Fri, Nov 23, 2018 at 01:47:32PM +0300, Kirill A. Shutemov wrote: > On Thu, Nov 22, 2018 at 01:32:23PM -0800, Matthew Wilcox wrote: > > After we establish a reference on the page, we check the pointer continues > > to be in the correct position in i_pages. There's no need to check the > > page->mapping or page->index afterwards; if those can change after we've > > got the reference, they can change after we return the page to the caller. > > Hm. IIRC, page->mapping can be set to NULL due truncation, but what about > index? When it can be changed? Truncation doesn't touch it. I think index can only be changed after the refcount has hit zero and the page is safely out of the pagecache. I agree that page->mapping can be set to NULL after the call to xas_reload() ... but then it can also happen after the check, so the check isn't really buying us anything that the xas_reload() call doesn't already check.
diff --git a/mm/filemap.c b/mm/filemap.c index 81adec8ee02cc..538531590ef2d 100644 --- a/mm/filemap.c +++ b/mm/filemap.c @@ -1776,16 +1776,6 @@ unsigned find_get_pages_contig(struct address_space *mapping, pgoff_t index, if (unlikely(page != xas_reload(&xas))) goto put_page; - /* - * 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 || page_to_pgoff(page) != xas.xa_index) { - put_page(page); - break; - } - pages[ret] = page; if (++ret == nr_pages) break;
After we establish a reference on the page, we check the pointer continues to be in the correct position in i_pages. There's no need to check the page->mapping or page->index afterwards; if those can change after we've got the reference, they can change after we return the page to the caller. Signed-off-by: Matthew Wilcox <willy@infradead.org> --- mm/filemap.c | 10 ---------- 1 file changed, 10 deletions(-)