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

[Christoph Hellwig <hch@infradead.org>]

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
> @@ -366,12 +366,12 @@ static inline bool iomap_block_needs_zeroing(const struct iomap_iter *iter,
>  		pos >= i_size_read(iter->inode);
>  }
>  
> -static loff_t iomap_readpage_iter(const struct iomap_iter *iter,
> -		struct iomap_readpage_ctx *ctx, loff_t offset)
> +static loff_t iomap_readpage_iter(struct iomap_iter *iter,
> +		struct iomap_readpage_ctx *ctx)
>  {
>  	const struct iomap *iomap = &iter->iomap;
> -	loff_t pos = iter->pos + offset;
> -	loff_t length = iomap_length(iter) - offset;
> +	loff_t pos = iter->pos;
> +	loff_t length = iomap_length(iter);
>  	struct folio *folio = ctx->cur_folio;
>  	struct iomap_folio_state *ifs;
>  	loff_t orig_pos = pos;
> @@ -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?

> -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.

Otherwise looks good:

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