Linux EXT4 FS development
 help / color / mirror / Atom feed
* [PATCH v4 0/5] iomap: trivial fixes for ext4 conversion
@ 2026-07-14  8:23 Zhang Yi
  2026-07-14  8:23 ` [PATCH v4 1/5] iomap: correct the range of a partial dirty clear Zhang Yi
                   ` (6 more replies)
  0 siblings, 7 replies; 16+ messages in thread
From: Zhang Yi @ 2026-07-14  8:23 UTC (permalink / raw)
  To: linux-fsdevel, linux-xfs
  Cc: linux-ext4, brauner, djwong, hch, joannelkoong, yi.zhang,
	yi.zhang, yizhang089, chengzhihao1, yangerkun, yukuai

Hi,

This is v4 of the trivial iomap fixes for the ext4 conversion. There are
no code changes relative to v3, the only update is the addition of a
Fixes tag in patch 03.

Changes since v3:
 - Collect RVB tags from Christoph.
 - Add fix tag in patch patch 03 ("iomap: fix incorrect did_zero setting
   in iomap_zero_iter()").
Changes since v2:
 - Collect RVB tags from Darrick and Joanne.
 - Remove CC stable tag in patch 01 as Joanne suggested.
Changes since v1:
 - Add fix tags to patch 01 and 04.
 - In patch 04, change ifs_set_range_uptodate() to always fall through
   to ifs_is_fully_uptodate(), preventing a false-positive uptodate
   mask.
 - Add patch 05, add comments for ifs_clear/set_range_dirty().

v3: https://lore.kernel.org/linux-fsdevel/20260713072906.1726059-1-yi.zhang@huaweicloud.com/
v2: https://lore.kernel.org/linux-fsdevel/20260520030357.679687-1-yi.zhang@huaweicloud.com/
v1: https://lore.kernel.org/linux-fsdevel/20260514062955.1183976-1-yi.zhang@huaweicloud.com/


Original Cover-letter:

This patch series contains a few trivial iomap-related fixes in
preparation for converting ext4 buffered I/O to use iomap. 

The first three patches are taken from my ext4 conversion series [1], as
suggested by Christoph. The fourth patch fixes a bug originally reported
by Sashiko during review of my series; although unrelated to the ext4
conversion, it is worth fixing on its own. Please see the following
patches for detail. The fifth patch add comments for
ifs_clear/set_range_dirty(), and the last patch avoids merging ioends
that have different private data.

Thanks,
Yi.

[1] https://lore.kernel.org/linux-ext4/20260511072344.191271-1-yi.zhang@huaweicloud.com/

Zhang Yi (5):
  iomap: correct the range of a partial dirty clear
  iomap: support invalidating partial folios
  iomap: fix incorrect did_zero setting in iomap_zero_iter()
  iomap: fix out-of-bounds bitmap_set() with zero-length range
  iomap: add comments for ifs_clear/set_range_dirty()

 fs/iomap/buffered-io.c | 58 ++++++++++++++++++++++++++++++++----------
 1 file changed, 44 insertions(+), 14 deletions(-)

-- 
2.52.0


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

* [PATCH v4 1/5] iomap: correct the range of a partial dirty clear
  2026-07-14  8:23 [PATCH v4 0/5] iomap: trivial fixes for ext4 conversion Zhang Yi
@ 2026-07-14  8:23 ` Zhang Yi
  2026-07-14  8:23 ` [PATCH v4 2/5] iomap: support invalidating partial folios Zhang Yi
                   ` (5 subsequent siblings)
  6 siblings, 0 replies; 16+ messages in thread
From: Zhang Yi @ 2026-07-14  8:23 UTC (permalink / raw)
  To: linux-fsdevel, linux-xfs
  Cc: linux-ext4, brauner, djwong, hch, joannelkoong, yi.zhang,
	yi.zhang, yizhang089, chengzhihao1, yangerkun, yukuai

From: Zhang Yi <yi.zhang@huawei.com>

The block range calculation in ifs_clear_range_dirty() is incorrect when
partially clearing a range in a folio. We cannot clear the dirty bit of
the first block or the last block if the start or end offset is not
blocksize-aligned. This has not yet caused any issues since we always
clear a whole folio in iomap_writeback_folio().

Fix this by rounding up the first block to blocksize alignment, and
calculate the last block by rounding down (using truncation). Correct
the nr_blks calculation accordingly.

Fixes: 4ce02c679722 ("iomap: Add per-block dirty state tracking to improve performance")
Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
Reviewed-by: Joanne Koong <joannelkoong@gmail.com>
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 fs/iomap/buffered-io.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
index d55b936e6986..48878bc4ff62 100644
--- a/fs/iomap/buffered-io.c
+++ b/fs/iomap/buffered-io.c
@@ -176,13 +176,17 @@ static void ifs_clear_range_dirty(struct folio *folio,
 {
 	struct inode *inode = folio->mapping->host;
 	unsigned int blks_per_folio = i_blocks_per_folio(inode, folio);
-	unsigned int first_blk = (off >> inode->i_blkbits);
-	unsigned int last_blk = (off + len - 1) >> inode->i_blkbits;
-	unsigned int nr_blks = last_blk - first_blk + 1;
+	unsigned int first_blk = round_up(off, i_blocksize(inode)) >>
+				 inode->i_blkbits;
+	unsigned int last_blk = (off + len) >> inode->i_blkbits;
 	unsigned long flags;
 
+	if (first_blk >= last_blk)
+		return;
+
 	spin_lock_irqsave(&ifs->state_lock, flags);
-	bitmap_clear(ifs->state, first_blk + blks_per_folio, nr_blks);
+	bitmap_clear(ifs->state, first_blk + blks_per_folio,
+		     last_blk - first_blk);
 	spin_unlock_irqrestore(&ifs->state_lock, flags);
 }
 
-- 
2.52.0


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

* [PATCH v4 2/5] iomap: support invalidating partial folios
  2026-07-14  8:23 [PATCH v4 0/5] iomap: trivial fixes for ext4 conversion Zhang Yi
  2026-07-14  8:23 ` [PATCH v4 1/5] iomap: correct the range of a partial dirty clear Zhang Yi
@ 2026-07-14  8:23 ` Zhang Yi
  2026-08-03  7:13   ` Zhang Yi
  2026-07-14  8:23 ` [PATCH v4 3/5] iomap: fix incorrect did_zero setting in iomap_zero_iter() Zhang Yi
                   ` (4 subsequent siblings)
  6 siblings, 1 reply; 16+ messages in thread
From: Zhang Yi @ 2026-07-14  8:23 UTC (permalink / raw)
  To: linux-fsdevel, linux-xfs
  Cc: linux-ext4, brauner, djwong, hch, joannelkoong, yi.zhang,
	yi.zhang, yizhang089, chengzhihao1, yangerkun, yukuai

From: Zhang Yi <yi.zhang@huawei.com>

Current iomap_invalidate_folio() can only invalidate an entire folio. If
we truncate a partial folio on a filesystem where the block size is
smaller than the folio size, it will leave behind dirty bits for the
truncated or punched blocks. During the write-back process, it will
attempt to map the invalid hole range. Fortunately, this has not caused
any real problems so far because the ->writeback_range() function
corrects the length.

However, the implementation of FALLOC_FL_ZERO_RANGE in ext4 depends on
the support for invalidating partial folios. When ext4 partially zeroes
out a dirty and unwritten folio, it does not perform a flush first like
XFS. Therefore, if the dirty bits of the corresponding area cannot be
cleared, the zeroed area after writeback remains in the written state
rather than reverting to the unwritten state. Fix this by supporting
invalidation of partial folios.

Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>
Reviewed-by: Joanne Koong <joannelkoong@gmail.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 fs/iomap/buffered-io.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
index 48878bc4ff62..aeeed6316cf9 100644
--- a/fs/iomap/buffered-io.c
+++ b/fs/iomap/buffered-io.c
@@ -761,6 +761,8 @@ void iomap_invalidate_folio(struct folio *folio, size_t offset, size_t len)
 		WARN_ON_ONCE(folio_test_writeback(folio));
 		folio_cancel_dirty(folio);
 		ifs_free(folio);
+	} else {
+		iomap_clear_range_dirty(folio, offset, len);
 	}
 }
 EXPORT_SYMBOL_GPL(iomap_invalidate_folio);
-- 
2.52.0


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

* [PATCH v4 3/5] iomap: fix incorrect did_zero setting in iomap_zero_iter()
  2026-07-14  8:23 [PATCH v4 0/5] iomap: trivial fixes for ext4 conversion Zhang Yi
  2026-07-14  8:23 ` [PATCH v4 1/5] iomap: correct the range of a partial dirty clear Zhang Yi
  2026-07-14  8:23 ` [PATCH v4 2/5] iomap: support invalidating partial folios Zhang Yi
