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

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

On Fri, Nov 15, 2024 at 09:53:14AM -0500, Brian Foster wrote:
> On Tue, Nov 12, 2024 at 09:00:35AM -0500, Brian Foster wrote:
> > 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..?
> > 
> 
> FWIW, I'm concurrently hacking on what I'd consider a longer term fix
> here, based on some of the earlier discussions. The idea is basically
> iomap provides a mechanism for the fs to attach a folio_batch of dirty
> folios to the iomap, which zero range can then use as the source of
> truth for which subranges to zero of an unwritten mapping.

That's fun! :)

I wonder, can this mechanism stretch to the generic buffered write path?
In which case, can you hang on to the folios long enough to issue
writeback on them too, if it's a synchronous write?

> It occurs to me that might lend itself a bit more to what you're looking
> for here by avoiding the need for a new instance of the iter loop (I
> assume there is some outstanding work that is affected by this?). Given
> that this series was kind of a side quest for a band-aid performance fix
> in the meantime, and it's not likely 6.13 material anyways, I think I'm
> going to put it in a holding pattern and keep it in the back pocket in
> favor of trying to move that alternate approach along, at least to where
> I can post an RFC for discussion.
> 
> If that doesn't work out or there proves some critical need for it in
> the meantime, then I'll post v4 for an easy fix. I'll post a v2 of patch
> 4 separately since that is an independent fix..

I thought the bug robots were complaining about the performance hit,
so at least this part should go in sooner than later.

--D

> Brian
> 
> > > > +	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
> > 
> > 
> 
>