On Thu, Dec 01, 2022 at 01:43:29PM +1100, Dave Chinner wrote: > On Wed, Nov 30, 2022 at 06:08:04PM -0800, Darrick J. Wong wrote: > > On Thu, Dec 01, 2022 at 11:52:14AM +1100, Dave Chinner wrote: > > > From: Dave Chinner <dchinner@xxxxxxxxxx> > > > > > > Unwritten extents can have page cache data over the range being > > > zeroed so we can't just skip them entirely. Fix this by checking for > > > an existing dirty folio over the unwritten range we are zeroing > > > and only performing zeroing if the folio is already dirty. > > > > Hm, I'll look at this tomorrow morning when I'm less bleary. From a > > cursory glance it looks ok though. > > > > > XXX: how do we detect a iomap containing a cow mapping over a hole > > > in iomap_zero_iter()? The XFS code implies this case also needs to > > > zero the page cache if there is data present, so trigger for page > > > cache lookup only in iomap_zero_iter() needs to handle this case as > > > well. > > > > I've been wondering for a while if we ought to rename iomap_iter.iomap > > to write_iomap and iomap_iter.srcmap to read_iomap, and change all the > > ->iomap_begin and ->iomap_end functions as needed. I think that would > > make it more clear to iomap users which one they're supposed to use. > > Right now we overload iomap_iter.iomap for reads and for writes if > > srcmap is a hole (or SHARED isn't set on iomap) and it's getting > > confusing to keep track of all that. > > *nod* > > We definitely need to clarify this - I find the overloading > confusing at the best of times. No idea what the solution to this > looks like, though... <nod> It probably means a largeish cleanup for 6.3. Maybe start with the patches I'd sent earlier that shortened the ->iomap_begin and ->iomap_end parameter list, and move on to redefining the two? https://lore.kernel.org/linux-xfs/166801778962.3992140.13451716594530581376.stgit@magnolia/ https://lore.kernel.org/linux-xfs/166801779522.3992140.4135946031734299717.stgit@magnolia/ > > I guess the hard part of all that is that writes to the pagecache don't > > touch storage; and writeback doesn't care about the source mapping since > > it's only using block granularity. > > Yup, that's why this code needs the IOMAP_F_STALE code to be in > place before we can use the page cache lookups like this. Well it's in for-next now, so it'll land in 6.2. --D > Cheers, > > Dave. > -- > Dave Chinner > david@xxxxxxxxxxxxx
