Re: [PATCH] [RFC] iomap: zeroing needs to be pagecache aware

["Darrick J. Wong" <djwong@kernel.org>]

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@redhat.com>
> > > 
> > > 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@fromorbit.com