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

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Wed, Jun 24, 2020 at 08:14:31AM +1000, Dave Chinner wrote:
> 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.

<nod> Thanks for the explanation.

/me wonders if someone could please check all the *_ops that point to
generic helpers to see if we're missing obvious things like lock
taking.  Particularly someone who wants to learn about xfs' locking
strategy; I promise it won't let out a ton of bees.

Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D


> 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