linux-fsdevel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v6 00/10] iomap: incremental per-operation iter advance
@ 2025-02-07 14:32 Brian Foster
  2025-02-07 14:32 ` [PATCH v6 01/10] iomap: factor out iomap length helper Brian Foster
                   ` (10 more replies)
  0 siblings, 11 replies; 15+ messages in thread
From: Brian Foster @ 2025-02-07 14:32 UTC (permalink / raw)
  To: linux-fsdevel; +Cc: linux-xfs, Christoph Hellwig, Darrick J . Wong

Hi all,

Just a couple comment changes, no code changes from v5.

Brian

v6:
- Comment updates in patches 6 and 7.
v5: https://lore.kernel.org/linux-fsdevel/20250205135821.178256-1-bfoster@redhat.com/
- Fixed refactoring bug in v4 by pulling 'processed' local var into
  patch 4.
v4: https://lore.kernel.org/linux-fsdevel/20250204133044.80551-1-bfoster@redhat.com/
- Reordered patches 1 and 2 to keep iter advance cleanups together.
- Split patch 3 from v3 into patches 3-6.
v3: https://lore.kernel.org/linux-fsdevel/20250130170949.916098-1-bfoster@redhat.com/
- Code style and comment fixups.
- Variable type fixups and rework of iomap_iter_advance() to return
  error/length separately.
- Advance the iter on unshare and zero range skip cases instead of
  returning length.
v2: https://lore.kernel.org/linux-fsdevel/20250122133434.535192-1-bfoster@redhat.com/
- 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 (10):
  iomap: factor out iomap length helper
  iomap: split out iomap check and reset logic from iter advance
  iomap: refactor iomap_iter() length check and tracepoint
  iomap: lift error code check out of iomap_iter_advance()
  iomap: lift iter termination logic from iomap_iter_advance()
  iomap: export iomap_iter_advance() and return remaining length
  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 |  67 +++++++++++++--------------
 fs/iomap/iter.c        | 102 ++++++++++++++++++++++++++---------------
 include/linux/iomap.h  |  32 ++++++++++---
 3 files changed, 122 insertions(+), 79 deletions(-)

-- 
2.48.1


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v6 01/10] iomap: factor out iomap length helper
  2025-02-07 14:32 [PATCH v6 00/10] iomap: incremental per-operation iter advance Brian Foster
@ 2025-02-07 14:32 ` Brian Foster
  2025-02-07 14:32 ` [PATCH v6 02/10] iomap: split out iomap check and reset logic from iter advance Brian Foster
                   ` (9 subsequent siblings)
  10 siblings, 0 replies; 15+ messages in thread
From: Brian Foster @ 2025-02-07 14:32 UTC (permalink / raw)
  To: linux-fsdevel; +Cc: linux-xfs, Christoph Hellwig, Darrick J . Wong

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>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>
---
 include/linux/iomap.h | 23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)

diff --git a/include/linux/iomap.h b/include/linux/iomap.h
index 75bf54e76f3b..f5ca71ac2fa2 100644
--- a/include/linux/iomap.h
+++ b/include/linux/iomap.h
@@ -231,18 +231,33 @@ struct iomap_iter {
 int iomap_iter(struct iomap_iter *iter, const struct iomap_ops *ops);
 
 /**
- * iomap_length - length of the current iomap iteration
+ * iomap_length_trim - trimmed length of the current iomap iteration
  * @iter: iteration structure
+ * @pos: File position to trim from.
+ * @len: Length of the mapping to trim to.
  *
- * Returns the length that the operation applies to for the current iteration.
+ * Returns a trimmed length that the operation applies to for the current
+ * iteration.
  */
-static inline u64 iomap_length(const struct iomap_iter *iter)
+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(iter->len, end - iter->pos);
+	return min(len, end - pos);
+}
+
+/**
+ * iomap_length - length of the current iomap iteration
+ * @iter: iteration structure
+ *
+ * Returns the length that the operation applies to for the current iteration.
+ */
+static inline u64 iomap_length(const struct iomap_iter *iter)
+{
+	return iomap_length_trim(iter, iter->pos, iter->len);
 }
 
 /**
-- 
2.48.1


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v6 02/10] iomap: split out iomap check and reset logic from iter advance
  2025-02-07 14:32 [PATCH v6 00/10] iomap: incremental per-operation iter advance Brian Foster
  2025-02-07 14:32 ` [PATCH v6 01/10] iomap: factor out iomap length helper Brian Foster
