Re: [PATCH 01/10] iomap: advance the iter directly on buffered read

[Brian Foster <bfoster@redhat.com>]

On Wed, Feb 12, 2025 at 10:50:12PM -0800, Christoph Hellwig wrote:
> On Wed, Feb 12, 2025 at 08:57:03AM -0500, Brian Foster wrote:
> > iomap buffered read advances the iter via iter.processed. To
> > continue separating iter advance from return status, update
> > iomap_readpage_iter() to advance the iter instead of returning the
> > number of bytes processed. In turn, drop the offset parameter and
> > sample the updated iter->pos at the start of the function. Update
> > the callers to loop based on remaining length in the current
> > iteration instead of number of bytes processed.
> > 
> > Signed-off-by: Brian Foster <bfoster@redhat.com>
> > ---
> >  fs/iomap/buffered-io.c | 44 +++++++++++++++++++-----------------------
> >  1 file changed, 20 insertions(+), 24 deletions(-)
> > 
> > diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
> > index ec227b45f3aa..44a366736289 100644
> > --- a/fs/iomap/buffered-io.c
> > +++ b/fs/iomap/buffered-io.c
...
> > @@ -438,25 +438,22 @@ static loff_t iomap_readpage_iter(const struct iomap_iter *iter,
> >  	 * we can skip trailing ones as they will be handled in the next
> >  	 * iteration.
> >  	 */
> > -	return pos - orig_pos + plen;
> > +	length = pos - orig_pos + plen;
> > +	return iomap_iter_advance(iter, &length);
> 
> At this point orig_pos should orig_pos should always be just iter->pos
> and we could trivially drop the variable, right?
> 

Hmm.. good point. I think it should be equivalent. I'll give it a test
and drop orig_pos if all works out.

> > -static loff_t iomap_read_folio_iter(const struct iomap_iter *iter,
> > +static loff_t iomap_read_folio_iter(struct iomap_iter *iter,
> >  		struct iomap_readpage_ctx *ctx)
> >  {
> > -	struct folio *folio = ctx->cur_folio;
> > -	size_t offset = offset_in_folio(folio, iter->pos);
> > -	loff_t length = min_t(loff_t, folio_size(folio) - offset,
> > -			      iomap_length(iter));
> > -	loff_t done, ret;
> > -
> > -	for (done = 0; done < length; done += ret) {
> > -		ret = iomap_readpage_iter(iter, ctx, done);
> > -		if (ret <= 0)
> > +	loff_t ret;
> > +
> > +	while (iomap_length(iter)) {
> > +		ret = iomap_readpage_iter(iter, ctx);
> > +		if (ret)
> >  			return ret;
> 
> This looks so much nicer!
> 
> > -static loff_t iomap_readahead_iter(const struct iomap_iter *iter,
> > +static loff_t iomap_readahead_iter(struct iomap_iter *iter,
> >  		struct iomap_readpage_ctx *ctx)
> >  {
> > -	loff_t length = iomap_length(iter);
> > -	loff_t done, ret;
> > +	loff_t ret;
> >  
> > -	for (done = 0; done < length; done += ret) {
> > +	while (iomap_length(iter) > 0) {
> 
> iomap_length can't really be negative, so we could just drop the "> 0"
> here.  Or if you think it's useful add it in the other loop above to
> be consistent.
> 

Indeed.. no particular reason for this that I recall, probably just a
thinko. Will fix.

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

Thanks!

Brian