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@xxxxxxxxxx>
Looks reasonable,
Reviewed-by: "Darrick J. Wong" <djwong@xxxxxxxxxx>
--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
>
>
