Re: [PATCH v2 3/7] iomap: refactor iter and advance continuation logic

[Brian Foster <bfoster@redhat.com>]

On Mon, Jan 27, 2025 at 09:34:15PM -0800, Christoph Hellwig wrote:
> On Wed, Jan 22, 2025 at 08:34:30AM -0500, Brian Foster wrote:
> > +	s64 ret;
> > +	bool stale = iter->iomap.flags & IOMAP_F_STALE;
> 
> Nit: I find code more redable if variables that initialized at
> declaration time (especially when derived from arguments) are
> before plain variable declarations.  Not a big thing here with just
> two of them, but variable counts keep growing over time.
> 

Ack. I have a couple pending changes for followon work I wanted to fold
into a v3 anyways, so I'll make that tweak.

> >  
> > -	if (iter->iomap.length && ops->iomap_end) {
> > +	if (!iter->iomap.length) {
> > +		trace_iomap_iter(iter, ops, _RET_IP_);
> > +		goto begin;
> > +	}
> 
> This might be a chance to split trace_iomap_iter into two trace points
> for the initial and following iterations?  Or maybe we shouldn't bother.
> 

Hmm.. not sure I see the value in a tracepoint just for the initial
case, but maybe we should just move trace_iomap_iter() to the top of the
function? We already have post-lookup tracepoints in iomap_iter_done()
to show the mappings, and that would remove the duplication. Hm?

> Otherwise this looks great:
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> 

Thanks.

Brian