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@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
...
> > @@ -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@xxxxxx>
>
Thanks!
Brian