@ 2026-07-14  8:23 ` Zhang Yi
  2026-07-14  8:23 ` [PATCH v4 4/5] iomap: fix out-of-bounds bitmap_set() with zero-length range Zhang Yi
                   ` (3 subsequent siblings)
  6 siblings, 0 replies; 16+ messages in thread
From: Zhang Yi @ 2026-07-14  8:23 UTC (permalink / raw)
  To: linux-fsdevel, linux-xfs
  Cc: linux-ext4, brauner, djwong, hch, joannelkoong, yi.zhang,
	yi.zhang, yizhang089, chengzhihao1, yangerkun, yukuai

From: Zhang Yi <yi.zhang@huawei.com>

The did_zero output parameter was unconditionally set after the loop,
which is incorrect. It should only be set when the zeroing operation
actually completes, not when IOMAP_F_STALE is set or when
IOMAP_F_FOLIO_BATCH is set but !folio causes the loop to break early,
or when iomap_iter_advance() returns an error.

This causes did_zero to be incorrectly set when zeroing a clean
unwritten extent because the loop exits early without actually zeroing
any data.

Fix it by using a local variable to track whether any folio was actually
zeroed, and only set did_zero after the loop if zeroing happened.

Fixes: 98eb8d95025b ("iomap: set did_zero to true when zeroing successfully")
Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 fs/iomap/buffered-io.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
index aeeed6316cf9..7f77a1f68160 100644
--- a/fs/iomap/buffered-io.c
+++ b/fs/iomap/buffered-io.c
@@ -1542,6 +1542,7 @@ static int iomap_zero_iter(struct iomap_iter *iter, bool *did_zero,
 		const struct iomap_write_ops *write_ops)
 {
 	u64 bytes = iomap_length(iter);
+	bool zeroed = false;
 	int status;
 
 	do {
@@ -1560,6 +1561,8 @@ static int iomap_zero_iter(struct iomap_iter *iter, bool *did_zero,
 		/* a NULL folio means we're done with a folio batch */
 		if (!folio) {
 			status = iomap_iter_advance_full(iter);
+			if (status)
+				return status;
 			break;
 		}
 
@@ -1570,6 +1573,7 @@ static int iomap_zero_iter(struct iomap_iter *iter, bool *did_zero,
 				bytes);
 
 		folio_zero_range(folio, offset, bytes);
+		zeroed = true;
 		folio_mark_accessed(folio);
 
 		ret = iomap_write_end(iter, bytes, bytes, folio);
@@ -1579,10 +1583,10 @@ static int iomap_zero_iter(struct iomap_iter *iter, bool *did_zero,
 
 		status = iomap_iter_advance(iter, bytes);
 		if (status)
-			break;
+			return status;
 	} while ((bytes = iomap_length(iter)) > 0);
 
-	if (did_zero)
+	if (did_zero && zeroed)
 		*did_zero = true;
 	return status;
 }
-- 
2.52.0


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

* [PATCH v4 4/5] iomap: fix out-of-bounds bitmap_set() with zero-length range
  2026-07-14  8:23 [PATCH v4 0/5] iomap: trivial fixes for ext4 conversion Zhang Yi
                   ` (2 preceding siblings ...)
  2026-07-14  8:23 ` [PATCH v4 3/5] iomap: fix incorrect did_zero setting in iomap_zero_iter() Zhang Yi
@ 2026-07-14  8:23 ` Zhang Yi
  2026-07-14  8:23 ` [PATCH v4 5/5] iomap: add comments for ifs_clear/set_range_dirty() Zhang Yi
                   ` (2 subsequent siblings)
  6 siblings, 0 replies; 16+ messages in thread
From: Zhang Yi @ 2026-07-14  8:23 UTC (permalink / raw)
  To: linux-fsdevel, linux-xfs
  Cc: linux-ext4, brauner, djwong, hch, joannelkoong, yi.zhang,
	yi.zhang, yizhang089, chengzhihao1, yangerkun, yukuai

From: Zhang Yi <yi.zhang@huawei.com>

ifs_set_range_dirty() and ifs_set_range_uptodate() compute last_blk
as (off + len - 1) >> i_blkbits.  When off is 0 and len is 0, the
unsigned subtraction underflows to SIZE_MAX, producing a huge
last_blk and nr_blks value that causes bitmap_set() to write far
beyond the ifs->state allocation.

Regarding ifs_set_range_uptodate(), it is temporarily safe because len
cannot be passed in as 0. However, for ifs_set_range_dirty() this is
reachable from __iomap_write_end(): when copy_folio_from_iter_atomic()
returns 0 (e.g. user buffer fault) and the folio is already uptodate,
the guard at the top of __iomap_write_end() does not trigger because
!folio_test_uptodate() is false, and iomap_set_range_dirty() is called
with copied == 0.

Add a !len guard to both functions before the computation, so that a
zero-length range is a no-op.

Fixes: 4ce02c679722 ("iomap: Add per-block dirty state tracking to improve performance")
Cc: <stable@vger.kernel.org> # v6.6
Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
Reviewed-by: Joanne Koong <joannelkoong@gmail.com>
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 fs/iomap/buffered-io.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
index 7f77a1f68160..a356efbae6b0 100644
--- a/fs/iomap/buffered-io.c
+++ b/fs/iomap/buffered-io.c
@@ -67,11 +67,13 @@ static bool ifs_set_range_uptodate(struct folio *folio,
 		struct iomap_folio_state *ifs, size_t off, size_t len)
 {
 	struct inode *inode = folio->mapping->host;
-	unsigned int first_blk = off >> inode->i_blkbits;
-	unsigned int last_blk = (off + len - 1) >> inode->i_blkbits;
-	unsigned int nr_blks = last_blk - first_blk + 1;
+	unsigned int first_blk, last_blk;
 
-	bitmap_set(ifs->state, first_blk, nr_blks);
+	if (len) {
+		first_blk = off >> inode->i_blkbits;
+		last_blk = (off + len - 1) >> inode->i_blkbits;
+		bitmap_set(ifs->state, first_blk, last_blk - first_blk + 1);
+	}
 	return ifs_is_fully_uptodate(folio, ifs);
 }
 
