[PATCH v2] ext4: don't report delalloc data as a hole on indirect-mapped inodes

[PATCH v2] ext4: don't report delalloc data as a hole on indirect-mapped inodes

[Daejun Park]

When a plain lookup finds no block, ext4_ind_map_blocks() sizes the hole
it reports by counting the empty subtrees under 'partial' in the on-disk
indirect tree.  That count knows nothing about delayed allocation, so a
range holding delalloc data that has not been written back yet is
reported as a plain hole.  The extent-mapped path does not have this
problem: it trims the hole it found at the first delayed extent before
returning it.

ext4_map_blocks() consults the extent status tree before it calls into
the mapping layer, so a query that starts exactly on the delayed block
still finds it.  A query that starts earlier does not, because the hole
reported for the earlier block already spans the delayed one.  On a
4k-block filesystem, lseek(SEEK_DATA) from offset 0 on a file whose only
data block is at logical block N and is still dirty returns:

  N = 0..11      direct blocks                 N << 12
  N = 12         first single-indirect block   N << 12
  N = 13..1035   inside that indirect block    -1 ENXIO
  N = 1036       first double-indirect block   N << 12

Blocks 12 and 1036 survive because the query lands on the delayed block
itself.  Blocks 13..1035 are swallowed by the 1024-block hole reported
for block 12.  fiemap loses the same data for the same reason.

An ext3 filesystem mounted as ext4 hits this during ordinary use: ext3
inodes stay indirect mapped, while mount -t ext4 turns on delayed
allocation even though mounting ext3 with -o delalloc is explicitly
rejected.  install(1) from coreutils uses SEEK_DATA to locate data in
its source file, so installing a sparse file that has not been written
back silently produces a destination of the right size holding none of
the data:

  truncate -s 1G img && mkfs.ext3 -F img && mount -t ext4 img mnt
  echo | dd of=mnt/src bs=1 count=1 seek=64K
  install mnt/src mnt/dst
  cmp mnt/src mnt/dst        # differ: char 65537

Reconciling a hole with the extent status tree does not depend on how
the hole was found, so instead of open coding it a second time, split
the delalloc handling out of ext4_ext_determine_insert_hole() into
ext4_determine_insert_hole(), which takes the hole the caller located,
and call that from ext4_ind_map_blocks() as well.  All the special cases
and the comments explaining them stay in one place.

Two things change for indirect-mapped inodes.  They now cache the holes
they report, as the extent-mapped path already does: ext4_punch_hole()
already puts EXTENT_STATUS_HOLE on such inodes, ext4_map_blocks() and
ext4_da_map_blocks() already handle a cached hole, the latter by
replacing it with the delayed extent, and consecutive hole entries
merge, so the tree does not grow with the length of a scan.  And when a
delayed extent covers the queried block itself - a race, since
ext4_map_blocks() found nothing in the tree just before - the hole is
now reported only up to the end of that extent instead of in full.

The indirect path passes hole_start == lblk, so the "delalloc extent in
front of the queried range" case cannot be reached there:
__es_tree_search() never returns an extent ending before the block it
was searched for.

With this the sweep above returns N << 12 for every N, while the
nodelalloc and extent-mapped controls are unchanged.  generic/225,
generic/285, generic/286, generic/436, generic/448 and generic/490 pass
on ext4 made both with and without the extent feature.

Fixes: facab4d9711e ("ext4: return hole from ext4_map_blocks()")
Reported-by: Alexander Monakov <amonakov@ispras.ru>
Closes: https://lore.kernel.org/linux-ext4/594c17d9-c00f-e485-96fb-cedf27ce3aa3@ispras.ru/
Suggested-by: Jan Kara <jack@suse.cz>
Cc: stable@vger.kernel.org
Signed-off-by: Daejun Park <daejun7.park@samsung.com>
---
v1: https://lore.kernel.org/linux-ext4/20260826052446epcms2p5a415be5fbfe23b8a32786f0ae6d05aea@epcms2p5/

