On Mon, Aug 26, 2024 at 11:11:42PM -0700, Christoph Hellwig wrote:
> On Thu, Aug 22, 2024 at 10:59:10AM -0400, Brian Foster wrote:
> > Note that we also flush for hole mappings because iomap_zero_range()
> > is used for partial folio zeroing in some cases. For example, if a
> > folio straddles EOF on a sub-page FSB size fs, the post-eof portion
> > is hole-backed and dirtied/written via mapped write, and then i_size
> > increases before writeback can occur (which otherwise zeroes the
> > post-eof portion of the EOF folio), then the folio becomes
> > inconsistent with disk until reclaimed.
>
> Eww. I'm not sure iomap_zero_range is the right way to handle this
> even if that's what we have now and it kinda work.
>
Yeah, I agree with that. That was one of the minor appeals (to me) of the
prototype I posted a while back that customizes iomap_truncate_page() to
do unconditional zeroing instead of being an almost pointless wrapper
for iomap_zero_range(). I don't quite recall if that explicitly handled
the hole case since it was quickly hacked together, but the general idea
is that it could be more purpose built for these partial zeroing cases
than zero range might be.
There are some other caveats with that approach that would still require
some variant of zero range, however, so I'm not immediately clear on
what the ideal high level solution looks like quite yet. I'd like to get
the existing mechanism working correctly, improve the test situation and
go from there.
> > + /*
> > + * We can skip pre-zeroed mappings so long as either the mapping was
> > + * clean before we started or we've flushed at least once since.
> > + * Otherwise we don't know whether the current mapping had dirty
> > + * pagecache, so flush it now, stale the current mapping, and proceed
> > + * from there.
> > + */
> > + if (srcmap->type == IOMAP_HOLE || srcmap->type == IOMAP_UNWRITTEN) {
>
> .. at very least the above needs to be documented here as a big fat
> reminder, though.
>
Sure, I'll include that explanation in the code comments in v2. Thanks.
Brian
> Otherwise the series looks sensible to me.
>
>
