| Message ID | 20250212135712.506987-2-bfoster@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | iomap: incremental advance conversion -- phase 2 | expand |
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>
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
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); } -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; } - return done; + return 0; } int iomap_read_folio(struct folio *folio, const struct iomap_ops *ops) @@ -493,15 +490,14 @@ int iomap_read_folio(struct folio *folio, const struct iomap_ops *ops) } EXPORT_SYMBOL_GPL(iomap_read_folio); -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) { if (ctx->cur_folio && - offset_in_folio(ctx->cur_folio, iter->pos + done) == 0) { + offset_in_folio(ctx->cur_folio, iter->pos) == 0) { if (!ctx->cur_folio_in_bio) folio_unlock(ctx->cur_folio); ctx->cur_folio = NULL; @@ -510,12 +506,12 @@ static loff_t iomap_readahead_iter(const struct iomap_iter *iter, ctx->cur_folio = readahead_folio(ctx->rac); ctx->cur_folio_in_bio = false; } - ret = iomap_readpage_iter(iter, ctx, done); - if (ret <= 0) + ret = iomap_readpage_iter(iter, ctx); + if (ret) return ret; } - return done; + return 0; } /**
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(-)