[PATCH 0/4] ext4: clean up ext4_ext_handle_unwritten_extents()

[PATCH 0/4] ext4: clean up ext4_ext_handle_unwritten_extents()

[Eric Whitney]

Changes made to ext4 over time have resulted in some cruft accumulating
in ext4_ext_handle_unwritten_extents().  This patch series removes
some dead and some redundant code, simplifies and corrects some error
handling, and adds explicit error logging when an unexpected internal
error or file system corruption may have occurred.

Eric Whitney (4):
  ext4: remove dead GET_BLOCKS_ZERO code
  ext4: remove redundant GET_BLOCKS_CONVERT code
  ext4: clean up GET_BLOCKS_PRE_IO error handling
  ext4: clean up ext4_ext_convert_to_initialized() error handling

 fs/ext4/extents.c | 81 ++++++++++++++++++++++++++---------------------
 1 file changed, 45 insertions(+), 36 deletions(-)

-- 
2.20.1

[PATCH 1/4] ext4: remove dead GET_BLOCKS_ZERO code

[Eric Whitney]

There's no call to ext4_map_blocks() in the current ext4 code with a
flags argument that combines EXT4_GET_BLOCKS_CONVERT and
EXT4_GET_BLOCKS_ZERO.  Remove the code that corresponds to this case
from ext4_ext_handle_unwritten_extents().

