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

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

[Brian Foster]

Hi all,

This is a first pass at supporting more incremental, per-operation
iomap_iter advancement. The motivation for this is folio_batch support
for zero range [1], where the fs provides a batch of folios to process
in certain situations. Since the batch may not be logically contiguous,
processing loops require a bit more flexibility than the typical offset
based iteration.

The current iteration model basically has the operation _iter() handler
lift the pos/length wrt to the current iomap out of the iomap_iter,
process it locally, then return the result to be stored in
iter.processed. The latter is overloaded with error status, so the
handler must decide whether to return error or a partial completion
(i.e. consider a short write). iomap_iter() then uses the result to
advance the iter and look up the next iomap.

The updated model proposed in this series is to allow an operation to
advance the iter itself as subranges are processed and then return
success or failure in iter.processed. Note that at least initially, this
is implemented as an optional mode to minimize churn. This series
converts operations that use iomap_write_begin(): buffered write,
unshare, and zero range.

The main advantage of this is that the future folio_batch work can be
plumbed down into the folio get path more naturally, and the
associated codepath can advance the iter itself when appropriate rather
than require each operation to manage the gaps in the range being
processed. Some secondary advantages are a little less boilerplate code
for walking ranges and more clear semantics for partial completions in
the event of errors, etc.

I'll post an RFC of the folio_batch work shortly after this to give an
example of how this is intended to be used. Otherwise, the changes here
actually aren't all that substantial. Patches 1-2 are prep work, patch 3
enables incremental advances, and patches 4-6 switch over a few
operations. Thoughts, reviews, flames appreciated.

Brian

v1:
- Reworked and fixed a bunch of functional issues.
RFC: https://lore.kernel.org/linux-fsdevel/20241125140623.20633-1-bfoster@redhat.com/

[1] https://lore.kernel.org/linux-fsdevel/20241119154656.774395-1-bfoster@redhat.com/

Brian Foster (6):
  iomap: split out iomap check and reset logic from iter advance
  iomap: factor out iomap length helper
  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 | 46 ++++++++++-----------------
 fs/iomap/iter.c        | 72 ++++++++++++++++++++++++++----------------
 include/linux/iomap.h  | 14 ++++++--
 3 files changed, 73 insertions(+), 59 deletions(-)

-- 
2.47.0

[PATCH 1/6] 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>
---
 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.0

[PATCH 2/6] 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 | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/include/linux/iomap.h b/include/linux/iomap.h
index 5675af6b740c..cbacccb3fb14 100644
--- a/include/linux/iomap.h
+++ b/include/linux/iomap.h
@@ -236,13 +236,19 @@ int iomap_iter(struct iomap_iter *iter, const struct iomap_ops *ops);
  *
  * Returns the 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(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);
+}
+
+static inline u64 iomap_length(const struct iomap_iter *iter)
+{
+	return __iomap_length(iter, iter->pos, iter->len);
 }
 
 /**
-- 
2.47.0

[PATCH 3/6] 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       | 27 +++++++++++++++++++++------
 include/linux/iomap.h |  4 ++++
 2 files changed, 25 insertions(+), 6 deletions(-)

diff --git a/fs/iomap/iter.c b/fs/iomap/iter.c
index 731ea7267f27..5fe0edb51fe5 100644
--- a/fs/iomap/iter.c
+++ b/fs/iomap/iter.c
@@ -25,7 +25,7 @@ static inline void iomap_iter_reset_iomap(struct iomap_iter *iter)
  * 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, s64 count)
+int iomap_iter_advance(struct iomap_iter *iter, s64 count)
 {
 	bool stale = iter->iomap.flags & IOMAP_F_STALE;
 	int ret = 1;
@@ -36,7 +36,7 @@ static inline int iomap_iter_advance(struct iomap_iter *iter, s64 count)
 		return -EIO;
 	iter->pos += count;
 	iter->len -= count;
-	if (!iter->len || (!count && !stale))
+	if (!iter->len || (!count && !stale && iomap_length(iter)))
 		ret = 0;
 
 	return ret;
@@ -49,6 +49,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_spos = iter->pos;
+
 	trace_iomap_iter_dstmap(iter->inode, &iter->iomap);
 	if (iter->srcmap.type != IOMAP_HOLE)
 		trace_iomap_iter_srcmap(iter->inode, &iter->srcmap);
@@ -74,10 +76,23 @@ int iomap_iter(struct iomap_iter *iter, const struct iomap_ops *ops)
 	int ret;
 
 	if (iter->iomap.length && 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)
+		ssize_t processed = iter->processed > 0 ? iter->processed : 0;
+		u64 olen = iter->len;
+
+		/*
+		 * If processed is zero, the op may have advanced the iter
+		 * itself. Update the processed and original length bytes based
+		 * on how far ->pos has advanced.
+		 */
+		if (!processed) {
+			processed = iter->pos - iter->iter_spos;
+			olen += processed;
+		}
+
+		ret = ops->iomap_end(iter->inode, iter->iter_spos,
+				__iomap_length(iter, iter->iter_spos, olen),
+				processed, iter->flags, &iter->iomap);
+		if (ret < 0 && !processed)
 			return ret;
 	}
 
diff --git a/include/linux/iomap.h b/include/linux/iomap.h
index cbacccb3fb14..704ed98159f7 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_spos: 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_spos;
 	s64 processed;
 	unsigned flags;
 	struct iomap iomap;
@@ -229,6 +232,7 @@ struct iomap_iter {
 };
 
 int iomap_iter(struct iomap_iter *iter, const struct iomap_ops *ops);
+int iomap_iter_advance(struct iomap_iter *iter, s64 count);
 
 /**
  * iomap_length - length of the current iomap iteration
-- 
2.47.0

[PATCH 4/6] 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 | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
index 955f19e27e47..3ece0bee803d 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,8 @@ 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;
+		loff_t length = iomap_length(iter);
 
 		bytes = iov_iter_count(i);
 retry:
@@ -1006,17 +1006,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.0

[PATCH 5/6] 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 | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
index 3ece0bee803d..5e33e52eff15 100644
--- a/fs/iomap/buffered-io.c
+++ b/fs/iomap/buffered-io.c
@@ -1268,18 +1268,16 @@ 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;
 
 	if (!iomap_want_unshare_iter(iter))
-		return length;
+		return iomap_length(iter);
 
 	do {
 		struct folio *folio;
 		int status;
 		size_t offset;
-		size_t bytes = min_t(u64, SIZE_MAX, length);
+		size_t bytes = min_t(u64, SIZE_MAX, iomap_length(iter));
+		loff_t pos = iter->pos;
 		bool ret;
 
 		status = iomap_write_begin(iter, pos, bytes, &folio);
@@ -1299,14 +1297,12 @@ static loff_t iomap_unshare_iter(struct iomap_iter *iter)
 
 		cond_resched();
 
-		pos += bytes;
-		written += bytes;
-		length -= bytes;
+		iomap_iter_advance(iter, bytes);
 
 		balance_dirty_pages_ratelimited(iter->inode->i_mapping);
-	} while (length > 0);
+	} while (iomap_length(iter) > 0);
 
-	return written;
+	return 0;
 }
 
 int
-- 
2.47.0

[PATCH 6/6] 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 5e33e52eff15..e0ae46b11413 100644
--- a/fs/iomap/buffered-io.c
+++ b/fs/iomap/buffered-io.c
@@ -1343,15 +1343,12 @@ 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;
-
 	do {
 		struct folio *folio;
 		int status;
 		size_t offset;
-		size_t bytes = min_t(u64, SIZE_MAX, length);
+		size_t bytes = min_t(u64, SIZE_MAX, iomap_length(iter));
+		loff_t pos = iter->pos;
 		bool ret;
 
 		status = iomap_write_begin(iter, pos, bytes, &folio);
@@ -1374,14 +1371,12 @@ 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);
+		iomap_iter_advance(iter, bytes);
+	} while (iomap_length(iter) > 0);
 
 	if (did_zero)
 		*did_zero = true;
-	return written;
+	return 0;
 }
 
 int
-- 
2.47.0

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

[Christoph Hellwig]

Looks good:

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

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

[Christoph Hellwig]

On Fri, Dec 13, 2024 at 09:36:06AM -0500, Brian Foster wrote:
> 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 | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/iomap.h b/include/linux/iomap.h
> index 5675af6b740c..cbacccb3fb14 100644
> --- a/include/linux/iomap.h
> +++ b/include/linux/iomap.h
> @@ -236,13 +236,19 @@ int iomap_iter(struct iomap_iter *iter, const struct iomap_ops *ops);
>   *
>   * Returns the 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(const struct iomap_iter *iter, loff_t pos,
> +		u64 len)

__iomap_length is not a a very good name for this, maybe something like
iomap_cap_length?  Also please move the kerneldoc comment for
iomap_length down to be next to it.

Re: [PATCH 3/6] iomap: support incremental iomap_iter advances

[Christoph Hellwig]

On Fri, Dec 13, 2024 at 09:36:07AM -0500, Brian Foster wrote:
> 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.

While the uses of the incremental advance later look nice, this bit
is pretty ugly.  I wonder if we could just move overy everything to
the incremental advance model, even if it isn't all that incremental,
that is always call iomap_iter_advance from the processing loop and
eventually remove the call in iomap_iter() entirely?

> @@ -36,7 +36,7 @@ static inline int iomap_iter_advance(struct iomap_iter *iter, s64 count)
>  		return -EIO;
>  	iter->pos += count;
>  	iter->len -= count;
> -	if (!iter->len || (!count && !stale))
> +	if (!iter->len || (!count && !stale && iomap_length(iter)))

This probably warrantd a comment even with the existing code, but really
needs one now.

> + * @iter_spos: The original start pos for the current iomap. Used for
> + *	incremental iter advance.

Maybe spell out the usage as iter_start_pos in the field name as spos
reads a little weird?

Re: [PATCH 4/6] iomap: advance the iter directly on buffered writes

[Christoph Hellwig]

On Fri, Dec 13, 2024 at 09:36:08AM -0500, Brian Foster wrote:
> +		loff_t pos = iter->pos;
> +		loff_t length = iomap_length(iter);

AFAICS we could just do away with these local variables as they should
never get out of sync with the values in the iter.  If so I'd love to see
that one.  If they can get out of sync and we actually need them, that
would warrant a comment.

Otherwise this looks good to me, and the same applies to the next two
patches.

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

[Brian Foster]

On Wed, Jan 08, 2025 at 11:02:31PM -0800, Christoph Hellwig wrote:
> On Fri, Dec 13, 2024 at 09:36:06AM -0500, Brian Foster wrote:
> > 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 | 10 ++++++++--
> >  1 file changed, 8 insertions(+), 2 deletions(-)
> > 
> > diff --git a/include/linux/iomap.h b/include/linux/iomap.h
> > index 5675af6b740c..cbacccb3fb14 100644
> > --- a/include/linux/iomap.h
> > +++ b/include/linux/iomap.h
> > @@ -236,13 +236,19 @@ int iomap_iter(struct iomap_iter *iter, const struct iomap_ops *ops);
> >   *
> >   * Returns the 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(const struct iomap_iter *iter, loff_t pos,
> > +		u64 len)
> 
> __iomap_length is not a a very good name for this, maybe something like
> iomap_cap_length?  Also please move the kerneldoc comment for
> iomap_length down to be next to it.
> 

Ok, seems reasonable, though I think I'd prefer something like
iomap_length_trim()..

Brian

Re: [PATCH 3/6] iomap: support incremental iomap_iter advances

[Brian Foster]

On Wed, Jan 08, 2025 at 11:07:22PM -0800, Christoph Hellwig wrote:
> On Fri, Dec 13, 2024 at 09:36:07AM -0500, Brian Foster wrote:
> > 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.
> 
> While the uses of the incremental advance later look nice, this bit
> is pretty ugly.  I wonder if we could just move overy everything to
> the incremental advance model, even if it isn't all that incremental,
> that is always call iomap_iter_advance from the processing loop and
> eventually remove the call in iomap_iter() entirely?
> 

Yeah, I agree this is a wart. Another option I thought about was
creating a new flag to declare which iteration mode a particular
operation uses, if for nothing else but to improve clarity.

FWIW my first pass at finding a solution here was actually with intent
to convert everything over, but then I got lost in the weeds of all the
various operations and gave up. I didn't want to spend forever changing
every op over for something before it was shown to work or be useful, so
my thought process was more to try and see this through for zero range
and if that pans out, follow up with changing everything else over as a
later step.

That said, this was early on before I had the idea fleshed out and I
don't recall all the details. I'll make a note to do another audit pass
at this and see how feasible it is. I suppose my immediate question on
that is: suppose the folio batch thing just doesn't pan out for whatever
reason.. would we think this is a worthwhile iteration cleanup on its
own?

> > @@ -36,7 +36,7 @@ static inline int iomap_iter_advance(struct iomap_iter *iter, s64 count)
> >  		return -EIO;
> >  	iter->pos += count;
> >  	iter->len -= count;
> > -	if (!iter->len || (!count && !stale))
> > +	if (!iter->len || (!count && !stale && iomap_length(iter)))
> 
> This probably warrantd a comment even with the existing code, but really
> needs one now.
> 

Ack.

> > + * @iter_spos: The original start pos for the current iomap. Used for
> > + *	incremental iter advance.
> 
> Maybe spell out the usage as iter_start_pos in the field name as spos
> reads a little weird?
>

Ack.

Brian 

Re: [PATCH 4/6] iomap: advance the iter directly on buffered writes

[Brian Foster]

On Wed, Jan 08, 2025 at 11:10:48PM -0800, Christoph Hellwig wrote:
> On Fri, Dec 13, 2024 at 09:36:08AM -0500, Brian Foster wrote:
> > +		loff_t pos = iter->pos;
> > +		loff_t length = iomap_length(iter);
> 
> AFAICS we could just do away with these local variables as they should
> never get out of sync with the values in the iter.  If so I'd love to see
> that one.  If they can get out of sync and we actually need them, that
> would warrant a comment.
> 
> Otherwise this looks good to me, and the same applies to the next two
> patches.
> 

Hmm.. they should not get out of sync, but that wasn't necessarily the
point here. For one, this is trying to be incremental and highlight the
actual logic changes, but also I didn't want to just go and replace
every usage of pos with iter->pos when it only needs to be read at a
certain point.

This might be a little more clear after the (non-squashed) fbatch
patches which move where pos is sampled (to handle that it can change at
that point) and drop some of the pos function params, but if we still
want to clean that up at the end I'd rather do it as a standalone patch
at that point.

All that said, length is only used for the bytes check so I can probably
kill that one off here.

Brian

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

[Christoph Hellwig]

On Fri, Jan 10, 2025 at 12:49:29PM -0500, Brian Foster wrote:
> > __iomap_length is not a a very good name for this, maybe something like
> > iomap_cap_length?  Also please move the kerneldoc comment for
> > iomap_length down to be next to it.
> > 
> 
> Ok, seems reasonable, though I think I'd prefer something like
> iomap_length_trim()..

Fine with me.

Re: [PATCH 3/6] iomap: support incremental iomap_iter advances

[Christoph Hellwig]

On Fri, Jan 10, 2025 at 12:50:26PM -0500, Brian Foster wrote:
> Yeah, I agree this is a wart. Another option I thought about was
> creating a new flag to declare which iteration mode a particular
> operation uses, if for nothing else but to improve clarity.

I actually really like the model where the processing loop always
advances.  It'll make a few things I have on my mind much easier.

That doesn't mean I want to force you to go all the way for the initial
patch series, but I'd love to see a full switchover, and preferably
without a too long window of having both.

> reason.. would we think this is a worthwhile iteration cleanup on its
> own?

Yes.

Re: [PATCH 3/6] iomap: support incremental iomap_iter advances

[Brian Foster]

On Sun, Jan 12, 2025 at 08:48:44PM -0800, Christoph Hellwig wrote:
> On Fri, Jan 10, 2025 at 12:50:26PM -0500, Brian Foster wrote:
> > Yeah, I agree this is a wart. Another option I thought about was
> > creating a new flag to declare which iteration mode a particular
> > operation uses, if for nothing else but to improve clarity.
> 
> I actually really like the model where the processing loop always
> advances.  It'll make a few things I have on my mind much easier.
> 
> That doesn't mean I want to force you to go all the way for the initial
> patch series, but I'd love to see a full switchover, and preferably
> without a too long window of having both.
> 

Ok, thanks. I'm on board with that, just need to dig back into it to be
certain of details or roadblocks..

Another thing that crossed my mind is that it might be preferable to
convert across at least one release cycle regardless, just from a risk
management standpoint. I.e., introduce for zero range in one release,
let the test robots and whatnot come at me with whatever issues that
might exist, and then follow up with broader changes from there. But
anyways, one thing at a time..

Brian

> > reason.. would we think this is a worthwhile iteration cleanup on its
> > own?
> 
> Yes.
> 

Re: [PATCH 4/6] iomap: advance the iter directly on buffered writes

[Christoph Hellwig]

On Fri, Jan 10, 2025 at 12:51:15PM -0500, Brian Foster wrote:
> This might be a little more clear after the (non-squashed) fbatch
> patches which move where pos is sampled (to handle that it can change at
> that point) and drop some of the pos function params, but if we still
> want to clean that up at the end I'd rather do it as a standalone patch
> at that point.

Yeah, that sounds reasonable.