[PATCH 0/6] iomap: misc buffered write path cleanups and prep

[PATCH 0/6] iomap: misc buffered write path cleanups and prep

[Brian Foster]

Hi all,

Here's a bit more fallout and prep. work associated with the folio batch
prototype posted a while back [1]. Work on that is still pending so it
isn't included here, but based on the iter advance cleanups most of
these seemed worthwhile as standalone cleanups. Mainly this just cleans
up some of the helpers and pushes some pos/len trimming further down in
the write begin path.

The fbatch thing is still in prototype stage, but for context the intent
here is that it can mostly now just bolt onto the folio lookup path
because we can advance the range that is skipped and return the next
folio along with the folio subrange for the caller to process.

Thoughts, reviews, flames appreciated.

Brian

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

Brian Foster (6):
  iomap: resample iter->pos after iomap_write_begin() calls
  iomap: drop unnecessary pos param from iomap_write_[begin|end]
  iomap: drop pos param from __iomap_[get|put]_folio()
  iomap: helper to trim pos/bytes to within folio
  iomap: push non-large folio check into get folio path
  iomap: rework iomap_write_begin() to return folio offset and length

 fs/iomap/buffered-io.c | 92 ++++++++++++++++++++++++------------------
 1 file changed, 53 insertions(+), 39 deletions(-)

-- 
2.49.0

[PATCH 1/6] iomap: resample iter->pos after iomap_write_begin() calls

[Brian Foster]

In preparation for removing the pos parameter, push the local pos
assignment down after calls to iomap_write_begin().

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

diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
index 5b08bd417b28..a41c8ffc4996 100644
--- a/fs/iomap/buffered-io.c
+++ b/fs/iomap/buffered-io.c
@@ -920,11 +920,11 @@ static int 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 */
 		u64 written;		/* Bytes have been written */
-		loff_t pos = iter->pos;
+		loff_t pos;
 
 		bytes = iov_iter_count(i);
 retry:
-		offset = pos & (chunk - 1);
+		offset = iter->pos & (chunk - 1);
 		bytes = min(chunk - offset, bytes);
 		status = balance_dirty_pages_ratelimited_flags(mapping,
 							       bdp_flags);
@@ -949,13 +949,14 @@ static int iomap_write_iter(struct iomap_iter *iter, struct iov_iter *i)
 			break;
 		}
 
-		status = iomap_write_begin(iter, pos, bytes, &folio);
+		status = iomap_write_begin(iter, iter->pos, bytes, &folio);
 		if (unlikely(status)) {
-			iomap_write_failed(iter->inode, pos, bytes);
+			iomap_write_failed(iter->inode, iter->pos, bytes);
 			break;
 		}
 		if (iter->iomap.flags & IOMAP_F_STALE)
 			break;
+		pos = iter->pos;
 
 		offset = offset_in_folio(folio, pos);
 		if (bytes > folio_size(folio) - offset)
