[PATCH v1 0/5] ext4: Minor fixes and improvements for atomic write series

[PATCH v1 0/5] ext4: Minor fixes and improvements for atomic write series

[Ritesh Harjani (IBM)]

Some minor fixes and improvements on top of ext4's dev branch [1] for atomic
write patch series.

[1]: https://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git/log/?h=dev

Patch-1: Is the fix needed after rebase on top of extent status cleanup.
Patch-2: warning fix for cocci
Patch-{3..5}: Minor improvements / simplifications.

Ritesh Harjani (IBM) (5):
  ext4: Unwritten to written conversion requires EXT4_EX_NOCACHE
  ext4: Simplify last in leaf check in ext4_map_query_blocks
  ext4: Rename and document EXT4_EX_FILTER to EXT4_EX_QUERY_FILTER
  ext4: Simplify flags in ext4_map_query_blocks()
  ext4: Add a WARN_ON_ONCE for querying LAST_IN_LEAF instead

 fs/ext4/ext4.h    | 8 +++++++-
 fs/ext4/extents.c | 6 ++++--
 fs/ext4/inode.c   | 9 +++------
 3 files changed, 14 insertions(+), 9 deletions(-)

--
2.49.0

[PATCH v1 1/5] ext4: Unwritten to written conversion requires EXT4_EX_NOCACHE

[Ritesh Harjani (IBM)]

This fixes the atomic write patch series after it was rebased on top of
extent status cache cleanup series i.e.

'commit 402e38e6b71f57 ("ext4: prevent stale extent cache entries caused by
concurrent I/O writeback")'

After the above series, EXT4_GET_BLOCKS_IO_CONVERT_EXT flag which has
EXT4_GET_BLOCKS_IO_SUBMIT flag set, requires that the io submit context
of any kind should pass EXT4_EX_NOCACHE to avoid caching unncecessary
extents in the extent status cache.

This patch fixes that by adding the EXT4_EX_NOCACHE flag in
ext4_convert_unwritten_extents_atomic() for unwritten to written
conversion calls to ext4_map_blocks().

