[PATCH v2 1/4] ext4: remove useless code in ext4_map_create_blocks

[PATCH v2 1/4] ext4: remove useless code in ext4_map_create_blocks

[Yang Erkun]

IO path with EXT4_GET_BLOCKS_PRE_IO means dio within i_size or
dioread_nolock buffer writeback, they all means we need a unwritten
extent(or this extent has already been initialized), and the split won't
zero the range we really write. So this check seems useless. Besides,
even if we repeatedly execute ext4_es_insert_extent, there won't
actually be any issues.

Reviewed-by: Zhang Yi <yi.zhang@huawei.com>
Signed-off-by: Yang Erkun <yangerkun@huawei.com>
---
 fs/ext4/inode.c | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index e99306a8f47c..e8bac93ca668 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -583,7 +583,6 @@ static int ext4_map_query_blocks(handle_t *handle, struct inode *inode,
 static int ext4_map_create_blocks(handle_t *handle, struct inode *inode,
 				  struct ext4_map_blocks *map, int flags)
 {
-	struct extent_status es;
 	unsigned int status;
 	int err, retval = 0;
 
@@ -644,16 +643,6 @@ static int ext4_map_create_blocks(handle_t *handle, struct inode *inode,
 			return err;
 	}
 
-	/*
-	 * If the extent has been zeroed out, we don't need to update
-	 * extent status tree.
-	 */
-	if (flags & EXT4_GET_BLOCKS_PRE_IO &&
-	    ext4_es_lookup_extent(inode, map->m_lblk, NULL, &es)) {
-		if (ext4_es_is_written(&es))
-			return retval;
-	}
-
 	status = map->m_flags & EXT4_MAP_UNWRITTEN ?
 			EXTENT_STATUS_UNWRITTEN : EXTENT_STATUS_WRITTEN;
 	ext4_es_insert_extent(inode, map->m_lblk, map->m_len, map->m_pblk,
-- 
2.39.2

[PATCH v2 2/4] ext4: rename EXT4_GET_BLOCKS_PRE_IO

[Yang Erkun]

This flag has been generalized to split an unwritten extent when we do
dio or dioread_nolock writeback, or to avoid merge new extents which was
created by extents split. Update some related comments too.

Signed-off-by: Yang Erkun <yangerkun@huawei.com>
---
 fs/ext4/ext4.h              | 21 +++++++++++++++------
 fs/ext4/extents.c           | 16 ++++++++--------
 include/trace/events/ext4.h |  2 +-
 3 files changed, 24 insertions(+), 15 deletions(-)

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 57087da6c7be..96d7d649ccb0 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -694,13 +694,22 @@ enum {
 	/* Caller is from the delayed allocation writeout path
 	 * finally doing the actual allocation of delayed blocks */
 #define EXT4_GET_BLOCKS_DELALLOC_RESERVE	0x0004
-	/* caller is from the direct IO path, request to creation of an
-	unwritten extents if not allocated, split the unwritten
-	extent if blocks has been preallocated already*/
-#define EXT4_GET_BLOCKS_PRE_IO			0x0008
-#define EXT4_GET_BLOCKS_CONVERT			0x0010
-#define EXT4_GET_BLOCKS_IO_CREATE_EXT		(EXT4_GET_BLOCKS_PRE_IO|\
+	/*
+	 * This means that we cannot merge newly allocated extents, and if we
+	 * found an unwritten extent, we need to split it.
+	 */
+#define EXT4_GET_BLOCKS_SPLIT_NOMERGE		0x0008
+	/*
+	 * Caller is from the dio or dioread_nolock buffer writeback,
+	 * request to creation of an unwritten extent if not exist or split
+	 * the found unwritten extent. Also do not merge the new create
+	 * unwritten extent, io end will convert unwritten to written, and
+	 * try to merge the written extent.
+	 */
+#define EXT4_GET_BLOCKS_IO_CREATE_EXT		(EXT4_GET_BLOCKS_SPLIT_NOMERGE|\
 					 EXT4_GET_BLOCKS_CREATE_UNWRIT_EXT)
+	/* Convert unwritten extent to initialized. */
+#define EXT4_GET_BLOCKS_CONVERT			0x0010
 	/* Eventual metadata allocation (due to growing extent tree)
 	 * should not fail, so try to use reserved blocks for that.*/
 #define EXT4_GET_BLOCKS_METADATA_NOFAIL		0x0020
diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index ca5499e9412b..241b5f5d29ad 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -333,7 +333,7 @@ ext4_force_split_extent_at(handle_t *handle, struct inode *inode,
 			   int nofail)
 {
 	int unwritten = ext4_ext_is_unwritten(path[path->p_depth].p_ext);
-	int flags = EXT4_EX_NOCACHE | EXT4_GET_BLOCKS_PRE_IO;
+	int flags = EXT4_EX_NOCACHE | EXT4_GET_BLOCKS_SPLIT_NOMERGE;
 
 	if (nofail)
 		flags |= EXT4_GET_BLOCKS_METADATA_NOFAIL | EXT4_EX_NOFAIL;
@@ -2002,7 +2002,7 @@ ext4_ext_insert_extent(handle_t *handle, struct inode *inode,
 	}
 
 	/* try to insert block into found extent and return */
-	if (ex && !(gb_flags & EXT4_GET_BLOCKS_PRE_IO)) {
+	if (ex && !(gb_flags & EXT4_GET_BLOCKS_SPLIT_NOMERGE)) {
 
 		/*
 		 * Try to see whether we should rather test the extent on
@@ -2181,7 +2181,7 @@ ext4_ext_insert_extent(handle_t *handle, struct inode *inode,
 
 merge:
 	/* try to merge extents */
-	if (!(gb_flags & EXT4_GET_BLOCKS_PRE_IO))
+	if (!(gb_flags & EXT4_GET_BLOCKS_SPLIT_NOMERGE))
 		ext4_ext_try_to_merge(handle, inode, path, nearex);
 
 	/* time to correct all indexes above */
@@ -3224,7 +3224,7 @@ static struct ext4_ext_path *ext4_split_extent_at(handle_t *handle,
 		else
 			ext4_ext_mark_initialized(ex);
 
-		if (!(flags & EXT4_GET_BLOCKS_PRE_IO))
+		if (!(flags & EXT4_GET_BLOCKS_SPLIT_NOMERGE))
 			ext4_ext_try_to_merge(handle, inode, path, ex);
 
 		err = ext4_ext_dirty(handle, inode, path + path->p_depth);
@@ -3368,7 +3368,7 @@ static struct ext4_ext_path *ext4_split_extent(handle_t *handle,
 
 	if (map->m_lblk + map->m_len < ee_block + ee_len) {
 		split_flag1 = split_flag & EXT4_EXT_MAY_ZEROOUT;
-		flags1 = flags | EXT4_GET_BLOCKS_PRE_IO;
+		flags1 = flags | EXT4_GET_BLOCKS_SPLIT_NOMERGE;
 		if (unwritten)
 			split_flag1 |= EXT4_EXT_MARK_UNWRIT1 |
 				       EXT4_EXT_MARK_UNWRIT2;
@@ -3739,7 +3739,7 @@ static struct ext4_ext_path *ext4_split_convert_extents(handle_t *handle,
 			      EXT4_EXT_MAY_ZEROOUT : 0;
 		split_flag |= (EXT4_EXT_MARK_UNWRIT2 | EXT4_EXT_DATA_VALID2);
 	}
-	flags |= EXT4_GET_BLOCKS_PRE_IO;
+	flags |= EXT4_GET_BLOCKS_SPLIT_NOMERGE;
 	return ext4_split_extent(handle, inode, path, map, split_flag, flags,
 				 allocated);
 }
@@ -3911,7 +3911,7 @@ ext4_ext_handle_unwritten_extents(handle_t *handle, struct inode *inode,
 						*allocated, newblock);
 
 	/* get_block() before submitting IO, split the extent */
-	if (flags & EXT4_GET_BLOCKS_PRE_IO) {
+	if (flags & EXT4_GET_BLOCKS_SPLIT_NOMERGE) {
 		path = ext4_split_convert_extents(handle, inode, map, path,
 				flags | EXT4_GET_BLOCKS_CONVERT, allocated);
 		if (IS_ERR(path))
@@ -5618,7 +5618,7 @@ static int ext4_insert_range(struct file *file, loff_t offset, loff_t len)
 			path = ext4_split_extent_at(handle, inode, path,
 					start_lblk, split_flag,
 					EXT4_EX_NOCACHE |
-					EXT4_GET_BLOCKS_PRE_IO |
+					EXT4_GET_BLOCKS_SPLIT_NOMERGE |
 					EXT4_GET_BLOCKS_METADATA_NOFAIL);
 		}
 
diff --git a/include/trace/events/ext4.h b/include/trace/events/ext4.h
index a374e7ea7e57..ada2b9223df5 100644
--- a/include/trace/events/ext4.h
+++ b/include/trace/events/ext4.h
@@ -39,7 +39,7 @@ struct partial_cluster;
 	{ EXT4_GET_BLOCKS_CREATE,		"CREATE" },		\
 	{ EXT4_GET_BLOCKS_UNWRIT_EXT,		"UNWRIT" },		\
 	{ EXT4_GET_BLOCKS_DELALLOC_RESERVE,	"DELALLOC" },		\
-	{ EXT4_GET_BLOCKS_PRE_IO,		"PRE_IO" },		\
+	{ EXT4_GET_BLOCKS_SPLIT_NOMERGE,	"SPLIT_NOMERGE" },	\
 	{ EXT4_GET_BLOCKS_CONVERT,		"CONVERT" },		\
 	{ EXT4_GET_BLOCKS_METADATA_NOFAIL,	"METADATA_NOFAIL" },	\
 	{ EXT4_GET_BLOCKS_NO_NORMALIZE,		"NO_NORMALIZE" },	\
-- 
2.39.2

[PATCH v2 3/4] ext4: cleanup for ext4_map_blocks

[Yang Erkun]

Retval from ext4_map_create_blocks means we really create some blocks,
cannot happened with m_flags without EXT4_MAP_UNWRITTEN and
EXT4_MAP_MAPPED.

Reviewed-by: Zhang Yi <yi.zhang@huawei.com>
Signed-off-by: Yang Erkun <yangerkun@huawei.com>
---
 fs/ext4/inode.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index e8bac93ca668..3d8ada26d5cd 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -799,7 +799,13 @@ int ext4_map_blocks(handle_t *handle, struct inode *inode,
 	down_write(&EXT4_I(inode)->i_data_sem);
 	retval = ext4_map_create_blocks(handle, inode, map, flags);
 	up_write((&EXT4_I(inode)->i_data_sem));
-	if (retval > 0 && map->m_flags & EXT4_MAP_MAPPED) {
+
+	if (retval < 0)
+		ext_debug(inode, "failed with err %d\n", retval);
+	if (retval <= 0)
+		return retval;
+
+	if (map->m_flags & EXT4_MAP_MAPPED) {
 		ret = check_block_validity(inode, map);
 		if (ret != 0)
 			return ret;
@@ -828,12 +834,8 @@ int ext4_map_blocks(handle_t *handle, struct inode *inode,
 				return ret;
 		}
 	}
-	if (retval > 0 && (map->m_flags & EXT4_MAP_UNWRITTEN ||
-				map->m_flags & EXT4_MAP_MAPPED))
-		ext4_fc_track_range(handle, inode, map->m_lblk,
-					map->m_lblk + map->m_len - 1);
-	if (retval < 0)
-		ext_debug(inode, "failed with err %d\n", retval);
+	ext4_fc_track_range(handle, inode, map->m_lblk, map->m_lblk +
+			    map->m_len - 1);
 	return retval;
 }
 
-- 
2.39.2

[PATCH v2 4/4] ext4: order mode should not take effect for DIO

[Yang Erkun]

Since the size will be updated after the DIO completes, the data
will not be shown to userspace before that.

Reviewed-by: Zhang Yi <yi.zhang@huawei.com>
Signed-off-by: Yang Erkun <yangerkun@huawei.com>
---
 fs/ext4/ext4.h              | 2 ++
 fs/ext4/inode.c             | 5 +++--
 include/trace/events/ext4.h | 1 +
 3 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 96d7d649ccb0..d0331697467d 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -715,6 +715,8 @@ enum {
 #define EXT4_GET_BLOCKS_METADATA_NOFAIL		0x0020
 	/* Don't normalize allocation size (used for fallocate) */
 #define EXT4_GET_BLOCKS_NO_NORMALIZE		0x0040
+	/* Get blocks from DIO */
+#define EXT4_GET_BLOCKS_DIO			0x0080
 	/* Convert written extents to unwritten */
 #define EXT4_GET_BLOCKS_CONVERT_UNWRITTEN	0x0100
 	/* Write zeros to newly created written extents */
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 3d8ada26d5cd..168dbcc9e921 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -818,6 +818,7 @@ int ext4_map_blocks(handle_t *handle, struct inode *inode,
 		if (map->m_flags & EXT4_MAP_NEW &&
 		    !(map->m_flags & EXT4_MAP_UNWRITTEN) &&
 		    !(flags & EXT4_GET_BLOCKS_ZERO) &&
+		    !(flags & EXT4_GET_BLOCKS_DIO) &&
 		    !ext4_is_quota_file(inode) &&
 		    ext4_should_order_data(inode)) {
 			loff_t start_byte =
@@ -3729,9 +3730,9 @@ static int ext4_iomap_alloc(struct inode *inode, struct ext4_map_blocks *map,
 	 * happening and thus expose allocated blocks to direct I/O reads.
 	 */
 	else if (((loff_t)map->m_lblk << blkbits) >= i_size_read(inode))
-		m_flags = EXT4_GET_BLOCKS_CREATE;
+		m_flags = EXT4_GET_BLOCKS_CREATE | EXT4_GET_BLOCKS_DIO;
 	else if (ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS))
-		m_flags = EXT4_GET_BLOCKS_IO_CREATE_EXT;
+		m_flags = EXT4_GET_BLOCKS_IO_CREATE_EXT | EXT4_GET_BLOCKS_DIO;
 
 	if (flags & IOMAP_ATOMIC)
 		ret = ext4_map_blocks_atomic_write(handle, inode, map, m_flags,
diff --git a/include/trace/events/ext4.h b/include/trace/events/ext4.h
index ada2b9223df5..de6d848f2e37 100644
--- a/include/trace/events/ext4.h
+++ b/include/trace/events/ext4.h
@@ -43,6 +43,7 @@ struct partial_cluster;
 	{ EXT4_GET_BLOCKS_CONVERT,		"CONVERT" },		\
 	{ EXT4_GET_BLOCKS_METADATA_NOFAIL,	"METADATA_NOFAIL" },	\
 	{ EXT4_GET_BLOCKS_NO_NORMALIZE,		"NO_NORMALIZE" },	\
+	{ EXT4_GET_BLOCKS_DIO,			"DIO" },		\
 	{ EXT4_GET_BLOCKS_CONVERT_UNWRITTEN,	"CONVERT_UNWRITTEN" },  \
 	{ EXT4_GET_BLOCKS_ZERO,			"ZERO" },		\
 	{ EXT4_GET_BLOCKS_IO_SUBMIT,		"IO_SUBMIT" },		\
-- 
2.39.2

Re: [PATCH v2 2/4] ext4: rename EXT4_GET_BLOCKS_PRE_IO

[Zhang Yi]

On 11/4/2025 9:17 PM, Yang Erkun wrote:
> This flag has been generalized to split an unwritten extent when we do
> dio or dioread_nolock writeback, or to avoid merge new extents which was
> created by extents split. Update some related comments too.
> 
> Signed-off-by: Yang Erkun <yangerkun@huawei.com>

Looks good to me.

Reviewed-by: Zhang Yi <yi.zhang@huawei.com>

> ---
>  fs/ext4/ext4.h              | 21 +++++++++++++++------
>  fs/ext4/extents.c           | 16 ++++++++--------
>  include/trace/events/ext4.h |  2 +-
>  3 files changed, 24 insertions(+), 15 deletions(-)
> 
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index 57087da6c7be..96d7d649ccb0 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -694,13 +694,22 @@ enum {
>  	/* Caller is from the delayed allocation writeout path
>  	 * finally doing the actual allocation of delayed blocks */
>  #define EXT4_GET_BLOCKS_DELALLOC_RESERVE	0x0004
> -	/* caller is from the direct IO path, request to creation of an
> -	unwritten extents if not allocated, split the unwritten
> -	extent if blocks has been preallocated already*/
> -#define EXT4_GET_BLOCKS_PRE_IO			0x0008
> -#define EXT4_GET_BLOCKS_CONVERT			0x0010
> -#define EXT4_GET_BLOCKS_IO_CREATE_EXT		(EXT4_GET_BLOCKS_PRE_IO|\
> +	/*
> +	 * This means that we cannot merge newly allocated extents, and if we
> +	 * found an unwritten extent, we need to split it.
> +	 */
> +#define EXT4_GET_BLOCKS_SPLIT_NOMERGE		0x0008
> +	/*
> +	 * Caller is from the dio or dioread_nolock buffer writeback,
> +	 * request to creation of an unwritten extent if not exist or split
> +	 * the found unwritten extent. Also do not merge the new create
> +	 * unwritten extent, io end will convert unwritten to written, and
> +	 * try to merge the written extent.
> +	 */
> +#define EXT4_GET_BLOCKS_IO_CREATE_EXT		(EXT4_GET_BLOCKS_SPLIT_NOMERGE|\
>  					 EXT4_GET_BLOCKS_CREATE_UNWRIT_EXT)
> +	/* Convert unwritten extent to initialized. */
> +#define EXT4_GET_BLOCKS_CONVERT			0x0010
>  	/* Eventual metadata allocation (due to growing extent tree)
>  	 * should not fail, so try to use reserved blocks for that.*/
>  #define EXT4_GET_BLOCKS_METADATA_NOFAIL		0x0020
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index ca5499e9412b..241b5f5d29ad 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -333,7 +333,7 @@ ext4_force_split_extent_at(handle_t *handle, struct inode *inode,
>  			   int nofail)
>  {
>  	int unwritten = ext4_ext_is_unwritten(path[path->p_depth].p_ext);
> -	int flags = EXT4_EX_NOCACHE | EXT4_GET_BLOCKS_PRE_IO;
> +	int flags = EXT4_EX_NOCACHE | EXT4_GET_BLOCKS_SPLIT_NOMERGE;
>  
>  	if (nofail)
>  		flags |= EXT4_GET_BLOCKS_METADATA_NOFAIL | EXT4_EX_NOFAIL;
> @@ -2002,7 +2002,7 @@ ext4_ext_insert_extent(handle_t *handle, struct inode *inode,
>  	}
>  
>  	/* try to insert block into found extent and return */
> -	if (ex && !(gb_flags & EXT4_GET_BLOCKS_PRE_IO)) {
> +	if (ex && !(gb_flags & EXT4_GET_BLOCKS_SPLIT_NOMERGE)) {
>  
>  		/*
>  		 * Try to see whether we should rather test the extent on
> @@ -2181,7 +2181,7 @@ ext4_ext_insert_extent(handle_t *handle, struct inode *inode,
>  
>  merge:
>  	/* try to merge extents */
> -	if (!(gb_flags & EXT4_GET_BLOCKS_PRE_IO))
> +	if (!(gb_flags & EXT4_GET_BLOCKS_SPLIT_NOMERGE))
>  		ext4_ext_try_to_merge(handle, inode, path, nearex);
>  
>  	/* time to correct all indexes above */
> @@ -3224,7 +3224,7 @@ static struct ext4_ext_path *ext4_split_extent_at(handle_t *handle,
>  		else
>  			ext4_ext_mark_initialized(ex);
>  
> -		if (!(flags & EXT4_GET_BLOCKS_PRE_IO))
> +		if (!(flags & EXT4_GET_BLOCKS_SPLIT_NOMERGE))
>  			ext4_ext_try_to_merge(handle, inode, path, ex);
>  
>  		err = ext4_ext_dirty(handle, inode, path + path->p_depth);
> @@ -3368,7 +3368,7 @@ static struct ext4_ext_path *ext4_split_extent(handle_t *handle,
>  
>  	if (map->m_lblk + map->m_len < ee_block + ee_len) {
>  		split_flag1 = split_flag & EXT4_EXT_MAY_ZEROOUT;
> -		flags1 = flags | EXT4_GET_BLOCKS_PRE_IO;
> +		flags1 = flags | EXT4_GET_BLOCKS_SPLIT_NOMERGE;
>  		if (unwritten)
>  			split_flag1 |= EXT4_EXT_MARK_UNWRIT1 |
>  				       EXT4_EXT_MARK_UNWRIT2;
> @@ -3739,7 +3739,7 @@ static struct ext4_ext_path *ext4_split_convert_extents(handle_t *handle,
>  			      EXT4_EXT_MAY_ZEROOUT : 0;
>  		split_flag |= (EXT4_EXT_MARK_UNWRIT2 | EXT4_EXT_DATA_VALID2);
>  	}
> -	flags |= EXT4_GET_BLOCKS_PRE_IO;
> +	flags |= EXT4_GET_BLOCKS_SPLIT_NOMERGE;
>  	return ext4_split_extent(handle, inode, path, map, split_flag, flags,
>  				 allocated);
>  }
> @@ -3911,7 +3911,7 @@ ext4_ext_handle_unwritten_extents(handle_t *handle, struct inode *inode,
>  						*allocated, newblock);
>  
>  	/* get_block() before submitting IO, split the extent */
> -	if (flags & EXT4_GET_BLOCKS_PRE_IO) {
> +	if (flags & EXT4_GET_BLOCKS_SPLIT_NOMERGE) {
>  		path = ext4_split_convert_extents(handle, inode, map, path,
>  				flags | EXT4_GET_BLOCKS_CONVERT, allocated);
>  		if (IS_ERR(path))
> @@ -5618,7 +5618,7 @@ static int ext4_insert_range(struct file *file, loff_t offset, loff_t len)
>  			path = ext4_split_extent_at(handle, inode, path,
>  					start_lblk, split_flag,
>  					EXT4_EX_NOCACHE |
> -					EXT4_GET_BLOCKS_PRE_IO |
> +					EXT4_GET_BLOCKS_SPLIT_NOMERGE |
>  					EXT4_GET_BLOCKS_METADATA_NOFAIL);
>  		}
>  
> diff --git a/include/trace/events/ext4.h b/include/trace/events/ext4.h
> index a374e7ea7e57..ada2b9223df5 100644
> --- a/include/trace/events/ext4.h
> +++ b/include/trace/events/ext4.h
> @@ -39,7 +39,7 @@ struct partial_cluster;
>  	{ EXT4_GET_BLOCKS_CREATE,		"CREATE" },		\
>  	{ EXT4_GET_BLOCKS_UNWRIT_EXT,		"UNWRIT" },		\
>  	{ EXT4_GET_BLOCKS_DELALLOC_RESERVE,	"DELALLOC" },		\
> -	{ EXT4_GET_BLOCKS_PRE_IO,		"PRE_IO" },		\
> +	{ EXT4_GET_BLOCKS_SPLIT_NOMERGE,	"SPLIT_NOMERGE" },	\
>  	{ EXT4_GET_BLOCKS_CONVERT,		"CONVERT" },		\
>  	{ EXT4_GET_BLOCKS_METADATA_NOFAIL,	"METADATA_NOFAIL" },	\
>  	{ EXT4_GET_BLOCKS_NO_NORMALIZE,		"NO_NORMALIZE" },	\

Re: [PATCH v2 1/4] ext4: remove useless code in ext4_map_create_blocks

[Jan Kara]

On Tue 04-11-25 21:17:47, Yang Erkun wrote:
> IO path with EXT4_GET_BLOCKS_PRE_IO means dio within i_size or
> dioread_nolock buffer writeback, they all means we need a unwritten
> extent(or this extent has already been initialized), and the split won't
> zero the range we really write. So this check seems useless. Besides,
> even if we repeatedly execute ext4_es_insert_extent, there won't
> actually be any issues.
> 
> Reviewed-by: Zhang Yi <yi.zhang@huawei.com>
> Signed-off-by: Yang Erkun <yangerkun@huawei.com>

I agree the check isn't needed for correctness but it seems to be
reasonable performance optimization for a common case of writing back
already written data in dioread_nolock mode?

								Honza

> ---
>  fs/ext4/inode.c | 11 -----------
>  1 file changed, 11 deletions(-)
> 
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index e99306a8f47c..e8bac93ca668 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -583,7 +583,6 @@ static int ext4_map_query_blocks(handle_t *handle, struct inode *inode,
>  static int ext4_map_create_blocks(handle_t *handle, struct inode *inode,
>  				  struct ext4_map_blocks *map, int flags)
>  {
> -	struct extent_status es;
>  	unsigned int status;
>  	int err, retval = 0;
>  
> @@ -644,16 +643,6 @@ static int ext4_map_create_blocks(handle_t *handle, struct inode *inode,
>  			return err;
>  	}
>  
> -	/*
> -	 * If the extent has been zeroed out, we don't need to update
> -	 * extent status tree.
> -	 */
> -	if (flags & EXT4_GET_BLOCKS_PRE_IO &&
> -	    ext4_es_lookup_extent(inode, map->m_lblk, NULL, &es)) {
> -		if (ext4_es_is_written(&es))
> -			return retval;
> -	}
> -
>  	status = map->m_flags & EXT4_MAP_UNWRITTEN ?
>  			EXTENT_STATUS_UNWRITTEN : EXTENT_STATUS_WRITTEN;
>  	ext4_es_insert_extent(inode, map->m_lblk, map->m_len, map->m_pblk,
> -- 
> 2.39.2
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v2 2/4] ext4: rename EXT4_GET_BLOCKS_PRE_IO

[Jan Kara]

On Tue 04-11-25 21:17:48, Yang Erkun wrote:
> This flag has been generalized to split an unwritten extent when we do
> dio or dioread_nolock writeback, or to avoid merge new extents which was
> created by extents split. Update some related comments too.
> 
> Signed-off-by: Yang Erkun <yangerkun@huawei.com>

Just a few spelling corrections. Otherwise I like the patch. Feel free to
add:

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

> +	/*
> +	 * Caller is from the dio or dioread_nolock buffer writeback,
						    ^^^ buffered IO

> +	 * request to creation of an unwritten extent if not exist or split
			^^ create an unwritten extent if it does not exist

> +	 * the found unwritten extent. Also do not merge the new create
							     ^^^ newly created

> +	 * unwritten extent, io end will convert unwritten to written, and
> +	 * try to merge the written extent.
> +	 */

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v2 3/4] ext4: cleanup for ext4_map_blocks

[Jan Kara]

On Tue 04-11-25 21:17:49, Yang Erkun wrote:
> Retval from ext4_map_create_blocks means we really create some blocks,
> cannot happened with m_flags without EXT4_MAP_UNWRITTEN and
> EXT4_MAP_MAPPED.
> 
> Reviewed-by: Zhang Yi <yi.zhang@huawei.com>
> Signed-off-by: Yang Erkun <yangerkun@huawei.com>

Looks good to me. Feel free to add:

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

								Honza

> ---
>  fs/ext4/inode.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index e8bac93ca668..3d8ada26d5cd 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -799,7 +799,13 @@ int ext4_map_blocks(handle_t *handle, struct inode *inode,
>  	down_write(&EXT4_I(inode)->i_data_sem);
>  	retval = ext4_map_create_blocks(handle, inode, map, flags);
>  	up_write((&EXT4_I(inode)->i_data_sem));
> -	if (retval > 0 && map->m_flags & EXT4_MAP_MAPPED) {
> +
> +	if (retval < 0)
> +		ext_debug(inode, "failed with err %d\n", retval);
> +	if (retval <= 0)
> +		return retval;
> +
> +	if (map->m_flags & EXT4_MAP_MAPPED) {
>  		ret = check_block_validity(inode, map);
>  		if (ret != 0)
>  			return ret;
> @@ -828,12 +834,8 @@ int ext4_map_blocks(handle_t *handle, struct inode *inode,
>  				return ret;
>  		}
>  	}
> -	if (retval > 0 && (map->m_flags & EXT4_MAP_UNWRITTEN ||
> -				map->m_flags & EXT4_MAP_MAPPED))
> -		ext4_fc_track_range(handle, inode, map->m_lblk,
> -					map->m_lblk + map->m_len - 1);
> -	if (retval < 0)
> -		ext_debug(inode, "failed with err %d\n", retval);
> +	ext4_fc_track_range(handle, inode, map->m_lblk, map->m_lblk +
> +			    map->m_len - 1);
>  	return retval;
>  }
>  
> -- 
> 2.39.2
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v2 4/4] ext4: order mode should not take effect for DIO

[Jan Kara]

On Tue 04-11-25 21:17:50, Yang Erkun wrote:
> Since the size will be updated after the DIO completes, the data
> will not be shown to userspace before that.
> 
> Reviewed-by: Zhang Yi <yi.zhang@huawei.com>
> Signed-off-by: Yang Erkun <yangerkun@huawei.com>

Hum, is there some measurable performance benefit from this? If yes, it
would be good to mention it in the changelog. If not, then why bother?

								Honza

> ---
>  fs/ext4/ext4.h              | 2 ++
>  fs/ext4/inode.c             | 5 +++--
>  include/trace/events/ext4.h | 1 +
>  3 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index 96d7d649ccb0..d0331697467d 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -715,6 +715,8 @@ enum {
>  #define EXT4_GET_BLOCKS_METADATA_NOFAIL		0x0020
>  	/* Don't normalize allocation size (used for fallocate) */
>  #define EXT4_GET_BLOCKS_NO_NORMALIZE		0x0040
> +	/* Get blocks from DIO */
> +#define EXT4_GET_BLOCKS_DIO			0x0080
>  	/* Convert written extents to unwritten */
>  #define EXT4_GET_BLOCKS_CONVERT_UNWRITTEN	0x0100
>  	/* Write zeros to newly created written extents */
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 3d8ada26d5cd..168dbcc9e921 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -818,6 +818,7 @@ int ext4_map_blocks(handle_t *handle, struct inode *inode,
>  		if (map->m_flags & EXT4_MAP_NEW &&
>  		    !(map->m_flags & EXT4_MAP_UNWRITTEN) &&
>  		    !(flags & EXT4_GET_BLOCKS_ZERO) &&
> +		    !(flags & EXT4_GET_BLOCKS_DIO) &&
>  		    !ext4_is_quota_file(inode) &&
>  		    ext4_should_order_data(inode)) {
>  			loff_t start_byte =
> @@ -3729,9 +3730,9 @@ static int ext4_iomap_alloc(struct inode *inode, struct ext4_map_blocks *map,
>  	 * happening and thus expose allocated blocks to direct I/O reads.
>  	 */
>  	else if (((loff_t)map->m_lblk << blkbits) >= i_size_read(inode))
> -		m_flags = EXT4_GET_BLOCKS_CREATE;
> +		m_flags = EXT4_GET_BLOCKS_CREATE | EXT4_GET_BLOCKS_DIO;
>  	else if (ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS))
> -		m_flags = EXT4_GET_BLOCKS_IO_CREATE_EXT;
> +		m_flags = EXT4_GET_BLOCKS_IO_CREATE_EXT | EXT4_GET_BLOCKS_DIO;
>  
>  	if (flags & IOMAP_ATOMIC)
>  		ret = ext4_map_blocks_atomic_write(handle, inode, map, m_flags,
> diff --git a/include/trace/events/ext4.h b/include/trace/events/ext4.h
> index ada2b9223df5..de6d848f2e37 100644
> --- a/include/trace/events/ext4.h
> +++ b/include/trace/events/ext4.h
> @@ -43,6 +43,7 @@ struct partial_cluster;
>  	{ EXT4_GET_BLOCKS_CONVERT,		"CONVERT" },		\
>  	{ EXT4_GET_BLOCKS_METADATA_NOFAIL,	"METADATA_NOFAIL" },	\
>  	{ EXT4_GET_BLOCKS_NO_NORMALIZE,		"NO_NORMALIZE" },	\
> +	{ EXT4_GET_BLOCKS_DIO,			"DIO" },		\
>  	{ EXT4_GET_BLOCKS_CONVERT_UNWRITTEN,	"CONVERT_UNWRITTEN" },  \
>  	{ EXT4_GET_BLOCKS_ZERO,			"ZERO" },		\
>  	{ EXT4_GET_BLOCKS_IO_SUBMIT,		"IO_SUBMIT" },		\
> -- 
> 2.39.2
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v2 1/4] ext4: remove useless code in ext4_map_create_blocks

[yangerkun]

在 2025/11/4 22:28, Jan Kara 写道:
> On Tue 04-11-25 21:17:47, Yang Erkun wrote:
>> IO path with EXT4_GET_BLOCKS_PRE_IO means dio within i_size or
>> dioread_nolock buffer writeback, they all means we need a unwritten
>> extent(or this extent has already been initialized), and the split won't
>> zero the range we really write. So this check seems useless. Besides,
>> even if we repeatedly execute ext4_es_insert_extent, there won't
>> actually be any issues.
>>
>> Reviewed-by: Zhang Yi <yi.zhang@huawei.com>
>> Signed-off-by: Yang Erkun <yangerkun@huawei.com>
> 
> I agree the check isn't needed for correctness but it seems to be
> reasonable performance optimization for a common case of writing back
> already written data in dioread_nolock mode?

Hi!

Thank you for your detailed review! I believe you are referring to
writing back a block within the written extent in dioread_nolock mode.
If that's the case, we might never enter ext4_map_create_blocks because
ext4_map_query_blocks will return the block as MAPPED. Please correct me
if I misunderstood!

Thanks,
Erkun.

> 
> 								Honza
> 
>> ---
>>   fs/ext4/inode.c | 11 -----------
>>   1 file changed, 11 deletions(-)
>>
>> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
>> index e99306a8f47c..e8bac93ca668 100644
>> --- a/fs/ext4/inode.c
>> +++ b/fs/ext4/inode.c
>> @@ -583,7 +583,6 @@ static int ext4_map_query_blocks(handle_t *handle, struct inode *inode,
>>   static int ext4_map_create_blocks(handle_t *handle, struct inode *inode,
>>   				  struct ext4_map_blocks *map, int flags)
>>   {
>> -	struct extent_status es;
>>   	unsigned int status;
>>   	int err, retval = 0;
>>   
>> @@ -644,16 +643,6 @@ static int ext4_map_create_blocks(handle_t *handle, struct inode *inode,
>>   			return err;
>>   	}
>>   
>> -	/*
>> -	 * If the extent has been zeroed out, we don't need to update
>> -	 * extent status tree.
>> -	 */
>> -	if (flags & EXT4_GET_BLOCKS_PRE_IO &&
>> -	    ext4_es_lookup_extent(inode, map->m_lblk, NULL, &es)) {
>> -		if (ext4_es_is_written(&es))
>> -			return retval;
>> -	}
>> -
>>   	status = map->m_flags & EXT4_MAP_UNWRITTEN ?
>>   			EXTENT_STATUS_UNWRITTEN : EXTENT_STATUS_WRITTEN;
>>   	ext4_es_insert_extent(inode, map->m_lblk, map->m_len, map->m_pblk,
>> -- 
>> 2.39.2
>>

Re: [PATCH v2 1/4] ext4: remove useless code in ext4_map_create_blocks

[Jan Kara]

On Wed 05-11-25 09:39:01, yangerkun wrote:
> 
> 
> 在 2025/11/4 22:28, Jan Kara 写道:
> > On Tue 04-11-25 21:17:47, Yang Erkun wrote:
> > > IO path with EXT4_GET_BLOCKS_PRE_IO means dio within i_size or
> > > dioread_nolock buffer writeback, they all means we need a unwritten
> > > extent(or this extent has already been initialized), and the split won't
> > > zero the range we really write. So this check seems useless. Besides,
> > > even if we repeatedly execute ext4_es_insert_extent, there won't
> > > actually be any issues.
> > > 
> > > Reviewed-by: Zhang Yi <yi.zhang@huawei.com>
> > > Signed-off-by: Yang Erkun <yangerkun@huawei.com>
> > 
> > I agree the check isn't needed for correctness but it seems to be
> > reasonable performance optimization for a common case of writing back
> > already written data in dioread_nolock mode?
> 
> Hi!
> 
> Thank you for your detailed review! I believe you are referring to
> writing back a block within the written extent in dioread_nolock mode.
> If that's the case, we might never enter ext4_map_create_blocks because
> ext4_map_query_blocks will return the block as MAPPED. Please correct me
> if I misunderstood!

No, you're correct and I was confused. Feel free to add:

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

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v2 4/4] ext4: order mode should not take effect for DIO

[yangerkun]

在 2025/11/4 23:07, Jan Kara 写道:
> On Tue 04-11-25 21:17:50, Yang Erkun wrote:
>> Since the size will be updated after the DIO completes, the data
>> will not be shown to userspace before that.
>>
>> Reviewed-by: Zhang Yi <yi.zhang@huawei.com>
>> Signed-off-by: Yang Erkun <yangerkun@huawei.com>
> 
> Hum, is there some measurable performance benefit from this? If yes, it
> would be good to mention it in the changelog. If not, then why bother?

Hi,

I discovered this while debugging. Today, I also conducted some
performance tests on this patch, but it appears that there is no
noticeable improvement. Therefore, I agree with your suggestion and will
remove it in the next version!


Thanks,
Erkun.

> 
> 								Honza
> 
>> ---
>>   fs/ext4/ext4.h              | 2 ++
>>   fs/ext4/inode.c             | 5 +++--
>>   include/trace/events/ext4.h | 1 +
>>   3 files changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
>> index 96d7d649ccb0..d0331697467d 100644
>> --- a/fs/ext4/ext4.h
>> +++ b/fs/ext4/ext4.h
>> @@ -715,6 +715,8 @@ enum {
>>   #define EXT4_GET_BLOCKS_METADATA_NOFAIL		0x0020
>>   	/* Don't normalize allocation size (used for fallocate) */
>>   #define EXT4_GET_BLOCKS_NO_NORMALIZE		0x0040
>> +	/* Get blocks from DIO */
>> +#define EXT4_GET_BLOCKS_DIO			0x0080
>>   	/* Convert written extents to unwritten */
>>   #define EXT4_GET_BLOCKS_CONVERT_UNWRITTEN	0x0100
>>   	/* Write zeros to newly created written extents */
>> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
>> index 3d8ada26d5cd..168dbcc9e921 100644
>> --- a/fs/ext4/inode.c
>> +++ b/fs/ext4/inode.c
>> @@ -818,6 +818,7 @@ int ext4_map_blocks(handle_t *handle, struct inode *inode,
>>   		if (map->m_flags & EXT4_MAP_NEW &&
>>   		    !(map->m_flags & EXT4_MAP_UNWRITTEN) &&
>>   		    !(flags & EXT4_GET_BLOCKS_ZERO) &&
>> +		    !(flags & EXT4_GET_BLOCKS_DIO) &&
>>   		    !ext4_is_quota_file(inode) &&
>>   		    ext4_should_order_data(inode)) {
>>   			loff_t start_byte =
>> @@ -3729,9 +3730,9 @@ static int ext4_iomap_alloc(struct inode *inode, struct ext4_map_blocks *map,
>>   	 * happening and thus expose allocated blocks to direct I/O reads.
>>   	 */
>>   	else if (((loff_t)map->m_lblk << blkbits) >= i_size_read(inode))
>> -		m_flags = EXT4_GET_BLOCKS_CREATE;
>> +		m_flags = EXT4_GET_BLOCKS_CREATE | EXT4_GET_BLOCKS_DIO;
>>   	else if (ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS))
>> -		m_flags = EXT4_GET_BLOCKS_IO_CREATE_EXT;
>> +		m_flags = EXT4_GET_BLOCKS_IO_CREATE_EXT | EXT4_GET_BLOCKS_DIO;
>>   
>>   	if (flags & IOMAP_ATOMIC)
>>   		ret = ext4_map_blocks_atomic_write(handle, inode, map, m_flags,
>> diff --git a/include/trace/events/ext4.h b/include/trace/events/ext4.h
>> index ada2b9223df5..de6d848f2e37 100644
>> --- a/include/trace/events/ext4.h
>> +++ b/include/trace/events/ext4.h
>> @@ -43,6 +43,7 @@ struct partial_cluster;
>>   	{ EXT4_GET_BLOCKS_CONVERT,		"CONVERT" },		\
>>   	{ EXT4_GET_BLOCKS_METADATA_NOFAIL,	"METADATA_NOFAIL" },	\
>>   	{ EXT4_GET_BLOCKS_NO_NORMALIZE,		"NO_NORMALIZE" },	\
>> +	{ EXT4_GET_BLOCKS_DIO,			"DIO" },		\
>>   	{ EXT4_GET_BLOCKS_CONVERT_UNWRITTEN,	"CONVERT_UNWRITTEN" },  \
>>   	{ EXT4_GET_BLOCKS_ZERO,			"ZERO" },		\
>>   	{ EXT4_GET_BLOCKS_IO_SUBMIT,		"IO_SUBMIT" },		\
>> -- 
>> 2.39.2
>>