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

[Brian Foster <bfoster@redhat.com>]

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 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.

> 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