[PATCH v1] iomap: simplify iomap_iter_advance()

[PATCH v1] iomap: simplify iomap_iter_advance()

[Joanne Koong]

Most callers of iomap_iter_advance() do not need the remaining length
returned. Get rid of the extra iomap_length() call that
iomap_iter_advance() does. If the caller wants the remaining length,
they can make the call to iomap_length().

Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
---
 fs/dax.c               | 28 ++++++++++++----------------
 fs/iomap/buffered-io.c | 16 +++++++++-------
 fs/iomap/direct-io.c   |  6 +++---
 fs/iomap/iter.c        | 14 +++++---------
 fs/iomap/seek.c        |  8 ++++----
 include/linux/iomap.h  |  6 ++----
 6 files changed, 35 insertions(+), 43 deletions(-)

diff --git a/fs/dax.c b/fs/dax.c
index 20ecf652c129..29e7a150b6f9 100644
--- a/fs/dax.c
+++ b/fs/dax.c
@@ -1534,7 +1534,7 @@ static int dax_zero_iter(struct iomap_iter *iter, bool *did_zero)
 
 	/* already zeroed?  we're done. */
 	if (srcmap->type == IOMAP_HOLE || srcmap->type == IOMAP_UNWRITTEN)
-		return iomap_iter_advance(iter, &length);
+		return iomap_iter_advance(iter, length);
 
 	/*
 	 * invalidate the pages whose sharing state is to be changed
@@ -1563,9 +1563,10 @@ static int dax_zero_iter(struct iomap_iter *iter, bool *did_zero)
 		if (ret < 0)
 			return ret;
 
-		ret = iomap_iter_advance(iter, &length);
+		ret = iomap_iter_advance(iter, length);
 		if (ret)
 			return ret;
+		length = iomap_length(iter);
 	} while (length > 0);
 
 	if (did_zero)
@@ -1624,7 +1625,7 @@ static int dax_iomap_iter(struct iomap_iter *iomi, struct iov_iter *iter)
 
 		if (iomap->type == IOMAP_HOLE || iomap->type == IOMAP_UNWRITTEN) {
 			done = iov_iter_zero(min(length, end - pos), iter);
-			return iomap_iter_advance(iomi, &done);
+			return iomap_iter_advance(iomi, done);
 		}
 	}
 
@@ -1709,11 +1710,12 @@ static int dax_iomap_iter(struct iomap_iter *iomi, struct iov_iter *iter)
 					map_len, iter);
 
 		length = xfer;
-		ret = iomap_iter_advance(iomi, &length);
+		ret = iomap_iter_advance(iomi, length);
 		if (!ret && xfer == 0)
 			ret = -EFAULT;
 		if (xfer < map_len)
 			break;
+		length = iomap_length(iomi);
 	}
 	dax_read_unlock(id);
 
@@ -1946,10 +1948,8 @@ static vm_fault_t dax_iomap_pte_fault(struct vm_fault *vmf, unsigned long *pfnp,
 			ret |= VM_FAULT_MAJOR;
 		}
 
-		if (!(ret & VM_FAULT_ERROR)) {
-			u64 length = PAGE_SIZE;
-			iter.status = iomap_iter_advance(&iter, &length);
-		}
+		if (!(ret & VM_FAULT_ERROR))
+			iter.status = iomap_iter_advance(&iter, PAGE_SIZE);
 	}
 
 	if (iomap_errp)
@@ -2061,10 +2061,8 @@ static vm_fault_t dax_iomap_pmd_fault(struct vm_fault *vmf, unsigned long *pfnp,
 			continue; /* actually breaks out of the loop */
 
 		ret = dax_fault_iter(vmf, &iter, pfnp, &xas, &entry, true);
-		if (ret != VM_FAULT_FALLBACK) {
-			u64 length = PMD_SIZE;
-			iter.status = iomap_iter_advance(&iter, &length);
-		}
+		if (ret != VM_FAULT_FALLBACK)
+			iter.status = iomap_iter_advance(&iter, PMD_SIZE);
 	}
 
 unlock_entry:
