[PATCH v3 0/6] ext4: make ext4_map_blocks() recognize delalloc only extent

[PATCH v3 0/6] ext4: make ext4_map_blocks() recognize delalloc only extent

[Zhang Yi]

v2->v3:
 - Rename ext4_ext_determine_hole() to ext4_ext_determine_insert_hole()
   and keep setting of 'map' inside ext4_ext_map_blocks().
 - Don't set EXT4_MAP_DELAYED in ext4_ext_determine_insert_hole()
   because it's unreliable, and revise the comments.
v1->v2:
 - Fix a long standing race issue between determine hole and inserting
   new delalloc extent analyzed by Jan Kara.
 - Change method of adjusting hole length, instead of skip holes in
   ext4_map_blocks(), now we find delalloc and correct length and type
   in ext4_ext_determine_hole().

v2: https://lore.kernel.org/linux-ext4/20231223110223.3650717-1-yi.zhang@huaweicloud.com/
v1: https://lore.kernel.org/linux-ext4/20231121093429.1827390-1-yi.zhang@huaweicloud.com/

Hello, all!

I'm working on switching ext4 buffer IO from buffer_head to iomap
and enable large folio on regular file recently [1], this patch set
is one of a preparation of this work. It first fix a long standing race
issue between bmap querying and adding new delalloc extents, then
correct the hole length returned by ext4_map_blocks() when user querying
map type and blocks range, after that, make this function and
ext4_set_iomap() are able to distinguish delayed allocated only mapping
from hole, finally BTW cleanup the ext4_iomap_begin_report().

This preparation patch set changes the ext4 map -> iomap converting logic
in ext4_set_iomap(), so that the later buffer IO conversion can use this
helper to connect iomap frame. This patch set is already passed
'kvm-xfstests -g auto' tests.

Thanks,
Yi.

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

Zhang Yi (6):
  ext4: refactor ext4_da_map_blocks()
  ext4: convert to exclusive lock while inserting delalloc extents
  ext4: correct the hole length returned by ext4_map_blocks()
  ext4: add a hole extent entry in cache after punch
  ext4: make ext4_map_blocks() distinguish delalloc only extent
  ext4: make ext4_set_iomap() recognize IOMAP_DELALLOC map type

 fs/ext4/ext4.h    |   4 +-
 fs/ext4/extents.c | 114 +++++++++++++++++++++++++++++-----------------
 fs/ext4/inode.c   |  84 +++++++++++-----------------------
 3 files changed, 103 insertions(+), 99 deletions(-)

-- 
2.39.2

[PATCH v3 1/6] ext4: refactor ext4_da_map_blocks()

[Zhang Yi]

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

