On Thu, 27 Aug 2020, Matthew Wilcox wrote: > We have a number of places where we look up a page in the page cache, > lock it, then have some kind of assertion that we got back the page we > asked for, eg filemap_fault(): > > page = find_get_page(mapping, offset); > ... > if (!lock_page_maybe_drop_mmap(vmf, page, &fpin)) > goto out_retry; > ... > VM_BUG_ON_PAGE(page_to_pgoff(page) != offset, page); > > but today I noticed this in shmem_undo_range(): > > pvec.nr = find_get_entries(mapping, index, > min(end - index, (pgoff_t)PAGEVEC_SIZE), > pvec.pages, indices); > ... > VM_BUG_ON_PAGE(page_to_pgoff(page) != index, page); > ... > if (!trylock_page(page)) > continue; > > So is page->index stable if we have a refcount on the page, Yes (once it has been found in the page cache - obviously not stable before it has been put into the page cache). > or is a lock on the page required? No. A lock on the page is required for page cache page->mapping to be stable, but not required for its page->index to remain stable. > A refcount on the page prevents it from being > split or freed. And there's plenty of comments along the lines of: > > mm/filemap.c: /* Leave page->index set: truncation lookup relies on it */ > > which indicates that once a page is removed from the page cache, its > index remains reliable (until it's freed). > > It might be nice to remove all these assertions from the callers and > bury them down in find_get_(entry,page,entries,...), but we can't do > that if we need the lock to check the index. If we don't need the lock, > then it should be safe to check as soon as we've checked that > page == xas_reload(). Yes. But you might then discover something violating the principle. I have an indistinct memory of spotting an instance once, maybe just in a prospective patchset that didn't reach the kernel; perhaps someone resetting page->index to 0 "for tidiness" before freeing; maybe page migration did that once upon a time, then got fixed. And of course beware of hugetlbfs, defining page->index differently (unless you have fixed that already). Hugh
