On Wed, Sep 16, 2020 at 07:04:46PM -0700, Hugh Dickins wrote:
> On Thu, 17 Sep 2020, Dave Chinner wrote:
> >
> > So....
> >
> > P0 p1
> >
> > hole punch starts
> > takes XFS_MMAPLOCK_EXCL
> > truncate_pagecache_range()
> > unmap_mapping_range(start, end)
> > <clears ptes>
> > <read fault>
> > do_fault_around()
> > ->map_pages
> > filemap_map_pages()
> > page mapping valid,
> > page is up to date
> > maps PTEs
> > <fault done>
> > truncate_inode_pages_range()
> > truncate_cleanup_page(page)
> > invalidates page
> > delete_from_page_cache_batch(page)
> > frees page
> > <pte now points to a freed page>
>
> No. filemap_map_pages() checks page->mapping after trylock_page(),
> before setting up the pte; and truncate_cleanup_page() does a one-page
> unmap_mapping_range() if page_mapped(), while holding page lock.
Ok, fair, I missed that.
So why does truncate_pagecache() talk about fault races and require
a second unmap range after the invalidation "for correctness" if
this sort of race cannot happen?
Why is that different to truncate_pagecache_range() which -doesn't-i
do that second removal? It's called for more than just hole_punch -
from the filesystem's persepective holepunch should do exactly the
same as truncate to the page cache, and for things like
COLLAPSE_RANGE it is absolutely essential because the data in that
range is -not zero- and will be stale if the mappings are not
invalidated completely....
Also, if page->mapping == NULL is sufficient to detect an invalidated
page in all cases, then why does page_cache_delete() explicitly
leave page->index intact:
page->mapping = NULL;
/* Leave page->index set: truncation lookup relies upon it */
Cheers,
Dave.
--
Dave Chinner
da...@fromorbit.com
