| Message ID | 20250219175050.83986-11-bfoster@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | iomap: incremental advance conversion -- phase 2 | expand |
On Wed, Feb 19, 2025 at 12:50:48PM -0500, Brian Foster wrote: > At this point, all iomap operations have been updated to advance the > iomap_iter directly before returning to iomap_iter(). Therefore, the > complexity of handling both the old and new semantics is no longer > required and can be removed from iomap_iter(). > > Update iomap_iter() to expect success or failure status in > iter.processed. As a precaution and developer hint to prevent > inadvertent use of old semantics, warn on a positive return code and > fail the operation. Remove the unnecessary advance and simplify the > termination logic. > > Signed-off-by: Brian Foster <bfoster@redhat.com> Looks reasonable, Reviewed-by: "Darrick J. Wong" <djwong@kernel.org> --D > --- > fs/iomap/iter.c | 39 +++++++++++++++------------------------ > 1 file changed, 15 insertions(+), 24 deletions(-) > > diff --git a/fs/iomap/iter.c b/fs/iomap/iter.c > index 0ebcabc7df52..e4dfe64029cc 100644 > --- a/fs/iomap/iter.c > +++ b/fs/iomap/iter.c > @@ -60,9 +60,8 @@ static inline void iomap_iter_done(struct iomap_iter *iter) > int iomap_iter(struct iomap_iter *iter, const struct iomap_ops *ops) > { > bool stale = iter->iomap.flags & IOMAP_F_STALE; > - ssize_t advanced = iter->processed > 0 ? iter->processed : 0; > - u64 olen = iter->len; > - s64 processed; > + ssize_t advanced; > + u64 olen; > int ret; > > trace_iomap_iter(iter, ops, _RET_IP_); > @@ -71,14 +70,11 @@ int iomap_iter(struct iomap_iter *iter, const struct iomap_ops *ops) > goto begin; > > /* > - * If iter.processed is zero, the op may still have advanced the iter > - * itself. Calculate the advanced and original length bytes based on how > - * far pos has advanced for ->iomap_end(). > + * Calculate how far the iter was advanced and the original length bytes > + * for ->iomap_end(). > */ > - if (!advanced) { > - advanced = iter->pos - iter->iter_start_pos; > - olen += advanced; > - } > + advanced = iter->pos - iter->iter_start_pos; > + olen = iter->len + advanced; > > if (ops->iomap_end) { > ret = ops->iomap_end(iter->inode, iter->iter_start_pos, > @@ -89,27 +85,22 @@ int iomap_iter(struct iomap_iter *iter, const struct iomap_ops *ops) > return ret; > } > > - processed = iter->processed; > - if (processed < 0) { > - iomap_iter_reset_iomap(iter); > - return processed; > - } > + /* detect old return semantics where this would advance */ > + if (WARN_ON_ONCE(iter->processed > 0)) > + iter->processed = -EIO; > > /* > - * Advance the iter and clear state from the previous iteration. This > - * passes iter->processed because that reflects the bytes processed but > - * not yet advanced by the iter handler. > - * > * Use iter->len to determine whether to continue onto the next mapping. > - * Explicitly terminate in the case where the current iter has not > + * Explicitly terminate on error status or if the current iter has not > * advanced at all (i.e. no work was done for some reason) unless the > * mapping has been marked stale and needs to be reprocessed. > */ > - ret = iomap_iter_advance(iter, &processed); > - if (!ret && iter->len > 0) > - ret = 1; > - if (ret > 0 && !advanced && !stale) > + if (iter->processed < 0) > + ret = iter->processed; > + else if (iter->len == 0 || (!advanced && !stale)) > ret = 0; > + else > + ret = 1; > iomap_iter_reset_iomap(iter); > if (ret <= 0) > return ret; > -- > 2.48.1 > >
diff --git a/fs/iomap/iter.c b/fs/iomap/iter.c index 0ebcabc7df52..e4dfe64029cc 100644 --- a/fs/iomap/iter.c +++ b/fs/iomap/iter.c @@ -60,9 +60,8 @@ static inline void iomap_iter_done(struct iomap_iter *iter) int iomap_iter(struct iomap_iter *iter, const struct iomap_ops *ops) { bool stale = iter->iomap.flags & IOMAP_F_STALE; - ssize_t advanced = iter->processed > 0 ? iter->processed : 0; - u64 olen = iter->len; - s64 processed; + ssize_t advanced; + u64 olen; int ret; trace_iomap_iter(iter, ops, _RET_IP_); @@ -71,14 +70,11 @@ int iomap_iter(struct iomap_iter *iter, const struct iomap_ops *ops) goto begin; /* - * If iter.processed is zero, the op may still have advanced the iter - * itself. Calculate the advanced and original length bytes based on how - * far pos has advanced for ->iomap_end(). + * Calculate how far the iter was advanced and the original length bytes + * for ->iomap_end(). */ - if (!advanced) { - advanced = iter->pos - iter->iter_start_pos; - olen += advanced; - } + advanced = iter->pos - iter->iter_start_pos; + olen = iter->len + advanced; if (ops->iomap_end) { ret = ops->iomap_end(iter->inode, iter->iter_start_pos, @@ -89,27 +85,22 @@ int iomap_iter(struct iomap_iter *iter, const struct iomap_ops *ops) return ret; } - processed = iter->processed; - if (processed < 0) { - iomap_iter_reset_iomap(iter); - return processed; - } + /* detect old return semantics where this would advance */ + if (WARN_ON_ONCE(iter->processed > 0)) + iter->processed = -EIO; /* - * Advance the iter and clear state from the previous iteration. This - * passes iter->processed because that reflects the bytes processed but - * not yet advanced by the iter handler. - * * Use iter->len to determine whether to continue onto the next mapping. - * Explicitly terminate in the case where the current iter has not + * Explicitly terminate on error status or if the current iter has not * advanced at all (i.e. no work was done for some reason) unless the * mapping has been marked stale and needs to be reprocessed. */ - ret = iomap_iter_advance(iter, &processed); - if (!ret && iter->len > 0) - ret = 1; - if (ret > 0 && !advanced && !stale) + if (iter->processed < 0) + ret = iter->processed; + else if (iter->len == 0 || (!advanced && !stale)) ret = 0; + else + ret = 1; iomap_iter_reset_iomap(iter); if (ret <= 0) return ret;
At this point, all iomap operations have been updated to advance the iomap_iter directly before returning to iomap_iter(). Therefore, the complexity of handling both the old and new semantics is no longer required and can be removed from iomap_iter(). Update iomap_iter() to expect success or failure status in iter.processed. As a precaution and developer hint to prevent inadvertent use of old semantics, warn on a positive return code and fail the operation. Remove the unnecessary advance and simplify the termination logic. Signed-off-by: Brian Foster <bfoster@redhat.com> --- fs/iomap/iter.c | 39 +++++++++++++++------------------------ 1 file changed, 15 insertions(+), 24 deletions(-)