Re: [PATCH v1] iomap: simplify iomap_iter_advance()

[Brian Foster <bfoster@redhat.com>]

On Thu, Sep 18, 2025 at 03:10:18PM -0700, Joanne Koong wrote:
> On Thu, Sep 18, 2025 at 1:14 PM Brian Foster <bfoster@redhat.com> wrote:
> >
> > On Thu, Sep 18, 2025 at 07:27:29AM -0700, Christoph Hellwig wrote:
> > > On Thu, Sep 18, 2025 at 07:31:33AM -0400, Brian Foster wrote:
> > > > IME the __iomap_iter_advance() would be the most low level and flexible
> > > > version, whereas the wrappers simplify things. There's also the point
> > > > that the wrapper seems the more common case, so maybe that makes things
> > > > cleaner if that one is used more often.
> > > >
> > > > But TBH I'm not sure there is strong precedent. I'm content if we can
> > > > retain the current variant for the callers that take advantage of it.
> > > > Another idea is you could rename the current function to
> > > > iomap_iter_advance_and_update_length_for_loopy_callers() and see what
> > > > alternative suggestions come up. ;)
> > >
> > > Yeah, __ names are a bit nasty.  I prefer to mostly limit them to
> > > local helpers, or to things with an obvious inline wrapper for the
> > > fast path.  So I your latest suggestions actually aims in the right
> > > directly, but maybe we can shorten the name a little and do something
> > > like:
> > >
> > > iomap_iter_advance_and_update_len
> > >
> > > although even that would probably lead a few lines to spill.
> > > iomap_iter_advance_len would be a shorter, but a little more confusing,
> > > but still better than __-naming, so maybe it should be fine with a good
> > > kerneldoc comment?
> > >
> >
> > Ack, anything like that is fine with me, even something like
> > iomap_iter_advance_and_length() with a comment that just points out it
> > also calls iomap_length().
> >
> > Another thought was to have one helper that returns the remaining length
> > or error and then a wrapper that translates the return (i.e. return ret
> > >= 0 ? 0 : ret). But when I thought more about it seemed like it just
> > created confusion.
> >
> > Brian
> >
> 
> I'm looking at this patch again and wondering if the second helper is
> all that necessary. I feel like if we're adding it because the caller
> could be confused/unclear about needing to update their local length
> variable, then wouldn't they also be confused about having to use
> iomap_iter_advance_and_length() instead of iomap_iter_advance()? I
> feel like if they know enough to know that they need to use
> iomap_iter_advance_and_length() instead of iomap_iter_advance(), then
> they know enough to update their local length variable themsevles
> through iomap_length(). imo it seems cleaner / less cluttery to just
> have iomap_iter_advance(). But I'm happy to add the
> "iomap_advance_and_length()" helper for v2 if you guys disagree and
> prefer having a 2nd helper.
> 

Eh fair point. As mentioned at the top I'm not that worried about it,
just wanted to entertain discussion on a potentially clean way to do
both (thanks). Sounds more like it's not worth it.

Looking back at the patch again, my only followup comment is for the
handful of cases where the newly added iomap_length() is clearly the
tail of a loop, could we instead add the call to the loop iteration
line? I.e.:

	do {
		...
		iomap_iter_advance();
	while ((length = iomap_length(iter)) > 0)

With that tweak I'm good with it:

Reviewed-by: Brian Foster <bfoster@redhat.com>

Brian

> 
> Thanks,
> Joanne
>