ext4: clean up ext4_ext_handle_unwritten_extents()
cover ext4: clean up ext4_ext_handle_unwritten_extents()
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(-)
Signature · 1 line
2.20.1
PATCH 1/4 ext4: remove dead GET_BLOCKS_ZERO code
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().
--- fs/ext4/extents.c | 8 -------- 1 file changed, 8 deletions(-)
fs/ext4/extents.c +0 -8
@@ -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 )
Signature · 1 line
2.20.1
Re: [PATCH 1/4] ext4: remove dead GET_BLOCKS_ZERO code
Quoted 6 lines · Eric Whitney
There's no call to
ext4_map_blocks()in the current ext4 code with a flags argument that combinesEXT4_GET_BLOCKS_CONVERTandEXT4_GET_BLOCKS_ZERO. Remove the code that corresponds to this case fromext4_ext_handle_unwritten_extents().Signed-off-by Eric Whitney <[email protected]>
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:
Quoted 24 lines
--- fs/ext4/extents.c | 8 -------- 1 file changed, 8 deletions(-)fs/ext4/extents.c +0
-8@@ -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
In reply to Eric WhitneyRemove 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.
--- fs/ext4/extents.c | 26 ++++++++------------------ 1 file changed, 8 insertions(+), 18 deletions(-)
fs/ext4/extents.c +8 -18
@@ -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; }
Signature · 1 line
2.20.1
PATCH 3/4 ext4: clean up GET_BLOCKS_PRE_IO error handling
In reply to Eric WhitneyIf 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.
--- fs/ext4/extents.c | 26 ++++++++++++++++++++------ 1 file changed, 20 insertions(+), 6 deletions(-)
fs/ext4/extents.c +20 -6
@@ -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 ;
Signature · 1 line
2.20.1
PATCH 4/4 ext4: clean up ext4_ext_convert_to_initialized() error handling
In reply to Eric WhitneyIf 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.
--- fs/ext4/extents.c | 23 ++++++++++++++++++----- 1 file changed, 18 insertions(+), 5 deletions(-)
fs/ext4/extents.c +18 -5
@@ -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 ;
Signature · 1 line
2.20.1
Theodore Y. Ts'o
maintainerQuoted 6 lines · 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.
Quoted 8 lines
Eric Whitney (4):
ext4: remove deadGET_BLOCKS_ZEROcode
ext4: remove redundantGET_BLOCKS_CONVERTcode
ext4: clean upGET_BLOCKS_PRE_IOerror handling
ext4: clean upext4_ext_convert_to_initialized()error handlingfs/ext4/extents.c | 81 ++++++++++++++++++++++++++--------------------- 1 file changed, 45 insertions(+), 36 deletions(-)
Thanks, I've applied this patch series.
- Ted