[PATCH v2 0/7] iomap: incremental per-operation iter advance

[PATCH v2 0/7] iomap: incremental per-operation iter advance

[Brian Foster]

Hi all,

Here's v2 of the iomap incremental iter advance series. This is mostly
the same idea as v1 with some of the cleanups noted below. The most
significant change is probably a bit more rework of the logic between
iomap_iter_advance() and iomap_iter() to make the former a bit more
generic. _advance() now returns the remaining length for the current
mapping for easier use in some of the iter handlers.

The other item of note for v1 was some discussion with Christoph over
changing over more of the operations to advance directly and avoid the
need to handle both types of advances in the iomap_iter() code. I don't
plan to address that in this series, but I've read through some of the
related code and think that perhaps this isn't as invasive as I
originally anticipated. Reason being that even some of the more complex
operations (i.e. direct I/O, buffered read) don't necessarily require
plumbing advances all the way down through the sub-helpers and whatnot
to eliminate the dependency from iomap_iter(). IOW, I think toplevel
direct I/O could just change to something like so:

	iter.processed = iomap_dio_iter(...);
	if (iter.processed > 0) {
		iomap_iter_advance(..., processed);
		iter.processed = 0;	/* success */
	}

... to translate the op-specific code into the updated iteration
semantics. Given that, I'll probably make a quick stab at that once this
series is settled and we'll see how that pans out.

Thoughts, reviews, flames appreciated.

Brian

v2:
- More refactoring of iomap_iter[_advance]() logic. Lifted out iter
  continuation and stale logic and improved comments.
- Renamed some poorly named helpers and variables.
- Return remaining length for current iter from _iter_advance() and use
  appropriately.
v1: https://lore.kernel.org/linux-fsdevel/20241213143610.1002526-1-bfoster@redhat.com/
- Reworked and fixed a bunch of functional issues.
RFC: https://lore.kernel.org/linux-fsdevel/20241125140623.20633-1-bfoster@redhat.com/

Brian Foster (7):
  iomap: split out iomap check and reset logic from iter advance
  iomap: factor out iomap length helper
  iomap: refactor iter and advance continuation logic
  iomap: support incremental iomap_iter advances
  iomap: advance the iter directly on buffered writes
  iomap: advance the iter directly on unshare range
  iomap: advance the iter directly on zero range

 fs/iomap/buffered-io.c | 50 ++++++++-------------
 fs/iomap/iter.c        | 99 ++++++++++++++++++++++++++----------------
 include/linux/iomap.h  | 20 ++++++---
 3 files changed, 94 insertions(+), 75 deletions(-)

-- 
2.47.1

[PATCH v2 1/7] iomap: split out iomap check and reset logic from iter advance

[Brian Foster]

In preparation for more granular iomap_iter advancing, break out
some of the logic associated with higher level iteration from
iomap_advance_iter(). Specifically, factor the iomap reset code into
a separate helper and lift the iomap.length check into the calling
code, similar to how ->iomap_end() calls are handled.

Signed-off-by: Brian Foster <bfoster@redhat.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 fs/iomap/iter.c | 49 ++++++++++++++++++++++++++-----------------------
 1 file changed, 26 insertions(+), 23 deletions(-)

diff --git a/fs/iomap/iter.c b/fs/iomap/iter.c
index 3790918646af..731ea7267f27 100644
--- a/fs/iomap/iter.c
+++ b/fs/iomap/iter.c
@@ -7,6 +7,13 @@
 #include <linux/iomap.h>
 #include "trace.h"
 
+static inline void iomap_iter_reset_iomap(struct iomap_iter *iter)
+{
+	iter->processed = 0;
+	memset(&iter->iomap, 0, sizeof(iter->iomap));
+	memset(&iter->srcmap, 0, sizeof(iter->srcmap));
+}
+
 /*
  * Advance to the next range we need to map.
  *
@@ -14,32 +21,24 @@
  * processed - it was aborted because the extent the iomap spanned may have been
  * changed during the operation. In this case, the iteration behaviour is to
  * remap the unprocessed range of the iter, and that means we may need to remap
- * even when we've made no progress (i.e. iter->processed = 0). Hence the
- * "finished iterating" case needs to distinguish between
- * (processed = 0) meaning we are done and (processed = 0 && stale) meaning we
- * need to remap the entire remaining range.
+ * even when we've made no progress (i.e. count = 0). Hence the "finished
+ * iterating" case needs to distinguish between (count = 0) meaning we are done
+ * and (count = 0 && stale) meaning we need to remap the entire remaining range.
  */