@ 2025-02-07 14:32 ` Brian Foster
  2025-02-07 14:32 ` [PATCH v6 03/10] iomap: refactor iomap_iter() length check and tracepoint Brian Foster
                   ` (8 subsequent siblings)
  10 siblings, 0 replies; 15+ messages in thread
From: Brian Foster @ 2025-02-07 14:32 UTC (permalink / raw)
  To: linux-fsdevel; +Cc: linux-xfs, Christoph Hellwig, Darrick J . Wong

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>
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>
---
 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.48.1


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v6 03/10] iomap: refactor iomap_iter() length check and tracepoint
  2025-02-07 14:32 [PATCH v6 00/10] iomap: incremental per-operation iter advance Brian Foster
  2025-02-07 14:32 ` [PATCH v6 01/10] iomap: factor out iomap length helper Brian Foster
  2025-02-07 14:32 ` [PATCH v6 02/10] iomap: split out iomap check and reset logic from iter advance Brian Foster
@ 2025-02-07 14:32 ` Brian Foster
  2025-02-07 14:32 ` [PATCH v6 04/10] iomap: lift error code check out of iomap_iter_advance() Brian Foster
                   ` (7 subsequent siblings)
  10 siblings, 0 replies; 15+ messages in thread
From: Brian Foster @ 2025-02-07 14:32 UTC (permalink / raw)
  To: linux-fsdevel; +Cc: linux-xfs, Christoph Hellwig, Darrick J . Wong

iomap_iter() checks iomap.length to skip individual code blocks not
appropriate for the initial case where there is no mapping in the
iter. To prepare for upcoming changes, refactor the code to jump
straight to the ->iomap_begin() handler in the initial case and move
the tracepoint to the top of the function so it always executes.

Signed-off-by: Brian Foster <bfoster@redhat.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>
---
 fs/iomap/iter.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/fs/iomap/iter.c b/fs/iomap/iter.c
index 731ea7267f27..a2ae99fe6431 100644
--- a/fs/iomap/iter.c
+++ b/fs/iomap/iter.c
@@ -73,7 +73,12 @@ int iomap_iter(struct iomap_iter *iter, const struct iomap_ops *ops)
 {
 	int ret;
 
-	if (iter->iomap.length && ops->iomap_end) {
+	trace_iomap_iter(iter, ops, _RET_IP_);
+
+	if (!iter->iomap.length)
+		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);
@@ -82,14 +87,12 @@ int iomap_iter(struct iomap_iter *iter, const struct iomap_ops *ops)
 	}
 
 	/* advance and clear state from the previous iteration */
-	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);
+	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)
-- 
2.48.1


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v6 04/10] iomap: lift error code check out of iomap_iter_advance()
  2025-02-07 14:32 [PATCH v6 00/10] iomap: incremental per-operation iter advance Brian Foster
                   ` (2 preceding siblings ...)
  2025-02-07 14:32 ` [PATCH v6 03/10] iomap: refactor iomap_iter() length check and tracepoint Brian Foster
@ 2025-02-07 14:32 ` Brian Foster
  2025-02-07 14:32 ` [PATCH v6 05/10] iomap: lift iter termination logic from iomap_iter_advance() Brian Foster
                   ` (6 subsequent siblings)
  10 siblings, 0 replies; 15+ messages in thread
From: Brian Foster @ 2025-02-07 14:32 UTC (permalink / raw)
  To: linux-fsdevel; +Cc: linux-xfs, Christoph Hellwig, Darrick J . Wong

The error code is only used to check whether iomap_iter() should
terminate due to an error returned in iter.processed. Lift the check
out of iomap_iter_advance() in preparation to make it more generic.

Signed-off-by: Brian Foster <bfoster@redhat.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>
---
 fs/iomap/iter.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/fs/iomap/iter.c b/fs/iomap/iter.c
index a2ae99fe6431..1db16be7b9f0 100644
--- a/fs/iomap/iter.c
+++ b/fs/iomap/iter.c
@@ -30,8 +30,6 @@ static inline int 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;
@@ -71,6 +69,7 @@ static inline void iomap_iter_done(struct iomap_iter *iter)
  */
 int iomap_iter(struct iomap_iter *iter, const struct iomap_ops *ops)
 {
+	s64 processed;
 	int ret;
 
 	trace_iomap_iter(iter, ops, _RET_IP_);
@@ -86,8 +85,14 @@ 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;
+	}
+
 	/* advance and clear state from the previous iteration */
-	ret = iomap_iter_advance(iter, iter->processed);
+	ret = iomap_iter_advance(iter, processed);
 	iomap_iter_reset_iomap(iter);
 	if (ret <= 0)
 		return ret;
-- 
2.48.1


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v6 05/10] iomap: lift iter termination logic from iomap_iter_advance()
  2025-02-07 14:32 [PATCH v6 00/10] iomap: incremental per-operation iter advance Brian Foster
                   ` (3 preceding siblings ...)
  2025-02-07 14:32 ` [PATCH v6 04/10] iomap: lift error code check out of iomap_iter_advance() Brian Foster
@ 2025-02-07 14:32 ` Brian Foster
  2025-02-07 14:32 ` [PATCH v6 06/10] iomap: export iomap_iter_advance() and return remaining length Brian Foster
                   ` (5 subsequent siblings)
  10 siblings, 0 replies; 15+ messages in thread