@@ -2190,7 +2188,6 @@ static int dax_range_compare_iter(struct iomap_iter *it_src,
 	const struct iomap *smap = &it_src->iomap;
 	const struct iomap *dmap = &it_dest->iomap;
 	loff_t pos1 = it_src->pos, pos2 = it_dest->pos;
-	u64 dest_len;
 	void *saddr, *daddr;
 	int id, ret;
 
@@ -2223,10 +2220,9 @@ static int dax_range_compare_iter(struct iomap_iter *it_src,
 	dax_read_unlock(id);
 
 advance:
-	dest_len = len;
-	ret = iomap_iter_advance(it_src, &len);
+	ret = iomap_iter_advance(it_src, len);
 	if (!ret)
-		ret = iomap_iter_advance(it_dest, &dest_len);
+		ret = iomap_iter_advance(it_dest, len);
 	return ret;
 
 out_unlock:
diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
index fd827398afd2..fe6588ab0922 100644
--- a/fs/iomap/buffered-io.c
+++ b/fs/iomap/buffered-io.c
@@ -373,7 +373,7 @@ static int iomap_readpage_iter(struct iomap_iter *iter,
 		ret = iomap_read_inline_data(iter, folio);
 		if (ret)
 			return ret;
-		return iomap_iter_advance(iter, &length);
+		return iomap_iter_advance(iter, length);
 	}
 
 	/* zero post-eof blocks as the page may be mapped */
@@ -434,7 +434,7 @@ static int iomap_readpage_iter(struct iomap_iter *iter,
 	 * iteration.
 	 */
 	length = pos - iter->pos + plen;
-	return iomap_iter_advance(iter, &length);
+	return iomap_iter_advance(iter, length);
 }
 
 static int iomap_read_folio_iter(struct iomap_iter *iter,
@@ -1036,7 +1036,7 @@ static int iomap_write_iter(struct iomap_iter *iter, struct iov_iter *i,
 			}
 		} else {
 			total_written += written;
-			iomap_iter_advance(iter, &written);
+			iomap_iter_advance(iter, written);
 		}
 	} while (iov_iter_count(i) && iomap_length(iter));
 
@@ -1305,7 +1305,7 @@ static int iomap_unshare_iter(struct iomap_iter *iter,
 	int status;
 
 	if (!iomap_want_unshare_iter(iter))
-		return iomap_iter_advance(iter, &bytes);
+		return iomap_iter_advance(iter, bytes);
 
 	do {
 		struct folio *folio;
@@ -1329,9 +1329,10 @@ static int iomap_unshare_iter(struct iomap_iter *iter,
 
 		balance_dirty_pages_ratelimited(iter->inode->i_mapping);
 
-		status = iomap_iter_advance(iter, &bytes);
+		status = iomap_iter_advance(iter, bytes);
 		if (status)
 			break;
+		bytes = iomap_length(iter);
 	} while (bytes > 0);
 
 	return status;
@@ -1404,9 +1405,10 @@ static int iomap_zero_iter(struct iomap_iter *iter, bool *did_zero,
 		if (WARN_ON_ONCE(!ret))
 			return -EIO;
 
-		status = iomap_iter_advance(iter, &bytes);
+		status = iomap_iter_advance(iter, bytes);
 		if (status)
 			break;
+		bytes = iomap_length(iter);
 	} while (bytes > 0);
 
 	if (did_zero)
@@ -1518,7 +1520,7 @@ static int iomap_folio_mkwrite_iter(struct iomap_iter *iter,
 		folio_mark_dirty(folio);
 	}
 
-	return iomap_iter_advance(iter, &length);
+	return iomap_iter_advance(iter, length);
 }
 
 vm_fault_t iomap_page_mkwrite(struct vm_fault *vmf, const struct iomap_ops *ops,
diff --git a/fs/iomap/direct-io.c b/fs/iomap/direct-io.c
index b84f6af2eb4c..e3544a6719a7 100644
--- a/fs/iomap/direct-io.c
+++ b/fs/iomap/direct-io.c
@@ -496,7 +496,7 @@ static int iomap_dio_bio_iter(struct iomap_iter *iter, struct iomap_dio *dio)
 	/* Undo iter limitation to current extent */
 	iov_iter_reexpand(dio->submit.iter, orig_count - copied);
 	if (copied)
-		return iomap_iter_advance(iter, &copied);
+		return iomap_iter_advance(iter, copied);
 	return ret;
 }
 
@@ -507,7 +507,7 @@ static int iomap_dio_hole_iter(struct iomap_iter *iter, struct iomap_dio *dio)
 	dio->size += length;
 	if (!length)
 		return -EFAULT;
-	return iomap_iter_advance(iter, &length);
+	return iomap_iter_advance(iter, length);
 }
 
 static int iomap_dio_inline_iter(struct iomap_iter *iomi, struct iomap_dio *dio)
@@ -539,7 +539,7 @@ static int iomap_dio_inline_iter(struct iomap_iter *iomi, struct iomap_dio *dio)
 	dio->size += copied;
 	if (!copied)
 		return -EFAULT;
-	return iomap_iter_advance(iomi, &copied);
+	return iomap_iter_advance(iomi, copied);
 }
 
 static int iomap_dio_iter(struct iomap_iter *iter, struct iomap_dio *dio)
diff --git a/fs/iomap/iter.c b/fs/iomap/iter.c
index cef77ca0c20b..91d2024e00da 100644
--- a/fs/iomap/iter.c
+++ b/fs/iomap/iter.c
@@ -13,17 +13,13 @@ static inline void iomap_iter_reset_iomap(struct iomap_iter *iter)
 	memset(&iter->srcmap, 0, sizeof(iter->srcmap));
 }
 
-/*
- * Advance the current iterator position and output the length remaining for the
- * current mapping.
- */
-int iomap_iter_advance(struct iomap_iter *iter, u64 *count)
+/* Advance the current iterator position and decrement the remaining length */
+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;
-	*count = iomap_length(iter);
+	iter->pos += count;
+	iter->len -= count;
 	return 0;
 }
 
diff --git a/fs/iomap/seek.c b/fs/iomap/seek.c
index 56db2dd4b10d..6cbc587c93da 100644
--- a/fs/iomap/seek.c
+++ b/fs/iomap/seek.c
@@ -16,13 +16,13 @@ static int iomap_seek_hole_iter(struct iomap_iter *iter,
 		*hole_pos = mapping_seek_hole_data(iter->inode->i_mapping,
 				iter->pos, iter->pos + length, SEEK_HOLE);
 		if (*hole_pos == iter->pos + length)
-			return iomap_iter_advance(iter, &length);
+			return iomap_iter_advance(iter, length);
 		return 0;
 	case IOMAP_HOLE:
 		*hole_pos = iter->pos;
 		return 0;
 	default:
-		return iomap_iter_advance(iter, &length);
+		return iomap_iter_advance(iter, length);
 	}
 }
 