-static inline int iomap_iter_advance(struct iomap_iter *iter)
+static inline int iomap_iter_advance(struct iomap_iter *iter, s64 count)
 {
 	bool stale = iter->iomap.flags & IOMAP_F_STALE;
 	int ret = 1;
 
-	/* handle the previous iteration (if any) */
-	if (iter->iomap.length) {
-		if (iter->processed < 0)
-			return iter->processed;
-		if (WARN_ON_ONCE(iter->processed > iomap_length(iter)))
-			return -EIO;
-		iter->pos += iter->processed;
-		iter->len -= iter->processed;
-		if (!iter->len || (!iter->processed && !stale))
-			ret = 0;
-	}
+	if (count < 0)
+		return count;
+	if (WARN_ON_ONCE(count > iomap_length(iter)))
+		return -EIO;
+	iter->pos += count;
+	iter->len -= count;
+	if (!iter->len || (!count && !stale))
+		ret = 0;
 
-	/* clear the per iteration state */
-	iter->processed = 0;
-	memset(&iter->iomap, 0, sizeof(iter->iomap));
-	memset(&iter->srcmap, 0, sizeof(iter->srcmap));
 	return ret;
 }
 
@@ -82,10 +81,14 @@ int iomap_iter(struct iomap_iter *iter, const struct iomap_ops *ops)
 			return ret;
 	}
 
+	/* advance and clear state from the previous iteration */
 	trace_iomap_iter(iter, ops, _RET_IP_);
-	ret = iomap_iter_advance(iter);
-	if (ret <= 0)
-		return ret;
+	if (iter->iomap.length) {
+		ret = iomap_iter_advance(iter, iter->processed);
+		iomap_iter_reset_iomap(iter);
+		if (ret <= 0)
+			return ret;
+	}
 
 	ret = ops->iomap_begin(iter->inode, iter->pos, iter->len, iter->flags,
 			       &iter->iomap, &iter->srcmap);
-- 
2.47.1

[PATCH v2 2/7] iomap: factor out iomap length helper

[Brian Foster]

In preparation to support more granular iomap iter advancing, factor
the pos/len values as parameters to length calculation.

Signed-off-by: Brian Foster <bfoster@redhat.com>
---
 include/linux/iomap.h | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/include/linux/iomap.h b/include/linux/iomap.h
