When is page->index stable?

When is page->index stable?

[Matthew Wilcox]

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, or is a lock
on the page required?  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().

Re: When is page->index stable?

[Hugh Dickins]

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

Re: When is page->index stable?

[Matthew Wilcox]

On Thu, Aug 27, 2020 at 01:52:47PM -0700, Hugh Dickins wrote:
> 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).

The other thing to beware of is swapcache, which also defines page->index
differently.  We went through this last year ...
https://lore.kernel.org/linux-mm/20190323033852.GC10344@bombadil.infradead.org/T/#u

As can be seen from that thread, even calling page_index() instead of
directly looking at page->index is insufficient because having a reference
on a page is insufficient to keep ClearPageSwapCache from being cleared.

What you're doing is safe, because you know mapping isn't a swapcache
mapping; it's a shmem mapping.

So I'm going to back out a good chunk of the work I did yesterday :-(