@@ -203,13 +205,17 @@ static void ifs_set_range_dirty(struct folio *folio,
 {
 	struct inode *inode = folio->mapping->host;
 	unsigned int blks_per_folio = i_blocks_per_folio(inode, folio);
-	unsigned int first_blk = (off >> inode->i_blkbits);
-	unsigned int last_blk = (off + len - 1) >> inode->i_blkbits;
-	unsigned int nr_blks = last_blk - first_blk + 1;
+	unsigned int first_blk, last_blk;
 	unsigned long flags;
 
+	if (!len)
+		return;
+
+	first_blk = off >> inode->i_blkbits;
+	last_blk = (off + len - 1) >> inode->i_blkbits;
 	spin_lock_irqsave(&ifs->state_lock, flags);
-	bitmap_set(ifs->state, first_blk + blks_per_folio, nr_blks);
+	bitmap_set(ifs->state, first_blk + blks_per_folio,
+		   last_blk - first_blk + 1);
 	spin_unlock_irqrestore(&ifs->state_lock, flags);
 }
 
-- 
2.52.0


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

* [PATCH v4 5/5] iomap: add comments for ifs_clear/set_range_dirty()
  2026-07-14  8:23 [PATCH v4 0/5] iomap: trivial fixes for ext4 conversion Zhang Yi
                   ` (3 preceding siblings ...)
  2026-07-14  8:23 ` [PATCH v4 4/5] iomap: fix out-of-bounds bitmap_set() with zero-length range Zhang Yi
@ 2026-07-14  8:23 ` Zhang Yi
  2026-07-22 15:34 ` [PATCH v4 0/5] iomap: trivial fixes for ext4 conversion Theodore Tso
  2026-07-23  9:26 ` Christian Brauner
  6 siblings, 0 replies; 16+ messages in thread
From: Zhang Yi @ 2026-07-14  8:23 UTC (permalink / raw)
  To: linux-fsdevel, linux-xfs
  Cc: linux-ext4, brauner, djwong, hch, joannelkoong, yi.zhang,
	yi.zhang, yizhang089, chengzhihao1, yangerkun, yukuai

From: Zhang Yi <yi.zhang@huawei.com>

The range alignment strategy differs between ifs_clear_range_dirty() and
ifs_set_range_dirty(). The former rounds inwards to clear only
fully-covered blocks, while the latter rounds outwards to mark any
partially-touched block as dirty. Add comments to document this
asymmetry in block range calculation.

Suggested-by: "Darrick J. Wong" <djwong@kernel.org>
Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
Reviewed-by: Joanne Koong <joannelkoong@gmail.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 fs/iomap/buffered-io.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
index a356efbae6b0..c12237ed7cde 100644
--- a/fs/iomap/buffered-io.c
+++ b/fs/iomap/buffered-io.c
@@ -173,6 +173,13 @@ static unsigned iomap_find_dirty_range(struct folio *folio, u64 *range_start,
 	return range_end - *range_start;
 }
 
+/*
+ * Clear the per-block dirty bits for the range [@off, @off + @len) within a
+ * folio.  The range is rounded inwards so that only blocks fully covered by
+ * the range are cleared.  This is required for operations like folio
+ * invalidation, where we must ensure a block is fully clean before discarding
+ * it.
+ */
 static void ifs_clear_range_dirty(struct folio *folio,
 		struct iomap_folio_state *ifs, size_t off, size_t len)
 {
@@ -200,6 +207,13 @@ static void iomap_clear_range_dirty(struct folio *folio, size_t off, size_t len)
 		ifs_clear_range_dirty(folio, ifs, off, len);
 }
 
+/*
+ * Set the per-block dirty bits for the range [@off, @off + @len) within a
+ * folio.  The range is rounded outwards so that any block partially touched
+ * by the range is marked dirty.  This ensures blocks containing even a
+ * single dirty byte will be included in subsequent writeback, preventing
+ * data loss when partial blocks are written.
+ */
 static void ifs_set_range_dirty(struct folio *folio,
 		struct iomap_folio_state *ifs, size_t off, size_t len)
 {
-- 
2.52.0


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

* Re: [PATCH v4 0/5] iomap: trivial fixes for ext4 conversion
  2026-07-14  8:23 [PATCH v4 0/5] iomap: trivial fixes for ext4 conversion Zhang Yi
                   ` (4 preceding siblings ...)
  2026-07-14  8:23 ` [PATCH v4 5/5] iomap: add comments for ifs_clear/set_range_dirty() Zhang Yi
@ 2026-07-22 15:34 ` Theodore Tso
  2026-07-23  1:25   ` Zhang Yi
  2026-07-23  9:27   ` Christian Brauner
  2026-07-23  9:26 ` Christian Brauner
  6 siblings, 2 replies; 16+ messages in thread
From: Theodore Tso @ 2026-07-22 15:34 UTC (permalink / raw)
  To: brauner
  Cc: linux-fsdevel, linux-xfs, linux-ext4, djwong, hch, joannelkoong,
	yi.zhang, yizhang089, chengzhihao1, yangerkun, yukuai, Zhang Yi

Hi Christian,

These fixes are needed for landing support for buffered writes using
iomap in ext4.  That won't be ready to land this development cycle,
but could we land these fixes now?

Thanks,

						- Ted

On Tue, Jul 14, 2026 at 04:23:20PM -0500, Zhang Yi wrote:
> Hi,
> 
> This is v4 of the trivial iomap fixes for the ext4 conversion. There are
> no code changes relative to v3, the only update is the addition of a
> Fixes tag in patch 03.
> 
> Changes since v3:
>  - Collect RVB tags from Christoph.
>  - Add fix tag in patch patch 03 ("iomap: fix incorrect did_zero setting
>    in iomap_zero_iter()").
> Changes since v2:
>  - Collect RVB tags from Darrick and Joanne.
>  - Remove CC stable tag in patch 01 as Joanne suggested.
> Changes since v1:
>  - Add fix tags to patch 01 and 04.
>  - In patch 04, change ifs_set_range_uptodate() to always fall through
>    to ifs_is_fully_uptodate(), preventing a false-positive uptodate
>    mask.
>  - Add patch 05, add comments for ifs_clear/set_range_dirty().
> 
> v3: https://lore.kernel.org/linux-fsdevel/20260713072906.1726059-1-yi.zhang@huaweicloud.com/
> v2: https://lore.kernel.org/linux-fsdevel/20260520030357.679687-1-yi.zhang@huaweicloud.com/
> v1: https://lore.kernel.org/linux-fsdevel/20260514062955.1183976-1-yi.zhang@huaweicloud.com/
> 
> 
> Original Cover-letter:
> 
> This patch series contains a few trivial iomap-related fixes in
> preparation for converting ext4 buffered I/O to use iomap. 
> 
> The first three patches are taken from my ext4 conversion series [1], as
> suggested by Christoph. The fourth patch fixes a bug originally reported
> by Sashiko during review of my series; although unrelated to the ext4
> conversion, it is worth fixing on its own. Please see the following
> patches for detail. The fifth patch add comments for
> ifs_clear/set_range_dirty(), and the last patch avoids merging ioends
> that have different private data.
> 
> Thanks,
> Yi.
> 
> [1] https://lore.kernel.org/linux-ext4/20260511072344.191271-1-yi.zhang@huaweicloud.com/
> 
> Zhang Yi (5):
>   iomap: correct the range of a partial dirty clear
>   iomap: support invalidating partial folios
>   iomap: fix incorrect did_zero setting in iomap_zero_iter()
>   iomap: fix out-of-bounds bitmap_set() with zero-length range
>   iomap: add comments for ifs_clear/set_range_dirty()
> 
>  fs/iomap/buffered-io.c | 58 ++++++++++++++++++++++++++++++++----------
>  1 file changed, 44 insertions(+), 14 deletions(-)
> 
> -- 
> 2.52.0
> 
> 

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

* Re: [PATCH v4 0/5] iomap: trivial fixes for ext4 conversion
  2026-07-22 15:34 ` [PATCH v4 0/5] iomap: trivial fixes for ext4 conversion Theodore Tso
@ 2026-07-23  1:25   ` Zhang Yi
  2026-07-23  9:27   ` Christian Brauner
  1 sibling, 0 replies; 16+ messages in thread
From: Zhang Yi @ 2026-07-23  1:25 UTC (permalink / raw)
  To: brauner
  Cc: Theodore Tso, linux-fsdevel, linux-xfs, linux-ext4, djwong, hch,
	joannelkoong, yi.zhang, yizhang089, chengzhihao1, yangerkun,
	yukuai

On 7/22/2026 11:34 PM, Theodore Tso wrote:
> Hi Christian,
> 
> These fixes are needed for landing support for buffered writes using
> iomap in ext4.  That won't be ready to land this development cycle,
> but could we land these fixes now?
> 
> Thanks,
> 
> 						- Ted

Here's another one. Could you please take this one along too?
Thank you.

https://lore.kernel.org/linux-fsdevel/20260713074206.1768006-1-yi.zhang@huaweicloud.com/

Best Regards
Yi.

> 
> On Tue, Jul 14, 2026 at 04:23:20PM -0500, Zhang Yi wrote:
>> Hi,
>>
>> This is v4 of the trivial iomap fixes for the ext4 conversion. There are
>> no code changes relative to v3, the only update is the addition of a
>> Fixes tag in patch 03.
>>
>> Changes since v3:
>>  - Collect RVB tags from Christoph.
>>  - Add fix tag in patch patch 03 ("iomap: fix incorrect did_zero setting
>>    in iomap_zero_iter()").
>> Changes since v2:
>>  - Collect RVB tags from Darrick and Joanne.
>>  - Remove CC stable tag in patch 01 as Joanne suggested.
>> Changes since v1:
>>  - Add fix tags to patch 01 and 04.
>>  - In patch 04, change ifs_set_range_uptodate() to always fall through
>>    to ifs_is_fully_uptodate(), preventing a false-positive uptodate
>>    mask.
>>  - Add patch 05, add comments for ifs_clear/set_range_dirty().
>>
>> v3: https://lore.kernel.org/linux-fsdevel/20260713072906.1726059-1-yi.zhang@huaweicloud.com/
>> v2: https://lore.kernel.org/linux-fsdevel/20260520030357.679687-1-yi.zhang@huaweicloud.com/
>> v1: https://lore.kernel.org/linux-fsdevel/20260514062955.1183976-1-yi.zhang@huaweicloud.com/
>>
>>
>> Original Cover-letter:
>>
>> This patch series contains a few trivial iomap-related fixes in
>> preparation for converting ext4 buffered I/O to use iomap. 
>>
>> The first three patches are taken from my ext4 conversion series [1], as
>> suggested by Christoph. The fourth patch fixes a bug originally reported
>> by Sashiko during review of my series; although unrelated to the ext4
>> conversion, it is worth fixing on its own. Please see the following
>> patches for detail. The fifth patch add comments for
>> ifs_clear/set_range_dirty(), and the last patch avoids merging ioends
>> that have different private data.
>>
>> Thanks,
>> Yi.
>>
>> [1] https://lore.kernel.org/linux-ext4/20260511072344.191271-1-yi.zhang@huaweicloud.com/
>>
>> Zhang Yi (5):
>>   iomap: correct the range of a partial dirty clear
>>   iomap: support invalidating partial folios
>>   iomap: fix incorrect did_zero setting in iomap_zero_iter()
>>   iomap: fix out-of-bounds bitmap_set() with zero-length range
>>   iomap: add comments for ifs_clear/set_range_dirty()
>>
>>  fs/iomap/buffered-io.c | 58 ++++++++++++++++++++++++++++++++----------
>>  1 file changed, 44 insertions(+), 14 deletions(-)
>>
>> -- 
>> 2.52.0
>>
>>


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

* Re: [PATCH v4 0/5] iomap: trivial fixes for ext4 conversion
  2026-07-14  8:23 [PATCH v4 0/5] iomap: trivial fixes for ext4 conversion Zhang Yi
                   ` (5 preceding siblings ...)
  2026-07-22 15:34 ` [PATCH v4 0/5] iomap: trivial fixes for ext4 conversion Theodore Tso
@ 2026-07-23  9:26 ` Christian Brauner
  6 siblings, 0 replies; 16+ messages in thread
From: Christian Brauner @ 2026-07-23  9:26 UTC (permalink / raw)
  To: linux-fsdevel, linux-xfs, Zhang Yi
  Cc: linux-ext4, djwong, hch, joannelkoong, yi.zhang, yizhang089,
	chengzhihao1, yangerkun, yukuai

On Tue, 14 Jul 2026 16:23:20 +0800, Zhang Yi wrote:
> iomap: trivial fixes for ext4 conversion
> 
> Hi,
> 
> This is v4 of the trivial iomap fixes for the ext4 conversion. There are
> no code changes relative to v3, the only update is the addition of a
> Fixes tag in patch 03.
> 
> [...]

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

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

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

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

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs.fixes

[1/5] iomap: correct the range of a partial dirty clear
      https://git.kernel.org/vfs/vfs/c/a8d61f707f72
[2/5] iomap: support invalidating partial folios
      https://git.kernel.org/vfs/vfs/c/60eb51b9fbe4
[3/5] iomap: fix incorrect did_zero setting in iomap_zero_iter()
      https://git.kernel.org/vfs/vfs/c/b85d5446ca3f
[4/5] iomap: fix out-of-bounds bitmap_set() with zero-length range
      https://git.kernel.org/vfs/vfs/c/62a9223d3b01
[5/5] iomap: add comments for ifs_clear/set_range_dirty()
      https://git.kernel.org/vfs/vfs/c/2da2f909f8e6


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

* Re: [PATCH v4 0/5] iomap: trivial fixes for ext4 conversion
  2026-07-22 15:34 ` [PATCH v4 0/5] iomap: trivial fixes for ext4 conversion Theodore Tso
  2026-07-23  1:25   ` Zhang Yi
@ 2026-07-23  9:27   ` Christian Brauner
  1 sibling, 0 replies; 16+ messages in thread
From: Christian Brauner @ 2026-07-23  9:27 UTC (permalink / raw)
  To: Theodore Tso
  Cc: brauner, linux-fsdevel, linux-xfs, linux-ext4, djwong, hch,
	joannelkoong, yi.zhang, yizhang089, chengzhihao1, yangerkun,
	yukuai, Zhang Yi

On 2026-07-22 11:34 -0400, Theodore Tso wrote:
> Hi Christian,
> 
> These fixes are needed for landing support for buffered writes using
> iomap in ext4.  That won't be ready to land this development cycle,
> but could we land these fixes now?

Yes, absolutely. I had it on my todo but just came back.


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

* Re: [PATCH v4 2/5] iomap: support invalidating partial folios
  2026-07-14  8:23 ` [PATCH v4 2/5] iomap: support invalidating partial folios Zhang Yi
@ 2026-08-03  7:13   ` Zhang Yi
  2026-08-04 14:59     ` Brian Foster
  0 siblings, 1 reply; 16+ messages in thread
From: Zhang Yi @ 2026-08-03  7:13 UTC (permalink / raw)
  To: linux-fsdevel, linux-xfs, linux-ext4
  Cc: brauner, djwong, hch, joannelkoong, Theodore Y. Ts'o,
	Jan Kara, Ojaswin Mujoo, yi.zhang, yizhang089, chengzhihao1,
	yangerkun, yukuai

On 7/14/2026 4:23 PM, Zhang Yi wrote:
> From: Zhang Yi <yi.zhang@huawei.com>
> 
> Current iomap_invalidate_folio() can only invalidate an entire folio. If
> we truncate a partial folio on a filesystem where the block size is
> smaller than the folio size, it will leave behind dirty bits for the
> truncated or punched blocks. During the write-back process, it will
> attempt to map the invalid hole range. Fortunately, this has not caused
> any real problems so far because the ->writeback_range() function
> corrects the length.
> 
> However, the implementation of FALLOC_FL_ZERO_RANGE in ext4 depends on
> the support for invalidating partial folios. When ext4 partially zeroes
> out a dirty and unwritten folio, it does not perform a flush first like
> XFS. Therefore, if the dirty bits of the corresponding area cannot be
> cleared, the zeroed area after writeback remains in the written state
> rather than reverting to the unwritten state. Fix this by supporting
> invalidation of partial folios.

Hi all,

While working on the v5 of the ext4 iomap conversion series[1], I've
observed a rare data inconsistency issue in xfstests generic/127. After
debugging, I found that the root cause lies in the fact that the current
patch does not cover all scenarios when handling partial folio
invalidation during punch hole operations in cases where
block size < folio size. Specifically, the sub-folio dirty state is not
properly cleaned up in all cases. I think we need to discuss the fix,
and I'd like to hear your suggestions.

Root cause:

In both iomap buffered write and mmap write paths, if the write range
covers an entire folio (regardless of whether the folio size is larger
than block size), an ifs (iomap_folio_state) is not allocated
immediately. Instead, it is deferred until writeback time, where it gets
created in iomap_writeback_folio(). This creates a problem: when ext4
performs a punch or zero_range operation on a partial range within such
a dirty folio, there is no way to clear the dirty state for the
corresponding blocks.

This leads to two specific issues:

1. After issuing FALLOC_FL_ZERO_RANGE on a range covering
   dirty+unwritten blocks within a large folio, the dirty state cannot
   be cleared. During subsequent writeback, zeroed data is still
   written back, and the final extent state for those blocks becomes
   written. As a result, the fix from this patch becomes ineffective in
   this case.

2. The aforementioned rare data inconsistency in xfstests generic/127.
   When performing a partial folio punch hole on a dirty large folio,
   truncate_inode_pages_range() zeros the partial folio and then splits
   the folio. This causes an incorrect 'end' offset calculation in
   truncate_inode_pages_range(), which then results in all split folios
   being truncated via truncate_inode_folio(), turning partial valid
   data into zeroes.  For example:

   Suppose we have a large folio of 4 pages, and we punch a range
   starting from the beginning to the middle of the last page.
   truncate_inode_pages_range() will go through two rounds of splitting.
   Normally, if an ifs is present, the split path would hit
   folio_split() -> filemap_release_folio() -> iomap_release_folio(),
   which would intercept the operation and refuse splitting because the
   folio is dirty.

   However, without an ifs, filemap_release_folio() returns early via
   folio_needs_release(), causing the interception to fail. In the first
   round, the folio is split into 3 folios (1, 1, 2). In the second
   round, we expect to split into 4 folios (1, 1, 1, 1). If the second
   split succeeds, everything is fine, because
   truncate_inode_pages_range() calculates end = 3, and only the first 3
   folios are truncated. However, the second split is allowed to fail.
   If it does fail, truncate_inode_pages_range() still uses end = 3 and
   ends up truncating all 3 folios, resulting in data loss.

Now I remember I previously submitted two patches [2] that always
allocated an ifs in the iomap buffered write path and the mmap fault
path for the block size < folio size case. However, Christoph pointed
out that the iomap design intentionally defers ifs allocation to avoid
unnecessary overhead and improve performance, and suggested creating the
ifs in iomap_invalidate_folio() instead [3]. There is a hurdle, though:
iomap_invalidate_folio() is only called when the folio has private data
(in truncate_inode_partial_folio(), folio_needs_release() is called
first to check the private flag). So if we go with this approach, we
need to ensure that folio_invalidate() can be called even when the folio
does not have an ifs.

One relatively simple approach is to always set the AS_RELEASE_ALWAYS
flag on ext4 inodes that go through the iomap path. I don't think this
would introduce significant overhead, but it doesn't feel very generic.

Another approach is to modify truncate_inode_partial_folio(). Regarding
this, I don't want to introduce other magics to achieve this, so perhaps
modify truncate_inode_partial_folio() to check whether
i_blocks_per_folio > 1 before calling folio_invalidate(), and call it if
so.

So, what do people think? Any better idea?

Thanks,
Yi.

[1] https://github.com/zhangyi089/linux/commits/ext4_buffered_iomap_v5-devel13/
[2] https://lore.kernel.org/linux-fsdevel/20240812121159.3775074-4-yi.zhang@huaweicloud.com/
    https://lore.kernel.org/linux-fsdevel/20240812121159.3775074-5-yi.zhang@huaweicloud.com/
[3] https://lore.kernel.org/linux-fsdevel/ZrxBfKi_DpThYo94@infradead.org/


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

* Re: [PATCH v4 2/5] iomap: support invalidating partial folios
  2026-08-03  7:13   ` Zhang Yi
@ 2026-08-04 14:59     ` Brian Foster
  2026-08-04 18:41       ` Darrick J. Wong
  2026-08-05  2:42       ` Zhang Yi
  0 siblings, 2 replies; 16+ messages in thread
From: Brian Foster @ 2026-08-04 14:59 UTC (permalink / raw)
  To: Zhang Yi
  Cc: linux-fsdevel, linux-xfs, linux-ext4, brauner, djwong, hch,
	joannelkoong, Theodore Y. Ts'o, Jan Kara, Ojaswin Mujoo,
	yi.zhang, yizhang089, chengzhihao1, yangerkun, yukuai

On Mon, Aug 03, 2026 at 03:13:28PM +0800, Zhang Yi wrote:
> On 7/14/2026 4:23 PM, Zhang Yi wrote:
> > From: Zhang Yi <yi.zhang@huawei.com>
> > 
> > Current iomap_invalidate_folio() can only invalidate an entire folio. If
> > we truncate a partial folio on a filesystem where the block size is
> > smaller than the folio size, it will leave behind dirty bits for the
> > truncated or punched blocks. During the write-back process, it will
> > attempt to map the invalid hole range. Fortunately, this has not caused
> > any real problems so far because the ->writeback_range() function
> > corrects the length.
> > 
> > However, the implementation of FALLOC_FL_ZERO_RANGE in ext4 depends on
> > the support for invalidating partial folios. When ext4 partially zeroes
> > out a dirty and unwritten folio, it does not perform a flush first like
> > XFS. Therefore, if the dirty bits of the corresponding area cannot be
> > cleared, the zeroed area after writeback remains in the written state
> > rather than reverting to the unwritten state. Fix this by supporting
> > invalidation of partial folios.
> 
> Hi all,
> 
> While working on the v5 of the ext4 iomap conversion series[1], I've
> observed a rare data inconsistency issue in xfstests generic/127. After
> debugging, I found that the root cause lies in the fact that the current
> patch does not cover all scenarios when handling partial folio
> invalidation during punch hole operations in cases where
> block size < folio size. Specifically, the sub-folio dirty state is not
> properly cleaned up in all cases. I think we need to discuss the fix,
> and I'd like to hear your suggestions.
> 
> Root cause:
> 
> In both iomap buffered write and mmap write paths, if the write range
> covers an entire folio (regardless of whether the folio size is larger
> than block size), an ifs (iomap_folio_state) is not allocated
> immediately. Instead, it is deferred until writeback time, where it gets
> created in iomap_writeback_folio(). This creates a problem: when ext4
> performs a punch or zero_range operation on a partial range within such
> a dirty folio, there is no way to clear the dirty state for the
> corresponding blocks.
> 
> This leads to two specific issues:
> 
> 1. After issuing FALLOC_FL_ZERO_RANGE on a range covering
>    dirty+unwritten blocks within a large folio, the dirty state cannot
>    be cleared. During subsequent writeback, zeroed data is still
>    written back, and the final extent state for those blocks becomes
>    written. As a result, the fix from this patch becomes ineffective in
>    this case.
> 

I suspect this may be (occasionally) the case even with an ifs. IIRC the
folio batch stuff made a tradeoff for just zeroing any folio passed in
from the fs that was determined to be dirty, regardless of whether
underlying block aligned ranges may be unwritten and still clean.

I think this is relatively harmless so long as the underlying range
returns zeroes from subsequent reads. The caveat may be if there are any
cases where we have a really large folio and some small portion of it is
dirtied and that causes some huge amount of unnecessary zeroing. I'm not
sure if we've hit something like that in practice though..

> 2. The aforementioned rare data inconsistency in xfstests generic/127.
>    When performing a partial folio punch hole on a dirty large folio,
>    truncate_inode_pages_range() zeros the partial folio and then splits
>    the folio. This causes an incorrect 'end' offset calculation in
>    truncate_inode_pages_range(), which then results in all split folios
>    being truncated via truncate_inode_folio(), turning partial valid
>    data into zeroes.  For example:
> 
>    Suppose we have a large folio of 4 pages, and we punch a range
>    starting from the beginning to the middle of the last page.
>    truncate_inode_pages_range() will go through two rounds of splitting.
>    Normally, if an ifs is present, the split path would hit
>    folio_split() -> filemap_release_folio() -> iomap_release_folio(),
>    which would intercept the operation and refuse splitting because the
>    folio is dirty.
> 
>    However, without an ifs, filemap_release_folio() returns early via
>    folio_needs_release(), causing the interception to fail. In the first
>    round, the folio is split into 3 folios (1, 1, 2). In the second
>    round, we expect to split into 4 folios (1, 1, 1, 1). If the second
>    split succeeds, everything is fine, because
>    truncate_inode_pages_range() calculates end = 3, and only the first 3
>    folios are truncated. However, the second split is allowed to fail.
>    If it does fail, truncate_inode_pages_range() still uses end = 3 and
>    ends up truncating all 3 folios, resulting in data loss.
> 

I was never really a huge fan of the ifs optimization thing, but I don't
recall what the performance benefits really were.

That aside, doesn't this seem like more of a bug within the
truncate_inode_pages_range() path? It's not clear to me if that's the
only problem wrt iomap, but I'd think if the end offset split down in
truncate_inode_partial_folio() fails, we should be able to return
partial progress or something instead of a true/false for the whole
thing. Would that allow this path to properly update the end index for
the full truncate loop and at least prevent throwing away a folio with
partial data like this?

Brian

> Now I remember I previously submitted two patches [2] that always
> allocated an ifs in the iomap buffered write path and the mmap fault
> path for the block size < folio size case. However, Christoph pointed
> out that the iomap design intentionally defers ifs allocation to avoid
> unnecessary overhead and improve performance, and suggested creating the
> ifs in iomap_invalidate_folio() instead [3]. There is a hurdle, though:
> iomap_invalidate_folio() is only called when the folio has private data
> (in truncate_inode_partial_folio(), folio_needs_release() is called
> first to check the private flag). So if we go with this approach, we
> need to ensure that folio_invalidate() can be called even when the folio
> does not have an ifs.
> 
> One relatively simple approach is to always set the AS_RELEASE_ALWAYS
> flag on ext4 inodes that go through the iomap path. I don't think this
> would introduce significant overhead, but it doesn't feel very generic.
> 
> Another approach is to modify truncate_inode_partial_folio(). Regarding
> this, I don't want to introduce other magics to achieve this, so perhaps
> modify truncate_inode_partial_folio() to check whether
> i_blocks_per_folio > 1 before calling folio_invalidate(), and call it if
> so.
> 
> So, what do people think? Any better idea?
> 
> Thanks,
> Yi.
> 
> [1] https://github.com/zhangyi089/linux/commits/ext4_buffered_iomap_v5-devel13/
> [2] https://lore.kernel.org/linux-fsdevel/20240812121159.3775074-4-yi.zhang@huaweicloud.com/
>     https://lore.kernel.org/linux-fsdevel/20240812121159.3775074-5-yi.zhang@huaweicloud.com/
> [3] https://lore.kernel.org/linux-fsdevel/ZrxBfKi_DpThYo94@infradead.org/
> 
> 


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

* Re: [PATCH v4 2/5] iomap: support invalidating partial folios
  2026-08-04 14:59     ` Brian Foster
@ 2026-08-04 18:41       ` Darrick J. Wong
  2026-08-06 12:18         ` Brian Foster
  2026-08-05  2:42       ` Zhang Yi
  1 sibling, 1 reply; 16+ messages in thread
From: Darrick J. Wong @ 2026-08-04 18:41 UTC (permalink / raw)
  To: Brian Foster
  Cc: Zhang Yi, linux-fsdevel, linux-xfs, linux-ext4, brauner, hch,
	joannelkoong, Theodore Y. Ts'o, Jan Kara, Ojaswin Mujoo,
	yi.zhang, yizhang089, chengzhihao1, yangerkun, yukuai

On Tue, Aug 04, 2026 at 10:59:36AM -0400, Brian Foster wrote:
> On Mon, Aug 03, 2026 at 03:13:28PM +0800, Zhang Yi wrote:
> > On 7/14/2026 4:23 PM, Zhang Yi wrote:
> > > From: Zhang Yi <yi.zhang@huawei.com>
> > > 
> > > Current iomap_invalidate_folio() can only invalidate an entire folio. If
> > > we truncate a partial folio on a filesystem where the block size is
> > > smaller than the folio size, it will leave behind dirty bits for the
> > > truncated or punched blocks. During the write-back process, it will
> > > attempt to map the invalid hole range. Fortunately, this has not caused
> > > any real problems so far because the ->writeback_range() function
> > > corrects the length.
> > > 
> > > However, the implementation of FALLOC_FL_ZERO_RANGE in ext4 depends on
> > > the support for invalidating partial folios. When ext4 partially zeroes
> > > out a dirty and unwritten folio, it does not perform a flush first like
> > > XFS. Therefore, if the dirty bits of the corresponding area cannot be
> > > cleared, the zeroed area after writeback remains in the written state
> > > rather than reverting to the unwritten state. Fix this by supporting
> > > invalidation of partial folios.
> > 
> > Hi all,
> > 
> > While working on the v5 of the ext4 iomap conversion series[1], I've
> > observed a rare data inconsistency issue in xfstests generic/127. After
> > debugging, I found that the root cause lies in the fact that the current
> > patch does not cover all scenarios when handling partial folio
> > invalidation during punch hole operations in cases where
> > block size < folio size. Specifically, the sub-folio dirty state is not
> > properly cleaned up in all cases. I think we need to discuss the fix,
> > and I'd like to hear your suggestions.
> > 
> > Root cause:
> > 
> > In both iomap buffered write and mmap write paths, if the write range
> > covers an entire folio (regardless of whether the folio size is larger
> > than block size), an ifs (iomap_folio_state) is not allocated
> > immediately. Instead, it is deferred until writeback time, where it gets
> > created in iomap_writeback_folio(). This creates a problem: when ext4
> > performs a punch or zero_range operation on a partial range within such
> > a dirty folio, there is no way to clear the dirty state for the
> > corresponding blocks.
> > 
> > This leads to two specific issues:
> > 
> > 1. After issuing FALLOC_FL_ZERO_RANGE on a range covering
> >    dirty+unwritten blocks within a large folio, the dirty state cannot
> >    be cleared. During subsequent writeback, zeroed data is still
> >    written back, and the final extent state for those blocks becomes
> >    written. As a result, the fix from this patch becomes ineffective in
> >    this case.
> > 
> 
> I suspect this may be (occasionally) the case even with an ifs. IIRC the
> folio batch stuff made a tradeoff for just zeroing any folio passed in
> from the fs that was determined to be dirty, regardless of whether
> underlying block aligned ranges may be unwritten and still clean.
> 
> I think this is relatively harmless so long as the underlying range
> returns zeroes from subsequent reads. The caveat may be if there are any
> cases where we have a really large folio and some small portion of it is
> dirtied and that causes some huge amount of unnecessary zeroing. I'm not
> sure if we've hit something like that in practice though..
> 
> > 2. The aforementioned rare data inconsistency in xfstests generic/127.
> >    When performing a partial folio punch hole on a dirty large folio,
> >    truncate_inode_pages_range() zeros the partial folio and then splits
> >    the folio. This causes an incorrect 'end' offset calculation in
> >    truncate_inode_pages_range(), which then results in all split folios
> >    being truncated via truncate_inode_folio(), turning partial valid
> >    data into zeroes.  For example:
> > 
> >    Suppose we have a large folio of 4 pages, and we punch a range
> >    starting from the beginning to the middle of the last page.
> >    truncate_inode_pages_range() will go through two rounds of splitting.
> >    Normally, if an ifs is present, the split path would hit
> >    folio_split() -> filemap_release_folio() -> iomap_release_folio(),
> >    which would intercept the operation and refuse splitting because the
> >    folio is dirty.
> > 
> >    However, without an ifs, filemap_release_folio() returns early via
> >    folio_needs_release(), causing the interception to fail. In the first
> >    round, the folio is split into 3 folios (1, 1, 2). In the second
> >    round, we expect to split into 4 folios (1, 1, 1, 1). If the second
> >    split succeeds, everything is fine, because
> >    truncate_inode_pages_range() calculates end = 3, and only the first 3
> >    folios are truncated. However, the second split is allowed to fail.
> >    If it does fail, truncate_inode_pages_range() still uses end = 3 and
> >    ends up truncating all 3 folios, resulting in data loss.
> > 
> 
> I was never really a huge fan of the ifs optimization thing, but I don't
> recall what the performance benefits really were.

IIRC at the time it was an optimization to avoid allocating an ifs for
the "uncommon" case of fsblock size < page size.  Then willy made it the
common case with large folios, so there's probably little point in
maintaining all this on/off complexity because large folios with no ifs
resulted in huge write amplification.

The only problem with always having an ifs is that splitting a large
folio with an ifs into smaller ones is (I think) currently not supported
because iomap doesn't know how to split the ifs and might not be able to
allocate the second one if memory is scarce.

--D

> That aside, doesn't this seem like more of a bug within the
> truncate_inode_pages_range() path? It's not clear to me if that's the
> only problem wrt iomap, but I'd think if the end offset split down in
> truncate_inode_partial_folio() fails, we should be able to return
> partial progress or something instead of a true/false for the whole
> thing. Would that allow this path to properly update the end index for
> the full truncate loop and at least prevent throwing away a folio with
> partial data like this?
> 
> Brian
> 
> > Now I remember I previously submitted two patches [2] that always
> > allocated an ifs in the iomap buffered write path and the mmap fault
> > path for the block size < folio size case. However, Christoph pointed
> > out that the iomap design intentionally defers ifs allocation to avoid
> > unnecessary overhead and improve performance, and suggested creating the
> > ifs in iomap_invalidate_folio() instead [3]. There is a hurdle, though:
> > iomap_invalidate_folio() is only called when the folio has private data
> > (in truncate_inode_partial_folio(), folio_needs_release() is called
> > first to check the private flag). So if we go with this approach, we
> > need to ensure that folio_invalidate() can be called even when the folio
> > does not have an ifs.
> > 
> > One relatively simple approach is to always set the AS_RELEASE_ALWAYS
> > flag on ext4 inodes that go through the iomap path. I don't think this
> > would introduce significant overhead, but it doesn't feel very generic.
> > 
> > Another approach is to modify truncate_inode_partial_folio(). Regarding
> > this, I don't want to introduce other magics to achieve this, so perhaps
> > modify truncate_inode_partial_folio() to check whether
> > i_blocks_per_folio > 1 before calling folio_invalidate(), and call it if
> > so.
> > 
> > So, what do people think? Any better idea?
> > 
> > Thanks,
> > Yi.
> > 
> > [1] https://github.com/zhangyi089/linux/commits/ext4_buffered_iomap_v5-devel13/
> > [2] https://lore.kernel.org/linux-fsdevel/20240812121159.3775074-4-yi.zhang@huaweicloud.com/
> >     https://lore.kernel.org/linux-fsdevel/20240812121159.3775074-5-yi.zhang@huaweicloud.com/
> > [3] https://lore.kernel.org/linux-fsdevel/ZrxBfKi_DpThYo94@infradead.org/
> > 
> > 
> 
> 

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

* Re: [PATCH v4 2/5] iomap: support invalidating partial folios
  2026-08-04 14:59     ` Brian Foster
  2026-08-04 18:41       ` Darrick J. Wong
@ 2026-08-05  2:42       ` Zhang Yi
  2026-08-06 12:17         ` Brian Foster
  1 sibling, 1 reply; 16+ messages in thread
From: Zhang Yi @ 2026-08-05  2:42 UTC (permalink / raw)
  To: Brian Foster
  Cc: linux-fsdevel, linux-xfs, linux-ext4, brauner, djwong, hch,
	joannelkoong, Theodore Y. Ts'o, Jan Kara, Ojaswin Mujoo,
	Matthew Wilcox, ziy, yi.zhang, yizhang089, chengzhihao1,
	yangerkun, yukuai

On 8/4/2026 10:59 PM, Brian Foster wrote:
> On Mon, Aug 03, 2026 at 03:13:28PM +0800, Zhang Yi wrote:
>> On 7/14/2026 4:23 PM, Zhang Yi wrote:
>>> From: Zhang Yi <yi.zhang@huawei.com>
>>>
>>> Current iomap_invalidate_folio() can only invalidate an entire folio. If
>>> we truncate a partial folio on a filesystem where the block size is
>>> smaller than the folio size, it will leave behind dirty bits for the
>>> truncated or punched blocks. During the write-back process, it will
>>> attempt to map the invalid hole range. Fortunately, this has not caused
>>> any real problems so far because the ->writeback_range() function
>>> corrects the length.
>>>
>>> However, the implementation of FALLOC_FL_ZERO_RANGE in ext4 depends on
>>> the support for invalidating partial folios. When ext4 partially zeroes
>>> out a dirty and unwritten folio, it does not perform a flush first like
>>> XFS. Therefore, if the dirty bits of the corresponding area cannot be
>>> cleared, the zeroed area after writeback remains in the written state
>>> rather than reverting to the unwritten state. Fix this by supporting
>>> invalidation of partial folios.
>>
>> Hi all,
>>
>> While working on the v5 of the ext4 iomap conversion series[1], I've
>> observed a rare data inconsistency issue in xfstests generic/127. After
>> debugging, I found that the root cause lies in the fact that the current
>> patch does not cover all scenarios when handling partial folio
>> invalidation during punch hole operations in cases where
>> block size < folio size. Specifically, the sub-folio dirty state is not
>> properly cleaned up in all cases. I think we need to discuss the fix,
>> and I'd like to hear your suggestions.
>>
>> Root cause:
>>
>> In both iomap buffered write and mmap write paths, if the write range
>> covers an entire folio (regardless of whether the folio size is larger
>> than block size), an ifs (iomap_folio_state) is not allocated
>> immediately. Instead, it is deferred until writeback time, where it gets
>> created in iomap_writeback_folio(). This creates a problem: when ext4
>> performs a punch or zero_range operation on a partial range within such
>> a dirty folio, there is no way to clear the dirty state for the
>> corresponding blocks.
>>
>> This leads to two specific issues:
>>
>> 1. After issuing FALLOC_FL_ZERO_RANGE on a range covering
>>    dirty+unwritten blocks within a large folio, the dirty state cannot
>>    be cleared. During subsequent writeback, zeroed data is still
>>    written back, and the final extent state for those blocks becomes
>>    written. As a result, the fix from this patch becomes ineffective in
>>    this case.
>>
> 
> I suspect this may be (occasionally) the case even with an ifs. IIRC the
> folio batch stuff made a tradeoff for just zeroing any folio passed in
> from the fs that was determined to be dirty, regardless of whether
> underlying block aligned ranges may be unwritten and still clean.
> 
> I think this is relatively harmless so long as the underlying range
> returns zeroes from subsequent reads. The caveat may be if there are any
> cases where we have a really large folio and some small portion of it is
> dirtied and that causes some huge amount of unnecessary zeroing. I'm not
> sure if we've hit something like that in practice though..

This does seem likely harmless, since at least the user data is as
expected. Moreover, the ext4 filesystem itself may retain a small
portion of a written extent due to reasons such as the inability to
split extents. So I understand that the result should be acceptable.

However, from the iomap perspective, it seems that we support sub-folio
handling, but not fully - and the traditional buffer_head path does not
exhibit this phenomenon, which is somewhat puzzling to me. So I was
wandering whether it's necessary to fill this case?

> 
>> 2. The aforementioned rare data inconsistency in xfstests generic/127.
>>    When performing a partial folio punch hole on a dirty large folio,
>>    truncate_inode_pages_range() zeros the partial folio and then splits
>>    the folio. This causes an incorrect 'end' offset calculation in
>>    truncate_inode_pages_range(), which then results in all split folios
>>    being truncated via truncate_inode_folio(), turning partial valid
>>    data into zeroes.  For example:
>>
>>    Suppose we have a large folio of 4 pages, and we punch a range
>>    starting from the beginning to the middle of the last page.
>>    truncate_inode_pages_range() will go through two rounds of splitting.
>>    Normally, if an ifs is present, the split path would hit
>>    folio_split() -> filemap_release_folio() -> iomap_release_folio(),
>>    which would intercept the operation and refuse splitting because the
>>    folio is dirty.
>>
>>    However, without an ifs, filemap_release_folio() returns early via
>>    folio_needs_release(), causing the interception to fail. In the first
>>    round, the folio is split into 3 folios (1, 1, 2). In the second
>>    round, we expect to split into 4 folios (1, 1, 1, 1). If the second
>>    split succeeds, everything is fine, because
>>    truncate_inode_pages_range() calculates end = 3, and only the first 3
>>    folios are truncated. However, the second split is allowed to fail.
>>    If it does fail, truncate_inode_pages_range() still uses end = 3 and
>>    ends up truncating all 3 folios, resulting in data loss.
>>
> 
> I was never really a huge fan of the ifs optimization thing, but I don't
> recall what the performance benefits really were.
> 
> That aside, doesn't this seem like more of a bug within the
> truncate_inode_pages_range() path? It's not clear to me if that's the
> only problem wrt iomap, but I'd think if the end offset split down in
> truncate_inode_partial_folio() fails, we should be able to return
> partial progress or something instead of a true/false for the whole
> thing. Would that allow this path to properly update the end index for
> the full truncate loop and at least prevent throwing away a folio with
> partial data like this?
> 

+ Matthew and Zi Yan to CC

I suspect this is because most filesystems these days, when splitting
large folios, generally carry fs-specific private metadata for sub-folio
management, and when a sub-folio is dirty, ->release_folio() will
directly return EBUSY. As a result, dirty folios cannot be split because
their private data cannot be released, which is why this issue has
remained undetected until now. I also do think this does appear to be a
bug in truncate_inode_partial_folio(). Matthew and Zi Yan, what do you
think of this?

Thanks,
Yi.

> Brian
> 
>> Now I remember I previously submitted two patches [2] that always
>> allocated an ifs in the iomap buffered write path and the mmap fault
>> path for the block size < folio size case. However, Christoph pointed
>> out that the iomap design intentionally defers ifs allocation to avoid
>> unnecessary overhead and improve performance, and suggested creating the
>> ifs in iomap_invalidate_folio() instead [3]. There is a hurdle, though:
>> iomap_invalidate_folio() is only called when the folio has private data
>> (in truncate_inode_partial_folio(), folio_needs_release() is called
>> first to check the private flag). So if we go with this approach, we
>> need to ensure that folio_invalidate() can be called even when the folio
>> does not have an ifs.
>>
>> One relatively simple approach is to always set the AS_RELEASE_ALWAYS
>> flag on ext4 inodes that go through the iomap path. I don't think this
>> would introduce significant overhead, but it doesn't feel very generic.
>>
>> Another approach is to modify truncate_inode_partial_folio(). Regarding
>> this, I don't want to introduce other magics to achieve this, so perhaps
>> modify truncate_inode_partial_folio() to check whether
>> i_blocks_per_folio > 1 before calling folio_invalidate(), and call it if
>> so.
>>
>> So, what do people think? Any better idea?
>>
>> Thanks,
>> Yi.
>>
>> [1] https://github.com/zhangyi089/linux/commits/ext4_buffered_iomap_v5-devel13/
>> [2] https://lore.kernel.org/linux-fsdevel/20240812121159.3775074-4-yi.zhang@huaweicloud.com/
>>     https://lore.kernel.org/linux-fsdevel/20240812121159.3775074-5-yi.zhang@huaweicloud.com/
>> [3] https://lore.kernel.org/linux-fsdevel/ZrxBfKi_DpThYo94@infradead.org/
>>
>>
> 
> 


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

* Re: [PATCH v4 2/5] iomap: support invalidating partial folios
  2026-08-05  2:42       ` Zhang Yi
@ 2026-08-06 12:17         ` Brian Foster
  0 siblings, 0 replies; 16+ messages in thread
From: Brian Foster @ 2026-08-06 12:17 UTC (permalink / raw)
  To: Zhang Yi
  Cc: linux-fsdevel, linux-xfs, linux-ext4, brauner, djwong, hch,
	joannelkoong, Theodore Y. Ts'o, Jan Kara, Ojaswin Mujoo,
	Matthew Wilcox, ziy, yi.zhang, yizhang089, chengzhihao1,
	yangerkun, yukuai

On Wed, Aug 05, 2026 at 10:42:50AM +0800, Zhang Yi wrote:
> On 8/4/2026 10:59 PM, Brian Foster wrote:
> > On Mon, Aug 03, 2026 at 03:13:28PM +0800, Zhang Yi wrote:
> >> On 7/14/2026 4:23 PM, Zhang Yi wrote:
> >>> From: Zhang Yi <yi.zhang@huawei.com>
> >>>
> >>> Current iomap_invalidate_folio() can only invalidate an entire folio. If
> >>> we truncate a partial folio on a filesystem where the block size is
> >>> smaller than the folio size, it will leave behind dirty bits for the
> >>> truncated or punched blocks. During the write-back process, it will
> >>> attempt to map the invalid hole range. Fortunately, this has not caused
> >>> any real problems so far because the ->writeback_range() function
> >>> corrects the length.
> >>>
> >>> However, the implementation of FALLOC_FL_ZERO_RANGE in ext4 depends on
> >>> the support for invalidating partial folios. When ext4 partially zeroes
> >>> out a dirty and unwritten folio, it does not perform a flush first like
> >>> XFS. Therefore, if the dirty bits of the corresponding area cannot be
> >>> cleared, the zeroed area after writeback remains in the written state
> >>> rather than reverting to the unwritten state. Fix this by supporting
> >>> invalidation of partial folios.
> >>
> >> Hi all,
> >>
> >> While working on the v5 of the ext4 iomap conversion series[1], I've
> >> observed a rare data inconsistency issue in xfstests generic/127. After
> >> debugging, I found that the root cause lies in the fact that the current
> >> patch does not cover all scenarios when handling partial folio
> >> invalidation during punch hole operations in cases where
> >> block size < folio size. Specifically, the sub-folio dirty state is not
> >> properly cleaned up in all cases. I think we need to discuss the fix,
> >> and I'd like to hear your suggestions.
> >>
> >> Root cause:
> >>
> >> In both iomap buffered write and mmap write paths, if the write range
> >> covers an entire folio (regardless of whether the folio size is larger
> >> than block size), an ifs (iomap_folio_state) is not allocated
> >> immediately. Instead, it is deferred until writeback time, where it gets
> >> created in iomap_writeback_folio(). This creates a problem: when ext4
> >> performs a punch or zero_range operation on a partial range within such
> >> a dirty folio, there is no way to clear the dirty state for the
> >> corresponding blocks.
> >>
> >> This leads to two specific issues:
> >>
> >> 1. After issuing FALLOC_FL_ZERO_RANGE on a range covering
> >>    dirty+unwritten blocks within a large folio, the dirty state cannot
> >>    be cleared. During subsequent writeback, zeroed data is still
> >>    written back, and the final extent state for those blocks becomes
> >>    written. As a result, the fix from this patch becomes ineffective in
> >>    this case.
> >>
> > 
> > I suspect this may be (occasionally) the case even with an ifs. IIRC the
> > folio batch stuff made a tradeoff for just zeroing any folio passed in
> > from the fs that was determined to be dirty, regardless of whether
> > underlying block aligned ranges may be unwritten and still clean.
> > 
> > I think this is relatively harmless so long as the underlying range
> > returns zeroes from subsequent reads. The caveat may be if there are any
> > cases where we have a really large folio and some small portion of it is
> > dirtied and that causes some huge amount of unnecessary zeroing. I'm not
> > sure if we've hit something like that in practice though..
> 
> This does seem likely harmless, since at least the user data is as
> expected. Moreover, the ext4 filesystem itself may retain a small
> portion of a written extent due to reasons such as the inability to
> split extents. So I understand that the result should be acceptable.
> 
> However, from the iomap perspective, it seems that we support sub-folio
> handling, but not fully - and the traditional buffer_head path does not
> exhibit this phenomenon, which is somewhat puzzling to me. So I was
> wandering whether it's necessary to fill this case?
> 

I don't think there is any inherent requirement to match exact behavior
between iomap and buffer heads, if that's what you mean?

Brian

> > 
> >> 2. The aforementioned rare data inconsistency in xfstests generic/127.
> >>    When performing a partial folio punch hole on a dirty large folio,
> >>    truncate_inode_pages_range() zeros the partial folio and then splits
> >>    the folio. This causes an incorrect 'end' offset calculation in
> >>    truncate_inode_pages_range(), which then results in all split folios
> >>    being truncated via truncate_inode_folio(), turning partial valid
> >>    data into zeroes.  For example:
> >>
> >>    Suppose we have a large folio of 4 pages, and we punch a range
> >>    starting from the beginning to the middle of the last page.
> >>    truncate_inode_pages_range() will go through two rounds of splitting.
> >>    Normally, if an ifs is present, the split path would hit
> >>    folio_split() -> filemap_release_folio() -> iomap_release_folio(),
> >>    which would intercept the operation and refuse splitting because the
> >>    folio is dirty.
> >>
> >>    However, without an ifs, filemap_release_folio() returns early via
> >>    folio_needs_release(), causing the interception to fail. In the first
> >>    round, the folio is split into 3 folios (1, 1, 2). In the second
> >>    round, we expect to split into 4 folios (1, 1, 1, 1). If the second
> >>    split succeeds, everything is fine, because
> >>    truncate_inode_pages_range() calculates end = 3, and only the first 3
> >>    folios are truncated. However, the second split is allowed to fail.
> >>    If it does fail, truncate_inode_pages_range() still uses end = 3 and
> >>    ends up truncating all 3 folios, resulting in data loss.
> >>
> > 
> > I was never really a huge fan of the ifs optimization thing, but I don't
> > recall what the performance benefits really were.
> > 
> > That aside, doesn't this seem like more of a bug within the
> > truncate_inode_pages_range() path? It's not clear to me if that's the
> > only problem wrt iomap, but I'd think if the end offset split down in
> > truncate_inode_partial_folio() fails, we should be able to return
> > partial progress or something instead of a true/false for the whole
> > thing. Would that allow this path to properly update the end index for
> > the full truncate loop and at least prevent throwing away a folio with
> > partial data like this?
> > 
> 
> + Matthew and Zi Yan to CC
> 
> I suspect this is because most filesystems these days, when splitting
> large folios, generally carry fs-specific private metadata for sub-folio
> management, and when a sub-folio is dirty, ->release_folio() will
> directly return EBUSY. As a result, dirty folios cannot be split because
> their private data cannot be released, which is why this issue has
> remained undetected until now. I also do think this does appear to be a
> bug in truncate_inode_partial_folio(). Matthew and Zi Yan, what do you
> think of this?
> 
> Thanks,
> Yi.
> 
> > Brian
> > 
> >> Now I remember I previously submitted two patches [2] that always
> >> allocated an ifs in the iomap buffered write path and the mmap fault
> >> path for the block size < folio size case. However, Christoph pointed
> >> out that the iomap design intentionally defers ifs allocation to avoid
> >> unnecessary overhead and improve performance, and suggested creating the
> >> ifs in iomap_invalidate_folio() instead [3]. There is a hurdle, though:
> >> iomap_invalidate_folio() is only called when the folio has private data
> >> (in truncate_inode_partial_folio(), folio_needs_release() is called
> >> first to check the private flag). So if we go with this approach, we
> >> need to ensure that folio_invalidate() can be called even when the folio
> >> does not have an ifs.
> >>
> >> One relatively simple approach is to always set the AS_RELEASE_ALWAYS
> >> flag on ext4 inodes that go through the iomap path. I don't think this
> >> would introduce significant overhead, but it doesn't feel very generic.
> >>
> >> Another approach is to modify truncate_inode_partial_folio(). Regarding
> >> this, I don't want to introduce other magics to achieve this, so perhaps
> >> modify truncate_inode_partial_folio() to check whether
> >> i_blocks_per_folio > 1 before calling folio_invalidate(), and call it if
> >> so.
> >>
> >> So, what do people think? Any better idea?
> >>
> >> Thanks,
> >> Yi.
> >>
> >> [1] https://github.com/zhangyi089/linux/commits/ext4_buffered_iomap_v5-devel13/
> >> [2] https://lore.kernel.org/linux-fsdevel/20240812121159.3775074-4-yi.zhang@huaweicloud.com/
> >>     https://lore.kernel.org/linux-fsdevel/20240812121159.3775074-5-yi.zhang@huaweicloud.com/
> >> [3] https://lore.kernel.org/linux-fsdevel/ZrxBfKi_DpThYo94@infradead.org/
> >>
> >>
> > 
> > 
> 


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

* Re: [PATCH v4 2/5] iomap: support invalidating partial folios
  2026-08-04 18:41       ` Darrick J. Wong
@ 2026-08-06 12:18         ` Brian Foster
  0 siblings, 0 replies; 16+ messages in thread
From: Brian Foster @ 2026-08-06 12:18 UTC (permalink / raw)
  To: Darrick J. Wong
  Cc: Zhang Yi, linux-fsdevel, linux-xfs, linux-ext4, brauner, hch,
	joannelkoong, Theodore Y. Ts'o, Jan Kara, Ojaswin Mujoo,
	yi.zhang, yizhang089, chengzhihao1, yangerkun, yukuai

On Tue, Aug 04, 2026 at 11:41:42AM -0700, Darrick J. Wong wrote:
> On Tue, Aug 04, 2026 at 10:59:36AM -0400, Brian Foster wrote:
> > On Mon, Aug 03, 2026 at 03:13:28PM +0800, Zhang Yi wrote:
> > > On 7/14/2026 4:23 PM, Zhang Yi wrote:
> > > > From: Zhang Yi <yi.zhang@huawei.com>
> > > > 
> > > > Current iomap_invalidate_folio() can only invalidate an entire folio. If
> > > > we truncate a partial folio on a filesystem where the block size is
> > > > smaller than the folio size, it will leave behind dirty bits for the
> > > > truncated or punched blocks. During the write-back process, it will
> > > > attempt to map the invalid hole range. Fortunately, this has not caused
> > > > any real problems so far because the ->writeback_range() function
> > > > corrects the length.
> > > > 
> > > > However, the implementation of FALLOC_FL_ZERO_RANGE in ext4 depends on
> > > > the support for invalidating partial folios. When ext4 partially zeroes
> > > > out a dirty and unwritten folio, it does not perform a flush first like
> > > > XFS. Therefore, if the dirty bits of the corresponding area cannot be
> > > > cleared, the zeroed area after writeback remains in the written state
> > > > rather than reverting to the unwritten state. Fix this by supporting
> > > > invalidation of partial folios.
> > > 
> > > Hi all,
> > > 
> > > While working on the v5 of the ext4 iomap conversion series[1], I've
> > > observed a rare data inconsistency issue in xfstests generic/127. After
> > > debugging, I found that the root cause lies in the fact that the current
> > > patch does not cover all scenarios when handling partial folio
> > > invalidation during punch hole operations in cases where
> > > block size < folio size. Specifically, the sub-folio dirty state is not
> > > properly cleaned up in all cases. I think we need to discuss the fix,
> > > and I'd like to hear your suggestions.
> > > 
> > > Root cause:
> > > 
> > > In both iomap buffered write and mmap write paths, if the write range
> > > covers an entire folio (regardless of whether the folio size is larger
> > > than block size), an ifs (iomap_folio_state) is not allocated
> > > immediately. Instead, it is deferred until writeback time, where it gets
> > > created in iomap_writeback_folio(). This creates a problem: when ext4
> > > performs a punch or zero_range operation on a partial range within such
> > > a dirty folio, there is no way to clear the dirty state for the
> > > corresponding blocks.
> > > 
> > > This leads to two specific issues:
> > > 
> > > 1. After issuing FALLOC_FL_ZERO_RANGE on a range covering
> > >    dirty+unwritten blocks within a large folio, the dirty state cannot
> > >    be cleared. During subsequent writeback, zeroed data is still
> > >    written back, and the final extent state for those blocks becomes
> > >    written. As a result, the fix from this patch becomes ineffective in
> > >    this case.
> > > 
> > 
> > I suspect this may be (occasionally) the case even with an ifs. IIRC the
> > folio batch stuff made a tradeoff for just zeroing any folio passed in
> > from the fs that was determined to be dirty, regardless of whether
> > underlying block aligned ranges may be unwritten and still clean.
> > 
> > I think this is relatively harmless so long as the underlying range
> > returns zeroes from subsequent reads. The caveat may be if there are any
> > cases where we have a really large folio and some small portion of it is
> > dirtied and that causes some huge amount of unnecessary zeroing. I'm not
> > sure if we've hit something like that in practice though..
> > 
> > > 2. The aforementioned rare data inconsistency in xfstests generic/127.
> > >    When performing a partial folio punch hole on a dirty large folio,
> > >    truncate_inode_pages_range() zeros the partial folio and then splits
> > >    the folio. This causes an incorrect 'end' offset calculation in
> > >    truncate_inode_pages_range(), which then results in all split folios
> > >    being truncated via truncate_inode_folio(), turning partial valid
> > >    data into zeroes.  For example:
> > > 
> > >    Suppose we have a large folio of 4 pages, and we punch a range
> > >    starting from the beginning to the middle of the last page.
> > >    truncate_inode_pages_range() will go through two rounds of splitting.
> > >    Normally, if an ifs is present, the split path would hit
> > >    folio_split() -> filemap_release_folio() -> iomap_release_folio(),
> > >    which would intercept the operation and refuse splitting because the
> > >    folio is dirty.
> > > 
> > >    However, without an ifs, filemap_release_folio() returns early via
> > >    folio_needs_release(), causing the interception to fail. In the first
> > >    round, the folio is split into 3 folios (1, 1, 2). In the second
> > >    round, we expect to split into 4 folios (1, 1, 1, 1). If the second
> > >    split succeeds, everything is fine, because
> > >    truncate_inode_pages_range() calculates end = 3, and only the first 3
> > >    folios are truncated. However, the second split is allowed to fail.
> > >    If it does fail, truncate_inode_pages_range() still uses end = 3 and
> > >    ends up truncating all 3 folios, resulting in data loss.
> > > 
> > 
> > I was never really a huge fan of the ifs optimization thing, but I don't
> > recall what the performance benefits really were.
> 
> IIRC at the time it was an optimization to avoid allocating an ifs for
> the "uncommon" case of fsblock size < page size.  Then willy made it the
> common case with large folios, so there's probably little point in
> maintaining all this on/off complexity because large folios with no ifs
> resulted in huge write amplification.
> 
> The only problem with always having an ifs is that splitting a large
> folio with an ifs into smaller ones is (I think) currently not supported
> because iomap doesn't know how to split the ifs and might not be able to
> allocate the second one if memory is scarce.
> 

I see.. Zhang Yi touches on this in the other reply as well.

I wonder how critical this behavior really is to the large folio case,
particularly if the base case is block size == page size and we wouldn't
need the ifs at all. I'd think if we're under memory pressure then at
some point writeback will trigger anyways and allow a release and split
to proceed..?

Brian

> --D
> 
> > That aside, doesn't this seem like more of a bug within the
> > truncate_inode_pages_range() path? It's not clear to me if that's the
> > only problem wrt iomap, but I'd think if the end offset split down in
> > truncate_inode_partial_folio() fails, we should be able to return
> > partial progress or something instead of a true/false for the whole
> > thing. Would that allow this path to properly update the end index for
> > the full truncate loop and at least prevent throwing away a folio with
> > partial data like this?
> > 
> > Brian
> > 
> > > Now I remember I previously submitted two patches [2] that always
> > > allocated an ifs in the iomap buffered write path and the mmap fault
> > > path for the block size < folio size case. However, Christoph pointed
> > > out that the iomap design intentionally defers ifs allocation to avoid
> > > unnecessary overhead and improve performance, and suggested creating the
> > > ifs in iomap_invalidate_folio() instead [3]. There is a hurdle, though:
> > > iomap_invalidate_folio() is only called when the folio has private data
> > > (in truncate_inode_partial_folio(), folio_needs_release() is called
> > > first to check the private flag). So if we go with this approach, we
> > > need to ensure that folio_invalidate() can be called even when the folio
> > > does not have an ifs.
> > > 
> > > One relatively simple approach is to always set the AS_RELEASE_ALWAYS
> > > flag on ext4 inodes that go through the iomap path. I don't think this
> > > would introduce significant overhead, but it doesn't feel very generic.
> > > 
> > > Another approach is to modify truncate_inode_partial_folio(). Regarding
> > > this, I don't want to introduce other magics to achieve this, so perhaps
> > > modify truncate_inode_partial_folio() to check whether
> > > i_blocks_per_folio > 1 before calling folio_invalidate(), and call it if
> > > so.
> > > 
> > > So, what do people think? Any better idea?
> > > 
> > > Thanks,
> > > Yi.
> > > 
> > > [1] https://github.com/zhangyi089/linux/commits/ext4_buffered_iomap_v5-devel13/
> > > [2] https://lore.kernel.org/linux-fsdevel/20240812121159.3775074-4-yi.zhang@huaweicloud.com/
> > >     https://lore.kernel.org/linux-fsdevel/20240812121159.3775074-5-yi.zhang@huaweicloud.com/
> > > [3] https://lore.kernel.org/linux-fsdevel/ZrxBfKi_DpThYo94@infradead.org/
> > > 
> > > 
> > 
> > 
> 


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

end of thread, other threads:[~2026-08-06 12:18 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-07-14  8:23 [PATCH v4 0/5] iomap: trivial fixes for ext4 conversion Zhang Yi
2026-07-14  8:23 ` [PATCH v4 1/5] iomap: correct the range of a partial dirty clear Zhang Yi
2026-07-14  8:23 ` [PATCH v4 2/5] iomap: support invalidating partial folios Zhang Yi
2026-08-03  7:13   ` Zhang Yi
2026-08-04 14:59     ` Brian Foster
2026-08-04 18:41       ` Darrick J. Wong
2026-08-06 12:18         ` Brian Foster
2026-08-05  2:42       ` Zhang Yi
2026-08-06 12:17         ` Brian Foster
2026-07-14  8:23 ` [PATCH v4 3/5] iomap: fix incorrect did_zero setting in iomap_zero_iter() Zhang Yi
2026-07-14  8:23 ` [PATCH v4 4/5] iomap: fix out-of-bounds bitmap_set() with zero-length range Zhang Yi
2026-07-14  8:23 ` [PATCH v4 5/5] iomap: add comments for ifs_clear/set_range_dirty() Zhang Yi
2026-07-22 15:34 ` [PATCH v4 0/5] iomap: trivial fixes for ext4 conversion Theodore Tso
2026-07-23  1:25   ` Zhang Yi
2026-07-23  9:27   ` Christian Brauner
2026-07-23  9:26 ` Christian Brauner

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox