Re: [PATCH] xfs: use MMAPLOCK around filemap_map_pages()

[Dave Chinner <david@fromorbit.com>]

On Tue, Jun 23, 2020 at 02:19:10PM -0700, Darrick J. Wong wrote:
> On Tue, Jun 23, 2020 at 03:20:59PM +1000, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> > 
> > The page faultround path ->map_pages is implemented in XFS via
> 
> What does "faultround" mean?

Typo - fault-around.

i.e. when we take a read page fault, the do_read_fault() code first
opportunistically tries to map a range of pages surrounding
surrounding the faulted page into the PTEs, not just the faulted
page. It uses ->map_pages() to do the page cache lookups for
cached pages in the range of the fault around and then inserts them
into the PTES is if finds any.

If the fault-around pass did not find the page fault page in cache
(i.e. vmf->page remains null) then it calls into do_fault(), which
ends up calling ->fault, which we then lock the MMAPLOCK and call
into filemap_fault() to populate the page cache and read the data
into it.

So, essentially, fault-around is a mechanism to reduce page faults
in the situation where previous readahead has brought adjacent pages
into the page cache by optimistically mapping up to
fault_around_bytes into PTEs on any given read page fault.

> I'm pretty convinced that this is merely another round of whackamole wrt
> taking the MMAPLOCK before relying on or doing anything to pages in the
> page cache, I just can't tell if 'faultround' is jargon or typo.

Well, it's whack-a-mole in that this is the first time I've actually
looked at what map_pages does. I knew there was fault-around in the
page fault path, but I know that it had it's own method for
page cache lookups and pte mapping, nor that it had it's own
truncate race checks to ensure it didn't map pages invalidated by
truncate into the PTEs.

There's so much technical debt hidden in the kernel code base. The
fact we're still finding places that assume only truncate can
invalidate the page cache over a file range indicates just how deep
this debt runs...

-Dave.
-- 
Dave Chinner
david@fromorbit.com