@@ -1276,15 +1277,16 @@ static int iomap_unshare_iter(struct iomap_iter *iter)
 	do {
 		struct folio *folio;
 		size_t offset;
-		loff_t pos = iter->pos;
+		loff_t pos;
 		bool ret;
 
 		bytes = min_t(u64, SIZE_MAX, bytes);
-		status = iomap_write_begin(iter, pos, bytes, &folio);
+		status = iomap_write_begin(iter, iter->pos, bytes, &folio);
 		if (unlikely(status))
 			return status;
 		if (iomap->flags & IOMAP_F_STALE)
 			break;
+		pos = iter->pos;
 
 		offset = offset_in_folio(folio, pos);
 		if (bytes > folio_size(folio) - offset)
@@ -1351,15 +1353,16 @@ static int iomap_zero_iter(struct iomap_iter *iter, bool *did_zero)
 	do {
 		struct folio *folio;
 		size_t offset;
-		loff_t pos = iter->pos;
+		loff_t pos;
 		bool ret;
 
 		bytes = min_t(u64, SIZE_MAX, bytes);
-		status = iomap_write_begin(iter, pos, bytes, &folio);
+		status = iomap_write_begin(iter, iter->pos, bytes, &folio);
 		if (status)
 			return status;
 		if (iter->iomap.flags & IOMAP_F_STALE)
 			break;
+		pos = iter->pos;
 
 		/* warn about zeroing folios beyond eof that won't write back */
 		WARN_ON_ONCE(folio_pos(folio) > iter->inode->i_size);
-- 
2.49.0

[PATCH 2/6] iomap: drop unnecessary pos param from iomap_write_[begin|end]

[Brian Foster]

iomap_write_begin() and iomap_write_end() both take the iter and
iter->pos as parameters. Drop the unnecessary pos parameter and
sample iter->pos within each function.

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

diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
index a41c8ffc4996..d1a50300a5dc 100644
--- a/fs/iomap/buffered-io.c
+++ b/fs/iomap/buffered-io.c
@@ -774,11 +774,12 @@ static int iomap_write_begin_inline(const struct iomap_iter *iter,
 	return iomap_read_inline_data(iter, folio);
 }
 
-static int iomap_write_begin(struct iomap_iter *iter, loff_t pos,
-		size_t len, struct folio **foliop)
+static int iomap_write_begin(struct iomap_iter *iter, size_t len,
+		struct folio **foliop)
 {
 	const struct iomap_folio_ops *folio_ops = iter->iomap.folio_ops;
 	const struct iomap *srcmap = iomap_iter_srcmap(iter);
+	loff_t pos = iter->pos;
 	struct folio *folio;
 	int status = 0;
 
@@ -883,10 +884,11 @@ static void iomap_write_end_inline(const struct iomap_iter *iter,
  * Returns true if all copied bytes have been written to the pagecache,
  * otherwise return false.
  */
-static bool iomap_write_end(struct iomap_iter *iter, loff_t pos, size_t len,
-		size_t copied, struct folio *folio)
+static bool iomap_write_end(struct iomap_iter *iter, size_t len, size_t copied,
+		struct folio *folio)
 {
 	const struct iomap *srcmap = iomap_iter_srcmap(iter);
+	loff_t pos = iter->pos;
 
 	if (srcmap->type == IOMAP_INLINE) {
 		iomap_write_end_inline(iter, folio, pos, copied);
@@ -949,7 +951,7 @@ static int iomap_write_iter(struct iomap_iter *iter, struct iov_iter *i)
 			break;
 		}
 
-		status = iomap_write_begin(iter, iter->pos, bytes, &folio);
+		status = iomap_write_begin(iter, bytes, &folio);
 		if (unlikely(status)) {
 			iomap_write_failed(iter->inode, iter->pos, bytes);
 			break;
@@ -966,7 +968,7 @@ static int iomap_write_iter(struct iomap_iter *iter, struct iov_iter *i)
 			flush_dcache_folio(folio);
 
 		copied = copy_folio_from_iter_atomic(folio, offset, bytes, i);
-		written = iomap_write_end(iter, pos, bytes, copied, folio) ?
+		written = iomap_write_end(iter, bytes, copied, folio) ?
 			  copied : 0;
 
 		/*
@@ -1281,7 +1283,7 @@ static int iomap_unshare_iter(struct iomap_iter *iter)
 		bool ret;
 
 		bytes = min_t(u64, SIZE_MAX, bytes);
-		status = iomap_write_begin(iter, iter->pos, bytes, &folio);
+		status = iomap_write_begin(iter, bytes, &folio);
 		if (unlikely(status))
 			return status;
 		if (iomap->flags & IOMAP_F_STALE)
@@ -1292,7 +1294,7 @@ static int iomap_unshare_iter(struct iomap_iter *iter)
 		if (bytes > folio_size(folio) - offset)
 			bytes = folio_size(folio) - offset;
 
-		ret = iomap_write_end(iter, pos, bytes, bytes, folio);
+		ret = iomap_write_end(iter, bytes, bytes, folio);
 		__iomap_put_folio(iter, pos, bytes, folio);
 		if (WARN_ON_ONCE(!ret))
 			return -EIO;
@@ -1357,7 +1359,7 @@ static int iomap_zero_iter(struct iomap_iter *iter, bool *did_zero)
 		bool ret;
 
 		bytes = min_t(u64, SIZE_MAX, bytes);
-		status = iomap_write_begin(iter, iter->pos, bytes, &folio);
+		status = iomap_write_begin(iter, bytes, &folio);
 		if (status)
 			return status;
 		if (iter->iomap.flags & IOMAP_F_STALE)
@@ -1373,7 +1375,7 @@ static int iomap_zero_iter(struct iomap_iter *iter, bool *did_zero)
 		folio_zero_range(folio, offset, bytes);
 		folio_mark_accessed(folio);
 
-		ret = iomap_write_end(iter, pos, bytes, bytes, folio);
+		ret = iomap_write_end(iter, bytes, bytes, folio);
 		__iomap_put_folio(iter, pos, bytes, folio);
 		if (WARN_ON_ONCE(!ret))
 			return -EIO;
-- 
2.49.0

[PATCH 3/6] iomap: drop pos param from __iomap_[get|put]_folio()

[Brian Foster]

Both helpers take the iter and pos as parameters. All callers
effectively pass iter->pos, so drop the unnecessary pos parameter.

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

diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
index d1a50300a5dc..5c08b2916bc7 100644
--- a/fs/iomap/buffered-io.c
+++ b/fs/iomap/buffered-io.c
@@ -741,10 +741,10 @@ static int __iomap_write_begin(const struct iomap_iter *iter, loff_t pos,
 	return 0;
 }
 
-static struct folio *__iomap_get_folio(struct iomap_iter *iter, loff_t pos,
-		size_t len)
+static struct folio *__iomap_get_folio(struct iomap_iter *iter, size_t len)
 {
 	const struct iomap_folio_ops *folio_ops = iter->iomap.folio_ops;
+	loff_t pos = iter->pos;
 
 	if (folio_ops && folio_ops->get_folio)
 		return folio_ops->get_folio(iter, pos, len);
@@ -752,10 +752,11 @@ static struct folio *__iomap_get_folio(struct iomap_iter *iter, loff_t pos,
 		return iomap_get_folio(iter, pos, len);
 }
 
-static void __iomap_put_folio(struct iomap_iter *iter, loff_t pos, size_t ret,
+static void __iomap_put_folio(struct iomap_iter *iter, size_t ret,
 		struct folio *folio)
 {
 	const struct iomap_folio_ops *folio_ops = iter->iomap.folio_ops;
+	loff_t pos = iter->pos;
 
 	if (folio_ops && folio_ops->put_folio) {
 		folio_ops->put_folio(iter->inode, pos, ret, folio);
@@ -793,7 +794,7 @@ static int iomap_write_begin(struct iomap_iter *iter, size_t len,
 	if (!mapping_large_folio_support(iter->inode->i_mapping))
 		len = min_t(size_t, len, PAGE_SIZE - offset_in_page(pos));
 
-	folio = __iomap_get_folio(iter, pos, len);
+	folio = __iomap_get_folio(iter, len);
 	if (IS_ERR(folio))
 		return PTR_ERR(folio);
 
@@ -834,7 +835,7 @@ static int iomap_write_begin(struct iomap_iter *iter, size_t len,
 	return 0;
 
 out_unlock:
-	__iomap_put_folio(iter, pos, 0, folio);
+	__iomap_put_folio(iter, 0, folio);
 
 	return status;
 }
@@ -983,7 +984,7 @@ static int iomap_write_iter(struct iomap_iter *iter, struct iov_iter *i)
 			i_size_write(iter->inode, pos + written);
 			iter->iomap.flags |= IOMAP_F_SIZE_CHANGED;
 		}
-		__iomap_put_folio(iter, pos, written, folio);
+		__iomap_put_folio(iter, written, folio);
 
 		if (old_size < pos)
 			pagecache_isize_extended(iter->inode, old_size, pos);
@@ -1295,7 +1296,7 @@ static int iomap_unshare_iter(struct iomap_iter *iter)
 			bytes = folio_size(folio) - offset;
 
 		ret = iomap_write_end(iter, bytes, bytes, folio);
-		__iomap_put_folio(iter, pos, bytes, folio);
+		__iomap_put_folio(iter, bytes, folio);
 		if (WARN_ON_ONCE(!ret))
 			return -EIO;
 
@@ -1376,7 +1377,7 @@ static int iomap_zero_iter(struct iomap_iter *iter, bool *did_zero)
 		folio_mark_accessed(folio);
 
 		ret = iomap_write_end(iter, bytes, bytes, folio);
-		__iomap_put_folio(iter, pos, bytes, folio);
+		__iomap_put_folio(iter, bytes, folio);
 		if (WARN_ON_ONCE(!ret))
 			return -EIO;
 
-- 
2.49.0

[PATCH 4/6] iomap: helper to trim pos/bytes to within folio

[Brian Foster]

Several buffered write based iteration callbacks duplicate logic to
trim the current pos and length to within the current folio. Factor
this into a helper to make it easier to relocate closer to folio
lookup.

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

diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
index 5c08b2916bc7..5ed3332e69dd 100644
--- a/fs/iomap/buffered-io.c
+++ b/fs/iomap/buffered-io.c
@@ -766,6 +766,21 @@ static void __iomap_put_folio(struct iomap_iter *iter, size_t ret,
 	}
 }
 
+/* trim pos and bytes to within a given folio */
+static loff_t iomap_trim_folio_range(struct iomap_iter *iter,
+		struct folio *folio, size_t *offset, u64 *bytes)
+{
+	loff_t pos = iter->pos;
+	size_t fsize = folio_size(folio);
+
+	WARN_ON_ONCE(pos < folio_pos(folio) || pos >= folio_pos(folio) + fsize);
+
+	*offset = offset_in_folio(folio, pos);
+	if (*bytes > fsize - *offset)
+		*bytes = fsize - *offset;
+	return pos;
+}
+
 static int iomap_write_begin_inline(const struct iomap_iter *iter,
 		struct folio *folio)
 {
@@ -920,7 +935,7 @@ static int iomap_write_iter(struct iomap_iter *iter, struct iov_iter *i)
 		struct folio *folio;
 		loff_t old_size;
 		size_t offset;		/* Offset into folio */
-		size_t bytes;		/* Bytes to write to folio */
+		u64 bytes;		/* Bytes to write to folio */
 		size_t copied;		/* Bytes copied from user */
 		u64 written;		/* Bytes have been written */
 		loff_t pos;
@@ -959,11 +974,8 @@ static int iomap_write_iter(struct iomap_iter *iter, struct iov_iter *i)
 		}
 		if (iter->iomap.flags & IOMAP_F_STALE)
 			break;
-		pos = iter->pos;
 
-		offset = offset_in_folio(folio, pos);
-		if (bytes > folio_size(folio) - offset)
-			bytes = folio_size(folio) - offset;
+		pos = iomap_trim_folio_range(iter, folio, &offset, &bytes);
 
 		if (mapping_writably_mapped(mapping))
 			flush_dcache_folio(folio);
@@ -1280,7 +1292,6 @@ static int iomap_unshare_iter(struct iomap_iter *iter)
 	do {
 		struct folio *folio;
 		size_t offset;
-		loff_t pos;
 		bool ret;
 
 		bytes = min_t(u64, SIZE_MAX, bytes);
@@ -1289,11 +1300,8 @@ static int iomap_unshare_iter(struct iomap_iter *iter)
 			return status;
 		if (iomap->flags & IOMAP_F_STALE)
 			break;
-		pos = iter->pos;
 
-		offset = offset_in_folio(folio, pos);
-		if (bytes > folio_size(folio) - offset)
-			bytes = folio_size(folio) - offset;
+		iomap_trim_folio_range(iter, folio, &offset, &bytes);
 
 		ret = iomap_write_end(iter, bytes, bytes, folio);
 		__iomap_put_folio(iter, bytes, folio);
@@ -1356,7 +1364,6 @@ static int iomap_zero_iter(struct iomap_iter *iter, bool *did_zero)
 	do {
 		struct folio *folio;
 		size_t offset;
-		loff_t pos;
 		bool ret;
 
 		bytes = min_t(u64, SIZE_MAX, bytes);
@@ -1365,14 +1372,11 @@ static int iomap_zero_iter(struct iomap_iter *iter, bool *did_zero)
 			return status;
 		if (iter->iomap.flags & IOMAP_F_STALE)
 			break;
-		pos = iter->pos;
 
 		/* warn about zeroing folios beyond eof that won't write back */
 		WARN_ON_ONCE(folio_pos(folio) > iter->inode->i_size);
-		offset = offset_in_folio(folio, pos);
-		if (bytes > folio_size(folio) - offset)
-			bytes = folio_size(folio) - offset;
 
+		iomap_trim_folio_range(iter, folio, &offset, &bytes);
 		folio_zero_range(folio, offset, bytes);
 		folio_mark_accessed(folio);
 
-- 
2.49.0

[PATCH 5/6] iomap: push non-large folio check into get folio path

[Brian Foster]

The len param to __iomap_get_folio() is primarily a folio allocation
hint. iomap_write_begin() already trims its local len variable based
on the provided folio, so move the large folio support check closer
to folio lookup.

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

diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
index 5ed3332e69dd..d3b30ebad9ea 100644
--- a/fs/iomap/buffered-io.c
+++ b/fs/iomap/buffered-io.c
@@ -746,6 +746,9 @@ static struct folio *__iomap_get_folio(struct iomap_iter *iter, size_t len)
 	const struct iomap_folio_ops *folio_ops = iter->iomap.folio_ops;
 	loff_t pos = iter->pos;
 
+	if (!mapping_large_folio_support(iter->inode->i_mapping))
+		len = min_t(size_t, len, PAGE_SIZE - offset_in_page(pos));
+
 	if (folio_ops && folio_ops->get_folio)
 		return folio_ops->get_folio(iter, pos, len);
 	else
@@ -806,9 +809,6 @@ static int iomap_write_begin(struct iomap_iter *iter, size_t len,
 	if (fatal_signal_pending(current))
 		return -EINTR;
 
-	if (!mapping_large_folio_support(iter->inode->i_mapping))
-		len = min_t(size_t, len, PAGE_SIZE - offset_in_page(pos));
-
 	folio = __iomap_get_folio(iter, len);
 	if (IS_ERR(folio))
 		return PTR_ERR(folio);
-- 
2.49.0

[PATCH 6/6] iomap: rework iomap_write_begin() to return folio offset and length

[Brian Foster]

iomap_write_begin() returns a folio based on current pos and
remaining length in the iter, and each caller then trims the
pos/length to the given folio. Clean this up a bit and let
iomap_write_begin() return the trimmed range along with the folio.

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

diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
index d3b30ebad9ea..2fde268c39fc 100644
--- a/fs/iomap/buffered-io.c
+++ b/fs/iomap/buffered-io.c
@@ -793,15 +793,22 @@ static int iomap_write_begin_inline(const struct iomap_iter *iter,
 	return iomap_read_inline_data(iter, folio);
 }
 
-static int iomap_write_begin(struct iomap_iter *iter, size_t len,
-		struct folio **foliop)
+/*
+ * Grab and prepare a folio for write based on iter state. Returns the folio,
+ * offset, and length. Callers can optionally pass a max length *plen,
+ * otherwise init to zero.
+ */
+static int iomap_write_begin(struct iomap_iter *iter, struct folio **foliop,
+		size_t *poffset, u64 *plen)
 {
 	const struct iomap_folio_ops *folio_ops = iter->iomap.folio_ops;
 	const struct iomap *srcmap = iomap_iter_srcmap(iter);
 	loff_t pos = iter->pos;
+	u64 len = min_t(u64, SIZE_MAX, iomap_length(iter));
 	struct folio *folio;
 	int status = 0;
 
+	len = *plen > 0 ? min_t(u64, len, *plen) : len;
 	BUG_ON(pos + len > iter->iomap.offset + iter->iomap.length);
 	if (srcmap != &iter->iomap)
 		BUG_ON(pos + len > srcmap->offset + srcmap->length);
@@ -833,8 +840,7 @@ static int iomap_write_begin(struct iomap_iter *iter, size_t len,
 		}
 	}
 
-	if (pos + len > folio_pos(folio) + folio_size(folio))
-		len = folio_pos(folio) + folio_size(folio) - pos;
+	pos = iomap_trim_folio_range(iter, folio, poffset, &len);
 
 	if (srcmap->type == IOMAP_INLINE)
 		status = iomap_write_begin_inline(iter, folio);
@@ -847,6 +853,7 @@ static int iomap_write_begin(struct iomap_iter *iter, size_t len,
 		goto out_unlock;
 
 	*foliop = folio;
+	*plen = len;
 	return 0;
 
 out_unlock:
@@ -967,7 +974,7 @@ static int iomap_write_iter(struct iomap_iter *iter, struct iov_iter *i)
 			break;
 		}
 
-		status = iomap_write_begin(iter, bytes, &folio);
+		status = iomap_write_begin(iter, &folio, &offset, &bytes);
 		if (unlikely(status)) {
 			iomap_write_failed(iter->inode, iter->pos, bytes);
 			break;
@@ -975,7 +982,7 @@ static int iomap_write_iter(struct iomap_iter *iter, struct iov_iter *i)
 		if (iter->iomap.flags & IOMAP_F_STALE)
 			break;
 
-		pos = iomap_trim_folio_range(iter, folio, &offset, &bytes);
+		pos = iter->pos;
 
 		if (mapping_writably_mapped(mapping))
 			flush_dcache_folio(folio);
@@ -1295,14 +1302,12 @@ static int iomap_unshare_iter(struct iomap_iter *iter)
 		bool ret;
 
 		bytes = min_t(u64, SIZE_MAX, bytes);
-		status = iomap_write_begin(iter, bytes, &folio);
+		status = iomap_write_begin(iter, &folio, &offset, &bytes);
 		if (unlikely(status))
 			return status;
 		if (iomap->flags & IOMAP_F_STALE)
 			break;
 
-		iomap_trim_folio_range(iter, folio, &offset, &bytes);
-
 		ret = iomap_write_end(iter, bytes, bytes, folio);
 		__iomap_put_folio(iter, bytes, folio);
 		if (WARN_ON_ONCE(!ret))
@@ -1367,7 +1372,7 @@ static int iomap_zero_iter(struct iomap_iter *iter, bool *did_zero)
 		bool ret;
 
 		bytes = min_t(u64, SIZE_MAX, bytes);
-		status = iomap_write_begin(iter, bytes, &folio);
+		status = iomap_write_begin(iter, &folio, &offset, &bytes);
 		if (status)
 			return status;
 		if (iter->iomap.flags & IOMAP_F_STALE)
@@ -1376,7 +1381,6 @@ static int iomap_zero_iter(struct iomap_iter *iter, bool *did_zero)
 		/* warn about zeroing folios beyond eof that won't write back */
 		WARN_ON_ONCE(folio_pos(folio) > iter->inode->i_size);
 
-		iomap_trim_folio_range(iter, folio, &offset, &bytes);
 		folio_zero_range(folio, offset, bytes);
 		folio_mark_accessed(folio);
 
-- 
2.49.0

Re: [PATCH 6/6] iomap: rework iomap_write_begin() to return folio offset and length

[Darrick J. Wong]

On Wed, Apr 30, 2025 at 03:01:12PM -0400, Brian Foster wrote:
> iomap_write_begin() returns a folio based on current pos and
> remaining length in the iter, and each caller then trims the
> pos/length to the given folio. Clean this up a bit and let
> iomap_write_begin() return the trimmed range along with the folio.
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>
> ---
>  fs/iomap/buffered-io.c | 26 +++++++++++++++-----------
>  1 file changed, 15 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
> index d3b30ebad9ea..2fde268c39fc 100644
> --- a/fs/iomap/buffered-io.c
> +++ b/fs/iomap/buffered-io.c
> @@ -793,15 +793,22 @@ static int iomap_write_begin_inline(const struct iomap_iter *iter,
>  	return iomap_read_inline_data(iter, folio);
>  }
>  
> -static int iomap_write_begin(struct iomap_iter *iter, size_t len,
> -		struct folio **foliop)
> +/*
> + * Grab and prepare a folio for write based on iter state. Returns the folio,
> + * offset, and length. Callers can optionally pass a max length *plen,
> + * otherwise init to zero.
> + */
> +static int iomap_write_begin(struct iomap_iter *iter, struct folio **foliop,
> +		size_t *poffset, u64 *plen)

Hmm, is this offset and length supposed to be bytes within the folio?
I find it a little odd that plen would be a u64 then, unless we're
preparing for folios that huge?  Or is that just to avoid integer
truncation issues?

--D

>  {
>  	const struct iomap_folio_ops *folio_ops = iter->iomap.folio_ops;
>  	const struct iomap *srcmap = iomap_iter_srcmap(iter);
>  	loff_t pos = iter->pos;
> +	u64 len = min_t(u64, SIZE_MAX, iomap_length(iter));
>  	struct folio *folio;
>  	int status = 0;
>  
> +	len = *plen > 0 ? min_t(u64, len, *plen) : len;
>  	BUG_ON(pos + len > iter->iomap.offset + iter->iomap.length);
>  	if (srcmap != &iter->iomap)
>  		BUG_ON(pos + len > srcmap->offset + srcmap->length);
> @@ -833,8 +840,7 @@ static int iomap_write_begin(struct iomap_iter *iter, size_t len,
>  		}
>  	}
>  
> -	if (pos + len > folio_pos(folio) + folio_size(folio))
> -		len = folio_pos(folio) + folio_size(folio) - pos;
> +	pos = iomap_trim_folio_range(iter, folio, poffset, &len);
>  
>  	if (srcmap->type == IOMAP_INLINE)
>  		status = iomap_write_begin_inline(iter, folio);
> @@ -847,6 +853,7 @@ static int iomap_write_begin(struct iomap_iter *iter, size_t len,
>  		goto out_unlock;
>  
>  	*foliop = folio;
> +	*plen = len;
>  	return 0;
>  
>  out_unlock:
> @@ -967,7 +974,7 @@ static int iomap_write_iter(struct iomap_iter *iter, struct iov_iter *i)
>  			break;
>  		}
>  
> -		status = iomap_write_begin(iter, bytes, &folio);
> +		status = iomap_write_begin(iter, &folio, &offset, &bytes);
>  		if (unlikely(status)) {
>  			iomap_write_failed(iter->inode, iter->pos, bytes);
>  			break;
> @@ -975,7 +982,7 @@ static int iomap_write_iter(struct iomap_iter *iter, struct iov_iter *i)
>  		if (iter->iomap.flags & IOMAP_F_STALE)
>  			break;
>  
> -		pos = iomap_trim_folio_range(iter, folio, &offset, &bytes);
> +		pos = iter->pos;
>  
>  		if (mapping_writably_mapped(mapping))
>  			flush_dcache_folio(folio);
> @@ -1295,14 +1302,12 @@ static int iomap_unshare_iter(struct iomap_iter *iter)
>  		bool ret;
>  
>  		bytes = min_t(u64, SIZE_MAX, bytes);
> -		status = iomap_write_begin(iter, bytes, &folio);
> +		status = iomap_write_begin(iter, &folio, &offset, &bytes);
>  		if (unlikely(status))
>  			return status;
>  		if (iomap->flags & IOMAP_F_STALE)
>  			break;
>  
> -		iomap_trim_folio_range(iter, folio, &offset, &bytes);
> -
>  		ret = iomap_write_end(iter, bytes, bytes, folio);
>  		__iomap_put_folio(iter, bytes, folio);
>  		if (WARN_ON_ONCE(!ret))
> @@ -1367,7 +1372,7 @@ static int iomap_zero_iter(struct iomap_iter *iter, bool *did_zero)
>  		bool ret;
>  
>  		bytes = min_t(u64, SIZE_MAX, bytes);
> -		status = iomap_write_begin(iter, bytes, &folio);
> +		status = iomap_write_begin(iter, &folio, &offset, &bytes);
>  		if (status)
>  			return status;
>  		if (iter->iomap.flags & IOMAP_F_STALE)
> @@ -1376,7 +1381,6 @@ static int iomap_zero_iter(struct iomap_iter *iter, bool *did_zero)
>  		/* warn about zeroing folios beyond eof that won't write back */
>  		WARN_ON_ONCE(folio_pos(folio) > iter->inode->i_size);
>  
> -		iomap_trim_folio_range(iter, folio, &offset, &bytes);
>  		folio_zero_range(folio, offset, bytes);
>  		folio_mark_accessed(folio);
>  
> -- 
> 2.49.0
> 
> 

Re: [PATCH 2/6] iomap: drop unnecessary pos param from iomap_write_[begin|end]

[Darrick J. Wong]

On Wed, Apr 30, 2025 at 03:01:08PM -0400, Brian Foster wrote:
> iomap_write_begin() and iomap_write_end() both take the iter and
> iter->pos as parameters. Drop the unnecessary pos parameter and
> sample iter->pos within each function.
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>

Looks ok,
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

--D

> ---
>  fs/iomap/buffered-io.c | 22 ++++++++++++----------
>  1 file changed, 12 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
> index a41c8ffc4996..d1a50300a5dc 100644
> --- a/fs/iomap/buffered-io.c
> +++ b/fs/iomap/buffered-io.c
> @@ -774,11 +774,12 @@ static int iomap_write_begin_inline(const struct iomap_iter *iter,
>  	return iomap_read_inline_data(iter, folio);
>  }
>  
> -static int iomap_write_begin(struct iomap_iter *iter, loff_t pos,
> -		size_t len, struct folio **foliop)
> +static int iomap_write_begin(struct iomap_iter *iter, size_t len,
> +		struct folio **foliop)
>  {
>  	const struct iomap_folio_ops *folio_ops = iter->iomap.folio_ops;
>  	const struct iomap *srcmap = iomap_iter_srcmap(iter);
> +	loff_t pos = iter->pos;
>  	struct folio *folio;
>  	int status = 0;
>  
> @@ -883,10 +884,11 @@ static void iomap_write_end_inline(const struct iomap_iter *iter,
>   * Returns true if all copied bytes have been written to the pagecache,
>   * otherwise return false.
>   */
> -static bool iomap_write_end(struct iomap_iter *iter, loff_t pos, size_t len,
> -		size_t copied, struct folio *folio)
> +static bool iomap_write_end(struct iomap_iter *iter, size_t len, size_t copied,
> +		struct folio *folio)
>  {
>  	const struct iomap *srcmap = iomap_iter_srcmap(iter);
> +	loff_t pos = iter->pos;
>  
>  	if (srcmap->type == IOMAP_INLINE) {
>  		iomap_write_end_inline(iter, folio, pos, copied);
> @@ -949,7 +951,7 @@ static int iomap_write_iter(struct iomap_iter *iter, struct iov_iter *i)
>  			break;
>  		}
>  
> -		status = iomap_write_begin(iter, iter->pos, bytes, &folio);
> +		status = iomap_write_begin(iter, bytes, &folio);
>  		if (unlikely(status)) {
>  			iomap_write_failed(iter->inode, iter->pos, bytes);
>  			break;
> @@ -966,7 +968,7 @@ static int iomap_write_iter(struct iomap_iter *iter, struct iov_iter *i)
>  			flush_dcache_folio(folio);
>  
>  		copied = copy_folio_from_iter_atomic(folio, offset, bytes, i);
> -		written = iomap_write_end(iter, pos, bytes, copied, folio) ?
> +		written = iomap_write_end(iter, bytes, copied, folio) ?
>  			  copied : 0;
>  
>  		/*
> @@ -1281,7 +1283,7 @@ static int iomap_unshare_iter(struct iomap_iter *iter)
>  		bool ret;
>  
>  		bytes = min_t(u64, SIZE_MAX, bytes);
> -		status = iomap_write_begin(iter, iter->pos, bytes, &folio);
> +		status = iomap_write_begin(iter, bytes, &folio);
>  		if (unlikely(status))
>  			return status;
>  		if (iomap->flags & IOMAP_F_STALE)
> @@ -1292,7 +1294,7 @@ static int iomap_unshare_iter(struct iomap_iter *iter)
>  		if (bytes > folio_size(folio) - offset)
>  			bytes = folio_size(folio) - offset;
>  
> -		ret = iomap_write_end(iter, pos, bytes, bytes, folio);
> +		ret = iomap_write_end(iter, bytes, bytes, folio);
>  		__iomap_put_folio(iter, pos, bytes, folio);
>  		if (WARN_ON_ONCE(!ret))
>  			return -EIO;
> @@ -1357,7 +1359,7 @@ static int iomap_zero_iter(struct iomap_iter *iter, bool *did_zero)
>  		bool ret;
>  
>  		bytes = min_t(u64, SIZE_MAX, bytes);
> -		status = iomap_write_begin(iter, iter->pos, bytes, &folio);
> +		status = iomap_write_begin(iter, bytes, &folio);
>  		if (status)
>  			return status;
>  		if (iter->iomap.flags & IOMAP_F_STALE)
> @@ -1373,7 +1375,7 @@ static int iomap_zero_iter(struct iomap_iter *iter, bool *did_zero)
>  		folio_zero_range(folio, offset, bytes);
>  		folio_mark_accessed(folio);
>  
> -		ret = iomap_write_end(iter, pos, bytes, bytes, folio);
> +		ret = iomap_write_end(iter, bytes, bytes, folio);
>  		__iomap_put_folio(iter, pos, bytes, folio);
>  		if (WARN_ON_ONCE(!ret))
>  			return -EIO;
> -- 
> 2.49.0
> 
> 

Re: [PATCH 3/6] iomap: drop pos param from __iomap_[get|put]_folio()

[Darrick J. Wong]

On Wed, Apr 30, 2025 at 03:01:09PM -0400, Brian Foster wrote:
> Both helpers take the iter and pos as parameters. All callers
> effectively pass iter->pos, so drop the unnecessary pos parameter.
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>

Looks ok,
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

--D

> ---
>  fs/iomap/buffered-io.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
> index d1a50300a5dc..5c08b2916bc7 100644
> --- a/fs/iomap/buffered-io.c
> +++ b/fs/iomap/buffered-io.c
> @@ -741,10 +741,10 @@ static int __iomap_write_begin(const struct iomap_iter *iter, loff_t pos,
>  	return 0;
>  }
>  
> -static struct folio *__iomap_get_folio(struct iomap_iter *iter, loff_t pos,
> -		size_t len)
> +static struct folio *__iomap_get_folio(struct iomap_iter *iter, size_t len)
>  {
>  	const struct iomap_folio_ops *folio_ops = iter->iomap.folio_ops;
> +	loff_t pos = iter->pos;
>  
>  	if (folio_ops && folio_ops->get_folio)
>  		return folio_ops->get_folio(iter, pos, len);
> @@ -752,10 +752,11 @@ static struct folio *__iomap_get_folio(struct iomap_iter *iter, loff_t pos,
>  		return iomap_get_folio(iter, pos, len);
>  }
>  
> -static void __iomap_put_folio(struct iomap_iter *iter, loff_t pos, size_t ret,
> +static void __iomap_put_folio(struct iomap_iter *iter, size_t ret,
>  		struct folio *folio)
>  {
>  	const struct iomap_folio_ops *folio_ops = iter->iomap.folio_ops;
> +	loff_t pos = iter->pos;
>  
>  	if (folio_ops && folio_ops->put_folio) {
>  		folio_ops->put_folio(iter->inode, pos, ret, folio);
> @@ -793,7 +794,7 @@ static int iomap_write_begin(struct iomap_iter *iter, size_t len,
>  	if (!mapping_large_folio_support(iter->inode->i_mapping))
>  		len = min_t(size_t, len, PAGE_SIZE - offset_in_page(pos));
>  
> -	folio = __iomap_get_folio(iter, pos, len);
> +	folio = __iomap_get_folio(iter, len);
>  	if (IS_ERR(folio))
>  		return PTR_ERR(folio);
>  
> @@ -834,7 +835,7 @@ static int iomap_write_begin(struct iomap_iter *iter, size_t len,
>  	return 0;
>  
>  out_unlock:
> -	__iomap_put_folio(iter, pos, 0, folio);
> +	__iomap_put_folio(iter, 0, folio);
>  
>  	return status;
>  }
> @@ -983,7 +984,7 @@ static int iomap_write_iter(struct iomap_iter *iter, struct iov_iter *i)
>  			i_size_write(iter->inode, pos + written);
>  			iter->iomap.flags |= IOMAP_F_SIZE_CHANGED;
>  		}
> -		__iomap_put_folio(iter, pos, written, folio);
> +		__iomap_put_folio(iter, written, folio);
>  
>  		if (old_size < pos)
>  			pagecache_isize_extended(iter->inode, old_size, pos);
> @@ -1295,7 +1296,7 @@ static int iomap_unshare_iter(struct iomap_iter *iter)
>  			bytes = folio_size(folio) - offset;
>  
>  		ret = iomap_write_end(iter, bytes, bytes, folio);
> -		__iomap_put_folio(iter, pos, bytes, folio);
> +		__iomap_put_folio(iter, bytes, folio);
>  		if (WARN_ON_ONCE(!ret))
>  			return -EIO;
>  
> @@ -1376,7 +1377,7 @@ static int iomap_zero_iter(struct iomap_iter *iter, bool *did_zero)
>  		folio_mark_accessed(folio);
>  
>  		ret = iomap_write_end(iter, bytes, bytes, folio);
> -		__iomap_put_folio(iter, pos, bytes, folio);
> +		__iomap_put_folio(iter, bytes, folio);
>  		if (WARN_ON_ONCE(!ret))
>  			return -EIO;
>  
> -- 
> 2.49.0
> 
> 

Re: [PATCH 6/6] iomap: rework iomap_write_begin() to return folio offset and length

[Brian Foster]

On Thu, May 01, 2025 at 03:22:29PM -0700, Darrick J. Wong wrote:
> On Wed, Apr 30, 2025 at 03:01:12PM -0400, Brian Foster wrote:
> > iomap_write_begin() returns a folio based on current pos and
> > remaining length in the iter, and each caller then trims the
> > pos/length to the given folio. Clean this up a bit and let
> > iomap_write_begin() return the trimmed range along with the folio.
> > 
> > Signed-off-by: Brian Foster <bfoster@redhat.com>
> > ---
> >  fs/iomap/buffered-io.c | 26 +++++++++++++++-----------
> >  1 file changed, 15 insertions(+), 11 deletions(-)
> > 
> > diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
> > index d3b30ebad9ea..2fde268c39fc 100644
> > --- a/fs/iomap/buffered-io.c
> > +++ b/fs/iomap/buffered-io.c
> > @@ -793,15 +793,22 @@ static int iomap_write_begin_inline(const struct iomap_iter *iter,
> >  	return iomap_read_inline_data(iter, folio);
> >  }
> >  
> > -static int iomap_write_begin(struct iomap_iter *iter, size_t len,
> > -		struct folio **foliop)
> > +/*
> > + * Grab and prepare a folio for write based on iter state. Returns the folio,
> > + * offset, and length. Callers can optionally pass a max length *plen,
> > + * otherwise init to zero.
> > + */
> > +static int iomap_write_begin(struct iomap_iter *iter, struct folio **foliop,
> > +		size_t *poffset, u64 *plen)
> 
> Hmm, is this offset and length supposed to be bytes within the folio?
> I find it a little odd that plen would be a u64 then, unless we're
> preparing for folios that huge?  Or is that just to avoid integer
> truncation issues?
> 

It was more the latter.. it's an input/output param for callers that use
iomap_length(). That returns a u64, so just trying to keep things
consistent. I suppose we could break the function decl into one param
per line with "in/out" type comments if you think that is useful..?

Brian

> --D
> 
> >  {
> >  	const struct iomap_folio_ops *folio_ops = iter->iomap.folio_ops;
> >  	const struct iomap *srcmap = iomap_iter_srcmap(iter);
> >  	loff_t pos = iter->pos;
> > +	u64 len = min_t(u64, SIZE_MAX, iomap_length(iter));
> >  	struct folio *folio;
> >  	int status = 0;
> >  
> > +	len = *plen > 0 ? min_t(u64, len, *plen) : len;
> >  	BUG_ON(pos + len > iter->iomap.offset + iter->iomap.length);
> >  	if (srcmap != &iter->iomap)
> >  		BUG_ON(pos + len > srcmap->offset + srcmap->length);
> > @@ -833,8 +840,7 @@ static int iomap_write_begin(struct iomap_iter *iter, size_t len,
> >  		}
> >  	}
> >  
> > -	if (pos + len > folio_pos(folio) + folio_size(folio))
> > -		len = folio_pos(folio) + folio_size(folio) - pos;
> > +	pos = iomap_trim_folio_range(iter, folio, poffset, &len);
> >  
> >  	if (srcmap->type == IOMAP_INLINE)
> >  		status = iomap_write_begin_inline(iter, folio);
> > @@ -847,6 +853,7 @@ static int iomap_write_begin(struct iomap_iter *iter, size_t len,
> >  		goto out_unlock;
> >  
> >  	*foliop = folio;
> > +	*plen = len;
> >  	return 0;
> >  
> >  out_unlock:
> > @@ -967,7 +974,7 @@ static int iomap_write_iter(struct iomap_iter *iter, struct iov_iter *i)
> >  			break;
> >  		}
> >  
> > -		status = iomap_write_begin(iter, bytes, &folio);
> > +		status = iomap_write_begin(iter, &folio, &offset, &bytes);
> >  		if (unlikely(status)) {
> >  			iomap_write_failed(iter->inode, iter->pos, bytes);
> >  			break;
> > @@ -975,7 +982,7 @@ static int iomap_write_iter(struct iomap_iter *iter, struct iov_iter *i)
> >  		if (iter->iomap.flags & IOMAP_F_STALE)
> >  			break;
> >  
> > -		pos = iomap_trim_folio_range(iter, folio, &offset, &bytes);
> > +		pos = iter->pos;
> >  
> >  		if (mapping_writably_mapped(mapping))
> >  			flush_dcache_folio(folio);
> > @@ -1295,14 +1302,12 @@ static int iomap_unshare_iter(struct iomap_iter *iter)
> >  		bool ret;
> >  
> >  		bytes = min_t(u64, SIZE_MAX, bytes);
> > -		status = iomap_write_begin(iter, bytes, &folio);
> > +		status = iomap_write_begin(iter, &folio, &offset, &bytes);
> >  		if (unlikely(status))
> >  			return status;
> >  		if (iomap->flags & IOMAP_F_STALE)
> >  			break;
> >  
> > -		iomap_trim_folio_range(iter, folio, &offset, &bytes);
> > -
> >  		ret = iomap_write_end(iter, bytes, bytes, folio);
> >  		__iomap_put_folio(iter, bytes, folio);
> >  		if (WARN_ON_ONCE(!ret))
> > @@ -1367,7 +1372,7 @@ static int iomap_zero_iter(struct iomap_iter *iter, bool *did_zero)
> >  		bool ret;
> >  
> >  		bytes = min_t(u64, SIZE_MAX, bytes);
> > -		status = iomap_write_begin(iter, bytes, &folio);
> > +		status = iomap_write_begin(iter, &folio, &offset, &bytes);
> >  		if (status)
> >  			return status;
> >  		if (iter->iomap.flags & IOMAP_F_STALE)
> > @@ -1376,7 +1381,6 @@ static int iomap_zero_iter(struct iomap_iter *iter, bool *did_zero)
> >  		/* warn about zeroing folios beyond eof that won't write back */
> >  		WARN_ON_ONCE(folio_pos(folio) > iter->inode->i_size);
> >  
> > -		iomap_trim_folio_range(iter, folio, &offset, &bytes);
> >  		folio_zero_range(folio, offset, bytes);
> >  		folio_mark_accessed(folio);
> >  
> > -- 
> > 2.49.0
> > 
> > 
> 

Re: [PATCH 6/6] iomap: rework iomap_write_begin() to return folio offset and length

[Darrick J. Wong]

On Fri, May 02, 2025 at 09:18:40AM -0400, Brian Foster wrote:
> On Thu, May 01, 2025 at 03:22:29PM -0700, Darrick J. Wong wrote:
> > On Wed, Apr 30, 2025 at 03:01:12PM -0400, Brian Foster wrote:
> > > iomap_write_begin() returns a folio based on current pos and
> > > remaining length in the iter, and each caller then trims the
> > > pos/length to the given folio. Clean this up a bit and let
> > > iomap_write_begin() return the trimmed range along with the folio.
> > > 
> > > Signed-off-by: Brian Foster <bfoster@redhat.com>
> > > ---
> > >  fs/iomap/buffered-io.c | 26 +++++++++++++++-----------
> > >  1 file changed, 15 insertions(+), 11 deletions(-)
> > > 
> > > diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
> > > index d3b30ebad9ea..2fde268c39fc 100644
> > > --- a/fs/iomap/buffered-io.c
> > > +++ b/fs/iomap/buffered-io.c
> > > @@ -793,15 +793,22 @@ static int iomap_write_begin_inline(const struct iomap_iter *iter,
> > >  	return iomap_read_inline_data(iter, folio);
> > >  }
> > >  
> > > -static int iomap_write_begin(struct iomap_iter *iter, size_t len,
> > > -		struct folio **foliop)
> > > +/*
> > > + * Grab and prepare a folio for write based on iter state. Returns the folio,
> > > + * offset, and length. Callers can optionally pass a max length *plen,
> > > + * otherwise init to zero.
> > > + */
> > > +static int iomap_write_begin(struct iomap_iter *iter, struct folio **foliop,
> > > +		size_t *poffset, u64 *plen)
> > 
> > Hmm, is this offset and length supposed to be bytes within the folio?
> > I find it a little odd that plen would be a u64 then, unless we're
> > preparing for folios that huge?  Or is that just to avoid integer
> > truncation issues?
> > 
> 
> It was more the latter.. it's an input/output param for callers that use
> iomap_length(). That returns a u64, so just trying to keep things
> consistent. I suppose we could break the function decl into one param
> per line with "in/out" type comments if you think that is useful..?

willy tells me we actually /are/ trying to keep things clear for some
day supporting folio_size > 2G so I guess the u64/size_t usage here is
ok then. :)

--D

> Brian
> 
> > --D
> > 
> > >  {
> > >  	const struct iomap_folio_ops *folio_ops = iter->iomap.folio_ops;
> > >  	const struct iomap *srcmap = iomap_iter_srcmap(iter);
> > >  	loff_t pos = iter->pos;
> > > +	u64 len = min_t(u64, SIZE_MAX, iomap_length(iter));
> > >  	struct folio *folio;
> > >  	int status = 0;
> > >  
> > > +	len = *plen > 0 ? min_t(u64, len, *plen) : len;
> > >  	BUG_ON(pos + len > iter->iomap.offset + iter->iomap.length);
> > >  	if (srcmap != &iter->iomap)
> > >  		BUG_ON(pos + len > srcmap->offset + srcmap->length);
> > > @@ -833,8 +840,7 @@ static int iomap_write_begin(struct iomap_iter *iter, size_t len,
> > >  		}
> > >  	}
> > >  
> > > -	if (pos + len > folio_pos(folio) + folio_size(folio))
> > > -		len = folio_pos(folio) + folio_size(folio) - pos;
> > > +	pos = iomap_trim_folio_range(iter, folio, poffset, &len);
> > >  
> > >  	if (srcmap->type == IOMAP_INLINE)
> > >  		status = iomap_write_begin_inline(iter, folio);
> > > @@ -847,6 +853,7 @@ static int iomap_write_begin(struct iomap_iter *iter, size_t len,
> > >  		goto out_unlock;
> > >  
> > >  	*foliop = folio;
> > > +	*plen = len;
> > >  	return 0;
> > >  
> > >  out_unlock:
> > > @@ -967,7 +974,7 @@ static int iomap_write_iter(struct iomap_iter *iter, struct iov_iter *i)
> > >  			break;
> > >  		}
> > >  
> > > -		status = iomap_write_begin(iter, bytes, &folio);
> > > +		status = iomap_write_begin(iter, &folio, &offset, &bytes);
> > >  		if (unlikely(status)) {
> > >  			iomap_write_failed(iter->inode, iter->pos, bytes);
> > >  			break;
> > > @@ -975,7 +982,7 @@ static int iomap_write_iter(struct iomap_iter *iter, struct iov_iter *i)
> > >  		if (iter->iomap.flags & IOMAP_F_STALE)
> > >  			break;
> > >  
> > > -		pos = iomap_trim_folio_range(iter, folio, &offset, &bytes);
> > > +		pos = iter->pos;
> > >  
> > >  		if (mapping_writably_mapped(mapping))
> > >  			flush_dcache_folio(folio);
> > > @@ -1295,14 +1302,12 @@ static int iomap_unshare_iter(struct iomap_iter *iter)
> > >  		bool ret;
> > >  
> > >  		bytes = min_t(u64, SIZE_MAX, bytes);
> > > -		status = iomap_write_begin(iter, bytes, &folio);
> > > +		status = iomap_write_begin(iter, &folio, &offset, &bytes);
> > >  		if (unlikely(status))
> > >  			return status;
> > >  		if (iomap->flags & IOMAP_F_STALE)
> > >  			break;
> > >  
> > > -		iomap_trim_folio_range(iter, folio, &offset, &bytes);
> > > -
> > >  		ret = iomap_write_end(iter, bytes, bytes, folio);
> > >  		__iomap_put_folio(iter, bytes, folio);
> > >  		if (WARN_ON_ONCE(!ret))
> > > @@ -1367,7 +1372,7 @@ static int iomap_zero_iter(struct iomap_iter *iter, bool *did_zero)
> > >  		bool ret;
> > >  
> > >  		bytes = min_t(u64, SIZE_MAX, bytes);
> > > -		status = iomap_write_begin(iter, bytes, &folio);
> > > +		status = iomap_write_begin(iter, &folio, &offset, &bytes);
> > >  		if (status)
> > >  			return status;
> > >  		if (iter->iomap.flags & IOMAP_F_STALE)
> > > @@ -1376,7 +1381,6 @@ static int iomap_zero_iter(struct iomap_iter *iter, bool *did_zero)
> > >  		/* warn about zeroing folios beyond eof that won't write back */
> > >  		WARN_ON_ONCE(folio_pos(folio) > iter->inode->i_size);
> > >  
> > > -		iomap_trim_folio_range(iter, folio, &offset, &bytes);
> > >  		folio_zero_range(folio, offset, bytes);
> > >  		folio_mark_accessed(folio);
> > >  
> > > -- 
> > > 2.49.0
> > > 
> > > 
> > 
> 
> 

Re: [PATCH 6/6] iomap: rework iomap_write_begin() to return folio offset and length

[Darrick J. Wong]

On Wed, Apr 30, 2025 at 03:01:12PM -0400, Brian Foster wrote:
> iomap_write_begin() returns a folio based on current pos and
> remaining length in the iter, and each caller then trims the
> pos/length to the given folio. Clean this up a bit and let
> iomap_write_begin() return the trimmed range along with the folio.
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>
> ---
>  fs/iomap/buffered-io.c | 26 +++++++++++++++-----------
>  1 file changed, 15 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
> index d3b30ebad9ea..2fde268c39fc 100644
> --- a/fs/iomap/buffered-io.c
> +++ b/fs/iomap/buffered-io.c
> @@ -793,15 +793,22 @@ static int iomap_write_begin_inline(const struct iomap_iter *iter,
>  	return iomap_read_inline_data(iter, folio);
>  }
>  
> -static int iomap_write_begin(struct iomap_iter *iter, size_t len,
> -		struct folio **foliop)
> +/*
> + * Grab and prepare a folio for write based on iter state. Returns the folio,
> + * offset, and length. Callers can optionally pass a max length *plen,
> + * otherwise init to zero.
> + */
> +static int iomap_write_begin(struct iomap_iter *iter, struct folio **foliop,
> +		size_t *poffset, u64 *plen)
>  {
>  	const struct iomap_folio_ops *folio_ops = iter->iomap.folio_ops;
>  	const struct iomap *srcmap = iomap_iter_srcmap(iter);
>  	loff_t pos = iter->pos;
> +	u64 len = min_t(u64, SIZE_MAX, iomap_length(iter));
>  	struct folio *folio;
>  	int status = 0;
>  
> +	len = *plen > 0 ? min_t(u64, len, *plen) : len;
>  	BUG_ON(pos + len > iter->iomap.offset + iter->iomap.length);
>  	if (srcmap != &iter->iomap)
>  		BUG_ON(pos + len > srcmap->offset + srcmap->length);
> @@ -833,8 +840,7 @@ static int iomap_write_begin(struct iomap_iter *iter, size_t len,
>  		}
>  	}
>  
> -	if (pos + len > folio_pos(folio) + folio_size(folio))
> -		len = folio_pos(folio) + folio_size(folio) - pos;
> +	pos = iomap_trim_folio_range(iter, folio, poffset, &len);

Do we still need this pos variable?  AFAICT it aliases iter->pos.  But
maybe that's something needed for whatever the subsequent series does?

>  
>  	if (srcmap->type == IOMAP_INLINE)
>  		status = iomap_write_begin_inline(iter, folio);
> @@ -847,6 +853,7 @@ static int iomap_write_begin(struct iomap_iter *iter, size_t len,
>  		goto out_unlock;
>  
>  	*foliop = folio;
> +	*plen = len;
>  	return 0;
>  
>  out_unlock:
> @@ -967,7 +974,7 @@ static int iomap_write_iter(struct iomap_iter *iter, struct iov_iter *i)
>  			break;
>  		}
>  
> -		status = iomap_write_begin(iter, bytes, &folio);
> +		status = iomap_write_begin(iter, &folio, &offset, &bytes);
>  		if (unlikely(status)) {
>  			iomap_write_failed(iter->inode, iter->pos, bytes);
>  			break;
> @@ -975,7 +982,7 @@ static int iomap_write_iter(struct iomap_iter *iter, struct iov_iter *i)
>  		if (iter->iomap.flags & IOMAP_F_STALE)
>  			break;
>  
> -		pos = iomap_trim_folio_range(iter, folio, &offset, &bytes);
> +		pos = iter->pos;

I wonder, do we still need this pos variable?  AFAICT it aliases
iter->pos too.

Aside from that, the series looks decent to me and it's nice to
eliminate some argument passing.

--D

>  
>  		if (mapping_writably_mapped(mapping))
>  			flush_dcache_folio(folio);
> @@ -1295,14 +1302,12 @@ static int iomap_unshare_iter(struct iomap_iter *iter)
>  		bool ret;
>  
>  		bytes = min_t(u64, SIZE_MAX, bytes);
> -		status = iomap_write_begin(iter, bytes, &folio);
> +		status = iomap_write_begin(iter, &folio, &offset, &bytes);
>  		if (unlikely(status))
>  			return status;
>  		if (iomap->flags & IOMAP_F_STALE)
>  			break;
>  
> -		iomap_trim_folio_range(iter, folio, &offset, &bytes);
> -
>  		ret = iomap_write_end(iter, bytes, bytes, folio);
>  		__iomap_put_folio(iter, bytes, folio);
>  		if (WARN_ON_ONCE(!ret))
> @@ -1367,7 +1372,7 @@ static int iomap_zero_iter(struct iomap_iter *iter, bool *did_zero)
>  		bool ret;
>  
>  		bytes = min_t(u64, SIZE_MAX, bytes);
> -		status = iomap_write_begin(iter, bytes, &folio);
> +		status = iomap_write_begin(iter, &folio, &offset, &bytes);
>  		if (status)
>  			return status;
>  		if (iter->iomap.flags & IOMAP_F_STALE)
> @@ -1376,7 +1381,6 @@ static int iomap_zero_iter(struct iomap_iter *iter, bool *did_zero)
>  		/* warn about zeroing folios beyond eof that won't write back */
>  		WARN_ON_ONCE(folio_pos(folio) > iter->inode->i_size);
>  
> -		iomap_trim_folio_range(iter, folio, &offset, &bytes);
>  		folio_zero_range(folio, offset, bytes);
>  		folio_mark_accessed(folio);
>  
> -- 
> 2.49.0
> 
> 

Re: [PATCH 1/6] iomap: resample iter->pos after iomap_write_begin() calls

[Christoph Hellwig]

Looks good:

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

Re: [PATCH 2/6] iomap: drop unnecessary pos param from iomap_write_[begin|end]

[Christoph Hellwig]

Looks good:

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

Re: [PATCH 3/6] iomap: drop pos param from __iomap_[get|put]_folio()

[Christoph Hellwig]

On Wed, Apr 30, 2025 at 03:01:09PM -0400, Brian Foster wrote:
> Both helpers take the iter and pos as parameters. All callers
> effectively pass iter->pos, so drop the unnecessary pos parameter.

Looks good:

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

Re: [PATCH 4/6] iomap: helper to trim pos/bytes to within folio

[Christoph Hellwig]

On Wed, Apr 30, 2025 at 03:01:10PM -0400, Brian Foster wrote:
> +/* trim pos and bytes to within a given folio */
> +static loff_t iomap_trim_folio_range(struct iomap_iter *iter,
> +		struct folio *folio, size_t *offset, u64 *bytes)
> +{
> +	loff_t pos = iter->pos;
> +	size_t fsize = folio_size(folio);
> +
> +	WARN_ON_ONCE(pos < folio_pos(folio) || pos >= folio_pos(folio) + fsize);

Should this be two separate WARN_ON_ONCE calls to see which one
triggered?

> +
> +	*offset = offset_in_folio(folio, pos);
> +	if (*bytes > fsize - *offset)
> +		*bytes = fsize - *offset;

	*bytes = min(*bytes, fsize - *offset);

?

Otherwise looks good:

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

Re: [PATCH 5/6] iomap: push non-large folio check into get folio path

[Christoph Hellwig]

Looks good:

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

Re: [PATCH 6/6] iomap: rework iomap_write_begin() to return folio offset and length

[Christoph Hellwig]

On Wed, Apr 30, 2025 at 03:01:12PM -0400, Brian Foster wrote:
> +	len = *plen > 0 ? min_t(u64, len, *plen) : len;

	len = min_not_zero(len, *plen);

Otherwise looks good:

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

Re: [PATCH 4/6] iomap: helper to trim pos/bytes to within folio

[Brian Foster]

On Mon, May 05, 2025 at 02:03:07AM -0700, Christoph Hellwig wrote:
> On Wed, Apr 30, 2025 at 03:01:10PM -0400, Brian Foster wrote:
> > +/* trim pos and bytes to within a given folio */
> > +static loff_t iomap_trim_folio_range(struct iomap_iter *iter,
> > +		struct folio *folio, size_t *offset, u64 *bytes)
> > +{
> > +	loff_t pos = iter->pos;
> > +	size_t fsize = folio_size(folio);
> > +
> > +	WARN_ON_ONCE(pos < folio_pos(folio) || pos >= folio_pos(folio) + fsize);
> 
> Should this be two separate WARN_ON_ONCE calls to see which one
> triggered?
> 

Sure, can't hurt.

> > +
> > +	*offset = offset_in_folio(folio, pos);
> > +	if (*bytes > fsize - *offset)
> > +		*bytes = fsize - *offset;
> 
> 	*bytes = min(*bytes, fsize - *offset);
> 

Yep.

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

Thanks.

Brian

Re: [PATCH 6/6] iomap: rework iomap_write_begin() to return folio offset and length

[Brian Foster]

On Fri, May 02, 2025 at 01:00:38PM -0700, Darrick J. Wong wrote:
> On Wed, Apr 30, 2025 at 03:01:12PM -0400, Brian Foster wrote:
> > iomap_write_begin() returns a folio based on current pos and
> > remaining length in the iter, and each caller then trims the
> > pos/length to the given folio. Clean this up a bit and let
> > iomap_write_begin() return the trimmed range along with the folio.
> > 
> > Signed-off-by: Brian Foster <bfoster@redhat.com>
> > ---
> >  fs/iomap/buffered-io.c | 26 +++++++++++++++-----------
> >  1 file changed, 15 insertions(+), 11 deletions(-)
> > 
> > diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
> > index d3b30ebad9ea..2fde268c39fc 100644
> > --- a/fs/iomap/buffered-io.c
> > +++ b/fs/iomap/buffered-io.c
> > @@ -793,15 +793,22 @@ static int iomap_write_begin_inline(const struct iomap_iter *iter,
> >  	return iomap_read_inline_data(iter, folio);
> >  }
> >  
> > -static int iomap_write_begin(struct iomap_iter *iter, size_t len,
> > -		struct folio **foliop)
> > +/*
> > + * Grab and prepare a folio for write based on iter state. Returns the folio,
> > + * offset, and length. Callers can optionally pass a max length *plen,
> > + * otherwise init to zero.
> > + */
> > +static int iomap_write_begin(struct iomap_iter *iter, struct folio **foliop,
> > +		size_t *poffset, u64 *plen)
> >  {
> >  	const struct iomap_folio_ops *folio_ops = iter->iomap.folio_ops;
> >  	const struct iomap *srcmap = iomap_iter_srcmap(iter);
> >  	loff_t pos = iter->pos;
> > +	u64 len = min_t(u64, SIZE_MAX, iomap_length(iter));
> >  	struct folio *folio;
> >  	int status = 0;
> >  
> > +	len = *plen > 0 ? min_t(u64, len, *plen) : len;
> >  	BUG_ON(pos + len > iter->iomap.offset + iter->iomap.length);
> >  	if (srcmap != &iter->iomap)
> >  		BUG_ON(pos + len > srcmap->offset + srcmap->length);
> > @@ -833,8 +840,7 @@ static int iomap_write_begin(struct iomap_iter *iter, size_t len,
> >  		}
> >  	}
> >  
> > -	if (pos + len > folio_pos(folio) + folio_size(folio))
> > -		len = folio_pos(folio) + folio_size(folio) - pos;
> > +	pos = iomap_trim_folio_range(iter, folio, poffset, &len);
> 
> Do we still need this pos variable?  AFAICT it aliases iter->pos.  But
> maybe that's something needed for whatever the subsequent series does?
> 

Hmm.. well I have one more patch that moves the BUG_ON() checks at the
top of the function to right after the trim call above. I kept that with
the folio batch patches because it's intended to accommodate that the
next folio might not be linear. That still doesn't technically need the
variable, but I've kept one in places where iter->pos would be read more
than once or twice because I just find the code cleaner that way.

OTOH, it does look like the param to __iomap_write_begin() could go
away, so that drops another use. I'd still probably use a local pos
variable down in that function though. I'll see about tacking one more
cleanup patch on for that either way.

> >  
> >  	if (srcmap->type == IOMAP_INLINE)
> >  		status = iomap_write_begin_inline(iter, folio);
> > @@ -847,6 +853,7 @@ static int iomap_write_begin(struct iomap_iter *iter, size_t len,
> >  		goto out_unlock;
> >  
> >  	*foliop = folio;
> > +	*plen = len;
> >  	return 0;
> >  
> >  out_unlock:
> > @@ -967,7 +974,7 @@ static int iomap_write_iter(struct iomap_iter *iter, struct iov_iter *i)
> >  			break;
> >  		}
> >  
> > -		status = iomap_write_begin(iter, bytes, &folio);
> > +		status = iomap_write_begin(iter, &folio, &offset, &bytes);
> >  		if (unlikely(status)) {
> >  			iomap_write_failed(iter->inode, iter->pos, bytes);
> >  			break;
> > @@ -975,7 +982,7 @@ static int iomap_write_iter(struct iomap_iter *iter, struct iov_iter *i)
> >  		if (iter->iomap.flags & IOMAP_F_STALE)
> >  			break;
> >  
> > -		pos = iomap_trim_folio_range(iter, folio, &offset, &bytes);
> > +		pos = iter->pos;
> 
> I wonder, do we still need this pos variable?  AFAICT it aliases
> iter->pos too.
> 

This one I kept around intentionally for the same reasoning as the the
__iomap_write_begin() case discussed above. If it's accessed more than a
couple times or so, I find it easier to read and help clarify when/if we
might expect pos to remain unchanged.

Brian

> Aside from that, the series looks decent to me and it's nice to
> eliminate some argument passing.
> 
> --D
> 
> >  
> >  		if (mapping_writably_mapped(mapping))
> >  			flush_dcache_folio(folio);
> > @@ -1295,14 +1302,12 @@ static int iomap_unshare_iter(struct iomap_iter *iter)
> >  		bool ret;
> >  
> >  		bytes = min_t(u64, SIZE_MAX, bytes);
> > -		status = iomap_write_begin(iter, bytes, &folio);
> > +		status = iomap_write_begin(iter, &folio, &offset, &bytes);
> >  		if (unlikely(status))
> >  			return status;
> >  		if (iomap->flags & IOMAP_F_STALE)
> >  			break;
> >  
> > -		iomap_trim_folio_range(iter, folio, &offset, &bytes);
> > -
> >  		ret = iomap_write_end(iter, bytes, bytes, folio);
> >  		__iomap_put_folio(iter, bytes, folio);
> >  		if (WARN_ON_ONCE(!ret))
> > @@ -1367,7 +1372,7 @@ static int iomap_zero_iter(struct iomap_iter *iter, bool *did_zero)
> >  		bool ret;
> >  
> >  		bytes = min_t(u64, SIZE_MAX, bytes);
> > -		status = iomap_write_begin(iter, bytes, &folio);
> > +		status = iomap_write_begin(iter, &folio, &offset, &bytes);
> >  		if (status)
> >  			return status;
> >  		if (iter->iomap.flags & IOMAP_F_STALE)
> > @@ -1376,7 +1381,6 @@ static int iomap_zero_iter(struct iomap_iter *iter, bool *did_zero)
> >  		/* warn about zeroing folios beyond eof that won't write back */
> >  		WARN_ON_ONCE(folio_pos(folio) > iter->inode->i_size);
> >  
> > -		iomap_trim_folio_range(iter, folio, &offset, &bytes);
> >  		folio_zero_range(folio, offset, bytes);
> >  		folio_mark_accessed(folio);
> >  
> > -- 
> > 2.49.0
> > 
> > 
> 

Re: [PATCH 6/6] iomap: rework iomap_write_begin() to return folio offset and length

[Brian Foster]

On Mon, May 05, 2025 at 02:05:03AM -0700, Christoph Hellwig wrote:
> On Wed, Apr 30, 2025 at 03:01:12PM -0400, Brian Foster wrote:
> > +	len = *plen > 0 ? min_t(u64, len, *plen) : len;
> 
> 	len = min_not_zero(len, *plen);
> 

Ah, nice. Thanks.

Brian

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