ext4: Minor fixes and improvements for atomic write series

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

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().

---
 fs/ext4/extents.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 8b834e13d306..7683558381dc 100644
@@ -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

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 | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

index ce0632094c50..459ffc6af1d3 100644
@@ -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

Makes sense, feel free to add:

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.

---
 fs/ext4/ext4.h  | 7 ++++++-
 fs/ext4/inode.c | 2 +-
 2 files changed, 7 insertions(+), 2 deletions(-)

index 201afaaa508a..c0489220d3c4 100644
@@ -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

index 459ffc6af1d3..d662ff486a82 100644
@@ -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

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.

---
 fs/ext4/ext4.h  | 3 ++-
 fs/ext4/inode.c | 4 +---
 2 files changed, 3 insertions(+), 4 deletions(-)

index c0489220d3c4..18373de980f2 100644
@@ -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

index d662ff486a82..5ddb65d6f8fb 100644
@@ -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

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.

---
 fs/ext4/extents.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

index 7683558381dc..ea5158703d2d 100644
@@ -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

Makes sense, feel free to add:

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 <[email protected]>