Re: [PATCH v3 2/4] iomap: lift zeroed mapping handling into iomap_zero_range()

[Brian Foster <bfoster@redhat.com>]

On Sun, Nov 10, 2024 at 10:03:44PM -0800, Christoph Hellwig wrote:
> On Fri, Nov 08, 2024 at 07:42:44AM -0500, Brian Foster wrote:
> > In preparation for special handling of subranges, lift the zeroed
> > mapping logic from the iterator into the caller.
> 
> What's that special code?  I don't really see anything added to this
> in the new code?  In general I would prefer if all code for the
> iteration would be kept in a single function in preparation for
> unrolling these loops.  If you want to keep this code separate
> from the write zeroes logic (which seems like a good idea) please
> just just move the actual real zeroing out of iomap_zero_iter into
> a separate helper similar to how we e.g. have multiple different
> implementations in the dio iterator.
> 

There is no special code... the special treatment is to check the dirty
state of a block unaligned start in isolation to decide whether to skip
or explicitly zero if dirty. The fallback logic is to check the dirty
state of the entire range and if needed, flush the mapping to push all
pending (dirty && unwritten) instances out to the fs so the iomap is up
to date and we can safely skip iomaps that are inherently zero on disk.

Hmm.. so I see the multiple iter modes for dio, but it looks like that
is inherent to the mapping type. That's not quite what I'm doing here,
so I'm not totally clear on what you're asking for. FWIW, I swizzled
this code around a few times and failed to ultimately find something I'd
consider elegant. For example, initial versions would have something
like another param to iomap_zero_iter() to skip the optimization logic
(i.e. don't skip zeroed extents for this call), which I think is more in
the spirit of what you're saying, but I ultimately found it cleaner to
open code that part. If you had something else in mind, could you share
some pseudocode or something to show the factoring..?

> > +	while ((ret = iomap_iter(&iter, ops)) > 0) {
> > +		const struct iomap *s = iomap_iter_srcmap(&iter);
> > +
> > +		if (s->type == IOMAP_HOLE || s->type == IOMAP_UNWRITTEN) {
> > +			loff_t p = iomap_length(&iter);
> 
> Also please stick to variable names that are readable and preferably
> the same as in the surrounding code, e.g. s -> srcmap p -> pos.
> 

Sure. I think I did this to avoid long lines, but I can change it.
Thanks.

Brian