From: Brian Foster @ 2025-02-07 14:32 UTC (permalink / raw)
  To: linux-fsdevel; +Cc: linux-xfs, Christoph Hellwig, Darrick J . Wong

The iter termination logic in iomap_iter_advance() is only needed by
iomap_iter() to determine whether to proceed with the next mapping
for an ongoing operation. The old logic sets ret to 1 and then
terminates if the operation is complete (iter->len == 0) or the
previous iteration performed no work and the mapping has not been
marked stale. The stale check exists to allow operations to
retry the current mapping if an inconsistency has been detected.

To further genericize iomap_iter_advance(), lift the termination
logic into iomap_iter() and update the former to return success (0)
or an error code. iomap_iter() continues on successful advance and
non-zero iter->len or otherwise terminates in the no progress (and
not stale) or error cases.

Signed-off-by: Brian Foster <bfoster@redhat.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>
---
 fs/iomap/iter.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/fs/iomap/iter.c b/fs/iomap/iter.c
index 1db16be7b9f0..8e0746ad80bd 100644
--- a/fs/iomap/iter.c
+++ b/fs/iomap/iter.c
@@ -27,17 +27,11 @@ static inline void iomap_iter_reset_iomap(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;
-
 	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 0;
 }
 
 static inline void iomap_iter_done(struct iomap_iter *iter)
@@ -69,6 +63,7 @@ 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;
 	s64 processed;
 	int ret;
 
@@ -91,8 +86,18 @@ int iomap_iter(struct iomap_iter *iter, const struct iomap_ops *ops)
 		return processed;
 	}
 
-	/* advance and clear state from the previous iteration */
+	/*
+	 * Advance the iter and clear state from the previous iteration. 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.
+	 */
 	ret = iomap_iter_advance(iter, processed);
+	if (!ret && iter->len > 0)
+		ret = 1;
+	if (ret > 0 && !iter->processed && !stale)
+		ret = 0;
 	iomap_iter_reset_iomap(iter);
 	if (ret <= 0)
 		return ret;
-- 
2.48.1


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v6 06/10] iomap: export iomap_iter_advance() and return remaining length
  2025-02-07 14:32 [PATCH v6 00/10] iomap: incremental per-operation iter advance Brian Foster
                   ` (4 preceding siblings ...)
  2025-02-07 14:32 ` [PATCH v6 05/10] iomap: lift iter termination logic from iomap_iter_advance() Brian Foster
@ 2025-02-07 14:32 ` Brian Foster
  2025-02-07 17:17   ` Darrick J. Wong
  2025-02-07 14:32 ` [PATCH v6 07/10] iomap: support incremental iomap_iter advances Brian Foster
                   ` (4 subsequent siblings)
  10 siblings, 1 reply; 15+ messages in thread
From: Brian Foster @ 2025-02-07 14:32 UTC (permalink / raw)
  To: linux-fsdevel; +Cc: linux-xfs, Christoph Hellwig, Darrick J . Wong

As a final step for generic iter advance, export the helper and
update it to return the remaining length of the current iteration
after the advance. This will usually be 0 in the iomap_iter() case,
but will be useful for the various operations that iterate on their
own and will be updated to advance as they progress.

Signed-off-by: Brian Foster <bfoster@redhat.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 fs/iomap/iter.c       | 22 ++++++++--------------
 include/linux/iomap.h |  1 +
 2 files changed, 9 insertions(+), 14 deletions(-)

diff --git a/fs/iomap/iter.c b/fs/iomap/iter.c
index 8e0746ad80bd..544cd7a5a16b 100644
--- a/fs/iomap/iter.c
+++ b/fs/iomap/iter.c
@@ -15,22 +15,16 @@ 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 output the length remaining for the
+ * current mapping.
  */
-static inline int iomap_iter_advance(struct iomap_iter *iter, s64 count)
+int iomap_iter_advance(struct iomap_iter *iter, u64 *count)
 {
-	if (WARN_ON_ONCE(count > iomap_length(iter)))
+	if (WARN_ON_ONCE(*count > iomap_length(iter)))
 		return -EIO;
-	iter->pos += count;
-	iter->len -= count;
+	iter->pos += *count;
+	iter->len -= *count;
+	*count = iomap_length(iter);
 	return 0;
 }
 
@@ -93,7 +87,7 @@ int iomap_iter(struct iomap_iter *iter, const struct iomap_ops *ops)
 	 * 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);
+	ret = iomap_iter_advance(iter, &processed);
 	if (!ret && iter->len > 0)
 		ret = 1;
 	if (ret > 0 && !iter->processed && !stale)
