On Thu, Jul 18, 2024 at 09:02:08AM -0400, Brian Foster wrote: > Hi all, > > This is a stab at fixing the iomap zero range problem where it doesn't > correctly handle the case of an unwritten mapping with dirty pagecache. > The gist is that we scan the mapping for dirty cache, zero any > already-dirty folios via buffered writes as normal, but then otherwise > skip clean ranges once we have a chance to validate those ranges against > races with writeback or reclaim. > > This is somewhat simplistic in terms of how it scans, but that is > intentional based on the existing use cases for zero range. From poking > around a bit, my current sense is that there isn't any user of zero > range that would ever expect to see more than a single dirty folio. The current code generally only zeroes a single filesystem block or less because that's all we need to zero for partial writes. This is not going to be true for very much longer with XFS forcealign functionality, and I suspect it's not true right now for large rt extent sizes when doing sub-extent writes. In these cases, we are going to have to zero multiple filesystem blocks during truncate, hole punch, unaligned writes, etc. So even if we don't do this now, I think this is something we will almost certainly be doing in the next kernel release or two. > Most > callers either straddle the EOF folio or flush in higher level code for > presumably (fs) context specific reasons. If somebody has an example to > the contrary, please let me know because I'd love to be able to use it > for testing. Check the xfs_inode_has_bigrtalloc() and xfs_inode_alloc_unitsize() cases. These are currently being worked on and expanded and factored so eventually these cases will all fall under xfs_inode_alloc_unitsize(). > The caveat to this approach is that it only works for filesystems that > implement folio_ops->iomap_valid(), which is currently just XFS. GFS2 > doesn't use ->iomap_valid() and does call zero range, but AFAICT it > doesn't actually export unwritten mappings so I suspect this is not a > problem. My understanding is that ext4 iomap support is in progress, but > I've not yet dug into what that looks like (though I suspect similar to > XFS). The concern is mainly that this leaves a landmine for fs that > might grow support for unwritten mappings && zero range but not > ->iomap_valid(). We'd likely never know zero range was broken for such > fs until stale data exposure problems start to materialize. > > I considered adding a fallback to just add a flush at the top of > iomap_zero_range() so at least all future users would be correct, but I > wanted to gate that on the absence of ->iomap_valid() and folio_ops > isn't provided until iomap_begin() time. I suppose another way around > that could be to add a flags param to iomap_zero_range() where the > caller could explicitly opt out of a flush, but that's still kind of > ugly. I dunno, maybe better than nothing..? We want to avoid the flush in this case if we can - what XFS does is a workaround for iomap not handling dirty data over unwritten extents. That first flush causes performance issues with certain truncate heavy workloads, so we really want to avoid it in the generic code if we can. > So IMO, this raises the question of whether this is just unnecessarily > overcomplicated. The KISS principle implies that it would also be > perfectly fine to do a conditional "flush and stale" in zero range > whenever we see the combination of an unwritten mapping and dirty > pagecache (the latter checked before or during ->iomap_begin()). That's > simple to implement and AFAICT would work/perform adequately and > generically for all filesystems. I have one or two prototypes of this > sort of thing if folks want to see it as an alternative. If we are going to zero the range, and the range is already unwritten, then why do we need to flush the data in the cache to make it clean and written before running the zeroing? Why not just invalidate the entire cache over the unwritten region and so return it all to containing zeroes (i.e. is unwritten!) without doing any IO. Yes, if some of the range is under writeback, the invalidation will have to wait for that to complete - invalidate_inode_pages2_range() does this for us - but after the invalidation those regions will now be written and iomap revalidation after page cache invalidation will detect this. So maybe the solution is simply to invalidate the cache over unwritten extents and then revalidate the iomap? If the iomap is still valid, then we can skip the unwritten extent completely. If the invalidation returned -EBUSY or the iomap is stale, then remap it and try again? If we don't have an iomap validation function, then we could check filemap_range_needs_writeback() before calling invalidate_inode_pages2_range() as that will tell us if there were folios that might have been under writeback during the invalidation. In that case, we can treat "needs writeback" the same as a failed iomap revalidation. So what am I missing? What does the flush actually accomplish that simply calling invalidate_inode_pages2_range() to throw the data we need to zero away doesn't? -Dave. -- Dave Chinner david@xxxxxxxxxxxxx
