On Thu, 17 Sep 2020, Dave Chinner wrote: > On Wed, Sep 16, 2020 at 07:04:46PM -0700, Hugh Dickins wrote: > > On Thu, 17 Sep 2020, Dave Chinner wrote: > > > <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? I thought the comment * unmap_mapping_range is called twice, first simply for * efficiency so that truncate_inode_pages does fewer * single-page unmaps. However after this first call, and * before truncate_inode_pages finishes, it is possible for * private pages to be COWed, which remain after * truncate_inode_pages finishes, hence the second * unmap_mapping_range call must be made for correctness. explains it fairly well. It's because POSIX demanded that when a file is truncated, the user will get SIGBUS on trying to access even the COWed pages beyond EOF in a MAP_PRIVATE mapping. Page lock on the cache page does not serialize the pages COWed from it very well. But there's no such SIGBUS requirement in the case of hole-punching, and trying to unmap those pages racily instantiated just after the punching cursor passed, would probably do more harm than good. > > 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.... I can't speak to COLLAPSE_RANGE. > > 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 */ Because there was, and I think still is (but might it now be xarrayed away?), code (mainly in mm/truncate.c) which finds it convenient to check page->index for end of range, without necessitating the overhead of getting page lock. I've no doubt it's an (minor) optimization that could be discarded if there were ever a need to invalidate page->index when deleting; but nobody has required that yet. Hugh