diff --git a/include/linux/iomap.h b/include/linux/iomap.h
index f5ca71ac2fa2..f304c602e5fe 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);
+int iomap_iter_advance(struct iomap_iter *iter, u64 *count);
 
 /**
  * iomap_length_trim - trimmed length of the current iomap iteration
-- 
2.48.1


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v6 07/10] iomap: support incremental iomap_iter advances
  2025-02-07 14:32 [PATCH v6 00/10] iomap: incremental per-operation iter advance Brian Foster
                   ` (5 preceding siblings ...)
  2025-02-07 14:32 ` [PATCH v6 06/10] iomap: export iomap_iter_advance() and return remaining length Brian Foster
@ 2025-02-07 14:32 ` Brian Foster
  2025-02-07 17:18   ` Darrick J. Wong
  2025-02-07 14:32 ` [PATCH v6 08/10] iomap: advance the iter directly on buffered writes Brian Foster
                   ` (3 subsequent siblings)
  10 siblings, 1 reply; 15+ messages in thread
From: Brian Foster @ 2025-02-07 14:32 UTC (permalink / raw)
  To: linux-fsdevel; +Cc: linux-xfs, Christoph Hellwig, Darrick J . Wong

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>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 fs/iomap/iter.c       | 32 +++++++++++++++++++++++++-------
 include/linux/iomap.h |  8 ++++++--
 2 files changed, 31 insertions(+), 9 deletions(-)

diff --git a/fs/iomap/iter.c b/fs/iomap/iter.c
index 544cd7a5a16b..0ebcabc7df52 100644
--- a/fs/iomap/iter.c
+++ b/fs/iomap/iter.c
@@ -35,6 +35,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);
@@ -58,6 +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;
 	int ret;
 
@@ -66,11 +70,22 @@ int iomap_iter(struct iomap_iter *iter, const struct iomap_ops *ops)
 	if (!iter->iomap.length)
 		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;
 	}
 
@@ -81,8 +96,11 @@ int iomap_iter(struct iomap_iter *iter, const struct iomap_ops *ops)
 	}
 
 	/*
-	 * Advance the iter and clear state from the previous iteration. Use
-	 * iter->len to determine whether to continue onto the next mapping.
+	 * 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
 	 * advanced at all (i.e. no work was done for some reason) unless the
 	 * mapping has been marked stale and needs to be reprocessed.
@@ -90,7 +108,7 @@ int iomap_iter(struct iomap_iter *iter, const struct iomap_ops *ops)
 	ret = iomap_iter_advance(iter, &processed);
 	if (!ret && iter->len > 0)
 		ret = 1;
-	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 f304c602e5fe..d832a540cc72 100644
--- a/include/linux/iomap.h
+++ b/include/linux/iomap.h
@@ -211,8 +211,11 @@ 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.
- * @processed: The number of bytes processed by the body in the most recent
- *	iteration, or a negative errno. 0 causes the iteration to stop.
+ * @iter_start_pos: The original start pos for the current iomap. Used for
+ *	incremental iter advance.
+ * @processed: The number of bytes the most recent iteration needs iomap_iter()
+ *	to advance the iter, zero if the iter was already advanced, or a
+ *	negative errno for an error during the operation.
  * @flags: Zero or more of the iomap_begin flags above.
  * @iomap: Map describing the I/O iteration
  * @srcmap: Source map for COW operations
@@ -221,6 +224,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.48.1


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v6 08/10] iomap: advance the iter directly on buffered writes
  2025-02-07 14:32 [PATCH v6 00/10] iomap: incremental per-operation iter advance Brian Foster
                   ` (6 preceding siblings ...)
  2025-02-07 14:32 ` [PATCH v6 07/10] iomap: support incremental iomap_iter advances Brian Foster
@ 2025-02-07 14:32 ` Brian Foster
  2025-02-07 14:32 ` [PATCH v6 09/10] iomap: advance the iter directly on unshare range Brian Foster
                   ` (2 subsequent siblings)
  10 siblings, 0 replies; 15+ messages in thread
From: Brian Foster @ 2025-02-07 14:32 UTC (permalink / raw)
  To: linux-fsdevel; +Cc: linux-xfs, Christoph Hellwig, Darrick J . Wong

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>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>
---
 fs/iomap/buffered-io.c | 20 +++++++-------------
 1 file changed, 7 insertions(+), 13 deletions(-)

diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
index d303e6c8900c..678c189faa58 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;
@@ -923,7 +921,8 @@ static loff_t iomap_write_iter(struct iomap_iter *iter, struct iov_iter *i)
 		size_t offset;		/* Offset into folio */
 		size_t bytes;		/* Bytes to write to folio */
 		size_t copied;		/* Bytes copied from user */
