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@xxxxxxxxxx>
> ---
> 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@xxxxxx>