v2: rather than open coding the delalloc trim in ext4_ind_map_blocks(),
    split it out of ext4_ext_determine_insert_hole() into
    ext4_determine_insert_hole() and call that from both paths, as Jan
    suggested.  Two things follow from sharing the whole function, both
    noted in the commit message: indirect-mapped inodes now cache the
    holes they report, and a delalloc extent covering the queried block
    now shortens the reported hole instead of leaving it whole.

 fs/ext4/ext4.h     |  3 +++
 fs/ext4/extents.c  | 26 +++++++++++++-------------
 fs/ext4/indirect.c | 11 +++++++++++
 3 files changed, 27 insertions(+), 13 deletions(-)

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 7fd078de265d..f5eff74631a4 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -3893,6 +3893,9 @@ extern void ext4_ext_tree_init(handle_t *handle, struct inode *inode);
 extern int ext4_ext_index_trans_blocks(struct inode *inode, int extents);
 extern int ext4_ext_map_blocks(handle_t *handle, struct inode *inode,
 			       struct ext4_map_blocks *map, int flags);
+ext4_lblk_t ext4_determine_insert_hole(struct inode *inode, ext4_lblk_t lblk,
+				       ext4_lblk_t hole_start,
+				       ext4_lblk_t hole_len);
 extern int ext4_ext_truncate(handle_t *, struct inode *);
 extern int ext4_ext_remove_space(struct inode *inode, ext4_lblk_t start,
 				 ext4_lblk_t end);
diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index 836396ea7912..e2b7565e2cb4 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -4190,21 +4190,19 @@ static int get_implied_cluster_alloc(struct super_block *sb,
 }
 
 /*
- * Determine hole length around the given logical block, first try to
- * locate and expand the hole from the given @path, and then adjust it
- * if it's partially or completely converted to delayed extents, insert
- * it into the extent cache tree if it's indeed a hole, finally return
- * the length of the determined extent.
+ * Adjust the hole [@hole_start, @hole_start + @hole_len) the caller found
+ * around the queried block @lblk if it's partially or completely converted
+ * to delayed extents, insert it into the extent cache tree if it's indeed a
+ * hole, finally return the length of the determined extent starting at
+ * @lblk.  @hole_start must not be behind @lblk.
  */
