On Fri, Sep 08, 2023 at 03:48:03PM +0100, Matthew Wilcox wrote: > I want to remove ->launder_folio. So I'm looking at commit e3db7691e9f3 > which introduced ->launder_page. The race described there is pretty > clear: > > invalidate_inode_pages2() may find the dirty bit has been set on a page > owing to the fact that the page may still be mapped after it was locked. > Only after the call to unmap_mapping_range() are we sure that the page > can no longer be dirtied. > > ie this happens: > > Task A Task B > mmaps a file, writes to page A > open(O_DIRECT) > read() > kiocb_invalidate_pages() > filemap_write_and_wait_range() > __filemap_fdatawrite_range() > filemap_fdatawrite_wbc() > do_writepages() > iomap_writepages() > write_cache_pages() > page A gets cleaned > writes to page A again > invalidate_inode_pages2_range() > folio_mapped() is true, so we unmap it > folio_launder() returns 0 > invalidate_complete_folio2() returns 0 > ret = -EBUSY > kiocb_invalidate_pages() returns EBUSY > > and the DIO read fails, despite it being totally reasonable to return > the now-stale data on storage. I think you've read the __iomap_dio_rw() call path incorrectly - kiocb_invalidate_pages() is only called from the DIO write submission call path, not the DIO read call path. For a DIO read, we only call kiocb_write_and_wait() to write back dirty cached pages and we don't invalidate anything in the page cache. Hence IO submission never calls invalidate_inode_pages2_range(), so the data returned by the DIO read will only contain the first write. On DIO read completion, we still don't do any page cache invalidation, so AFAICT it returns stale data that doesn't include the second mmap write task A made to the file while the DIO read was in progress. This looks like the iomap DIO read path is working as intended to me... > A DIO write would be a different matter; > we really do need to get page A out of cache. No, I don't think we need to. On a dio write, if invalidate_inode_pages2_range() fails with -EBUSY as per above it will result in iomap_dio_rw() returning -ENOTBLK to the filesystem. That gets caught at xfs_file_write_iter() and the DIO write is then executed as buffered IO instead. This means the write is then directed through the page cache and so second write that Task A made is captured and does not get lost. IOWs, this looks like it is all working as it is supposed to, without the need to implement ->launder_folio()... Cheers, Dave. -- Dave Chinner david@xxxxxxxxxxxxx
