Re: When is page->index stable?

[Hugh Dickins <hughd@google.com>]

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