@@ -59,12 +59,12 @@ static int iomap_seek_data_iter(struct iomap_iter *iter,
 
 	switch (iter->iomap.type) {
 	case IOMAP_HOLE:
-		return iomap_iter_advance(iter, &length);
+		return iomap_iter_advance(iter, length);
 	case IOMAP_UNWRITTEN:
 		*hole_pos = mapping_seek_hole_data(iter->inode->i_mapping,
 				iter->pos, iter->pos + length, SEEK_DATA);
 		if (*hole_pos < 0)
-			return iomap_iter_advance(iter, &length);
+			return iomap_iter_advance(iter, length);
 		return 0;
 	default:
 		*hole_pos = iter->pos;
diff --git a/include/linux/iomap.h b/include/linux/iomap.h
index 73dceabc21c8..4469b2318b08 100644
--- a/include/linux/iomap.h
+++ b/include/linux/iomap.h
@@ -245,7 +245,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);
+int iomap_iter_advance(struct iomap_iter *iter, u64 count);
 
 /**
  * iomap_length_trim - trimmed length of the current iomap iteration
@@ -282,9 +282,7 @@ static inline u64 iomap_length(const struct iomap_iter *iter)
  */
 static inline int iomap_iter_advance_full(struct iomap_iter *iter)
 {
-	u64 length = iomap_length(iter);
-
-	return iomap_iter_advance(iter, &length);
+	return iomap_iter_advance(iter, iomap_length(iter));
 }
 
 /**
-- 
2.47.3

Re: [PATCH v1] iomap: simplify iomap_iter_advance()

[Brian Foster]

On Tue, Sep 16, 2025 at 05:40:01PM -0700, Joanne Koong wrote:
> Most callers of iomap_iter_advance() do not need the remaining length
> returned. Get rid of the extra iomap_length() call that
> iomap_iter_advance() does. If the caller wants the remaining length,
> they can make the call to iomap_length().
> 
> Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
> ---

Indeed this does clean up some of the calls that create a local var
purely to work around the interface. OTOH, the reason I made the advance
update the length was so it was clear the remaining length would be used
correctly in those looping callers. Otherwise I'm not sure it's clear
the bytes/length value needs to be updated and that felt like an easy
mistake to make to me.

I don't really have a strong preference so I'll defer to whatever the
majority opinion is. I wonder though if something else worth considering
is to rename the current helper to __iomap_iter_advance() (or whatever)
and make your updated variant of iomap_iter_advance() into a wrapper of
it.

Brian

>  fs/dax.c               | 28 ++++++++++++----------------
>  fs/iomap/buffered-io.c | 16 +++++++++-------
>  fs/iomap/direct-io.c   |  6 +++---
>  fs/iomap/iter.c        | 14 +++++---------
>  fs/iomap/seek.c        |  8 ++++----
>  include/linux/iomap.h  |  6 ++----
>  6 files changed, 35 insertions(+), 43 deletions(-)
> 
> diff --git a/fs/dax.c b/fs/dax.c
> index 20ecf652c129..29e7a150b6f9 100644
> --- a/fs/dax.c
> +++ b/fs/dax.c
> @@ -1534,7 +1534,7 @@ static int dax_zero_iter(struct iomap_iter *iter, bool *did_zero)
>  
>  	/* already zeroed?  we're done. */
>  	if (srcmap->type == IOMAP_HOLE || srcmap->type == IOMAP_UNWRITTEN)
> -		return iomap_iter_advance(iter, &length);
> +		return iomap_iter_advance(iter, length);
>  
>  	/*
>  	 * invalidate the pages whose sharing state is to be changed
> @@ -1563,9 +1563,10 @@ static int dax_zero_iter(struct iomap_iter *iter, bool *did_zero)
>  		if (ret < 0)
>  			return ret;
>  
> -		ret = iomap_iter_advance(iter, &length);
> +		ret = iomap_iter_advance(iter, length);
>  		if (ret)
>  			return ret;
> +		length = iomap_length(iter);
>  	} while (length > 0);
>  
>  	if (did_zero)
> @@ -1624,7 +1625,7 @@ static int dax_iomap_iter(struct iomap_iter *iomi, struct iov_iter *iter)
>  
>  		if (iomap->type == IOMAP_HOLE || iomap->type == IOMAP_UNWRITTEN) {
>  			done = iov_iter_zero(min(length, end - pos), iter);
> -			return iomap_iter_advance(iomi, &done);
> +			return iomap_iter_advance(iomi, done);
>  		}
>  	}
>  
> @@ -1709,11 +1710,12 @@ static int dax_iomap_iter(struct iomap_iter *iomi, struct iov_iter *iter)
>  					map_len, iter);
>  
>  		length = xfer;
> -		ret = iomap_iter_advance(iomi, &length);
> +		ret = iomap_iter_advance(iomi, length);
>  		if (!ret && xfer == 0)
>  			ret = -EFAULT;
>  		if (xfer < map_len)
>  			break;
> +		length = iomap_length(iomi);
>  	}
>  	dax_read_unlock(id);
>  
> @@ -1946,10 +1948,8 @@ static vm_fault_t dax_iomap_pte_fault(struct vm_fault *vmf, unsigned long *pfnp,
>  			ret |= VM_FAULT_MAJOR;
>  		}
>  
> -		if (!(ret & VM_FAULT_ERROR)) {
> -			u64 length = PAGE_SIZE;
> -			iter.status = iomap_iter_advance(&iter, &length);
> -		}
> +		if (!(ret & VM_FAULT_ERROR))
> +			iter.status = iomap_iter_advance(&iter, PAGE_SIZE);
>  	}
>  
>  	if (iomap_errp)
> @@ -2061,10 +2061,8 @@ static vm_fault_t dax_iomap_pmd_fault(struct vm_fault *vmf, unsigned long *pfnp,
>  			continue; /* actually breaks out of the loop */
>  
>  		ret = dax_fault_iter(vmf, &iter, pfnp, &xas, &entry, true);
> -		if (ret != VM_FAULT_FALLBACK) {
> -			u64 length = PMD_SIZE;
> -			iter.status = iomap_iter_advance(&iter, &length);
> -		}
> +		if (ret != VM_FAULT_FALLBACK)
> +			iter.status = iomap_iter_advance(&iter, PMD_SIZE);
>  	}
>  
>  unlock_entry:
> @@ -2190,7 +2188,6 @@ static int dax_range_compare_iter(struct iomap_iter *it_src,
>  	const struct iomap *smap = &it_src->iomap;
>  	const struct iomap *dmap = &it_dest->iomap;
>  	loff_t pos1 = it_src->pos, pos2 = it_dest->pos;
> -	u64 dest_len;
>  	void *saddr, *daddr;
>  	int id, ret;
>  
> @@ -2223,10 +2220,9 @@ static int dax_range_compare_iter(struct iomap_iter *it_src,
>  	dax_read_unlock(id);
>  
>  advance:
> -	dest_len = len;
> -	ret = iomap_iter_advance(it_src, &len);
> +	ret = iomap_iter_advance(it_src, len);
>  	if (!ret)
> -		ret = iomap_iter_advance(it_dest, &dest_len);
> +		ret = iomap_iter_advance(it_dest, len);
>  	return ret;
>  
>  out_unlock:
> diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
> index fd827398afd2..fe6588ab0922 100644
> --- a/fs/iomap/buffered-io.c
> +++ b/fs/iomap/buffered-io.c
> @@ -373,7 +373,7 @@ static int iomap_readpage_iter(struct iomap_iter *iter,
>  		ret = iomap_read_inline_data(iter, folio);
>  		if (ret)
>  			return ret;
> -		return iomap_iter_advance(iter, &length);
> +		return iomap_iter_advance(iter, length);
>  	}
>  
>  	/* zero post-eof blocks as the page may be mapped */
> @@ -434,7 +434,7 @@ static int iomap_readpage_iter(struct iomap_iter *iter,
>  	 * iteration.
>  	 */
>  	length = pos - iter->pos + plen;
> -	return iomap_iter_advance(iter, &length);
> +	return iomap_iter_advance(iter, length);
>  }
>  
>  static int iomap_read_folio_iter(struct iomap_iter *iter,
> @@ -1036,7 +1036,7 @@ static int iomap_write_iter(struct iomap_iter *iter, struct iov_iter *i,
>  			}
>  		} else {
>  			total_written += written;
> -			iomap_iter_advance(iter, &written);
> +			iomap_iter_advance(iter, written);
>  		}
>  	} while (iov_iter_count(i) && iomap_length(iter));
>  
> @@ -1305,7 +1305,7 @@ static int iomap_unshare_iter(struct iomap_iter *iter,
>  	int status;
>  
>  	if (!iomap_want_unshare_iter(iter))
> -		return iomap_iter_advance(iter, &bytes);
> +		return iomap_iter_advance(iter, bytes);
>  
>  	do {
>  		struct folio *folio;
> @@ -1329,9 +1329,10 @@ static int iomap_unshare_iter(struct iomap_iter *iter,
>  
>  		balance_dirty_pages_ratelimited(iter->inode->i_mapping);
>  
> -		status = iomap_iter_advance(iter, &bytes);
> +		status = iomap_iter_advance(iter, bytes);
>  		if (status)
>  			break;
> +		bytes = iomap_length(iter);
>  	} while (bytes > 0);
>  
>  	return status;
> @@ -1404,9 +1405,10 @@ static int iomap_zero_iter(struct iomap_iter *iter, bool *did_zero,
>  		if (WARN_ON_ONCE(!ret))
>  			return -EIO;
>  
> -		status = iomap_iter_advance(iter, &bytes);
> +		status = iomap_iter_advance(iter, bytes);
>  		if (status)
>  			break;
> +		bytes = iomap_length(iter);
>  	} while (bytes > 0);
>  
>  	if (did_zero)
> @@ -1518,7 +1520,7 @@ static int iomap_folio_mkwrite_iter(struct iomap_iter *iter,
>  		folio_mark_dirty(folio);
>  	}
>  
> -	return iomap_iter_advance(iter, &length);
> +	return iomap_iter_advance(iter, length);
>  }
>  
>  vm_fault_t iomap_page_mkwrite(struct vm_fault *vmf, const struct iomap_ops *ops,
> diff --git a/fs/iomap/direct-io.c b/fs/iomap/direct-io.c
> index b84f6af2eb4c..e3544a6719a7 100644
> --- a/fs/iomap/direct-io.c
> +++ b/fs/iomap/direct-io.c
> @@ -496,7 +496,7 @@ static int iomap_dio_bio_iter(struct iomap_iter *iter, struct iomap_dio *dio)
>  	/* Undo iter limitation to current extent */
>  	iov_iter_reexpand(dio->submit.iter, orig_count - copied);
>  	if (copied)
> -		return iomap_iter_advance(iter, &copied);
> +		return iomap_iter_advance(iter, copied);
>  	return ret;
>  }
>  
> @@ -507,7 +507,7 @@ static int iomap_dio_hole_iter(struct iomap_iter *iter, struct iomap_dio *dio)
>  	dio->size += length;
>  	if (!length)
>  		return -EFAULT;
> -	return iomap_iter_advance(iter, &length);
> +	return iomap_iter_advance(iter, length);
>  }
>  
>  static int iomap_dio_inline_iter(struct iomap_iter *iomi, struct iomap_dio *dio)
> @@ -539,7 +539,7 @@ static int iomap_dio_inline_iter(struct iomap_iter *iomi, struct iomap_dio *dio)
>  	dio->size += copied;
>  	if (!copied)
>  		return -EFAULT;
> -	return iomap_iter_advance(iomi, &copied);
> +	return iomap_iter_advance(iomi, copied);
>  }
>  
>  static int iomap_dio_iter(struct iomap_iter *iter, struct iomap_dio *dio)
> diff --git a/fs/iomap/iter.c b/fs/iomap/iter.c
> index cef77ca0c20b..91d2024e00da 100644
> --- a/fs/iomap/iter.c
> +++ b/fs/iomap/iter.c
> @@ -13,17 +13,13 @@ static inline void iomap_iter_reset_iomap(struct iomap_iter *iter)
>  	memset(&iter->srcmap, 0, sizeof(iter->srcmap));
>  }
>  
> -/*
> - * Advance the current iterator position and output the length remaining for the
> - * current mapping.
> - */
> -int iomap_iter_advance(struct iomap_iter *iter, u64 *count)
> +/* Advance the current iterator position and decrement the remaining length */
> +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;
> -	*count = iomap_length(iter);
> +	iter->pos += count;
> +	iter->len -= count;
>  	return 0;
>  }
>  
> diff --git a/fs/iomap/seek.c b/fs/iomap/seek.c
> index 56db2dd4b10d..6cbc587c93da 100644
> --- a/fs/iomap/seek.c
> +++ b/fs/iomap/seek.c
> @@ -16,13 +16,13 @@ static int iomap_seek_hole_iter(struct iomap_iter *iter,
>  		*hole_pos = mapping_seek_hole_data(iter->inode->i_mapping,
>  				iter->pos, iter->pos + length, SEEK_HOLE);
>  		if (*hole_pos == iter->pos + length)
> -			return iomap_iter_advance(iter, &length);
> +			return iomap_iter_advance(iter, length);
>  		return 0;
>  	case IOMAP_HOLE:
>  		*hole_pos = iter->pos;
>  		return 0;
>  	default:
> -		return iomap_iter_advance(iter, &length);
> +		return iomap_iter_advance(iter, length);
>  	}
>  }
>  
> @@ -59,12 +59,12 @@ static int iomap_seek_data_iter(struct iomap_iter *iter,
>  
>  	switch (iter->iomap.type) {
>  	case IOMAP_HOLE:
> -		return iomap_iter_advance(iter, &length);
> +		return iomap_iter_advance(iter, length);
>  	case IOMAP_UNWRITTEN:
>  		*hole_pos = mapping_seek_hole_data(iter->inode->i_mapping,
>  				iter->pos, iter->pos + length, SEEK_DATA);
>  		if (*hole_pos < 0)
> -			return iomap_iter_advance(iter, &length);
> +			return iomap_iter_advance(iter, length);
>  		return 0;
>  	default:
>  		*hole_pos = iter->pos;
> diff --git a/include/linux/iomap.h b/include/linux/iomap.h
> index 73dceabc21c8..4469b2318b08 100644
> --- a/include/linux/iomap.h
> +++ b/include/linux/iomap.h
> @@ -245,7 +245,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);
> +int iomap_iter_advance(struct iomap_iter *iter, u64 count);
>  
>  /**
>   * iomap_length_trim - trimmed length of the current iomap iteration
> @@ -282,9 +282,7 @@ static inline u64 iomap_length(const struct iomap_iter *iter)
>   */
>  static inline int iomap_iter_advance_full(struct iomap_iter *iter)
>  {
> -	u64 length = iomap_length(iter);
> -
> -	return iomap_iter_advance(iter, &length);
> +	return iomap_iter_advance(iter, iomap_length(iter));
>  }
>  
>  /**
> -- 
> 2.47.3
> 

Re: [PATCH v1] iomap: simplify iomap_iter_advance()

[Joanne Koong]

On Wed, Sep 17, 2025 at 6:08 AM Brian Foster <bfoster@redhat.com> wrote:
>
> On Tue, Sep 16, 2025 at 05:40:01PM -0700, Joanne Koong wrote:
> > Most callers of iomap_iter_advance() do not need the remaining length
> > returned. Get rid of the extra iomap_length() call that
> > iomap_iter_advance() does. If the caller wants the remaining length,
> > they can make the call to iomap_length().
> >
> > Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
> > ---
>
> Indeed this does clean up some of the calls that create a local var
> purely to work around the interface. OTOH, the reason I made the advance
> update the length was so it was clear the remaining length would be used
> correctly in those looping callers. Otherwise I'm not sure it's clear
> the bytes/length value needs to be updated and that felt like an easy
> mistake to make to me.
>
> I don't really have a strong preference so I'll defer to whatever the
> majority opinion is. I wonder though if something else worth considering
> is to rename the current helper to __iomap_iter_advance() (or whatever)
> and make your updated variant of iomap_iter_advance() into a wrapper of
> it.

That idea sounds good to me too, though I wonder for the naming of it
if iomap_iter_advance() should remain the current helper and
__iomap_iter_advance() should be for this more-minimal version of it.
From what I've seen elsewhere, it seems like the __ prefix is used for
minimum behavior and then the non-__ version of it is for that
behavior + extra stuff.


Thanks,
Joanne
>
> Brian

Re: [PATCH v1] iomap: simplify iomap_iter_advance()

[Brian Foster]

On Wed, Sep 17, 2025 at 02:54:09PM -0700, Joanne Koong wrote:
> On Wed, Sep 17, 2025 at 6:08 AM Brian Foster <bfoster@redhat.com> wrote:
> >
> > On Tue, Sep 16, 2025 at 05:40:01PM -0700, Joanne Koong wrote:
> > > Most callers of iomap_iter_advance() do not need the remaining length
> > > returned. Get rid of the extra iomap_length() call that
> > > iomap_iter_advance() does. If the caller wants the remaining length,
> > > they can make the call to iomap_length().
> > >
> > > Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
> > > ---
> >
> > Indeed this does clean up some of the calls that create a local var
> > purely to work around the interface. OTOH, the reason I made the advance
> > update the length was so it was clear the remaining length would be used
> > correctly in those looping callers. Otherwise I'm not sure it's clear
> > the bytes/length value needs to be updated and that felt like an easy
> > mistake to make to me.
> >
> > I don't really have a strong preference so I'll defer to whatever the
> > majority opinion is. I wonder though if something else worth considering
> > is to rename the current helper to __iomap_iter_advance() (or whatever)
> > and make your updated variant of iomap_iter_advance() into a wrapper of
> > it.
> 
> That idea sounds good to me too, though I wonder for the naming of it
> if iomap_iter_advance() should remain the current helper and
> __iomap_iter_advance() should be for this more-minimal version of it.
> From what I've seen elsewhere, it seems like the __ prefix is used for
> minimum behavior and then the non-__ version of it is for that
> behavior + extra stuff.
> 

IME the __iomap_iter_advance() would be the most low level and flexible
version, whereas the wrappers simplify things. There's also the point
that the wrapper seems the more common case, so maybe that makes things
cleaner if that one is used more often.

But TBH I'm not sure there is strong precedent. I'm content if we can
retain the current variant for the callers that take advantage of it.
Another idea is you could rename the current function to
iomap_iter_advance_and_update_length_for_loopy_callers() and see what
alternative suggestions come up. ;)

Brian

> 
> Thanks,
> Joanne
> >
> > Brian
> 

Re: [PATCH v1] iomap: simplify iomap_iter_advance()

[Christoph Hellwig]

On Thu, Sep 18, 2025 at 07:31:33AM -0400, Brian Foster wrote:
> IME the __iomap_iter_advance() would be the most low level and flexible
> version, whereas the wrappers simplify things. There's also the point
> that the wrapper seems the more common case, so maybe that makes things
> cleaner if that one is used more often.
> 
> But TBH I'm not sure there is strong precedent. I'm content if we can
> retain the current variant for the callers that take advantage of it.
> Another idea is you could rename the current function to
> iomap_iter_advance_and_update_length_for_loopy_callers() and see what
> alternative suggestions come up. ;)

Yeah, __ names are a bit nasty.  I prefer to mostly limit them to
local helpers, or to things with an obvious inline wrapper for the
fast path.  So I your latest suggestions actually aims in the right
directly, but maybe we can shorten the name a little and do something
like:

iomap_iter_advance_and_update_len

although even that would probably lead a few lines to spill.
iomap_iter_advance_len would be a shorter, but a little more confusing,
but still better than __-naming, so maybe it should be fine with a good
kerneldoc comment?

Re: [PATCH v1] iomap: simplify iomap_iter_advance()

[Brian Foster]

On Thu, Sep 18, 2025 at 07:27:29AM -0700, Christoph Hellwig wrote:
> On Thu, Sep 18, 2025 at 07:31:33AM -0400, Brian Foster wrote:
> > IME the __iomap_iter_advance() would be the most low level and flexible
> > version, whereas the wrappers simplify things. There's also the point
> > that the wrapper seems the more common case, so maybe that makes things
> > cleaner if that one is used more often.
> > 
> > But TBH I'm not sure there is strong precedent. I'm content if we can
> > retain the current variant for the callers that take advantage of it.
> > Another idea is you could rename the current function to
> > iomap_iter_advance_and_update_length_for_loopy_callers() and see what
> > alternative suggestions come up. ;)
> 
> Yeah, __ names are a bit nasty.  I prefer to mostly limit them to
> local helpers, or to things with an obvious inline wrapper for the
> fast path.  So I your latest suggestions actually aims in the right
> directly, but maybe we can shorten the name a little and do something
> like:
> 
> iomap_iter_advance_and_update_len
> 
> although even that would probably lead a few lines to spill.
> iomap_iter_advance_len would be a shorter, but a little more confusing,
> but still better than __-naming, so maybe it should be fine with a good
> kerneldoc comment?
> 

Ack, anything like that is fine with me, even something like
iomap_iter_advance_and_length() with a comment that just points out it
also calls iomap_length().

Another thought was to have one helper that returns the remaining length
or error and then a wrapper that translates the return (i.e. return ret
>= 0 ? 0 : ret). But when I thought more about it seemed like it just
created confusion.

Brian

Re: [PATCH v1] iomap: simplify iomap_iter_advance()

[Joanne Koong]

On Thu, Sep 18, 2025 at 1:14 PM Brian Foster <bfoster@redhat.com> wrote:
>
> On Thu, Sep 18, 2025 at 07:27:29AM -0700, Christoph Hellwig wrote:
> > On Thu, Sep 18, 2025 at 07:31:33AM -0400, Brian Foster wrote:
> > > IME the __iomap_iter_advance() would be the most low level and flexible
> > > version, whereas the wrappers simplify things. There's also the point
> > > that the wrapper seems the more common case, so maybe that makes things
> > > cleaner if that one is used more often.
> > >
> > > But TBH I'm not sure there is strong precedent. I'm content if we can
> > > retain the current variant for the callers that take advantage of it.
> > > Another idea is you could rename the current function to
> > > iomap_iter_advance_and_update_length_for_loopy_callers() and see what
> > > alternative suggestions come up. ;)
> >
> > Yeah, __ names are a bit nasty.  I prefer to mostly limit them to
> > local helpers, or to things with an obvious inline wrapper for the
> > fast path.  So I your latest suggestions actually aims in the right
> > directly, but maybe we can shorten the name a little and do something
> > like:
> >
> > iomap_iter_advance_and_update_len
> >
> > although even that would probably lead a few lines to spill.
> > iomap_iter_advance_len would be a shorter, but a little more confusing,
> > but still better than __-naming, so maybe it should be fine with a good
> > kerneldoc comment?
> >
>
> Ack, anything like that is fine with me, even something like
> iomap_iter_advance_and_length() with a comment that just points out it
> also calls iomap_length().
>
> Another thought was to have one helper that returns the remaining length
> or error and then a wrapper that translates the return (i.e. return ret
> >= 0 ? 0 : ret). But when I thought more about it seemed like it just
> created confusion.
>
> Brian
>

I'm looking at this patch again and wondering if the second helper is
all that necessary. I feel like if we're adding it because the caller
could be confused/unclear about needing to update their local length
variable, then wouldn't they also be confused about having to use
iomap_iter_advance_and_length() instead of iomap_iter_advance()? I
feel like if they know enough to know that they need to use
iomap_iter_advance_and_length() instead of iomap_iter_advance(), then
they know enough to update their local length variable themsevles
through iomap_length(). imo it seems cleaner / less cluttery to just
have iomap_iter_advance(). But I'm happy to add the
"iomap_advance_and_length()" helper for v2 if you guys disagree and
prefer having a 2nd helper.


Thanks,
Joanne

Re: [PATCH v1] iomap: simplify iomap_iter_advance()

[Brian Foster]

On Thu, Sep 18, 2025 at 03:10:18PM -0700, Joanne Koong wrote:
> On Thu, Sep 18, 2025 at 1:14 PM Brian Foster <bfoster@redhat.com> wrote:
> >
> > On Thu, Sep 18, 2025 at 07:27:29AM -0700, Christoph Hellwig wrote:
> > > On Thu, Sep 18, 2025 at 07:31:33AM -0400, Brian Foster wrote:
> > > > IME the __iomap_iter_advance() would be the most low level and flexible
> > > > version, whereas the wrappers simplify things. There's also the point
> > > > that the wrapper seems the more common case, so maybe that makes things
> > > > cleaner if that one is used more often.
> > > >
> > > > But TBH I'm not sure there is strong precedent. I'm content if we can
> > > > retain the current variant for the callers that take advantage of it.
> > > > Another idea is you could rename the current function to
> > > > iomap_iter_advance_and_update_length_for_loopy_callers() and see what
> > > > alternative suggestions come up. ;)
> > >
> > > Yeah, __ names are a bit nasty.  I prefer to mostly limit them to
> > > local helpers, or to things with an obvious inline wrapper for the
> > > fast path.  So I your latest suggestions actually aims in the right
> > > directly, but maybe we can shorten the name a little and do something
> > > like:
> > >
> > > iomap_iter_advance_and_update_len
> > >
> > > although even that would probably lead a few lines to spill.
> > > iomap_iter_advance_len would be a shorter, but a little more confusing,
> > > but still better than __-naming, so maybe it should be fine with a good
> > > kerneldoc comment?
> > >
> >
> > Ack, anything like that is fine with me, even something like
> > iomap_iter_advance_and_length() with a comment that just points out it
> > also calls iomap_length().
> >
> > Another thought was to have one helper that returns the remaining length
> > or error and then a wrapper that translates the return (i.e. return ret
> > >= 0 ? 0 : ret). But when I thought more about it seemed like it just
> > created confusion.
> >
> > Brian
> >
> 
> I'm looking at this patch again and wondering if the second helper is
> all that necessary. I feel like if we're adding it because the caller
> could be confused/unclear about needing to update their local length
> variable, then wouldn't they also be confused about having to use
> iomap_iter_advance_and_length() instead of iomap_iter_advance()? I
> feel like if they know enough to know that they need to use
> iomap_iter_advance_and_length() instead of iomap_iter_advance(), then
> they know enough to update their local length variable themsevles
> through iomap_length(). imo it seems cleaner / less cluttery to just
> have iomap_iter_advance(). But I'm happy to add the
> "iomap_advance_and_length()" helper for v2 if you guys disagree and
> prefer having a 2nd helper.
> 

Eh fair point. As mentioned at the top I'm not that worried about it,
just wanted to entertain discussion on a potentially clean way to do
both (thanks). Sounds more like it's not worth it.

Looking back at the patch again, my only followup comment is for the
handful of cases where the newly added iomap_length() is clearly the
tail of a loop, could we instead add the call to the loop iteration
line? I.e.:

	do {
		...
		iomap_iter_advance();
	while ((length = iomap_length(iter)) > 0)

With that tweak I'm good with it:

Reviewed-by: Brian Foster <bfoster@redhat.com>

Brian

> 
> Thanks,
> Joanne
> 

Re: [PATCH v1] iomap: simplify iomap_iter_advance()

[Joanne Koong]

On Fri, Sep 19, 2025 at 4:31 AM Brian Foster <bfoster@redhat.com> wrote:
>
> On Thu, Sep 18, 2025 at 03:10:18PM -0700, Joanne Koong wrote:
> > On Thu, Sep 18, 2025 at 1:14 PM Brian Foster <bfoster@redhat.com> wrote:
> > >
> > > On Thu, Sep 18, 2025 at 07:27:29AM -0700, Christoph Hellwig wrote:
> > > > On Thu, Sep 18, 2025 at 07:31:33AM -0400, Brian Foster wrote:
> > > > > IME the __iomap_iter_advance() would be the most low level and flexible
> > > > > version, whereas the wrappers simplify things. There's also the point
> > > > > that the wrapper seems the more common case, so maybe that makes things
> > > > > cleaner if that one is used more often.
> > > > >
> > > > > But TBH I'm not sure there is strong precedent. I'm content if we can
> > > > > retain the current variant for the callers that take advantage of it.
> > > > > Another idea is you could rename the current function to
> > > > > iomap_iter_advance_and_update_length_for_loopy_callers() and see what
> > > > > alternative suggestions come up. ;)
> > > >
> > > > Yeah, __ names are a bit nasty.  I prefer to mostly limit them to
> > > > local helpers, or to things with an obvious inline wrapper for the
> > > > fast path.  So I your latest suggestions actually aims in the right
> > > > directly, but maybe we can shorten the name a little and do something
> > > > like:
> > > >
> > > > iomap_iter_advance_and_update_len
> > > >
> > > > although even that would probably lead a few lines to spill.
> > > > iomap_iter_advance_len would be a shorter, but a little more confusing,
> > > > but still better than __-naming, so maybe it should be fine with a good
> > > > kerneldoc comment?
> > > >
> > >
> > > Ack, anything like that is fine with me, even something like
> > > iomap_iter_advance_and_length() with a comment that just points out it
> > > also calls iomap_length().
> > >
> > > Another thought was to have one helper that returns the remaining length
> > > or error and then a wrapper that translates the return (i.e. return ret
> > > >= 0 ? 0 : ret). But when I thought more about it seemed like it just
> > > created confusion.
> > >
> > > Brian
> > >
> >
> > I'm looking at this patch again and wondering if the second helper is
> > all that necessary. I feel like if we're adding it because the caller
> > could be confused/unclear about needing to update their local length
> > variable, then wouldn't they also be confused about having to use
> > iomap_iter_advance_and_length() instead of iomap_iter_advance()? I
> > feel like if they know enough to know that they need to use
> > iomap_iter_advance_and_length() instead of iomap_iter_advance(), then
> > they know enough to update their local length variable themsevles
> > through iomap_length(). imo it seems cleaner / less cluttery to just
> > have iomap_iter_advance(). But I'm happy to add the
> > "iomap_advance_and_length()" helper for v2 if you guys disagree and
> > prefer having a 2nd helper.
> >
>
> Eh fair point. As mentioned at the top I'm not that worried about it,
> just wanted to entertain discussion on a potentially clean way to do
> both (thanks). Sounds more like it's not worth it.
>
> Looking back at the patch again, my only followup comment is for the
> handful of cases where the newly added iomap_length() is clearly the
> tail of a loop, could we instead add the call to the loop iteration
> line? I.e.:
>
>         do {
>                 ...
>                 iomap_iter_advance();
>         while ((length = iomap_length(iter)) > 0)
>

This sounds great, I will add this in for v2. Thanks for the review and
discussion on this.


> With that tweak I'm good with it:
>
> Reviewed-by: Brian Foster <bfoster@redhat.com>
>
> Brian
>
> >
> > Thanks,
> > Joanne
> >
>