-		size_t written;		/* Bytes have been written */
+		u64 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.48.1


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v6 09/10] iomap: advance the iter directly on unshare range
  2025-02-07 14:32 [PATCH v6 00/10] iomap: incremental per-operation iter advance Brian Foster
                   ` (7 preceding siblings ...)
  2025-02-07 14:32 ` [PATCH v6 08/10] iomap: advance the iter directly on buffered writes Brian Foster
@ 2025-02-07 14:32 ` Brian Foster
  2025-02-07 14:32 ` [PATCH v6 10/10] iomap: advance the iter directly on zero range Brian Foster
  2025-02-10 11:48 ` [PATCH v6 00/10] iomap: incremental per-operation iter advance Christian Brauner
  10 siblings, 0 replies; 15+ messages in thread
From: Brian Foster @ 2025-02-07 14:32 UTC (permalink / raw)
  To: linux-fsdevel; +Cc: linux-xfs, Christoph Hellwig, Darrick J . Wong

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>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>
---
 fs/iomap/buffered-io.c | 23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
index 678c189faa58..f953bf66beb1 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;
+	u64 bytes = iomap_length(iter);
+	int status;
 
 	if (!iomap_want_unshare_iter(iter))
-		return length;
+		return iomap_iter_advance(iter, &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,14 @@ 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);
 
-	return written;
+		status = iomap_iter_advance(iter, &bytes);
+		if (status)
+			break;
+	} while (bytes > 0);
+
+	return status;
 }
 
 int
-- 
2.48.1


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v6 10/10] iomap: advance the iter directly on zero range
  2025-02-07 14:32 [PATCH v6 00/10] iomap: incremental per-operation iter advance Brian Foster
                   ` (8 preceding siblings ...)
  2025-02-07 14:32 ` [PATCH v6 09/10] iomap: advance the iter directly on unshare range Brian Foster
@ 2025-02-07 14:32 ` Brian Foster
  2025-02-10 11:48 ` [PATCH v6 00/10] iomap: incremental per-operation iter advance Christian Brauner
  10 siblings, 0 replies; 15+ messages in thread
From: Brian Foster @ 2025-02-07 14:32 UTC (permalink / raw)
  To: linux-fsdevel; +Cc: linux-xfs, Christoph Hellwig, Darrick J . Wong

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>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>
---
 fs/iomap/buffered-io.c | 24 +++++++++++++-----------
 1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
index f953bf66beb1..ec227b45f3aa 100644
--- a/fs/iomap/buffered-io.c
+++ b/fs/iomap/buffered-io.c
@@ -1345,17 +1345,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;
+	u64 bytes = iomap_length(iter);
+	int status;
 
 	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;
@@ -1376,14 +1375,14 @@ static loff_t iomap_zero_iter(struct iomap_iter *iter, bool *did_zero)
 		if (WARN_ON_ONCE(!ret))
 			return -EIO;
 
-		pos += bytes;
-		length -= bytes;
-		written += bytes;
-	} while (length > 0);
+		status = iomap_iter_advance(iter, &bytes);
+		if (status)
+			break;
+	} while (bytes > 0);
 
 	if (did_zero)
 		*did_zero = true;
-	return written;
+	return status;
 }
 
 int
@@ -1436,11 +1435,14 @@ iomap_zero_range(struct inode *inode, loff_t pos, loff_t len, bool *did_zero,
 
 		if (srcmap->type == IOMAP_HOLE ||
 		    srcmap->type == IOMAP_UNWRITTEN) {
-			loff_t proc = iomap_length(&iter);
+			s64 proc;
 
 			if (range_dirty) {
 				range_dirty = false;
 				proc = iomap_zero_iter_flush_and_stale(&iter);
+			} else {
+				u64 length = iomap_length(&iter);
+				proc = iomap_iter_advance(&iter, &length);
 			}
 			iter.processed = proc;
 			continue;
-- 
2.48.1


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH v6 06/10] iomap: export iomap_iter_advance() and return remaining length
  2025-02-07 14:32 ` [PATCH v6 06/10] iomap: export iomap_iter_advance() and return remaining length Brian Foster