Signed-off-by: Eric Whitney <enwlinux@gmail.com>
---
 fs/ext4/extents.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index f2b577b315a0..59a90492b9dd 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -3826,14 +3826,6 @@ ext4_ext_handle_unwritten_extents(handle_t *handle, struct inode *inode,
 	}
 	/* IO end_io complete, convert the filled extent to written */
 	if (flags & EXT4_GET_BLOCKS_CONVERT) {
-		if (flags & EXT4_GET_BLOCKS_ZERO) {
-			if (allocated > map->m_len)
-				allocated = map->m_len;
-			err = ext4_issue_zeroout(inode, map->m_lblk, newblock,
-						 allocated);
-			if (err < 0)
-				goto out2;
-		}
 		ret = ext4_convert_unwritten_extents_endio(handle, inode, map,
 							   ppath);
 		if (ret >= 0)
-- 
2.20.1

Re: [PATCH 1/4] ext4: remove dead GET_BLOCKS_ZERO code

[Ritesh Harjani]

On 5/1/20 12:23 AM, Eric Whitney wrote:
> There's no call to ext4_map_blocks() in the current ext4 code with a
> flags argument that combines EXT4_GET_BLOCKS_CONVERT and
> EXT4_GET_BLOCKS_ZERO.  Remove the code that corresponds to this case
> from ext4_ext_handle_unwritten_extents().
> 
> Signed-off-by: Eric Whitney <enwlinux@gmail.com>

As I see it. Yes, this flag was mainly added for DAX handling at two 
places but mostly with below purpose.

Purpose:- Since DAX earlier using PRE_IO flag and then to convert
unwritten to written, it added this extra functionality to zero out.
Since ext4_map_blocks already implements the unwritten to written
functionality, so PRE_IO along with below combination of flags was
removed from DAX path.

Now none of that DAX code path uses below code anyways. So your patch
justifies killing below code snip.


Feel free to add:
Reviewed-by: Ritesh Harjani <riteshh@linux.ibm.com>

> ---
>   fs/ext4/extents.c | 8 --------
>   1 file changed, 8 deletions(-)
> 
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index f2b577b315a0..59a90492b9dd 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -3826,14 +3826,6 @@ ext4_ext_handle_unwritten_extents(handle_t *handle, struct inode *inode,
>   	}
>   	/* IO end_io complete, convert the filled extent to written */
>   	if (flags & EXT4_GET_BLOCKS_CONVERT) {
> -		if (flags & EXT4_GET_BLOCKS_ZERO) {
> -			if (allocated > map->m_len)
> -				allocated = map->m_len;
> -			err = ext4_issue_zeroout(inode, map->m_lblk, newblock,
> -						 allocated);
> -			if (err < 0)
> -				goto out2;
> -		}
>   		ret = ext4_convert_unwritten_extents_endio(handle, inode, map,
>   							   ppath);
>   		if (ret >= 0)
> 

[PATCH 2/4] ext4: remove redundant GET_BLOCKS_CONVERT code

[Eric Whitney]

Remove the redundant code assigning values to ext4_map_blocks components
in ext4_ext_handle_unwritten_extents() for the EXT4_GET_BLOCKS_CONVERT
case, using the code at the function exit instead.  Clean up and reorder
that code to eliminate more redundancy and improve readability.

Signed-off-by: Eric Whitney <enwlinux@gmail.com>
---
 fs/ext4/extents.c | 26 ++++++++------------------
 1 file changed, 8 insertions(+), 18 deletions(-)

diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index 59a90492b9dd..74aad2d77130 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -3826,20 +3826,14 @@ ext4_ext_handle_unwritten_extents(handle_t *handle, struct inode *inode,
 	}
 	/* IO end_io complete, convert the filled extent to written */
 	if (flags & EXT4_GET_BLOCKS_CONVERT) {
-		ret = ext4_convert_unwritten_extents_endio(handle, inode, map,
+		err = ext4_convert_unwritten_extents_endio(handle, inode, map,
 							   ppath);
-		if (ret >= 0)
-			ext4_update_inode_fsync_trans(handle, inode, 1);
-		else
-			err = ret;
-		map->m_flags |= EXT4_MAP_MAPPED;
-		map->m_pblk = newblock;
-		if (allocated > map->m_len)
-			allocated = map->m_len;
-		map->m_len = allocated;
-		goto out2;
+		if (err < 0)
+			goto out2;
+		ext4_update_inode_fsync_trans(handle, inode, 1);
+		goto map_out;
 	}
-	/* buffered IO case */
+	/* buffered IO cases */
 	/*
 	 * repeat fallocate creation request
 	 * we already have an unwritten extent
@@ -3873,18 +3867,14 @@ ext4_ext_handle_unwritten_extents(handle_t *handle, struct inode *inode,
 	} else
 		allocated = ret;
 	map->m_flags |= EXT4_MAP_NEW;
-	if (allocated > map->m_len)
-		allocated = map->m_len;
-	map->m_len = allocated;
-
 map_out:
 	map->m_flags |= EXT4_MAP_MAPPED;
 out1:
+	map->m_pblk = newblock;
 	if (allocated > map->m_len)
 		allocated = map->m_len;
-	ext4_ext_show_leaf(inode, path);
-	map->m_pblk = newblock;
 	map->m_len = allocated;
+	ext4_ext_show_leaf(inode, path);
 out2:
 	return err ? err : allocated;
 }
-- 
2.20.1

[PATCH 3/4] ext4: clean up GET_BLOCKS_PRE_IO error handling

[Eric Whitney]

If the call to ext4_split_convert_extents() fails in the
EXT4_GET_BLOCKS_PRE_IO case within ext4_ext_handle_unwritten_extents(),
error out through the exit point at function end rather than jumping
through an intermediate point.  Fix the error handling in the event
ext4_split_convert_extents() returns 0, which it shouldn't do when
splitting an existing extent.  The current code returns the passed in
value of allocated (which is likely non-zero) while failing to set
m_flags, m_pblk, and m_len.

Signed-off-by: Eric Whitney <enwlinux@gmail.com>
---
 fs/ext4/extents.c | 26 ++++++++++++++++++++------
 1 file changed, 20 insertions(+), 6 deletions(-)

diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index 74aad2d77130..fc99f6c357cd 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -3815,12 +3815,25 @@ ext4_ext_handle_unwritten_extents(handle_t *handle, struct inode *inode,
 	trace_ext4_ext_handle_unwritten_extents(inode, map, flags,
 						    allocated, newblock);
 
-	/* get_block() before submit the IO, split the extent */
+	/* get_block() before submitting IO, split the extent */
 	if (flags & EXT4_GET_BLOCKS_PRE_IO) {
 		ret = ext4_split_convert_extents(handle, inode, map, ppath,
 					 flags | EXT4_GET_BLOCKS_CONVERT);
-		if (ret <= 0)
-			goto out;
+		if (ret < 0) {
+			err = ret;
+			goto out2;
+		}
+		/*
+		 * shouldn't get a 0 return when splitting an extent unless
+		 * m_len is 0 (bug) or extent has been corrupted
+		 */
+		if (unlikely(ret == 0)) {
+			EXT4_ERROR_INODE(inode,
+					 "unexpected ret == 0, m_len = %u",
+					 map->m_len);
+			err = -EFSCORRUPTED;
+			goto out2;
+		}
 		map->m_flags |= EXT4_MAP_UNWRITTEN;
 		goto out;
 	}
@@ -3860,12 +3873,13 @@ ext4_ext_handle_unwritten_extents(handle_t *handle, struct inode *inode,
 	ret = ext4_ext_convert_to_initialized(handle, inode, map, ppath, flags);
 	if (ret >= 0)
 		ext4_update_inode_fsync_trans(handle, inode, 1);
-out:
+
 	if (ret <= 0) {
 		err = ret;
 		goto out2;
-	} else
-		allocated = ret;
+	}
+out:
+	allocated = ret;
 	map->m_flags |= EXT4_MAP_NEW;
 map_out:
 	map->m_flags |= EXT4_MAP_MAPPED;
-- 
2.20.1

[PATCH 4/4] ext4: clean up ext4_ext_convert_to_initialized() error handling

[Eric Whitney]

If ext4_ext_convert_to_initialized() fails when called within
ext4_ext_handle_unwritten_extents(), immediately error out through the
exit point at function end.  Fix the error handling in the event
ext4_ext_convert_to_initialized() returns 0, which it shouldn't do when
converting an existing extent.  The current code returns the passed in
value of allocated (which is likely non-zero) while failing to set
m_flags, m_pblk, and m_len.

Signed-off-by: Eric Whitney <enwlinux@gmail.com>
---
 fs/ext4/extents.c | 23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)

diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index fc99f6c357cd..202787977e3d 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -3869,15 +3869,28 @@ ext4_ext_handle_unwritten_extents(handle_t *handle, struct inode *inode,
 		goto out1;
 	}
 
-	/* buffered write, writepage time, convert*/
+	/*
+	 * Default case when (flags & EXT4_GET_BLOCKS_CREATE) == 1.
+	 * For buffered writes, at writepage time, etc.  Convert a
+	 * discovered unwritten extent to written.
+	 */
 	ret = ext4_ext_convert_to_initialized(handle, inode, map, ppath, flags);
-	if (ret >= 0)
-		ext4_update_inode_fsync_trans(handle, inode, 1);
-
-	if (ret <= 0) {
+	if (ret < 0) {
 		err = ret;
 		goto out2;
 	}
+	ext4_update_inode_fsync_trans(handle, inode, 1);
+	/*
+	 * shouldn't get a 0 return when converting an unwritten extent
+	 * unless m_len is 0 (bug) or extent has been corrupted
+	 */
+	if (unlikely(ret == 0)) {
+		EXT4_ERROR_INODE(inode, "unexpected ret == 0, m_len = %u",
+				 map->m_len);
+		err = -EFSCORRUPTED;
+		goto out2;
+	}
+
 out:
 	allocated = ret;
 	map->m_flags |= EXT4_MAP_NEW;
-- 
2.20.1

Re: [PATCH 0/4] ext4: clean up ext4_ext_handle_unwritten_extents()

[Theodore Y. Ts'o]

On Thu, Apr 30, 2020 at 02:53:16PM -0400, Eric Whitney wrote:
> Changes made to ext4 over time have resulted in some cruft accumulating
> in ext4_ext_handle_unwritten_extents().  This patch series removes
> some dead and some redundant code, simplifies and corrects some error
> handling, and adds explicit error logging when an unexpected internal
> error or file system corruption may have occurred.
> 
> Eric Whitney (4):
>   ext4: remove dead GET_BLOCKS_ZERO code
>   ext4: remove redundant GET_BLOCKS_CONVERT code
>   ext4: clean up GET_BLOCKS_PRE_IO error handling
>   ext4: clean up ext4_ext_convert_to_initialized() error handling
> 
>  fs/ext4/extents.c | 81 ++++++++++++++++++++++++++---------------------
>  1 file changed, 45 insertions(+), 36 deletions(-)

Thanks, I've applied this patch series.

						- Ted