Re: [PATCH 2/2] iomap: make zero range flush conditional on unwritten mappings

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

On Thu, Aug 29, 2024 at 11:05:59AM -0400, Brian Foster wrote:
> On Wed, Aug 28, 2024 at 10:41:56PM -0700, Christoph Hellwig wrote:
> > On Wed, Aug 28, 2024 at 08:35:47AM -0400, Brian Foster wrote:
> > > Yeah, it was buried in a separate review around potentially killing off
> > > iomap_truncate_page():
> > > 
> > > https://lore.kernel.org/linux-fsdevel/ZlxUpYvb9dlOHFR3@bfoster/
> > > 
> > > The idea is pretty simple.. use the same kind of check this patch does
> > > for doing a flush, but instead open code and isolate it to
> > > iomap_truncate_page() so we can just default to doing the buffered write
> > > instead.
> > > 
> > > Note that I don't think this replaces the need for patch 1, but it might
> > > arguably make further optimization of the flush kind of pointless
> > > because I'm not sure zero range would ever be called from somewhere that
> > > doesn't flush already.
> > > 
> > > The tradeoffs I can think of are this might introduce some false
> > > positives where an EOF folio might be dirty but a sub-folio size block
> > > backing EOF might be clean, and again that callers like truncate and
> > > write extension would need to both truncate the eof page and zero the
> > > broader post-eof range. Neither of those seem all that significant to
> > > me, but just my .02.
> > 
> > Looking at that patch and your current series I kinda like not having
> > to deal with the dirty caches in the loop, and in fact I'd also prefer
> > to not do any writeback from the low-level zero helpers if we can.
> > That is not doing your patch 1 but instead auditing the callers if
> > any of them needs them and documenting the expectation.

I looked, and was pretty sure that XFS is the only one that has that
expectation.

> I agree this seems better in some ways, but I don't like complicating or
> putting more responsibility on the callers. I think if we had a high
> level iomap function that wrapped a combination of this proposed variant
> of truncate_page() and zero_range() for general inode size changes, that
> might alleviate that concern.
> 
> Otherwise IME even if we audited and fixed all callers today, over time
> we'll just reintroduce the same sorts of errors if the low level
> mechanisms aren't made to function correctly.

Yeah.  What /are/ the criteria for needing the flush and wait?  AFAICT,
a filesystem only needs the flush if it's possible to have dirty
pagecache backed either by a hole or an unwritten extent, right?

I suppose we could amend the iomap ops so that filesystems could signal
that they allow either of those things, and then we wouldn't have to
query the mapping for filesystems that don't, right?  IOWs, one can opt
out of safety features if there's no risk of a garbage, right?

(Also: does xfs allow dirty page cache backed by a hole?  I didn't think
that was possible.)

> > But please let Dave and Darrick chime in first before investing any
> > work into this.
> > 
> > 
> 
> Based on the feedback to v2, it sounds like there's general consensus on
> the approach modulo some code factoring discussion. Unless there is
> objection, I think I'll stick with that for now for the sake of progress
> and keep this option in mind on the back burner. None of this is really
> that hard to change if we come up with something better.
> 
> Brian
> 
>