Refactor and cleanup ext4_da_map_blocks(), reduce some unnecessary
parameters and branches, no logic changes.

Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
Reviewed-by: Jan Kara <jack@suse.cz>
---
 fs/ext4/inode.c | 39 +++++++++++++++++----------------------
 1 file changed, 17 insertions(+), 22 deletions(-)

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 61277f7f8722..5b0d3075be12 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -1704,7 +1704,6 @@ static int ext4_da_map_blocks(struct inode *inode, sector_t iblock,
 	/* Lookup extent status tree firstly */
 	if (ext4_es_lookup_extent(inode, iblock, NULL, &es)) {
 		if (ext4_es_is_hole(&es)) {
-			retval = 0;
 			down_read(&EXT4_I(inode)->i_data_sem);
 			goto add_delayed;
 		}
@@ -1749,26 +1748,9 @@ static int ext4_da_map_blocks(struct inode *inode, sector_t iblock,
 		retval = ext4_ext_map_blocks(NULL, inode, map, 0);
 	else
 		retval = ext4_ind_map_blocks(NULL, inode, map, 0);
-
-add_delayed:
-	if (retval == 0) {
-		int ret;
-
-		/*
-		 * XXX: __block_prepare_write() unmaps passed block,
-		 * is it OK?
-		 */
-
-		ret = ext4_insert_delayed_block(inode, map->m_lblk);
-		if (ret != 0) {
-			retval = ret;
-			goto out_unlock;
-		}
-
-		map_bh(bh, inode->i_sb, invalid_block);
-		set_buffer_new(bh);
-		set_buffer_delay(bh);
-	} else if (retval > 0) {
+	if (retval < 0)
+		goto out_unlock;
+	if (retval > 0) {
 		unsigned int status;
 
 		if (unlikely(retval != map->m_len)) {
@@ -1783,11 +1765,24 @@ static int ext4_da_map_blocks(struct inode *inode, sector_t iblock,
 				EXTENT_STATUS_UNWRITTEN : EXTENT_STATUS_WRITTEN;
 		ext4_es_insert_extent(inode, map->m_lblk, map->m_len,
 				      map->m_pblk, status);
+		goto out_unlock;
 	}
 
+add_delayed:
+	/*
+	 * XXX: __block_prepare_write() unmaps passed block,
+	 * is it OK?
+	 */
+	retval = ext4_insert_delayed_block(inode, map->m_lblk);
+	if (retval)
+		goto out_unlock;
+
+	map_bh(bh, inode->i_sb, invalid_block);
+	set_buffer_new(bh);
+	set_buffer_delay(bh);
+
 out_unlock:
 	up_read((&EXT4_I(inode)->i_data_sem));
-
 	return retval;
 }
 
-- 
2.39.2

[PATCH v3 2/6] ext4: convert to exclusive lock while inserting delalloc extents

[Zhang Yi]

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

ext4_da_map_blocks() only hold i_data_sem in shared mode and i_rwsem
when inserting delalloc extents, it could be raced by another querying
path of ext4_map_blocks() without i_rwsem, .e.g buffered read path.
Suppose we buffered read a file containing just a hole, and without any
cached extents tree, then it is raced by another delayed buffered write
to the same area or the near area belongs to the same hole, and the new
delalloc extent could be overwritten to a hole extent.

 pread()                           pwrite()
  filemap_read_folio()
   ext4_mpage_readpages()
    ext4_map_blocks()
     down_read(i_data_sem)
     ext4_ext_determine_hole()
     //find hole
     ext4_ext_put_gap_in_cache()
      ext4_es_find_extent_range()
      //no delalloc extent
                                    ext4_da_map_blocks()
                                     down_read(i_data_sem)
                                     ext4_insert_delayed_block()
                                     //insert delalloc extent
      ext4_es_insert_extent()
      //overwrite delalloc extent to hole

This race could lead to inconsistent delalloc extents tree and
incorrect reserved space counter. Fix this by converting to hold
i_data_sem in exclusive mode when adding a new delalloc extent in
ext4_da_map_blocks().

Cc: stable@vger.kernel.org
Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
Suggested-by: Jan Kara <jack@suse.cz>
Reviewed-by: Jan Kara <jack@suse.cz>
---
 fs/ext4/inode.c | 25 +++++++++++--------------
 1 file changed, 11 insertions(+), 14 deletions(-)

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 5b0d3075be12..142c67f5c7fc 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -1703,10 +1703,8 @@ static int ext4_da_map_blocks(struct inode *inode, sector_t iblock,
 
 	/* Lookup extent status tree firstly */
 	if (ext4_es_lookup_extent(inode, iblock, NULL, &es)) {
-		if (ext4_es_is_hole(&es)) {
-			down_read(&EXT4_I(inode)->i_data_sem);
+		if (ext4_es_is_hole(&es))
 			goto add_delayed;
-		}
 
 		/*
 		 * Delayed extent could be allocated by fallocate.
@@ -1748,8 +1746,10 @@ static int ext4_da_map_blocks(struct inode *inode, sector_t iblock,
 		retval = ext4_ext_map_blocks(NULL, inode, map, 0);
 	else
 		retval = ext4_ind_map_blocks(NULL, inode, map, 0);
-	if (retval < 0)
-		goto out_unlock;
+	if (retval < 0) {
+		up_read(&EXT4_I(inode)->i_data_sem);
+		return retval;
+	}
 	if (retval > 0) {
 		unsigned int status;
 
@@ -1765,24 +1765,21 @@ static int ext4_da_map_blocks(struct inode *inode, sector_t iblock,
 				EXTENT_STATUS_UNWRITTEN : EXTENT_STATUS_WRITTEN;
 		ext4_es_insert_extent(inode, map->m_lblk, map->m_len,
 				      map->m_pblk, status);
-		goto out_unlock;
+		up_read(&EXT4_I(inode)->i_data_sem);
+		return retval;
 	}
+	up_read(&EXT4_I(inode)->i_data_sem);
 
 add_delayed:
-	/*
-	 * XXX: __block_prepare_write() unmaps passed block,
-	 * is it OK?
-	 */
+	down_write(&EXT4_I(inode)->i_data_sem);
 	retval = ext4_insert_delayed_block(inode, map->m_lblk);
+	up_write(&EXT4_I(inode)->i_data_sem);
 	if (retval)
-		goto out_unlock;
+		return retval;
 
 	map_bh(bh, inode->i_sb, invalid_block);
 	set_buffer_new(bh);
 	set_buffer_delay(bh);
-
-out_unlock:
-	up_read((&EXT4_I(inode)->i_data_sem));
 	return retval;
 }
 
-- 
2.39.2

[PATCH v3 3/6] ext4: correct the hole length returned by ext4_map_blocks()

[Zhang Yi]

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

In ext4_map_blocks(), if we can't find a range of mapping in the
extents cache, we are calling ext4_ext_map_blocks() to search the real
path and ext4_ext_determine_hole() to determine the hole range. But if
the querying range was partially or completely overlaped by a delalloc
extent, we can't find it in the real extent path, so the returned hole
length could be incorrect.

Fortunately, ext4_ext_put_gap_in_cache() have already handle delalloc
extent, but it searches start from the expanded hole_start, doesn't
start from the querying range, so the delalloc extent found could not be
the one that overlaped the querying range, plus, it also didn't adjust
the hole length. Let's just remove ext4_ext_put_gap_in_cache(), handle
delalloc and insert adjusted hole extent in ext4_ext_determine_hole().

Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
Suggested-by: Jan Kara <jack@suse.cz>
---
 fs/ext4/extents.c | 111 +++++++++++++++++++++++++++++-----------------
 1 file changed, 70 insertions(+), 41 deletions(-)

diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index d5efe076d3d3..e0b7e48c4c67 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -2229,7 +2229,7 @@ static int ext4_fill_es_cache_info(struct inode *inode,
 
 
 /*
- * ext4_ext_determine_hole - determine hole around given block
+ * ext4_ext_find_hole - find hole around given block according to the given path
  * @inode:	inode we lookup in
  * @path:	path in extent tree to @lblk
  * @lblk:	pointer to logical block around which we want to determine hole
@@ -2241,9 +2241,9 @@ static int ext4_fill_es_cache_info(struct inode *inode,
  * The function returns the length of a hole starting at @lblk. We update @lblk
  * to the beginning of the hole if we managed to find it.
  */
-static ext4_lblk_t ext4_ext_determine_hole(struct inode *inode,
-					   struct ext4_ext_path *path,
-					   ext4_lblk_t *lblk)
+static ext4_lblk_t ext4_ext_find_hole(struct inode *inode,
+				      struct ext4_ext_path *path,
+				      ext4_lblk_t *lblk)
 {
 	int depth = ext_depth(inode);
 	struct ext4_extent *ex;
@@ -2270,30 +2270,6 @@ static ext4_lblk_t ext4_ext_determine_hole(struct inode *inode,
 	return len;
 }
 
-/*
- * ext4_ext_put_gap_in_cache:
- * calculate boundaries of the gap that the requested block fits into
- * and cache this gap
- */
-static void
-ext4_ext_put_gap_in_cache(struct inode *inode, ext4_lblk_t hole_start,
-			  ext4_lblk_t hole_len)
-{
-	struct extent_status es;
-
-	ext4_es_find_extent_range(inode, &ext4_es_is_delayed, hole_start,
-				  hole_start + hole_len - 1, &es);
-	if (es.es_len) {
-		/* There's delayed extent containing lblock? */
-		if (es.es_lblk <= hole_start)
-			return;
-		hole_len = min(es.es_lblk - hole_start, hole_len);
-	}
-	ext_debug(inode, " -> %u:%u\n", hole_start, hole_len);
-	ext4_es_insert_extent(inode, hole_start, hole_len, ~0,
-			      EXTENT_STATUS_HOLE);
-}
-
 /*
  * ext4_ext_rm_idx:
  * removes index from the index block.
@@ -4062,6 +4038,69 @@ static int get_implied_cluster_alloc(struct super_block *sb,
 	return 0;
 }
 
+/*
+ * 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.
+ */
+static ext4_lblk_t ext4_ext_determine_insert_hole(struct inode *inode,
+						  struct ext4_ext_path *path,
+						  ext4_lblk_t lblk)
+{
+	ext4_lblk_t hole_start, 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);
+	if (!es.es_len)
+		goto insert_hole;
+
+	/*
+	 * There's a delalloc extent in the hole, handle it if the delalloc
+	 * extent is in front of, behind and straddle the queried range.
+	 */
+	if (lblk >= es.es_lblk + es.es_len) {
+		/*
+		 * The delalloc extent is in front of the queried range,
+		 * find again from the queried start block.
+		 */
+		len -= lblk - hole_start;
+		hole_start = lblk;
+		goto again;
+	} else if (in_range(lblk, es.es_lblk, es.es_len)) {
+		/*
+		 * The delalloc extent containing lblk, it must have been
+		 * added after ext4_map_blocks() checked the extent status
+		 * tree, adjust the length to the delalloc extent's after
+		 * lblk.
+		 */
+		len = es.es_lblk + es.es_len - lblk;
+		return len;
+	} else {
+		/*
+		 * The delalloc extent is partially or completely behind
+		 * the queried range, update hole length until the
+		 * beginning of the delalloc extent.
+		 */
+		len = min(es.es_lblk - hole_start, len);
+	}
+
+insert_hole:
+	/* Put just found gap into cache to speed up subsequent requests */
+	ext_debug(inode, " -> %u:%u\n", hole_start, len);
+	ext4_es_insert_extent(inode, hole_start, len, ~0, EXTENT_STATUS_HOLE);
+
+	/* Update hole_len to reflect hole size after lblk */
+	if (hole_start != lblk)
+		len -= lblk - hole_start;
+
+	return len;
+}
 
 /*
  * Block allocation/map/preallocation routine for extents based files
@@ -4179,22 +4218,12 @@ int ext4_ext_map_blocks(handle_t *handle, struct inode *inode,
 	 * we couldn't try to create block if create flag is zero
 	 */
 	if ((flags & EXT4_GET_BLOCKS_CREATE) == 0) {
-		ext4_lblk_t hole_start, hole_len;
+		ext4_lblk_t len;
 
-		hole_start = map->m_lblk;
-		hole_len = ext4_ext_determine_hole(inode, path, &hole_start);
-		/*
-		 * put just found gap into cache to speed up
-		 * subsequent requests
-		 */
-		ext4_ext_put_gap_in_cache(inode, hole_start, hole_len);
+		len = ext4_ext_determine_insert_hole(inode, path, map->m_lblk);
 
-		/* Update hole_len to reflect hole size after map->m_lblk */
-		if (hole_start != map->m_lblk)
-			hole_len -= map->m_lblk - hole_start;
 		map->m_pblk = 0;
-		map->m_len = min_t(unsigned int, map->m_len, hole_len);
-
+		map->m_len = min_t(unsigned int, map->m_len, len);
 		goto out;
 	}
 
-- 
2.39.2

Re: [PATCH v3 3/6] ext4: correct the hole length returned by ext4_map_blocks()

[Jan Kara]

On Fri 05-01-24 11:30:15, Zhang Yi wrote:
> From: Zhang Yi <yi.zhang@huawei.com>
> 
> In ext4_map_blocks(), if we can't find a range of mapping in the
> extents cache, we are calling ext4_ext_map_blocks() to search the real
> path and ext4_ext_determine_hole() to determine the hole range. But if
> the querying range was partially or completely overlaped by a delalloc
> extent, we can't find it in the real extent path, so the returned hole
> length could be incorrect.
> 
> Fortunately, ext4_ext_put_gap_in_cache() have already handle delalloc
> extent, but it searches start from the expanded hole_start, doesn't
> start from the querying range, so the delalloc extent found could not be
> the one that overlaped the querying range, plus, it also didn't adjust
> the hole length. Let's just remove ext4_ext_put_gap_in_cache(), handle
> delalloc and insert adjusted hole extent in ext4_ext_determine_hole().
> 
> Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
> Suggested-by: Jan Kara <jack@suse.cz>

Thanks! Looks good. Feel free to add:

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

								Honza

> ---
>  fs/ext4/extents.c | 111 +++++++++++++++++++++++++++++-----------------
>  1 file changed, 70 insertions(+), 41 deletions(-)
> 
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index d5efe076d3d3..e0b7e48c4c67 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -2229,7 +2229,7 @@ static int ext4_fill_es_cache_info(struct inode *inode,
>  
>  
>  /*
> - * ext4_ext_determine_hole - determine hole around given block
> + * ext4_ext_find_hole - find hole around given block according to the given path
>   * @inode:	inode we lookup in
>   * @path:	path in extent tree to @lblk
>   * @lblk:	pointer to logical block around which we want to determine hole
> @@ -2241,9 +2241,9 @@ static int ext4_fill_es_cache_info(struct inode *inode,
>   * The function returns the length of a hole starting at @lblk. We update @lblk
>   * to the beginning of the hole if we managed to find it.
>   */
> -static ext4_lblk_t ext4_ext_determine_hole(struct inode *inode,
> -					   struct ext4_ext_path *path,
> -					   ext4_lblk_t *lblk)
> +static ext4_lblk_t ext4_ext_find_hole(struct inode *inode,
> +				      struct ext4_ext_path *path,
> +				      ext4_lblk_t *lblk)
>  {
>  	int depth = ext_depth(inode);
>  	struct ext4_extent *ex;
> @@ -2270,30 +2270,6 @@ static ext4_lblk_t ext4_ext_determine_hole(struct inode *inode,
>  	return len;
>  }
>  
> -/*
> - * ext4_ext_put_gap_in_cache:
> - * calculate boundaries of the gap that the requested block fits into
> - * and cache this gap
> - */
> -static void
> -ext4_ext_put_gap_in_cache(struct inode *inode, ext4_lblk_t hole_start,
> -			  ext4_lblk_t hole_len)
> -{
> -	struct extent_status es;
> -
> -	ext4_es_find_extent_range(inode, &ext4_es_is_delayed, hole_start,
> -				  hole_start + hole_len - 1, &es);
> -	if (es.es_len) {
> -		/* There's delayed extent containing lblock? */
> -		if (es.es_lblk <= hole_start)
> -			return;
> -		hole_len = min(es.es_lblk - hole_start, hole_len);
> -	}
> -	ext_debug(inode, " -> %u:%u\n", hole_start, hole_len);
> -	ext4_es_insert_extent(inode, hole_start, hole_len, ~0,
> -			      EXTENT_STATUS_HOLE);
> -}
> -
>  /*
>   * ext4_ext_rm_idx:
>   * removes index from the index block.
> @@ -4062,6 +4038,69 @@ static int get_implied_cluster_alloc(struct super_block *sb,
>  	return 0;
>  }
>  
> +/*
> + * 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.
> + */
> +static ext4_lblk_t ext4_ext_determine_insert_hole(struct inode *inode,
> +						  struct ext4_ext_path *path,
> +						  ext4_lblk_t lblk)
> +{
> +	ext4_lblk_t hole_start, 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);
> +	if (!es.es_len)
> +		goto insert_hole;
> +
> +	/*
> +	 * There's a delalloc extent in the hole, handle it if the delalloc
> +	 * extent is in front of, behind and straddle the queried range.
> +	 */
> +	if (lblk >= es.es_lblk + es.es_len) {
> +		/*
> +		 * The delalloc extent is in front of the queried range,
> +		 * find again from the queried start block.
> +		 */
> +		len -= lblk - hole_start;
> +		hole_start = lblk;
> +		goto again;
> +	} else if (in_range(lblk, es.es_lblk, es.es_len)) {
> +		/*
> +		 * The delalloc extent containing lblk, it must have been
> +		 * added after ext4_map_blocks() checked the extent status
> +		 * tree, adjust the length to the delalloc extent's after
> +		 * lblk.
> +		 */
> +		len = es.es_lblk + es.es_len - lblk;
> +		return len;
> +	} else {
> +		/*
> +		 * The delalloc extent is partially or completely behind
> +		 * the queried range, update hole length until the
> +		 * beginning of the delalloc extent.
> +		 */
> +		len = min(es.es_lblk - hole_start, len);
> +	}
> +
> +insert_hole:
> +	/* Put just found gap into cache to speed up subsequent requests */
> +	ext_debug(inode, " -> %u:%u\n", hole_start, len);
> +	ext4_es_insert_extent(inode, hole_start, len, ~0, EXTENT_STATUS_HOLE);
> +
> +	/* Update hole_len to reflect hole size after lblk */
> +	if (hole_start != lblk)
> +		len -= lblk - hole_start;
> +
> +	return len;
> +}
>  
>  /*
>   * Block allocation/map/preallocation routine for extents based files
> @@ -4179,22 +4218,12 @@ int ext4_ext_map_blocks(handle_t *handle, struct inode *inode,
>  	 * we couldn't try to create block if create flag is zero
>  	 */
>  	if ((flags & EXT4_GET_BLOCKS_CREATE) == 0) {
> -		ext4_lblk_t hole_start, hole_len;
> +		ext4_lblk_t len;
>  
> -		hole_start = map->m_lblk;
> -		hole_len = ext4_ext_determine_hole(inode, path, &hole_start);
> -		/*
> -		 * put just found gap into cache to speed up
> -		 * subsequent requests
> -		 */
> -		ext4_ext_put_gap_in_cache(inode, hole_start, hole_len);
> +		len = ext4_ext_determine_insert_hole(inode, path, map->m_lblk);
>  
> -		/* Update hole_len to reflect hole size after map->m_lblk */
> -		if (hole_start != map->m_lblk)
> -			hole_len -= map->m_lblk - hole_start;
>  		map->m_pblk = 0;
> -		map->m_len = min_t(unsigned int, map->m_len, hole_len);
> -
> +		map->m_len = min_t(unsigned int, map->m_len, len);
>  		goto out;
>  	}
>  
> -- 
> 2.39.2
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v3 3/6] ext4: correct the hole length returned by ext4_map_blocks()

[Zhang Yi]

On 2024/1/5 18:17, Jan Kara wrote:
> On Fri 05-01-24 11:30:15, Zhang Yi wrote:
>> From: Zhang Yi <yi.zhang@huawei.com>
>>
>> In ext4_map_blocks(), if we can't find a range of mapping in the
>> extents cache, we are calling ext4_ext_map_blocks() to search the real
>> path and ext4_ext_determine_hole() to determine the hole range. But if
>> the querying range was partially or completely overlaped by a delalloc
>> extent, we can't find it in the real extent path, so the returned hole
>> length could be incorrect.
>>
>> Fortunately, ext4_ext_put_gap_in_cache() have already handle delalloc
>> extent, but it searches start from the expanded hole_start, doesn't
>> start from the querying range, so the delalloc extent found could not be
>> the one that overlaped the querying range, plus, it also didn't adjust
>> the hole length. Let's just remove ext4_ext_put_gap_in_cache(), handle
>> delalloc and insert adjusted hole extent in ext4_ext_determine_hole().
>>
>> Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
>> Suggested-by: Jan Kara <jack@suse.cz>
> 
> Thanks! Looks good. Feel free to add:
> 
> Reviewed-by: Jan Kara <jack@suse.cz>
> 

Thanks a lot for your review!

Yi.

> 
>> ---
>>  fs/ext4/extents.c | 111 +++++++++++++++++++++++++++++-----------------
>>  1 file changed, 70 insertions(+), 41 deletions(-)
>>
>> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
>> index d5efe076d3d3..e0b7e48c4c67 100644
>> --- a/fs/ext4/extents.c
>> +++ b/fs/ext4/extents.c
>> @@ -2229,7 +2229,7 @@ static int ext4_fill_es_cache_info(struct inode *inode,
>>  
>>  
>>  /*
>> - * ext4_ext_determine_hole - determine hole around given block
>> + * ext4_ext_find_hole - find hole around given block according to the given path
>>   * @inode:	inode we lookup in
>>   * @path:	path in extent tree to @lblk
>>   * @lblk:	pointer to logical block around which we want to determine hole
>> @@ -2241,9 +2241,9 @@ static int ext4_fill_es_cache_info(struct inode *inode,
>>   * The function returns the length of a hole starting at @lblk. We update @lblk
>>   * to the beginning of the hole if we managed to find it.
>>   */
>> -static ext4_lblk_t ext4_ext_determine_hole(struct inode *inode,
>> -					   struct ext4_ext_path *path,
>> -					   ext4_lblk_t *lblk)
>> +static ext4_lblk_t ext4_ext_find_hole(struct inode *inode,
>> +				      struct ext4_ext_path *path,
>> +				      ext4_lblk_t *lblk)
>>  {
>>  	int depth = ext_depth(inode);
>>  	struct ext4_extent *ex;
>> @@ -2270,30 +2270,6 @@ static ext4_lblk_t ext4_ext_determine_hole(struct inode *inode,
>>  	return len;
>>  }
>>  
>> -/*
>> - * ext4_ext_put_gap_in_cache:
>> - * calculate boundaries of the gap that the requested block fits into
>> - * and cache this gap
>> - */
>> -static void
>> -ext4_ext_put_gap_in_cache(struct inode *inode, ext4_lblk_t hole_start,
>> -			  ext4_lblk_t hole_len)
>> -{
>> -	struct extent_status es;
>> -
>> -	ext4_es_find_extent_range(inode, &ext4_es_is_delayed, hole_start,
>> -				  hole_start + hole_len - 1, &es);
>> -	if (es.es_len) {
>> -		/* There's delayed extent containing lblock? */
>> -		if (es.es_lblk <= hole_start)
>> -			return;
>> -		hole_len = min(es.es_lblk - hole_start, hole_len);
>> -	}
>> -	ext_debug(inode, " -> %u:%u\n", hole_start, hole_len);
>> -	ext4_es_insert_extent(inode, hole_start, hole_len, ~0,
>> -			      EXTENT_STATUS_HOLE);
>> -}
>> -
>>  /*
>>   * ext4_ext_rm_idx:
>>   * removes index from the index block.
>> @@ -4062,6 +4038,69 @@ static int get_implied_cluster_alloc(struct super_block *sb,
>>  	return 0;
>>  }
>>  
>> +/*
>> + * 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.
>> + */
>> +static ext4_lblk_t ext4_ext_determine_insert_hole(struct inode *inode,
>> +						  struct ext4_ext_path *path,
>> +						  ext4_lblk_t lblk)
>> +{
>> +	ext4_lblk_t hole_start, 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);
>> +	if (!es.es_len)
>> +		goto insert_hole;
>> +
>> +	/*
>> +	 * There's a delalloc extent in the hole, handle it if the delalloc
>> +	 * extent is in front of, behind and straddle the queried range.
>> +	 */
>> +	if (lblk >= es.es_lblk + es.es_len) {
>> +		/*
>> +		 * The delalloc extent is in front of the queried range,
>> +		 * find again from the queried start block.
>> +		 */
>> +		len -= lblk - hole_start;
>> +		hole_start = lblk;
>> +		goto again;
>> +	} else if (in_range(lblk, es.es_lblk, es.es_len)) {
>> +		/*
>> +		 * The delalloc extent containing lblk, it must have been
>> +		 * added after ext4_map_blocks() checked the extent status
>> +		 * tree, adjust the length to the delalloc extent's after
>> +		 * lblk.
>> +		 */
>> +		len = es.es_lblk + es.es_len - lblk;
>> +		return len;
>> +	} else {
>> +		/*
>> +		 * The delalloc extent is partially or completely behind
>> +		 * the queried range, update hole length until the
>> +		 * beginning of the delalloc extent.
>> +		 */
>> +		len = min(es.es_lblk - hole_start, len);
>> +	}
>> +
>> +insert_hole:
>> +	/* Put just found gap into cache to speed up subsequent requests */
>> +	ext_debug(inode, " -> %u:%u\n", hole_start, len);
>> +	ext4_es_insert_extent(inode, hole_start, len, ~0, EXTENT_STATUS_HOLE);
>> +
>> +	/* Update hole_len to reflect hole size after lblk */
>> +	if (hole_start != lblk)
>> +		len -= lblk - hole_start;
>> +
>> +	return len;
>> +}
>>  
>>  /*
>>   * Block allocation/map/preallocation routine for extents based files
>> @@ -4179,22 +4218,12 @@ int ext4_ext_map_blocks(handle_t *handle, struct inode *inode,
>>  	 * we couldn't try to create block if create flag is zero
>>  	 */
>>  	if ((flags & EXT4_GET_BLOCKS_CREATE) == 0) {
>> -		ext4_lblk_t hole_start, hole_len;
>> +		ext4_lblk_t len;
>>  
>> -		hole_start = map->m_lblk;
>> -		hole_len = ext4_ext_determine_hole(inode, path, &hole_start);
>> -		/*
>> -		 * put just found gap into cache to speed up
>> -		 * subsequent requests
>> -		 */
>> -		ext4_ext_put_gap_in_cache(inode, hole_start, hole_len);
>> +		len = ext4_ext_determine_insert_hole(inode, path, map->m_lblk);
>>  
>> -		/* Update hole_len to reflect hole size after map->m_lblk */
>> -		if (hole_start != map->m_lblk)
>> -			hole_len -= map->m_lblk - hole_start;
>>  		map->m_pblk = 0;
>> -		map->m_len = min_t(unsigned int, map->m_len, hole_len);
>> -
>> +		map->m_len = min_t(unsigned int, map->m_len, len);
>>  		goto out;
>>  	}
>>  
>> -- 
>> 2.39.2
>>

[PATCH v3 4/6] ext4: add a hole extent entry in cache after punch

[Zhang Yi]

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

In order to cache hole extents in the extent status tree and keep the
hole length as long as possible, re-add a hole entry to the cache just
after punching a hole.

Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
Reviewed-by: Jan Kara <jack@suse.cz>
---
 fs/ext4/inode.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 142c67f5c7fc..1b5e6409f958 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -4000,12 +4000,12 @@ int ext4_punch_hole(struct file *file, loff_t offset, loff_t length)
 
 	/* If there are blocks to remove, do it */
 	if (stop_block > first_block) {
+		ext4_lblk_t hole_len = stop_block - first_block;
 
 		down_write(&EXT4_I(inode)->i_data_sem);
 		ext4_discard_preallocations(inode, 0);
 
-		ext4_es_remove_extent(inode, first_block,
-				      stop_block - first_block);
+		ext4_es_remove_extent(inode, first_block, hole_len);
 
 		if (ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS))
 			ret = ext4_ext_remove_space(inode, first_block,
@@ -4014,6 +4014,8 @@ int ext4_punch_hole(struct file *file, loff_t offset, loff_t length)
 			ret = ext4_ind_remove_space(handle, inode, first_block,
 						    stop_block);
 
+		ext4_es_insert_extent(inode, first_block, hole_len, ~0,
+				      EXTENT_STATUS_HOLE);
 		up_write(&EXT4_I(inode)->i_data_sem);
 	}
 	ext4_fc_track_range(handle, inode, first_block, stop_block);
-- 
2.39.2

[PATCH v3 5/6] ext4: make ext4_map_blocks() distinguish delalloc only extent

[Zhang Yi]

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

Add a new map flag EXT4_MAP_DELAYED to indicate the mapping range is a
delayed allocated only (not unwritten) one, and making
ext4_map_blocks() can distinguish it, no longer mixing it with holes.

Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
---
 fs/ext4/ext4.h    | 4 +++-
 fs/ext4/extents.c | 7 +++++--
 fs/ext4/inode.c   | 2 ++
 3 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index a5d784872303..55195909d32f 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -252,8 +252,10 @@ struct ext4_allocation_request {
 #define EXT4_MAP_MAPPED		BIT(BH_Mapped)
 #define EXT4_MAP_UNWRITTEN	BIT(BH_Unwritten)
 #define EXT4_MAP_BOUNDARY	BIT(BH_Boundary)
+#define EXT4_MAP_DELAYED	BIT(BH_Delay)
 #define EXT4_MAP_FLAGS		(EXT4_MAP_NEW | EXT4_MAP_MAPPED |\
-				 EXT4_MAP_UNWRITTEN | EXT4_MAP_BOUNDARY)
+				 EXT4_MAP_UNWRITTEN | EXT4_MAP_BOUNDARY |\
+				 EXT4_MAP_DELAYED)
 
 struct ext4_map_blocks {
 	ext4_fsblk_t m_pblk;
diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index e0b7e48c4c67..6b64319a7df8 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -4076,8 +4076,11 @@ static ext4_lblk_t ext4_ext_determine_insert_hole(struct inode *inode,
 		/*
 		 * The delalloc extent containing lblk, it must have been
 		 * added after ext4_map_blocks() checked the extent status
-		 * tree, adjust the length to the delalloc extent's after
-		 * lblk.
+		 * tree so we are not holding i_rwsem and delalloc info is
+		 * only stabilized by i_data_sem we are going to release
+		 * soon. Don't modify the extent status tree and report
+		 * extent as a hole, just adjust the length to the delalloc
+		 * extent's after lblk.
 		 */
 		len = es.es_lblk + es.es_len - lblk;
 		return len;
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 1b5e6409f958..c141bf6d8db2 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -515,6 +515,8 @@ int ext4_map_blocks(handle_t *handle, struct inode *inode,
 			map->m_len = retval;
 		} else if (ext4_es_is_delayed(&es) || ext4_es_is_hole(&es)) {
 			map->m_pblk = 0;
+			map->m_flags |= ext4_es_is_delayed(&es) ?
+					EXT4_MAP_DELAYED : 0;
 			retval = es.es_len - (map->m_lblk - es.es_lblk);
 			if (retval > map->m_len)
 				retval = map->m_len;
-- 
2.39.2

Re: [PATCH v3 5/6] ext4: make ext4_map_blocks() distinguish delalloc only extent

[Jan Kara]

On Fri 05-01-24 11:30:17, Zhang Yi wrote:
> From: Zhang Yi <yi.zhang@huawei.com>
> 
> Add a new map flag EXT4_MAP_DELAYED to indicate the mapping range is a
> delayed allocated only (not unwritten) one, and making
> ext4_map_blocks() can distinguish it, no longer mixing it with holes.
> 
> Signed-off-by: Zhang Yi <yi.zhang@huawei.com>

Looks good. Feel free to add:

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

								Honza

> ---
>  fs/ext4/ext4.h    | 4 +++-
>  fs/ext4/extents.c | 7 +++++--
>  fs/ext4/inode.c   | 2 ++
>  3 files changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index a5d784872303..55195909d32f 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -252,8 +252,10 @@ struct ext4_allocation_request {
>  #define EXT4_MAP_MAPPED		BIT(BH_Mapped)
>  #define EXT4_MAP_UNWRITTEN	BIT(BH_Unwritten)
>  #define EXT4_MAP_BOUNDARY	BIT(BH_Boundary)
> +#define EXT4_MAP_DELAYED	BIT(BH_Delay)
>  #define EXT4_MAP_FLAGS		(EXT4_MAP_NEW | EXT4_MAP_MAPPED |\
> -				 EXT4_MAP_UNWRITTEN | EXT4_MAP_BOUNDARY)
> +				 EXT4_MAP_UNWRITTEN | EXT4_MAP_BOUNDARY |\
> +				 EXT4_MAP_DELAYED)
>  
>  struct ext4_map_blocks {
>  	ext4_fsblk_t m_pblk;
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index e0b7e48c4c67..6b64319a7df8 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -4076,8 +4076,11 @@ static ext4_lblk_t ext4_ext_determine_insert_hole(struct inode *inode,
>  		/*
>  		 * The delalloc extent containing lblk, it must have been
>  		 * added after ext4_map_blocks() checked the extent status
> -		 * tree, adjust the length to the delalloc extent's after
> -		 * lblk.
> +		 * tree so we are not holding i_rwsem and delalloc info is
> +		 * only stabilized by i_data_sem we are going to release
> +		 * soon. Don't modify the extent status tree and report
> +		 * extent as a hole, just adjust the length to the delalloc
> +		 * extent's after lblk.
>  		 */
>  		len = es.es_lblk + es.es_len - lblk;
>  		return len;
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 1b5e6409f958..c141bf6d8db2 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -515,6 +515,8 @@ int ext4_map_blocks(handle_t *handle, struct inode *inode,
>  			map->m_len = retval;
>  		} else if (ext4_es_is_delayed(&es) || ext4_es_is_hole(&es)) {
>  			map->m_pblk = 0;
> +			map->m_flags |= ext4_es_is_delayed(&es) ?
> +					EXT4_MAP_DELAYED : 0;
>  			retval = es.es_len - (map->m_lblk - es.es_lblk);
>  			if (retval > map->m_len)
>  				retval = map->m_len;
> -- 
> 2.39.2
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH v3 6/6] ext4: make ext4_set_iomap() recognize IOMAP_DELALLOC map type

[Zhang Yi]

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

Since ext4_map_blocks() can recognize a delayed allocated only extent,
make ext4_set_iomap() can also recognize it, and remove the useless
separate check in ext4_iomap_begin_report().

Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
Reviewed-by: Jan Kara <jack@suse.cz>
---
 fs/ext4/inode.c | 32 +++-----------------------------
 1 file changed, 3 insertions(+), 29 deletions(-)

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index c141bf6d8db2..0458d7f0c059 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -3261,6 +3261,9 @@ static void ext4_set_iomap(struct inode *inode, struct iomap *iomap,
 		iomap->addr = (u64) map->m_pblk << blkbits;
 		if (flags & IOMAP_DAX)
 			iomap->addr += EXT4_SB(inode->i_sb)->s_dax_part_off;
+	} else if (map->m_flags & EXT4_MAP_DELAYED) {
+		iomap->type = IOMAP_DELALLOC;
+		iomap->addr = IOMAP_NULL_ADDR;
 	} else {
 		iomap->type = IOMAP_HOLE;
 		iomap->addr = IOMAP_NULL_ADDR;
@@ -3423,35 +3426,11 @@ const struct iomap_ops ext4_iomap_overwrite_ops = {
 	.iomap_end		= ext4_iomap_end,
 };
 
-static bool ext4_iomap_is_delalloc(struct inode *inode,
-				   struct ext4_map_blocks *map)
-{
-	struct extent_status es;
-	ext4_lblk_t offset = 0, end = map->m_lblk + map->m_len - 1;
-
-	ext4_es_find_extent_range(inode, &ext4_es_is_delayed,
-				  map->m_lblk, end, &es);
-
-	if (!es.es_len || es.es_lblk > end)
-		return false;
-
-	if (es.es_lblk > map->m_lblk) {
-		map->m_len = es.es_lblk - map->m_lblk;
-		return false;
-	}
-
-	offset = map->m_lblk - es.es_lblk;
-	map->m_len = es.es_len - offset;
-
-	return true;
-}
-
 static int ext4_iomap_begin_report(struct inode *inode, loff_t offset,
 				   loff_t length, unsigned int flags,
 				   struct iomap *iomap, struct iomap *srcmap)
 {
 	int ret;
-	bool delalloc = false;
 	struct ext4_map_blocks map;
 	u8 blkbits = inode->i_blkbits;
 
@@ -3492,13 +3471,8 @@ static int ext4_iomap_begin_report(struct inode *inode, loff_t offset,
 	ret = ext4_map_blocks(NULL, inode, &map, 0);
 	if (ret < 0)
 		return ret;
-	if (ret == 0)
-		delalloc = ext4_iomap_is_delalloc(inode, &map);
-
 set_iomap:
 	ext4_set_iomap(inode, iomap, &map, offset, length, flags);
-	if (delalloc && iomap->type == IOMAP_HOLE)
-		iomap->type = IOMAP_DELALLOC;
 
 	return 0;
 }
-- 
2.39.2