@ 2025-02-07 17:17   ` Darrick J. Wong
  0 siblings, 0 replies; 15+ messages in thread
From: Darrick J. Wong @ 2025-02-07 17:17 UTC (permalink / raw)
  To: Brian Foster; +Cc: linux-fsdevel, linux-xfs, Christoph Hellwig

On Fri, Feb 07, 2025 at 09:32:49AM -0500, Brian Foster wrote:
> As a final step for generic iter advance, export the helper and
> update it to return the remaining length of the current iteration
> after the advance. This will usually be 0 in the iomap_iter() case,
> but will be useful for the various operations that iterate on their
> own and will be updated to advance as they progress.
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>

Looks good to me now,
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

--D

> ---
>  fs/iomap/iter.c       | 22 ++++++++--------------
>  include/linux/iomap.h |  1 +
>  2 files changed, 9 insertions(+), 14 deletions(-)
> 
> diff --git a/fs/iomap/iter.c b/fs/iomap/iter.c
> index 8e0746ad80bd..544cd7a5a16b 100644
> --- a/fs/iomap/iter.c
> +++ b/fs/iomap/iter.c
> @@ -15,22 +15,16 @@ 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 output the length remaining for the
> + * current mapping.
>   */
> -static inline int iomap_iter_advance(struct iomap_iter *iter, s64 count)
> +int iomap_iter_advance(struct iomap_iter *iter, u64 *count)
>  {
> -	if (WARN_ON_ONCE(count > iomap_length(iter)))
> +	if (WARN_ON_ONCE(*count > iomap_length(iter)))
>  		return -EIO;
> -	iter->pos += count;
> -	iter->len -= count;
> +	iter->pos += *count;
> +	iter->len -= *count;
> +	*count = iomap_length(iter);
>  	return 0;
>  }
>  
> @@ -93,7 +87,7 @@ int iomap_iter(struct iomap_iter *iter, const struct iomap_ops *ops)
>  	 * 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);
> +	ret = iomap_iter_advance(iter, &processed);
>  	if (!ret && iter->len > 0)
>  		ret = 1;
>  	if (ret > 0 && !iter->processed && !stale)
> diff --git a/include/linux/iomap.h b/include/linux/iomap.h
> index f5ca71ac2fa2..f304c602e5fe 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);
> +int iomap_iter_advance(struct iomap_iter *iter, u64 *count);
>  
>  /**
>   * iomap_length_trim - trimmed length of the current iomap iteration
> -- 
> 2.48.1
> 
> 

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v6 07/10] iomap: support incremental iomap_iter advances
  2025-02-07 14:32 ` [PATCH v6 07/10] iomap: support incremental iomap_iter advances Brian Foster
@ 2025-02-07 17:18   ` Darrick J. Wong
  0 siblings, 0 replies; 15+ messages in thread
From: Darrick J. Wong @ 2025-02-07 17:18 UTC (permalink / raw)
  To: Brian Foster; +Cc: linux-fsdevel, linux-xfs, Christoph Hellwig

On Fri, Feb 07, 2025 at 09:32:50AM -0500, Brian Foster wrote:
> 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>
> Reviewed-by: Christoph Hellwig <hch@lst.de>

Thanks for the documentation update, even if some of it is temporary.
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

--D

> ---
>  fs/iomap/iter.c       | 32 +++++++++++++++++++++++++-------
>  include/linux/iomap.h |  8 ++++++--
>  2 files changed, 31 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/iomap/iter.c b/fs/iomap/iter.c
> index 544cd7a5a16b..0ebcabc7df52 100644
> --- a/fs/iomap/iter.c
> +++ b/fs/iomap/iter.c
> @@ -35,6 +35,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);
> @@ -58,6 +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;
>  	int ret;
>  
> @@ -66,11 +70,22 @@ int iomap_iter(struct iomap_iter *iter, const struct iomap_ops *ops)
>  	if (!iter->iomap.length)
>  		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;
>  	}
>  
> @@ -81,8 +96,11 @@ int iomap_iter(struct iomap_iter *iter, const struct iomap_ops *ops)
>  	}
>  
>  	/*
> -	 * Advance the iter and clear state from the previous iteration. Use
> -	 * iter->len to determine whether to continue onto the next mapping.
> +	 * 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
>  	 * advanced at all (i.e. no work was done for some reason) unless the
>  	 * mapping has been marked stale and needs to be reprocessed.
> @@ -90,7 +108,7 @@ int iomap_iter(struct iomap_iter *iter, const struct iomap_ops *ops)
>  	ret = iomap_iter_advance(iter, &processed);
>  	if (!ret && iter->len > 0)
>  		ret = 1;
> -	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 f304c602e5fe..d832a540cc72 100644
> --- a/include/linux/iomap.h
> +++ b/include/linux/iomap.h
> @@ -211,8 +211,11 @@ 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.
> - * @processed: The number of bytes processed by the body in the most recent
> - *	iteration, or a negative errno. 0 causes the iteration to stop.
> + * @iter_start_pos: The original start pos for the current iomap. Used for
> + *	incremental iter advance.
> + * @processed: The number of bytes the most recent iteration needs iomap_iter()
> + *	to advance the iter, zero if the iter was already advanced, or a
> + *	negative errno for an error during the operation.
>   * @flags: Zero or more of the iomap_begin flags above.
>   * @iomap: Map describing the I/O iteration
>   * @srcmap: Source map for COW operations
> @@ -221,6 +224,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.48.1
> 
> 

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v6 00/10] iomap: incremental per-operation iter advance
  2025-02-07 14:32 [PATCH v6 00/10] iomap: incremental per-operation iter advance Brian Foster
                   ` (9 preceding siblings ...)
  2025-02-07 14:32 ` [PATCH v6 10/10] iomap: advance the iter directly on zero range Brian Foster
@ 2025-02-10 11:48 ` Christian Brauner
  2025-02-10 13:53   ` Brian Foster
  10 siblings, 1 reply; 15+ messages in thread
From: Christian Brauner @ 2025-02-10 11:48 UTC (permalink / raw)
  To: linux-fsdevel, Brian Foster
  Cc: Christian Brauner, linux-xfs, Christoph Hellwig, Darrick J . Wong

On Fri, 07 Feb 2025 09:32:43 -0500, Brian Foster wrote:
> Just a couple comment changes, no code changes from v5.
> 
> Brian
> 
> v6:
> - Comment updates in patches 6 and 7.
> v5: https://lore.kernel.org/linux-fsdevel/20250205135821.178256-1-bfoster@redhat.com/
> - Fixed refactoring bug in v4 by pulling 'processed' local var into
>   patch 4.
> v4: https://lore.kernel.org/linux-fsdevel/20250204133044.80551-1-bfoster@redhat.com/
> - Reordered patches 1 and 2 to keep iter advance cleanups together.
> - Split patch 3 from v3 into patches 3-6.
> v3: https://lore.kernel.org/linux-fsdevel/20250130170949.916098-1-bfoster@redhat.com/
> - Code style and comment fixups.
> - Variable type fixups and rework of iomap_iter_advance() to return
>   error/length separately.
> - Advance the iter on unshare and zero range skip cases instead of
>   returning length.
> v2: https://lore.kernel.org/linux-fsdevel/20250122133434.535192-1-bfoster@redhat.com/
> - 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, you would help me a lot if you keep adding the full cover letter
message in each series. I always retain the cover letter when I merge
series such as this. So for work like yours it serves as a design/spec
document that we can always go back to when we see bugs in that code
months/years down the line.

---

Applied to the vfs-6.15.iomap branch of the vfs/vfs.git tree.
Patches in the vfs-6.15.iomap branch should appear in linux-next soon.

Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.

It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.

Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs-6.15.iomap

[01/10] iomap: factor out iomap length helper
        https://git.kernel.org/vfs/vfs/c/abb0ea1923a6
[02/10] iomap: split out iomap check and reset logic from iter advance
        https://git.kernel.org/vfs/vfs/c/2e4b0b6cf533
[03/10] iomap: refactor iomap_iter() length check and tracepoint
        https://git.kernel.org/vfs/vfs/c/f47998386623
[04/10] iomap: lift error code check out of iomap_iter_advance()
        https://git.kernel.org/vfs/vfs/c/9183b2a0e439
[05/10] iomap: lift iter termination logic from iomap_iter_advance()
        https://git.kernel.org/vfs/vfs/c/b26f2ea1cd06
[06/10] iomap: export iomap_iter_advance() and return remaining length
        https://git.kernel.org/vfs/vfs/c/b51d30ff51f9
[07/10] iomap: support incremental iomap_iter advances
        https://git.kernel.org/vfs/vfs/c/bc264fea0f6f
[08/10] iomap: advance the iter directly on buffered writes
        https://git.kernel.org/vfs/vfs/c/1a1a3b574b97
[09/10] iomap: advance the iter directly on unshare range
        https://git.kernel.org/vfs/vfs/c/e60837da4d9d
[10/10] iomap: advance the iter directly on zero range
        https://git.kernel.org/vfs/vfs/c/cbad829cef3b

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v6 00/10] iomap: incremental per-operation iter advance
  2025-02-10 11:48 ` [PATCH v6 00/10] iomap: incremental per-operation iter advance Christian Brauner