Fixes: ba601987dbb4 ("ext4: Add multi-fsblock atomic write support with bigalloc")
Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
---
 fs/ext4/extents.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index 8b834e13d306..7683558381dc 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -4826,7 +4826,7 @@ int ext4_convert_unwritten_extents_atomic(handle_t *handle, struct inode *inode,
 	struct ext4_map_blocks map;
 	unsigned int blkbits = inode->i_blkbits;
 	unsigned int credits = 0;
-	int flags = EXT4_GET_BLOCKS_IO_CONVERT_EXT;
+	int flags = EXT4_GET_BLOCKS_IO_CONVERT_EXT | EXT4_EX_NOCACHE;
 
 	map.m_lblk = offset >> blkbits;
 	max_blocks = EXT4_MAX_BLOCKS(len, offset, blkbits);
-- 
2.49.0

Re: [PATCH v1 1/5] ext4: Unwritten to written conversion requires EXT4_EX_NOCACHE

[Ojaswin Mujoo]

On Mon, May 19, 2025 at 11:49:26PM +0530, Ritesh Harjani (IBM) wrote:
> This fixes the atomic write patch series after it was rebased on top of
> extent status cache cleanup series i.e.
> 
> 'commit 402e38e6b71f57 ("ext4: prevent stale extent cache entries caused by
> concurrent I/O writeback")'
> 
> After the above series, EXT4_GET_BLOCKS_IO_CONVERT_EXT flag which has
> EXT4_GET_BLOCKS_IO_SUBMIT flag set, requires that the io submit context
> of any kind should pass EXT4_EX_NOCACHE to avoid caching unncecessary
> extents in the extent status cache.
> 
> This patch fixes that by adding the EXT4_EX_NOCACHE flag in
> ext4_convert_unwritten_extents_atomic() for unwritten to written
> conversion calls to ext4_map_blocks().
> 
> Fixes: ba601987dbb4 ("ext4: Add multi-fsblock atomic write support with bigalloc")
> Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
> ---
>  fs/ext4/extents.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index 8b834e13d306..7683558381dc 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -4826,7 +4826,7 @@ int ext4_convert_unwritten_extents_atomic(handle_t *handle, struct inode *inode,
>  	struct ext4_map_blocks map;
>  	unsigned int blkbits = inode->i_blkbits;
>  	unsigned int credits = 0;
> -	int flags = EXT4_GET_BLOCKS_IO_CONVERT_EXT;
> +	int flags = EXT4_GET_BLOCKS_IO_CONVERT_EXT | EXT4_EX_NOCACHE;

Makes sense, feel free to add:

Reviewed-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>

Thanks,
ojaswin

>  
>  	map.m_lblk = offset >> blkbits;
>  	max_blocks = EXT4_MAX_BLOCKS(len, offset, blkbits);
> -- 
> 2.49.0
> 

[PATCH v1 2/5] ext4: Simplify last in leaf check in ext4_map_query_blocks

[Ritesh Harjani (IBM)]

This simplifies the check for last in leaf in ext4_map_query_blocks()
and fixes this cocci warning.

cocci warnings: (new ones prefixed by >>)
>> fs/ext4/inode.c:573:49-51: WARNING !A || A && B is equivalent to !A || B

Fixes: 2e7bad830aa9 ("ext4: Add support for EXT4_GET_BLOCKS_QUERY_LEAF_BLOCKS")
Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202505191524.auftmOwK-lkp@intel.com/
Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
---
 fs/ext4/inode.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index ce0632094c50..459ffc6af1d3 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -570,8 +570,7 @@ static int ext4_map_query_blocks(handle_t *handle, struct inode *inode,
 	 * - if the last in leaf is the full requested range
 	 */
 	if (!(map->m_flags & EXT4_MAP_QUERY_LAST_IN_LEAF) ||
-			((map->m_flags & EXT4_MAP_QUERY_LAST_IN_LEAF) &&
-			 (map->m_len == orig_mlen))) {
+			map->m_len == orig_mlen) {
 		status = map->m_flags & EXT4_MAP_UNWRITTEN ?
 				EXTENT_STATUS_UNWRITTEN : EXTENT_STATUS_WRITTEN;
 		ext4_es_insert_extent(inode, map->m_lblk, map->m_len,
-- 
2.49.0

Re: [PATCH v1 2/5] ext4: Simplify last in leaf check in ext4_map_query_blocks

[Ojaswin Mujoo]

On Mon, May 19, 2025 at 11:49:27PM +0530, Ritesh Harjani (IBM) wrote:
> This simplifies the check for last in leaf in ext4_map_query_blocks()
> and fixes this cocci warning.
> 
> cocci warnings: (new ones prefixed by >>)
> >> fs/ext4/inode.c:573:49-51: WARNING !A || A && B is equivalent to !A || B
> 
> Fixes: 2e7bad830aa9 ("ext4: Add support for EXT4_GET_BLOCKS_QUERY_LEAF_BLOCKS")
> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202505191524.auftmOwK-lkp@intel.com/
> Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>

Makes sense, feel free to add:

Reviewed-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>

> ---
>  fs/ext4/inode.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index ce0632094c50..459ffc6af1d3 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -570,8 +570,7 @@ static int ext4_map_query_blocks(handle_t *handle, struct inode *inode,
>  	 * - if the last in leaf is the full requested range
>  	 */
>  	if (!(map->m_flags & EXT4_MAP_QUERY_LAST_IN_LEAF) ||
> -			((map->m_flags & EXT4_MAP_QUERY_LAST_IN_LEAF) &&
> -			 (map->m_len == orig_mlen))) {
> +			map->m_len == orig_mlen) {
>  		status = map->m_flags & EXT4_MAP_UNWRITTEN ?
>  				EXTENT_STATUS_UNWRITTEN : EXTENT_STATUS_WRITTEN;
>  		ext4_es_insert_extent(inode, map->m_lblk, map->m_len,
> -- 
> 2.49.0
> 

[PATCH v1 3/5] ext4: Rename and document EXT4_EX_FILTER to EXT4_EX_QUERY_FILTER

[Ritesh Harjani (IBM)]

Rename EXT4_EX_FILTER to EXT4_EX_QUERY_FILTER to better describe its
purpose as a filter mask used specifically in ext4_map_query_blocks().
Add a comment explaining that this macro is used to filter flags needed
when querying the on-disk extent tree.

We will later use EXT4_EX_QUERY_FILTER mask to add another
EXT4_GET_BLOCKS_QUERY needed to lookup in on-disk extent tree.

Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
---
 fs/ext4/ext4.h  | 7 ++++++-
 fs/ext4/inode.c | 2 +-
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 201afaaa508a..c0489220d3c4 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -757,7 +757,12 @@ enum {
 #define EXT4_EX_NOCACHE				0x40000000
 #define EXT4_EX_FORCE_CACHE			0x20000000
 #define EXT4_EX_NOFAIL				0x10000000
-#define EXT4_EX_FILTER				0x70000000
+/*
+ * ext4_map_query_blocks() uses this filter mask to filter the flags needed to
+ * pass while lookup/querying of on disk extent tree.
+ */
+#define EXT4_EX_QUERY_FILTER	(EXT4_EX_NOCACHE | EXT4_EX_FORCE_CACHE |\
+				 EXT4_EX_NOFAIL)
 
 /*
  * Flags used by ext4_free_blocks
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 459ffc6af1d3..d662ff486a82 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -546,7 +546,7 @@ static int ext4_map_query_blocks(handle_t *handle, struct inode *inode,
 	unsigned int orig_mlen = map->m_len;
 	unsigned int query_flags = flags & EXT4_GET_BLOCKS_QUERY_LAST_IN_LEAF;
 
-	flags &= EXT4_EX_FILTER;
+	flags &= EXT4_EX_QUERY_FILTER;
 	if (ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS))
 		retval = ext4_ext_map_blocks(handle, inode, map,
 					     flags | query_flags);
-- 
2.49.0

[PATCH v1 4/5] ext4: Simplify flags in ext4_map_query_blocks()

[Ritesh Harjani (IBM)]

Now that we have EXT4_EX_QUERY_FILTER mask, let's use that to simplify
the filtering of flags for passing to ext4_ext_map_blocks() in
ext4_map_query_blocks() function. This allows us to kill the query_flags
local variable which is not needed anymore.

Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
---
 fs/ext4/ext4.h  | 3 ++-
 fs/ext4/inode.c | 4 +---
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index c0489220d3c4..18373de980f2 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -762,7 +762,8 @@ enum {
  * pass while lookup/querying of on disk extent tree.
  */
 #define EXT4_EX_QUERY_FILTER	(EXT4_EX_NOCACHE | EXT4_EX_FORCE_CACHE |\
-				 EXT4_EX_NOFAIL)
+				 EXT4_EX_NOFAIL |\
+				 EXT4_GET_BLOCKS_QUERY_LAST_IN_LEAF)
 
 /*
  * Flags used by ext4_free_blocks
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index d662ff486a82..5ddb65d6f8fb 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -544,12 +544,10 @@ static int ext4_map_query_blocks(handle_t *handle, struct inode *inode,
 	unsigned int status;
 	int retval;
 	unsigned int orig_mlen = map->m_len;
-	unsigned int query_flags = flags & EXT4_GET_BLOCKS_QUERY_LAST_IN_LEAF;
 
 	flags &= EXT4_EX_QUERY_FILTER;
 	if (ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS))
-		retval = ext4_ext_map_blocks(handle, inode, map,
-					     flags | query_flags);
+		retval = ext4_ext_map_blocks(handle, inode, map, flags);
 	else
 		retval = ext4_ind_map_blocks(handle, inode, map, flags);
 
-- 
2.49.0

[PATCH v1 5/5] ext4: Add a WARN_ON_ONCE for querying LAST_IN_LEAF instead

[Ritesh Harjani (IBM)]

We added the documentation in ext4_map_blocks() for usage of
EXT4_GET_BLOCKS_QUERY_LAST_IN_LEAF flag. But It's better to add
a WARN_ON_ONCE in case if anyone tries using this flag with CREATE to
avoid a random issue later. Since depth can change with CREATE and it
needs to be re-calculated before using it in there.

Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
---
 fs/ext4/extents.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index 7683558381dc..ea5158703d2d 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -4444,9 +4444,11 @@ int ext4_ext_map_blocks(handle_t *handle, struct inode *inode,
 	 * need to re-calculate the depth as it might have changed due to block
 	 * allocation.
 	 */
-	if (flags & EXT4_GET_BLOCKS_QUERY_LAST_IN_LEAF)
+	if (flags & EXT4_GET_BLOCKS_QUERY_LAST_IN_LEAF) {
+		WARN_ON_ONCE(flags & EXT4_GET_BLOCKS_CREATE);
 		if (!err && ex && (ex == EXT_LAST_EXTENT(path[depth].p_hdr)))
 			map->m_flags |= EXT4_MAP_QUERY_LAST_IN_LEAF;
+	}
 
 	ext4_free_ext_path(path);
 
-- 
2.49.0

Re: [PATCH v1 5/5] ext4: Add a WARN_ON_ONCE for querying LAST_IN_LEAF instead

[Ojaswin Mujoo]

On Mon, May 19, 2025 at 11:49:30PM +0530, Ritesh Harjani (IBM) wrote:
> We added the documentation in ext4_map_blocks() for usage of
> EXT4_GET_BLOCKS_QUERY_LAST_IN_LEAF flag. But It's better to add
> a WARN_ON_ONCE in case if anyone tries using this flag with CREATE to
> avoid a random issue later. Since depth can change with CREATE and it
> needs to be re-calculated before using it in there.
> 
> Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>

Makes sense, feel free to add:

Reviewed-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>

> ---
>  fs/ext4/extents.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index 7683558381dc..ea5158703d2d 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -4444,9 +4444,11 @@ int ext4_ext_map_blocks(handle_t *handle, struct inode *inode,
>  	 * need to re-calculate the depth as it might have changed due to block
>  	 * allocation.
>  	 */
> -	if (flags & EXT4_GET_BLOCKS_QUERY_LAST_IN_LEAF)
> +	if (flags & EXT4_GET_BLOCKS_QUERY_LAST_IN_LEAF) {
> +		WARN_ON_ONCE(flags & EXT4_GET_BLOCKS_CREATE);
>  		if (!err && ex && (ex == EXT_LAST_EXTENT(path[depth].p_hdr)))
>  			map->m_flags |= EXT4_MAP_QUERY_LAST_IN_LEAF;
> +	}
>  
>  	ext4_free_ext_path(path);
>  
> -- 
> 2.49.0
> 

Re: [PATCH v1 0/5] ext4: Minor fixes and improvements for atomic write series

[Theodore Ts'o]

On Mon, 19 May 2025 23:49:25 +0530, Ritesh Harjani (IBM) wrote:
> Some minor fixes and improvements on top of ext4's dev branch [1] for atomic
> write patch series.
> 
> [1]: https://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git/log/?h=dev
> 
> Patch-1: Is the fix needed after rebase on top of extent status cleanup.
> Patch-2: warning fix for cocci
> Patch-{3..5}: Minor improvements / simplifications.
> 
> [...]

Applied, thanks!

[1/5] ext4: Unwritten to written conversion requires EXT4_EX_NOCACHE
      commit: 3be4bb7e71477c0b9708bd6f1975db0ffc72cce4
[2/5] ext4: Simplify last in leaf check in ext4_map_query_blocks
      commit: 797ac3ca24ca1af396e84d16c79c523cfefe1737
[3/5] ext4: Rename and document EXT4_EX_FILTER to EXT4_EX_QUERY_FILTER
      commit: 9597376bdb6e5830448ba40aacd3ebd705fe35cc
[4/5] ext4: Simplify flags in ext4_map_query_blocks()
      commit: 618320daa9673ce2b6adae5ad6fbf16e878ff6c9
[5/5] ext4: Add a WARN_ON_ONCE for querying LAST_IN_LEAF instead
      commit: 7acd1b315cdcc03b11a3aa1f9c9c85d99ddb4f0e

Best regards,
-- 
Theodore Ts'o <tytso@mit.edu>