index 75bf54e76f3b..b6f7d96156f2 100644
--- a/include/linux/iomap.h
+++ b/include/linux/iomap.h
@@ -230,6 +230,16 @@ struct iomap_iter {
 
 int iomap_iter(struct iomap_iter *iter, const struct iomap_ops *ops);
 
+static inline u64 iomap_length_trim(const struct iomap_iter *iter, loff_t pos,
+		u64 len)
+{
+	u64 end = iter->iomap.offset + iter->iomap.length;
+
+	if (iter->srcmap.type != IOMAP_HOLE)
+		end = min(end, iter->srcmap.offset + iter->srcmap.length);
+	return min(len, end - pos);
+}
+
 /**
  * iomap_length - length of the current iomap iteration
  * @iter: iteration structure
@@ -238,11 +248,7 @@ int iomap_iter(struct iomap_iter *iter, const struct iomap_ops *ops);
  */
 static inline u64 iomap_length(const struct iomap_iter *iter)
 {
-	u64 end = iter->iomap.offset + iter->iomap.length;
-
-	if (iter->srcmap.type != IOMAP_HOLE)
-		end = min(end, iter->srcmap.offset + iter->srcmap.length);
-	return min(iter->len, end - iter->pos);
+	return iomap_length_trim(iter, iter->pos, iter->len);
 }
 
 /**
-- 
2.47.1

[PATCH v2 3/7] iomap: refactor iter and advance continuation logic

[Brian Foster]

In preparation for future changes and more generic use of
iomap_iter_advance(), lift the high level iter continuation logic
out of iomap_iter_advance() into the caller. Also add some comments
and rework iomap_iter() to jump straight to ->iomap_begin() on the
first iteration.

Signed-off-by: Brian Foster <bfoster@redhat.com>
---
 fs/iomap/iter.c       | 54 +++++++++++++++++++++++--------------------
 include/linux/iomap.h |  1 +
 2 files changed, 30 insertions(+), 25 deletions(-)

diff --git a/fs/iomap/iter.c b/fs/iomap/iter.c
index 731ea7267f27..260ec702ddd5 100644
--- a/fs/iomap/iter.c
+++ b/fs/iomap/iter.c
@@ -15,31 +15,19 @@ static inline void iomap_iter_reset_iomap(struct iomap_iter *iter)
 }
 
 /*
- * Advance to the next range we need to map.
- *
- * If the iomap is marked IOMAP_F_STALE, it means the existing map was not fully
- * processed - it was aborted because the extent the iomap spanned may have been
- * changed during the operation. In this case, the iteration behaviour is to
- * remap the unprocessed range of the iter, and that means we may need to remap
- * even when we've made no progress (i.e. count = 0). Hence the "finished
- * iterating" case needs to distinguish between (count = 0) meaning we are done
- * and (count = 0 && stale) meaning we need to remap the entire remaining range.
+ * Advance the current iterator position and return the length remaining for the
+ * current mapping.
  */
-static inline int iomap_iter_advance(struct iomap_iter *iter, s64 count)
+s64 iomap_iter_advance(struct iomap_iter *iter, s64 count)
 {
-	bool stale = iter->iomap.flags & IOMAP_F_STALE;
-	int ret = 1;
-
 	if (count < 0)
 		return count;
 	if (WARN_ON_ONCE(count > iomap_length(iter)))
 		return -EIO;
 	iter->pos += count;
 	iter->len -= count;
-	if (!iter->len || (!count && !stale))
-		ret = 0;
 
-	return ret;
+	return iomap_length(iter);
 }
 
 static inline void iomap_iter_done(struct iomap_iter *iter)
@@ -71,9 +59,15 @@ static inline void iomap_iter_done(struct iomap_iter *iter)
  */
 int iomap_iter(struct iomap_iter *iter, const struct iomap_ops *ops)
 {
-	int ret;
+	s64 ret;
+	bool stale = iter->iomap.flags & IOMAP_F_STALE;
 
-	if (iter->iomap.length && ops->iomap_end) {
+	if (!iter->iomap.length) {
+		trace_iomap_iter(iter, ops, _RET_IP_);
+		goto begin;
+	}
+
+	if (ops->iomap_end) {
 		ret = ops->iomap_end(iter->inode, iter->pos, iomap_length(iter),
 				iter->processed > 0 ? iter->processed : 0,
 				iter->flags, &iter->iomap);
@@ -81,15 +75,25 @@ int iomap_iter(struct iomap_iter *iter, const struct iomap_ops *ops)
 			return ret;
 	}
 
-	/* advance and clear state from the previous iteration */
+	/*
+	 * Advance the iter and clear state from the previous iteration. The
+	 * remaining length of the previous iteration should be zero by this
+	 * point, so use iter->len to determine whether to continue onto the
+	 * next mapping. Explicitly terminate in the case where 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.
+	 */
 	trace_iomap_iter(iter, ops, _RET_IP_);
-	if (iter->iomap.length) {
-		ret = iomap_iter_advance(iter, iter->processed);
-		iomap_iter_reset_iomap(iter);
-		if (ret <= 0)
-			return ret;
-	}
+	ret = iomap_iter_advance(iter, iter->processed);
+	if (!ret)
+		ret = iter->len;
+	if (ret > 0 && !iter->processed && !stale)
+		ret = 0;
+	iomap_iter_reset_iomap(iter);
+	if (ret <= 0)
+		return ret;
 
+begin:
 	ret = ops->iomap_begin(iter->inode, iter->pos, iter->len, iter->flags,
 			       &iter->iomap, &iter->srcmap);
 	if (ret < 0)
diff --git a/include/linux/iomap.h b/include/linux/iomap.h
index b6f7d96156f2..88bde0259035 100644
--- a/include/linux/iomap.h
+++ b/include/linux/iomap.h
@@ -229,6 +229,7 @@ struct iomap_iter {
 };
 
 int iomap_iter(struct iomap_iter *iter, const struct iomap_ops *ops);
+s64 iomap_iter_advance(struct iomap_iter *iter, s64 count);
 
 static inline u64 iomap_length_trim(const struct iomap_iter *iter, loff_t pos,
 		u64 len)
-- 
2.47.1

[PATCH v2 4/7] iomap: support incremental iomap_iter advances

[Brian Foster]

The current iomap_iter iteration model reads the mapping from the
filesystem, processes the subrange of the operation associated with
the current mapping, and returns the number of bytes processed back
to the iteration code. The latter advances the position and
remaining length of the iter in preparation for the next iteration.

At the _iter() handler level, this tends to produce a processing
loop where the local code pulls the current position and remaining
length out of the iter, iterates it locally based on file offset,
and then breaks out when the associated range has been fully
processed.

This works well enough for current handlers, but upcoming
enhancements require a bit more flexibility in certain situations.
Enhancements for zero range will lead to a situation where the
processing loop is no longer a pure ascending offset walk, but
rather dictated by pagecache state and folio lookup. Since folio
lookup and write preparation occur at different levels, it is more
difficult to manage position and length outside of the iter.

To provide more flexibility to certain iomap operations, introduce
support for incremental iomap_iter advances from within the
operation itself. This allows more granular advances for operations
that might not use the typical file offset based walk.

Note that the semantics for operations that use incremental advances
is slightly different than traditional operations. Operations that
advance the iter directly are expected to return success or failure
(i.e. 0 or negative error code) in iter.processed rather than the
number of bytes processed.

Signed-off-by: Brian Foster <bfoster@redhat.com>
---
 fs/iomap/iter.c       | 32 +++++++++++++++++++++++++-------
 include/linux/iomap.h |  3 +++
 2 files changed, 28 insertions(+), 7 deletions(-)

diff --git a/fs/iomap/iter.c b/fs/iomap/iter.c
index 260ec702ddd5..191c54976c9f 100644
--- a/fs/iomap/iter.c
+++ b/fs/iomap/iter.c
@@ -37,6 +37,8 @@ static inline void iomap_iter_done(struct iomap_iter *iter)
 	WARN_ON_ONCE(iter->iomap.offset + iter->iomap.length <= iter->pos);
 	WARN_ON_ONCE(iter->iomap.flags & IOMAP_F_STALE);
 
+	iter->iter_start_pos = iter->pos;
+
 	trace_iomap_iter_dstmap(iter->inode, &iter->iomap);
 	if (iter->srcmap.type != IOMAP_HOLE)
 		trace_iomap_iter_srcmap(iter->inode, &iter->srcmap);
@@ -61,23 +63,39 @@ int iomap_iter(struct iomap_iter *iter, const struct iomap_ops *ops)
 {
 	s64 ret;
 	bool stale = iter->iomap.flags & IOMAP_F_STALE;
+	ssize_t advanced = iter->processed > 0 ? iter->processed : 0;
+	u64 olen = iter->len;
 
 	if (!iter->iomap.length) {
 		trace_iomap_iter(iter, ops, _RET_IP_);
 		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().
+	 */
+	if (!advanced) {
+		advanced = iter->pos - iter->iter_start_pos;
+		olen += advanced;
+	}
+
 	if (ops->iomap_end) {
-		ret = ops->iomap_end(iter->inode, iter->pos, iomap_length(iter),
-				iter->processed > 0 ? iter->processed : 0,
-				iter->flags, &iter->iomap);
-		if (ret < 0 && !iter->processed)
+		ret = ops->iomap_end(iter->inode, iter->iter_start_pos,
+				iomap_length_trim(iter, iter->iter_start_pos,
+						  olen),
+				advanced, iter->flags, &iter->iomap);
+		if (ret < 0 && !advanced)
 			return ret;
 	}
 
 	/*
-	 * Advance the iter and clear state from the previous iteration. The
-	 * remaining length of the previous iteration should be zero by this
+	 * 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.
+	 *
+	 * The remaining length of the previous iteration should be zero by this
 	 * point, so use iter->len to determine whether to continue onto the
 	 * next mapping. Explicitly terminate in the case where the current iter
 	 * has not advanced at all (i.e. no work was done for some reason)
@@ -87,7 +105,7 @@ int iomap_iter(struct iomap_iter *iter, const struct iomap_ops *ops)
 	ret = iomap_iter_advance(iter, iter->processed);
 	if (!ret)
 		ret = iter->len;
-	if (ret > 0 && !iter->processed && !stale)
+	if (ret > 0 && !advanced && !stale)
 		ret = 0;
 	iomap_iter_reset_iomap(iter);
 	if (ret <= 0)
diff --git a/include/linux/iomap.h b/include/linux/iomap.h
index 88bde0259035..24fefc5fa868 100644
--- a/include/linux/iomap.h
+++ b/include/linux/iomap.h
@@ -211,6 +211,8 @@ struct iomap_ops {
  *	calls to iomap_iter().  Treat as read-only in the body.
  * @len: The remaining length of the file segment we're operating on.
  *	It is updated at the same time as @pos.
+ * @iter_start_pos: The original start pos for the current iomap. Used for
+ *	incremental iter advance.
  * @processed: The number of bytes processed by the body in the most recent
  *	iteration, or a negative errno. 0 causes the iteration to stop.
  * @flags: Zero or more of the iomap_begin flags above.
@@ -221,6 +223,7 @@ struct iomap_iter {
 	struct inode *inode;
 	loff_t pos;
 	u64 len;
+	loff_t iter_start_pos;
 	s64 processed;
 	unsigned flags;
 	struct iomap iomap;
-- 
2.47.1

[PATCH v2 5/7] iomap: advance the iter directly on buffered writes

[Brian Foster]

Modify the buffered write path to advance the iter directly. Replace
the local pos and length calculations with direct advances and loop
based on iter state instead.

Also remove the -EAGAIN return hack as it is no longer necessary now
that separate return channels exist for processing progress and error
returns. For example, the existing write handler must return either a
count of bytes written or error if the write is interrupted, but
presumably wants to return -EAGAIN directly in order to break the higher
level iomap_iter() loop.

Since the current iteration may have made some progress, it unwinds the
iter on the way out to return the error while ensuring that portion of
the write can be retried. If -EAGAIN occurs at any point beyond the
first iteration, iomap_file_buffered_write() will then observe progress
based on iter->pos to return a short write.

With incremental advances on the iomap_iter, iomap_write_iter() can
simply return the error. iomap_iter() completes whatever progress was
made based on iomap_iter position and still breaks out of the iter loop
based on the error code in iter.processed. The end result of the write
is similar in terms of being a short write if progress was made or error
return otherwise.

Signed-off-by: Brian Foster <bfoster@redhat.com>
---
 fs/iomap/buffered-io.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
index d303e6c8900c..5ce5ac13765a 100644
--- a/fs/iomap/buffered-io.c
+++ b/fs/iomap/buffered-io.c
@@ -909,8 +909,6 @@ static bool iomap_write_end(struct iomap_iter *iter, loff_t pos, size_t len,
 
 static loff_t iomap_write_iter(struct iomap_iter *iter, struct iov_iter *i)
 {
-	loff_t length = iomap_length(iter);
-	loff_t pos = iter->pos;
 	ssize_t total_written = 0;
 	long status = 0;
 	struct address_space *mapping = iter->inode->i_mapping;
@@ -924,6 +922,7 @@ static loff_t iomap_write_iter(struct iomap_iter *iter, struct iov_iter *i)
 		size_t bytes;		/* Bytes to write to folio */
 		size_t copied;		/* Bytes copied from user */
 		size_t written;		/* Bytes have been written */
+		loff_t pos = iter->pos;
 
 		bytes = iov_iter_count(i);
 retry:
@@ -934,8 +933,8 @@ static loff_t iomap_write_iter(struct iomap_iter *iter, struct iov_iter *i)
 		if (unlikely(status))
 			break;
 
-		if (bytes > length)
-			bytes = length;
+		if (bytes > iomap_length(iter))
+			bytes = iomap_length(iter);
 
 		/*
 		 * Bring in the user page that we'll copy from _first_.
@@ -1006,17 +1005,12 @@ static loff_t iomap_write_iter(struct iomap_iter *iter, struct iov_iter *i)
 				goto retry;
 			}
 		} else {
-			pos += written;
 			total_written += written;
-			length -= written;
+			iomap_iter_advance(iter, written);
 		}
-	} while (iov_iter_count(i) && length);
+	} while (iov_iter_count(i) && iomap_length(iter));
 
-	if (status == -EAGAIN) {
-		iov_iter_revert(i, total_written);
-		return -EAGAIN;
-	}
-	return total_written ? total_written : status;
+	return total_written ? 0 : status;
 }
 
 ssize_t
-- 
2.47.1

[PATCH v2 6/7] iomap: advance the iter directly on unshare range

[Brian Foster]

Modify unshare range to advance the iter directly. Replace the local
pos and length calculations with direct advances and loop based on
iter state instead.

Signed-off-by: Brian Foster <bfoster@redhat.com>
---
 fs/iomap/buffered-io.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
index 5ce5ac13765a..ea140d3098ff 100644
--- a/fs/iomap/buffered-io.c
+++ b/fs/iomap/buffered-io.c
@@ -1267,20 +1267,19 @@ EXPORT_SYMBOL_GPL(iomap_write_delalloc_release);
 static loff_t iomap_unshare_iter(struct iomap_iter *iter)
 {
 	struct iomap *iomap = &iter->iomap;
-	loff_t pos = iter->pos;
-	loff_t length = iomap_length(iter);
-	loff_t written = 0;
+	size_t bytes = iomap_length(iter);
 
 	if (!iomap_want_unshare_iter(iter))
-		return length;
+		return bytes;
 
 	do {
 		struct folio *folio;
 		int status;
 		size_t offset;
-		size_t bytes = min_t(u64, SIZE_MAX, length);
+		loff_t pos = iter->pos;
 		bool ret;
 
+		bytes = min_t(u64, SIZE_MAX, bytes);
 		status = iomap_write_begin(iter, pos, bytes, &folio);
 		if (unlikely(status))
 			return status;
@@ -1298,14 +1297,10 @@ static loff_t iomap_unshare_iter(struct iomap_iter *iter)
 
 		cond_resched();
 
-		pos += bytes;
-		written += bytes;
-		length -= bytes;
-
 		balance_dirty_pages_ratelimited(iter->inode->i_mapping);
-	} while (length > 0);
+	} while ((bytes = iomap_iter_advance(iter, bytes)) > 0);
 
-	return written;
+	return bytes < 0 ? bytes : 0;
 }
 
 int
-- 
2.47.1

[PATCH v2 7/7] iomap: advance the iter directly on zero range

[Brian Foster]

Modify zero range to advance the iter directly. Replace the local pos
and length calculations with direct advances and loop based on iter
state instead.

Signed-off-by: Brian Foster <bfoster@redhat.com>
---
 fs/iomap/buffered-io.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
index ea140d3098ff..83ef6715a4b6 100644
--- a/fs/iomap/buffered-io.c
+++ b/fs/iomap/buffered-io.c
@@ -1341,17 +1341,16 @@ static inline int iomap_zero_iter_flush_and_stale(struct iomap_iter *i)
 
 static loff_t iomap_zero_iter(struct iomap_iter *iter, bool *did_zero)
 {
-	loff_t pos = iter->pos;
-	loff_t length = iomap_length(iter);
-	loff_t written = 0;
+	size_t bytes = iomap_length(iter);
 
 	do {
 		struct folio *folio;
 		int status;
 		size_t offset;
-		size_t bytes = min_t(u64, SIZE_MAX, length);
+		loff_t pos = iter->pos;
 		bool ret;
 
+		bytes = min_t(u64, SIZE_MAX, bytes);
 		status = iomap_write_begin(iter, pos, bytes, &folio);
 		if (status)
 			return status;
@@ -1371,15 +1370,11 @@ static loff_t iomap_zero_iter(struct iomap_iter *iter, bool *did_zero)
 		__iomap_put_folio(iter, pos, bytes, folio);
 		if (WARN_ON_ONCE(!ret))
 			return -EIO;
-
-		pos += bytes;
-		length -= bytes;
-		written += bytes;
-	} while (length > 0);
+	} while ((bytes = iomap_iter_advance(iter, bytes)) > 0);
 
 	if (did_zero)
 		*did_zero = true;
-	return written;
+	return bytes < 0 ? bytes : 0;
 }
 
 int
-- 
2.47.1

Re: [PATCH v2 2/7] iomap: factor out iomap length helper

[Christoph Hellwig]

> +static inline u64 iomap_length_trim(const struct iomap_iter *iter, loff_t pos,
> +		u64 len)
> +{
> +	u64 end = iter->iomap.offset + iter->iomap.length;
> +
> +	if (iter->srcmap.type != IOMAP_HOLE)
> +		end = min(end, iter->srcmap.offset + iter->srcmap.length);
> +	return min(len, end - pos);

Does this helper warrant a kerneldoc comment similar to iomap_length?

Otherwise looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH v2 3/7] iomap: refactor iter and advance continuation logic

[Christoph Hellwig]

On Wed, Jan 22, 2025 at 08:34:30AM -0500, Brian Foster wrote:
> +	s64 ret;
> +	bool stale = iter->iomap.flags & IOMAP_F_STALE;

Nit: I find code more redable if variables that initialized at
declaration time (especially when derived from arguments) are
before plain variable declarations.  Not a big thing here with just
two of them, but variable counts keep growing over time.

>  
> -	if (iter->iomap.length && ops->iomap_end) {
> +	if (!iter->iomap.length) {
> +		trace_iomap_iter(iter, ops, _RET_IP_);
> +		goto begin;
> +	}

This might be a chance to split trace_iomap_iter into two trace points
for the initial and following iterations?  Or maybe we shouldn't bother.

Otherwise this looks great:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH v2 4/7] iomap: support incremental iomap_iter advances

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH v2 5/7] iomap: advance the iter directly on buffered writes

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH v2 6/7] iomap: advance the iter directly on unshare range

[Christoph Hellwig]

On Wed, Jan 22, 2025 at 08:34:33AM -0500, Brian Foster wrote:
> +	size_t bytes = iomap_length(iter);

> +		bytes = min_t(u64, SIZE_MAX, bytes);

bytes needs to be a u64 for the min logic to work on 32-bit systems.

Re: [PATCH v2 7/7] iomap: advance the iter directly on zero range

[Christoph Hellwig]

> +	size_t bytes = iomap_length(iter);

Same thing about the type of bytes here.

Otherwise looks good;

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH v2 2/7] iomap: factor out iomap length helper

[Brian Foster]

On Mon, Jan 27, 2025 at 09:26:15PM -0800, Christoph Hellwig wrote:
> > +static inline u64 iomap_length_trim(const struct iomap_iter *iter, loff_t pos,
> > +		u64 len)
> > +{
> > +	u64 end = iter->iomap.offset + iter->iomap.length;
> > +
> > +	if (iter->srcmap.type != IOMAP_HOLE)
> > +		end = min(end, iter->srcmap.offset + iter->srcmap.length);
> > +	return min(len, end - pos);
> 
> Does this helper warrant a kerneldoc comment similar to iomap_length?
> 

Can't hurt I suppose. I'll add one.

> Otherwise looks good:
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> 

Thanks.

Brian

Re: [PATCH v2 3/7] iomap: refactor iter and advance continuation logic

[Brian Foster]

On Mon, Jan 27, 2025 at 09:34:15PM -0800, Christoph Hellwig wrote:
> On Wed, Jan 22, 2025 at 08:34:30AM -0500, Brian Foster wrote:
> > +	s64 ret;
> > +	bool stale = iter->iomap.flags & IOMAP_F_STALE;
> 
> Nit: I find code more redable if variables that initialized at
> declaration time (especially when derived from arguments) are
> before plain variable declarations.  Not a big thing here with just
> two of them, but variable counts keep growing over time.
> 

Ack. I have a couple pending changes for followon work I wanted to fold
into a v3 anyways, so I'll make that tweak.

> >  
> > -	if (iter->iomap.length && ops->iomap_end) {
> > +	if (!iter->iomap.length) {
> > +		trace_iomap_iter(iter, ops, _RET_IP_);
> > +		goto begin;
> > +	}
> 
> This might be a chance to split trace_iomap_iter into two trace points
> for the initial and following iterations?  Or maybe we shouldn't bother.
> 

Hmm.. not sure I see the value in a tracepoint just for the initial
case, but maybe we should just move trace_iomap_iter() to the top of the
function? We already have post-lookup tracepoints in iomap_iter_done()
to show the mappings, and that would remove the duplication. Hm?

> Otherwise this looks great:
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> 

Thanks.

Brian

Re: [PATCH v2 6/7] iomap: advance the iter directly on unshare range

[Brian Foster]

On Mon, Jan 27, 2025 at 09:39:01PM -0800, Christoph Hellwig wrote:
> On Wed, Jan 22, 2025 at 08:34:33AM -0500, Brian Foster wrote:
> > +	size_t bytes = iomap_length(iter);
> 
> > +		bytes = min_t(u64, SIZE_MAX, bytes);
> 
> bytes needs to be a u64 for the min logic to work on 32-bit systems.
> 

Ah, thanks. FYI, I also have the following change from followon work to
fold into this to completely remove advances via iter.processed:

-       if (!iomap_want_unshare_iter(iter))
-               return bytes;
+       if (!iomap_want_unshare_iter(iter)) {
+               iomap_iter_advance(iter, bytes);
+               return 0;
+       }

And the analogous change in the next patch for zero range (unwritten &&
!range_dirty) as well.

Finally, I'm still working through converting the rest of the ops to use
iomap_iter_advance(), but I was thinking about renaming iter.processed
to iter.status as a final step. Thoughts on a rename in general or on
the actual name?

Brian

Re: [PATCH v2 6/7] iomap: advance the iter directly on unshare range

[Brian Foster]

On Mon, Jan 27, 2025 at 09:39:01PM -0800, Christoph Hellwig wrote:
> On Wed, Jan 22, 2025 at 08:34:33AM -0500, Brian Foster wrote:
> > +	size_t bytes = iomap_length(iter);
> 
> > +		bytes = min_t(u64, SIZE_MAX, bytes);
> 
> bytes needs to be a u64 for the min logic to work on 32-bit systems.
> 

Err.. I think there's another bug here. I changed iomap_iter_advance()
to return s64 so it could return length or an error, but never changed
bytes over from size_t.

But that raises another question. I'd want bytes to be s64 here to
support the current factoring, but iomap_length() returns a u64. In
poking around a bit I _think_ this is practically safe because the high
level operations are bound by loff_t (int64_t), so IIUC that means we
shouldn't actually see a length that doesn't fit in s64.

That said, that still seems a bit grotty. Perhaps one option could be to
tweak iomap_length() to return something like this:

	min_t(u64, SSIZE_MAX, end);

... to at least makes things explicit.

Another option could be to rework advance back to something like:

	int iomap_iter_advance(..., u64 *count);

... but where it returns 0 or -EIO and advances/updates *count directly.
That would mean I'd have to tweak some of the loop factoring and lift
out the error passthru assignment logic from iomap_iter(). The latter
doesn't seem like a big deal. It's mostly pointless after these changes.
I'd guess the (i.e. iomap_file_unshare()) loop logic would look more
like:

	do {
		...
		ret = iomap_iter_advance(iter, &bytes);
	} while (!ret && bytes > 0);

	return ret;

Hmm.. now that I write it out that doesn't seem so bad. It does clean up
the return path a bit. I think I'll play around with that, but let me
know if there are other thoughts or ideas..

Brian

Re: [PATCH v2 3/7] iomap: refactor iter and advance continuation logic

[Christoph Hellwig]

On Tue, Jan 28, 2025 at 08:55:05AM -0500, Brian Foster wrote:
> > > +	if (!iter->iomap.length) {
> > > +		trace_iomap_iter(iter, ops, _RET_IP_);
> > > +		goto begin;
> > > +	}
> > 
> > This might be a chance to split trace_iomap_iter into two trace points
> > for the initial and following iterations?  Or maybe we shouldn't bother.
> > 
> 
> Hmm.. not sure I see the value in a tracepoint just for the initial
> case, but maybe we should just move trace_iomap_iter() to the top of the
> function? We already have post-lookup tracepoints in iomap_iter_done()
> to show the mappings, and that would remove the duplication. Hm?

Sounds sensible.

Re: [PATCH v2 6/7] iomap: advance the iter directly on unshare range

[Christoph Hellwig]

On Tue, Jan 28, 2025 at 12:59:09PM -0500, Brian Foster wrote:
> But that raises another question. I'd want bytes to be s64 here to
> support the current factoring, but iomap_length() returns a u64. In
> poking around a bit I _think_ this is practically safe because the high
> level operations are bound by loff_t (int64_t), so IIUC that means we
> shouldn't actually see a length that doesn't fit in s64.
> 
> That said, that still seems a bit grotty. Perhaps one option could be to
> tweak iomap_length() to return something like this:
> 
> 	min_t(u64, SSIZE_MAX, end);
> 
> ... to at least makes things explicit.

Yeah.  I'm actually not sure why went want to support 64-bit ranges.
I don't even remember if this comes from Dave's really first version
or was my idea, but in hindsight just sticking to ssize_t bounds
would have been smarter.

> I'd guess the (i.e. iomap_file_unshare()) loop logic would look more
> like:
> 
> 	do {
> 		...
> 		ret = iomap_iter_advance(iter, &bytes);
> 	} while (!ret && bytes > 0);
> 
> 	return ret;
> 
> Hmm.. now that I write it out that doesn't seem so bad. It does clean up
> the return path a bit. I think I'll play around with that, but let me
> know if there are other thoughts or ideas..

Given that all the kernel read/write code mixes up bytes and negative
return values I think doing that in iomap is also fine.  But you are
deeper into the code right now, and if you think splitting the errno
and bytes is cleaner that sounds perfectly fine to me as well.  In
general not overloading a single return value with two things tends
to lead to cleaner code.

Although the above sniplet (I´m not sure how representative it is
anyway) would be a bit nicer as the slightly more verbose version
below:

	do {
		...
		ret = iomap_iter_advance(iter, &bytes);
		if (ret)
			return ret;
	} while (bytes > 0);

Re: [PATCH v2 6/7] iomap: advance the iter directly on unshare range

[Christoph Hellwig]

On Tue, Jan 28, 2025 at 08:57:27AM -0500, Brian Foster wrote:
> And the analogous change in the next patch for zero range (unwritten &&
> !range_dirty) as well.
> 
> Finally, I'm still working through converting the rest of the ops to use
> iomap_iter_advance(), but I was thinking about renaming iter.processed
> to iter.status as a final step. Thoughts on a rename in general or on
> the actual name?

Yeah, having a processed with either a count or status has proven to
not be the greatest design ever, and once that is gone picking a better
name is a good idea.  status sounds fine.  The variables tend to be
name error or err, maybe that's a little better than status?  I don't
really care strongly either way.

Re: [PATCH v2 6/7] iomap: advance the iter directly on unshare range

[Brian Foster]

On Tue, Jan 28, 2025 at 09:56:10PM -0800, Christoph Hellwig wrote:
> On Tue, Jan 28, 2025 at 12:59:09PM -0500, Brian Foster wrote:
> > But that raises another question. I'd want bytes to be s64 here to
> > support the current factoring, but iomap_length() returns a u64. In
> > poking around a bit I _think_ this is practically safe because the high
> > level operations are bound by loff_t (int64_t), so IIUC that means we
> > shouldn't actually see a length that doesn't fit in s64.
> > 
> > That said, that still seems a bit grotty. Perhaps one option could be to
> > tweak iomap_length() to return something like this:
> > 
> > 	min_t(u64, SSIZE_MAX, end);
> > 
> > ... to at least makes things explicit.
> 
> Yeah.  I'm actually not sure why went want to support 64-bit ranges.
> I don't even remember if this comes from Dave's really first version
> or was my idea, but in hindsight just sticking to ssize_t bounds
> would have been smarter.
> 

Ok, thanks.

> > I'd guess the (i.e. iomap_file_unshare()) loop logic would look more
> > like:
> > 
> > 	do {
> > 		...
> > 		ret = iomap_iter_advance(iter, &bytes);
> > 	} while (!ret && bytes > 0);
> > 
> > 	return ret;
> > 
> > Hmm.. now that I write it out that doesn't seem so bad. It does clean up
> > the return path a bit. I think I'll play around with that, but let me
> > know if there are other thoughts or ideas..
> 
> Given that all the kernel read/write code mixes up bytes and negative
> return values I think doing that in iomap is also fine.  But you are
> deeper into the code right now, and if you think splitting the errno
> and bytes is cleaner that sounds perfectly fine to me as well.  In
> general not overloading a single return value with two things tends
> to lead to cleaner code.
> 

Eh, I like the factoring that the combined return allows better, but I
don't want to get too clever and introduce type issues and whatnot in
the middle of these patches if I can help it. From what I see so far the
change to split out the error return uglifies things slightly in
iomap_iter(), but the flipside is that with the error check lifted out
the advance call from iomap_iter() can go away completely once
everything is switched over.

So if we do go with the int return for now (still testing), I might
revisit a change back to a combined s64 return (perhaps along with the
iomap_length() tweak above) in the future as a standalone cleanup when
this is all more settled and I have more mental bandwidth to think about
it. Thanks for the input.

> Although the above sniplet (I´m not sure how representative it is
> anyway) would be a bit nicer as the slightly more verbose version
> below:
> 
> 	do {
> 		...
> 		ret = iomap_iter_advance(iter, &bytes);
> 		if (ret)
> 			return ret;
> 	} while (bytes > 0);
> 

Ack.

Brian