On Tue, Aug 13, 2019 at 08:14:34AM -0700, Darrick J. Wong wrote:
> + /*
> + * Now that we've locked both pages, make sure they still
> + * represent the data we're interested in. If not, someone
> + * is invalidating pages on us and we lose.
> + */
> + if (src_page->mapping != src->i_mapping ||
> + src_page->index != srcoff >> PAGE_SHIFT ||
> + dest_page->mapping != dest->i_mapping ||
> + dest_page->index != destoff >> PAGE_SHIFT) {
> + same = false;
> + goto unlock;
> + }
It is my understanding that you don't need to check the ->index here.
If I'm wrong about that, I'd really appreciate being corrected, because
the page cache locking is subtle.
You call read_mapping_page() which returns the page with an elevated
refcount. That means the page can't go back to the page allocator and
be allocated again. It can, because it's unlocked, still be truncated,
so the check for ->mapping after locking it is needed. But the check
for ->index being correct was done by find_get_entry().
See pagecache_get_page() -- if we specify FGP_LOCK, then it will lock
the page, check the ->mapping but not check ->index. OK, it does check
->index, but in a VM_BUG_ON(), so it's not something that ought to be
able to be wrong.