-static ext4_lblk_t ext4_ext_determine_insert_hole(struct inode *inode,
-						  struct ext4_ext_path *path,
-						  ext4_lblk_t lblk)
+ext4_lblk_t ext4_determine_insert_hole(struct inode *inode, ext4_lblk_t lblk,
+				       ext4_lblk_t hole_start,
+				       ext4_lblk_t hole_len)
 {
-	ext4_lblk_t hole_start, len;
+	ext4_lblk_t len = hole_len;
 	struct extent_status es;
 
-	hole_start = lblk;
-	len = ext4_ext_find_hole(inode, path, &hole_start);
 again:
 	ext4_es_find_extent_range(inode, &ext4_es_is_delayed, hole_start,
 				  hole_start + len - 1, &es);
@@ -4371,9 +4369,11 @@ int ext4_ext_map_blocks(handle_t *handle, struct inode *inode,
 	 * we couldn't try to create block if flags doesn't contain EXT4_GET_BLOCKS_CREATE
 	 */
 	if ((flags & EXT4_GET_BLOCKS_CREATE) == 0) {
-		ext4_lblk_t len;
+		ext4_lblk_t hole_start = map->m_lblk, len;
 
-		len = ext4_ext_determine_insert_hole(inode, path, map->m_lblk);
+		len = ext4_ext_find_hole(inode, path, &hole_start);
+		len = ext4_determine_insert_hole(inode, map->m_lblk,
+						 hole_start, len);
 
 		map->m_pblk = 0;
 		map->m_len = min_t(unsigned int, map->m_len, len);
diff --git a/fs/ext4/indirect.c b/fs/ext4/indirect.c
index 5aec759eed70..9dc0759c88ef 100644
--- a/fs/ext4/indirect.c
+++ b/fs/ext4/indirect.c
@@ -586,6 +586,17 @@ int ext4_ind_map_blocks(handle_t *handle, struct inode *inode,
 		for (i = partial - chain + 1; i < depth; i++)
 			count = count * epb + (epb - offsets[i] - 1);
 		count++;
+
+		/*
+		 * The count knows nothing about delayed allocation, so let
+		 * the common helper reconcile it with the extent status
+		 * tree.  Clamp it first: with a large block size a subtree
+		 * can be bigger than the logical block space.
+		 */
+		count = umin(count, EXT_MAX_BLOCKS - map->m_lblk);
+		count = ext4_determine_insert_hole(inode, map->m_lblk,
+						   map->m_lblk, count);
+
 		/* Fill in size of a hole we found */
 		map->m_pblk = 0;
 		map->m_len = umin(map->m_len, count);

base-commit: 9091c97be34083587a75db174aab51551d8e8543
-- 
2.43.0

Re: [PATCH v2] ext4: don't report delalloc data as a hole on indirect-mapped inodes

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Critical] Caching a hole in the extent status tree when ext4_get_branch() fails due to an I/O error.

Pre-existing issues:
- [Critical] Transient -ENOMEM or -EFSCORRUPTED errors in ext4_get_branch() cause existing indirect blocks to be orphaned and overwritten.
--

commit 4a371ba0722d0395bf8f94f94d508b0175eed067
Author: Daejun Park <daejun7.park@samsung.com>

ext4: don't report delalloc data as a hole on indirect-mapped inodes

When a plain lookup finds no block, ext4_ind_map_blocks() currently
sizes the hole without considering delayed allocation, which causes
delayed data to be reported as a plain hole. This commit reconciles
the hole with the extent status tree by factoring out
ext4_determine_insert_hole() and using it for indirect-mapped inodes.
Indirect-mapped inodes now cache the holes they report.

Closes: https://lore.kernel.org/linux-ext4/594c17d9-c00f-e485-96fb-cedf27ce3aa3@ispras.ru/

[ ... ]

> diff --git a/fs/ext4/indirect.c b/fs/ext4/indirect.c
> index 5aec759eed705..9dc0759c88ef4 100644
> --- a/fs/ext4/indirect.c
> +++ b/fs/ext4/indirect.c
> @@ -586,6 +586,17 @@ int ext4_ind_map_blocks(handle_t *handle, struct inode *inode,
>  		for (i = partial - chain + 1; i < depth; i++)
>  			count = count * epb + (epb - offsets[i] - 1);
>  		count++;
> +
> +		/*
> +		 * The count knows nothing about delayed allocation, so let
> +		 * the common helper reconcile it with the extent status
> +		 * tree.  Clamp it first: with a large block size a subtree
> +		 * can be bigger than the logical block space.
> +		 */
> +		count = umin(count, EXT_MAX_BLOCKS - map->m_lblk);
> +		count = ext4_determine_insert_hole(inode, map->m_lblk,
> +						   map->m_lblk, count);

[Severity: Critical]
If ext4_get_branch() failed with an error like -EIO, -ENOMEM, or
-EFSCORRUPTED, does calling ext4_determine_insert_hole() here incorrectly
cache this transient error as a permanent hole in the extent status tree?

If (flags & EXT4_GET_BLOCKS_CREATE) == 0, the code enters this branch before
checking err, so subsequent reads might find this cached hole and return
zeros instead of retrying the disk read.

> +
>  		/* Fill in size of a hole we found */
>  		map->m_pblk = 0;
>  		map->m_len = umin(map->m_len, count);

[Severity: Critical]
This is a pre-existing issue, but looking just below this hunk in
ext4_ind_map_blocks():

	/* Failed read of indirect block */
	if (err == -EIO)
		goto cleanup;

	/*
	 * Okay, we need to do block allocation.
	*/

Does this check fail to handle errors other than -EIO from
ext4_get_branch()?

If ext4_get_branch() fails with -ENOMEM or -EFSCORRUPTED when
EXT4_GET_BLOCKS_CREATE is set, the code falls through to block allocation.
Will this cause existing indirect blocks to be orphaned and overwritten,
leading to a disk space leak and permanent replacement of valid file data
with zeros?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260827043928epcms2p54597035aadd1eca0556ae9b0585694f9@epcms2p5?part=1

Re: [PATCH v2] ext4: don't report delalloc data as a hole on indirect-mapped inodes

[Jan Kara]

On Thu 27-08-26 13:39:28, Daejun Park wrote:
> When a plain lookup finds no block, ext4_ind_map_blocks() sizes the hole
> it reports by counting the empty subtrees under 'partial' in the on-disk
> indirect tree.  That count knows nothing about delayed allocation, so a
> range holding delalloc data that has not been written back yet is
> reported as a plain hole.  The extent-mapped path does not have this
> problem: it trims the hole it found at the first delayed extent before
> returning it.
> 
> ext4_map_blocks() consults the extent status tree before it calls into
> the mapping layer, so a query that starts exactly on the delayed block
> still finds it.  A query that starts earlier does not, because the hole
> reported for the earlier block already spans the delayed one.  On a
> 4k-block filesystem, lseek(SEEK_DATA) from offset 0 on a file whose only
> data block is at logical block N and is still dirty returns:
> 
>   N = 0..11      direct blocks                 N << 12
>   N = 12         first single-indirect block   N << 12
>   N = 13..1035   inside that indirect block    -1 ENXIO
>   N = 1036       first double-indirect block   N << 12
> 
> Blocks 12 and 1036 survive because the query lands on the delayed block
> itself.  Blocks 13..1035 are swallowed by the 1024-block hole reported
> for block 12.  fiemap loses the same data for the same reason.
> 
> An ext3 filesystem mounted as ext4 hits this during ordinary use: ext3
> inodes stay indirect mapped, while mount -t ext4 turns on delayed
> allocation even though mounting ext3 with -o delalloc is explicitly
> rejected.  install(1) from coreutils uses SEEK_DATA to locate data in
> its source file, so installing a sparse file that has not been written
> back silently produces a destination of the right size holding none of
> the data:
> 
>   truncate -s 1G img && mkfs.ext3 -F img && mount -t ext4 img mnt
>   echo | dd of=mnt/src bs=1 count=1 seek=64K
>   install mnt/src mnt/dst
>   cmp mnt/src mnt/dst        # differ: char 65537
> 
> Reconciling a hole with the extent status tree does not depend on how
> the hole was found, so instead of open coding it a second time, split
> the delalloc handling out of ext4_ext_determine_insert_hole() into
> ext4_determine_insert_hole(), which takes the hole the caller located,
> and call that from ext4_ind_map_blocks() as well.  All the special cases
> and the comments explaining them stay in one place.
> 
> Two things change for indirect-mapped inodes.  They now cache the holes
> they report, as the extent-mapped path already does: ext4_punch_hole()
> already puts EXTENT_STATUS_HOLE on such inodes, ext4_map_blocks() and
> ext4_da_map_blocks() already handle a cached hole, the latter by
> replacing it with the delayed extent, and consecutive hole entries
> merge, so the tree does not grow with the length of a scan.  And when a
> delayed extent covers the queried block itself - a race, since
> ext4_map_blocks() found nothing in the tree just before - the hole is
> now reported only up to the end of that extent instead of in full.
> 
> The indirect path passes hole_start == lblk, so the "delalloc extent in
> front of the queried range" case cannot be reached there:
> __es_tree_search() never returns an extent ending before the block it
> was searched for.
> 
> With this the sweep above returns N << 12 for every N, while the
> nodelalloc and extent-mapped controls are unchanged.  generic/225,
> generic/285, generic/286, generic/436, generic/448 and generic/490 pass
> on ext4 made both with and without the extent feature.
> 
> Fixes: facab4d9711e ("ext4: return hole from ext4_map_blocks()")
> Reported-by: Alexander Monakov <amonakov@ispras.ru>
> Closes: https://lore.kernel.org/linux-ext4/594c17d9-c00f-e485-96fb-cedf27ce3aa3@ispras.ru/
> Suggested-by: Jan Kara <jack@suse.cz>
> Cc: stable@vger.kernel.org
> Signed-off-by: Daejun Park <daejun7.park@samsung.com>

Looks good to me. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
> v1: https://lore.kernel.org/linux-ext4/20260826052446epcms2p5a415be5fbfe23b8a32786f0ae6d05aea@epcms2p5/
> 
> v2: rather than open coding the delalloc trim in ext4_ind_map_blocks(),
>     split it out of ext4_ext_determine_insert_hole() into
>     ext4_determine_insert_hole() and call that from both paths, as Jan
>     suggested.  Two things follow from sharing the whole function, both
>     noted in the commit message: indirect-mapped inodes now cache the
>     holes they report, and a delalloc extent covering the queried block
>     now shortens the reported hole instead of leaving it whole.
> 
>  fs/ext4/ext4.h     |  3 +++
>  fs/ext4/extents.c  | 26 +++++++++++++-------------
>  fs/ext4/indirect.c | 11 +++++++++++
>  3 files changed, 27 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index 7fd078de265d..f5eff74631a4 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -3893,6 +3893,9 @@ extern void ext4_ext_tree_init(handle_t *handle, struct inode *inode);
>  extern int ext4_ext_index_trans_blocks(struct inode *inode, int extents);
>  extern int ext4_ext_map_blocks(handle_t *handle, struct inode *inode,
>  			       struct ext4_map_blocks *map, int flags);
> +ext4_lblk_t ext4_determine_insert_hole(struct inode *inode, ext4_lblk_t lblk,
> +				       ext4_lblk_t hole_start,
> +				       ext4_lblk_t hole_len);
>  extern int ext4_ext_truncate(handle_t *, struct inode *);
>  extern int ext4_ext_remove_space(struct inode *inode, ext4_lblk_t start,
>  				 ext4_lblk_t end);
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index 836396ea7912..e2b7565e2cb4 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -4190,21 +4190,19 @@ static int get_implied_cluster_alloc(struct super_block *sb,
>  }
>  
>  /*
> - * Determine hole length around the given logical block, first try to
> - * locate and expand the hole from the given @path, and then adjust it
> - * if it's partially or completely converted to delayed extents, insert
> - * it into the extent cache tree if it's indeed a hole, finally return
> - * the length of the determined extent.
> + * Adjust the hole [@hole_start, @hole_start + @hole_len) the caller found
> + * around the queried block @lblk if it's partially or completely converted
> + * to delayed extents, insert it into the extent cache tree if it's indeed a
> + * hole, finally return the length of the determined extent starting at
> + * @lblk.  @hole_start must not be behind @lblk.
>   */
> -static ext4_lblk_t ext4_ext_determine_insert_hole(struct inode *inode,
> -						  struct ext4_ext_path *path,
> -						  ext4_lblk_t lblk)
> +ext4_lblk_t ext4_determine_insert_hole(struct inode *inode, ext4_lblk_t lblk,
> +				       ext4_lblk_t hole_start,
> +				       ext4_lblk_t hole_len)
>  {
> -	ext4_lblk_t hole_start, len;
> +	ext4_lblk_t len = hole_len;
>  	struct extent_status es;
>  
> -	hole_start = lblk;
> -	len = ext4_ext_find_hole(inode, path, &hole_start);
>  again:
>  	ext4_es_find_extent_range(inode, &ext4_es_is_delayed, hole_start,
>  				  hole_start + len - 1, &es);
> @@ -4371,9 +4369,11 @@ int ext4_ext_map_blocks(handle_t *handle, struct inode *inode,
>  	 * we couldn't try to create block if flags doesn't contain EXT4_GET_BLOCKS_CREATE
>  	 */
>  	if ((flags & EXT4_GET_BLOCKS_CREATE) == 0) {
> -		ext4_lblk_t len;
> +		ext4_lblk_t hole_start = map->m_lblk, len;
>  
> -		len = ext4_ext_determine_insert_hole(inode, path, map->m_lblk);
> +		len = ext4_ext_find_hole(inode, path, &hole_start);
> +		len = ext4_determine_insert_hole(inode, map->m_lblk,
> +						 hole_start, len);
>  
>  		map->m_pblk = 0;
>  		map->m_len = min_t(unsigned int, map->m_len, len);
> diff --git a/fs/ext4/indirect.c b/fs/ext4/indirect.c
> index 5aec759eed70..9dc0759c88ef 100644
> --- a/fs/ext4/indirect.c
> +++ b/fs/ext4/indirect.c
> @@ -586,6 +586,17 @@ int ext4_ind_map_blocks(handle_t *handle, struct inode *inode,
>  		for (i = partial - chain + 1; i < depth; i++)
>  			count = count * epb + (epb - offsets[i] - 1);
>  		count++;
> +
> +		/*
> +		 * The count knows nothing about delayed allocation, so let
> +		 * the common helper reconcile it with the extent status
> +		 * tree.  Clamp it first: with a large block size a subtree
> +		 * can be bigger than the logical block space.
> +		 */
> +		count = umin(count, EXT_MAX_BLOCKS - map->m_lblk);
> +		count = ext4_determine_insert_hole(inode, map->m_lblk,
> +						   map->m_lblk, count);
> +
>  		/* Fill in size of a hole we found */
>  		map->m_pblk = 0;
>  		map->m_len = umin(map->m_len, count);
> 
> base-commit: 9091c97be34083587a75db174aab51551d8e8543
> -- 
> 2.43.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR