diff mbox series

[v3,3/7] iomap: refactor iter and advance continuation logic

Message ID 20250130170949.916098-4-bfoster@redhat.com (mailing list archive)
State New
Headers show
Series iomap: incremental per-operation iter advance | expand

Commit Message

Brian Foster Jan. 30, 2025, 5:09 p.m. UTC 
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       | 66 ++++++++++++++++++++++++-------------------
 include/linux/iomap.h |  1 +
 2 files changed, 38 insertions(+), 29 deletions(-)

Comments

Christoph Hellwig Jan. 31, 2025, 8:08 a.m. UTC | #1 
On Thu, Jan 30, 2025 at 12:09:44PM -0500, Brian Foster wrote:
> 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.

It took me a bit to reoncile the commit log with the changes.

What this does is:

 1) factor out a iomap_iter_reset_iomap caller from iomap_iter_advance
 2) pass an explicit count to iomap_iter_advance instead of derіving
    it from iter->processed inside of iomap_iter_advance
 3) only call iomap_iter_advance condititional on iter->iomap.length,
    and thus skipping the code that is now in iomap_iter_reset_iomap
    when iter->iomap.length is 0.

All this looks fine, although I wonder why we didn't do 3) before and
if there is a risk of a regression for some weird corner case.

I hate nitpicking too much, but maybe split the three steps into
separate patches so that 3) is clearly documented and can be bisected
if problems arise?
Brian Foster Jan. 31, 2025, 12:50 p.m. UTC | #2 
On Fri, Jan 31, 2025 at 12:08:03AM -0800, Christoph Hellwig wrote:
> On Thu, Jan 30, 2025 at 12:09:44PM -0500, Brian Foster wrote:
> > 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.
> 
> It took me a bit to reoncile the commit log with the changes.
> 
> What this does is:
> 
>  1) factor out a iomap_iter_reset_iomap caller from iomap_iter_advance
>  2) pass an explicit count to iomap_iter_advance instead of derіving
>     it from iter->processed inside of iomap_iter_advance
>  3) only call iomap_iter_advance condititional on iter->iomap.length,
>     and thus skipping the code that is now in iomap_iter_reset_iomap
>     when iter->iomap.length is 0.
> 
> All this looks fine, although I wonder why we didn't do 3) before and
> if there is a risk of a regression for some weird corner case.
> 
> I hate nitpicking too much, but maybe split the three steps into
> separate patches so that 3) is clearly documented and can be bisected
> if problems arise?
> 
> 

No problem. I originally had this split up, then combined some of it
because the changes seemed trivial, then I think it became a little too
convoluted again. I think I should be able to split this back up into
two or three incremental patches..

Brian
diff mbox series

Patch

diff --git a/fs/iomap/iter.c b/fs/iomap/iter.c
index 731ea7267f27..0a13d50e9ffd 100644
--- a/fs/iomap/iter.c
+++ b/fs/iomap/iter.c
@@ -15,31 +15,17 @@  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)
+int iomap_iter_advance(struct iomap_iter *iter, u64 *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)))
+	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;
+	iter->pos += *count;
+	iter->len -= *count;
+	*count = iomap_length(iter);
+	return 0;
 }
 
 static inline void iomap_iter_done(struct iomap_iter *iter)
@@ -71,9 +57,16 @@  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;
 
-	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);
@@ -81,15 +74,30 @@  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_);
-	if (iter->iomap.length) {
-		ret = iomap_iter_advance(iter, iter->processed);
+	processed = iter->processed;
+	if (processed < 0) {
 		iomap_iter_reset_iomap(iter);
-		if (ret <= 0)
-			return ret;
+		return processed;
 	}
 
+	/*
+	 * 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.
+	 */
+	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;
+
+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 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