@ 2025-02-10 13:53   ` Brian Foster
  0 siblings, 0 replies; 15+ messages in thread
From: Brian Foster @ 2025-02-10 13:53 UTC (permalink / raw)
  To: Christian Brauner
  Cc: linux-fsdevel, linux-xfs, Christoph Hellwig, Darrick J . Wong

On Mon, Feb 10, 2025 at 12:48:14PM +0100, Christian Brauner wrote:
> On Fri, 07 Feb 2025 09:32:43 -0500, Brian Foster wrote:
> > Just a couple comment changes, no code changes from v5.
> > 
> > Brian
> > 
> > v6:
> > - Comment updates in patches 6 and 7.
> > v5: https://lore.kernel.org/linux-fsdevel/20250205135821.178256-1-bfoster@redhat.com/
> > - Fixed refactoring bug in v4 by pulling 'processed' local var into
> >   patch 4.
> > v4: https://lore.kernel.org/linux-fsdevel/20250204133044.80551-1-bfoster@redhat.com/
> > - Reordered patches 1 and 2 to keep iter advance cleanups together.
> > - Split patch 3 from v3 into patches 3-6.
> > v3: https://lore.kernel.org/linux-fsdevel/20250130170949.916098-1-bfoster@redhat.com/
> > - Code style and comment fixups.
> > - Variable type fixups and rework of iomap_iter_advance() to return
> >   error/length separately.
> > - Advance the iter on unshare and zero range skip cases instead of
> >   returning length.
> > v2: https://lore.kernel.org/linux-fsdevel/20250122133434.535192-1-bfoster@redhat.com/
> > - 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, you would help me a lot if you keep adding the full cover letter
> message in each series. I always retain the cover letter when I merge
> series such as this. So for work like yours it serves as a design/spec
> document that we can always go back to when we see bugs in that code
> months/years down the line.
> 

Ah. Sorry, I wasn't aware of that. I tend to use the cover letter of the
series as a quick update and/or message to reviewers, which ultimately
evolves from the high level technical description in v1 to more brief
notes as the changelog shrinks.

Going forward I'll try to keep the high level technical description from
v1 as its own section and carry that forward in subsequent versions.
Thanks for the feedback.

Brian

> ---
> 
> Applied to the vfs-6.15.iomap branch of the vfs/vfs.git tree.
> Patches in the vfs-6.15.iomap branch should appear in linux-next soon.
> 
> Please report any outstanding bugs that were missed during review in a
> new review to the original patch series allowing us to drop it.
> 
> It's encouraged to provide Acked-bys and Reviewed-bys even though the
> patch has now been applied. If possible patch trailers will be updated.
> 
> Note that commit hashes shown below are subject to change due to rebase,
> trailer updates or similar. If in doubt, please check the listed branch.
> 
> tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
> branch: vfs-6.15.iomap
> 
> [01/10] iomap: factor out iomap length helper
>         https://git.kernel.org/vfs/vfs/c/abb0ea1923a6
> [02/10] iomap: split out iomap check and reset logic from iter advance
>         https://git.kernel.org/vfs/vfs/c/2e4b0b6cf533
> [03/10] iomap: refactor iomap_iter() length check and tracepoint
>         https://git.kernel.org/vfs/vfs/c/f47998386623
> [04/10] iomap: lift error code check out of iomap_iter_advance()
>         https://git.kernel.org/vfs/vfs/c/9183b2a0e439
> [05/10] iomap: lift iter termination logic from iomap_iter_advance()
>         https://git.kernel.org/vfs/vfs/c/b26f2ea1cd06
> [06/10] iomap: export iomap_iter_advance() and return remaining length
>         https://git.kernel.org/vfs/vfs/c/b51d30ff51f9
> [07/10] iomap: support incremental iomap_iter advances
>         https://git.kernel.org/vfs/vfs/c/bc264fea0f6f
> [08/10] iomap: advance the iter directly on buffered writes
>         https://git.kernel.org/vfs/vfs/c/1a1a3b574b97
> [09/10] iomap: advance the iter directly on unshare range
>         https://git.kernel.org/vfs/vfs/c/e60837da4d9d
> [10/10] iomap: advance the iter directly on zero range
>         https://git.kernel.org/vfs/vfs/c/cbad829cef3b
> 


^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2025-02-10 13:51 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-02-07 14:32 [PATCH v6 00/10] iomap: incremental per-operation iter advance Brian Foster
2025-02-07 14:32 ` [PATCH v6 01/10] iomap: factor out iomap length helper Brian Foster
2025-02-07 14:32 ` [PATCH v6 02/10] iomap: split out iomap check and reset logic from iter advance Brian Foster
2025-02-07 14:32 ` [PATCH v6 03/10] iomap: refactor iomap_iter() length check and tracepoint Brian Foster
2025-02-07 14:32 ` [PATCH v6 04/10] iomap: lift error code check out of iomap_iter_advance() Brian Foster
2025-02-07 14:32 ` [PATCH v6 05/10] iomap: lift iter termination logic from iomap_iter_advance() Brian Foster
2025-02-07 14:32 ` [PATCH v6 06/10] iomap: export iomap_iter_advance() and return remaining length Brian Foster
2025-02-07 17:17   ` Darrick J. Wong
2025-02-07 14:32 ` [PATCH v6 07/10] iomap: support incremental iomap_iter advances Brian Foster
2025-02-07 17:18   ` Darrick J. Wong
2025-02-07 14:32 ` [PATCH v6 08/10] iomap: advance the iter directly on buffered writes Brian Foster
2025-02-07 14:32 ` [PATCH v6 09/10] iomap: advance the iter directly on unshare range Brian Foster
2025-02-07 14:32 ` [PATCH v6 10/10] iomap: advance the iter directly on zero range Brian Foster
2025-02-10 11:48 ` [PATCH v6 00/10] iomap: incremental per-operation iter advance Christian Brauner
2025-02-10 13:53   ` Brian Foster

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).