Now that the eofblocks code has been removed, we don't need to assign
0 to err before calling ext4_ext_insert_extent() since it will assign
a return value to ret anyway. The variable free_on_err can be
eliminated and replaced by a reference to allocated_clusters which
clearly conveys the idea that newly allocated blocks should be freed
when recovering from an extent insertion failure. The error handling
code itself should be restructured so that it errors out immediately on
an insertion failure in the case where no new blocks have been allocated
(bigalloc) rather than proceeding further into the mapping code. The
initializer for fb_flags can also be rearranged for improved
readability. Finally, insert a missing space in nearby code.
No known bugs are addressed by this patch - it's simply a cleanup.
Signed-off-by: Eric Whitney <[email protected]>
---
fs/ext4/extents.c | 30 +++++++++++++++++-------------
1 file changed, 17 insertions(+), 13 deletions(-)
diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index bc96529d1509..c2743d024c11 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -4176,7 +4176,7 @@ int ext4_ext_map_blocks(handle_t *handle, struct inode *inode,
struct ext4_extent newex, *ex, *ex2;
struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb);
ext4_fsblk_t newblock = 0;
- int free_on_err = 0, err = 0, depth, ret;
+ int err = 0, depth, ret;
unsigned int allocated = 0, offset = 0;
unsigned int allocated_clusters = 0;
struct ext4_allocation_request ar;
@@ -4374,7 +4374,6 @@ int ext4_ext_map_blocks(handle_t *handle, struct inode *inode,
goto out2;
ext_debug("allocate new block: goal %llu, found %llu/%u\n",
ar.goal, newblock, allocated);
- free_on_err = 1;
allocated_clusters = ar.len;
ar.len = EXT4_C2B(sbi, ar.len) - offset;
if (ar.len > allocated)
@@ -4385,23 +4384,28 @@ int ext4_ext_map_blocks(handle_t *handle, struct inode *inode,
ext4_ext_store_pblock(&newex, newblock + offset);
newex.ee_len = cpu_to_le16(ar.len);
/* Mark unwritten */
- if (flags & EXT4_GET_BLOCKS_UNWRIT_EXT){
+ if (flags & EXT4_GET_BLOCKS_UNWRIT_EXT) {
ext4_ext_mark_unwritten(&newex);
map->m_flags |= EXT4_MAP_UNWRITTEN;
}
- err = 0;
err = ext4_ext_insert_extent(handle, inode, &path, &newex, flags);
+ if (err) {
+ if (allocated_clusters) {
+ int fb_flags = 0;
- if (err && free_on_err) {
- int fb_flags = flags & EXT4_GET_BLOCKS_DELALLOC_RESERVE ?
- EXT4_FREE_BLOCKS_NO_QUOT_UPDATE : 0;
- /* free data blocks we just allocated */
- /* not a good idea to call discard here directly,
- * but otherwise we'd need to call it every free() */
- ext4_discard_preallocations(inode);
- ext4_free_blocks(handle, inode, NULL, newblock,
- EXT4_C2B(sbi, allocated_clusters), fb_flags);
+ /*
+ * free data blocks we just allocated.
+ * not a good idea to call discard here directly,
+ * but otherwise we'd need to call it every free().
+ */
+ ext4_discard_preallocations(inode);
+ if (flags & EXT4_GET_BLOCKS_DELALLOC_RESERVE)
+ fb_flags = EXT4_FREE_BLOCKS_NO_QUOT_UPDATE;
+ ext4_free_blocks(handle, inode, NULL, newblock,
+ EXT4_C2B(sbi, allocated_clusters),
+ fb_flags);
+ }
goto out2;
}
--
2.11.0
On 3/12/20 2:20 AM, Eric Whitney wrote:
> Now that the eofblocks code has been removed, we don't need to assign
> 0 to err before calling ext4_ext_insert_extent() since it will assign
> a return value to ret anyway. The variable free_on_err can be
> eliminated and replaced by a reference to allocated_clusters which
> clearly conveys the idea that newly allocated blocks should be freed
> when recovering from an extent insertion failure. The error handling
> code itself should be restructured so that it errors out immediately on
> an insertion failure in the case where no new blocks have been allocated
> (bigalloc) rather than proceeding further into the mapping code. The
Agreed.
> initializer for fb_flags can also be rearranged for improved
> readability. Finally, insert a missing space in nearby code.
>
> No known bugs are addressed by this patch - it's simply a cleanup.
>
> Signed-off-by: Eric Whitney <[email protected]>
Nice cleanup.
Reviewed-by: Ritesh Harjani <[email protected]>
> ---
> fs/ext4/extents.c | 30 +++++++++++++++++-------------
> 1 file changed, 17 insertions(+), 13 deletions(-)
>
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index bc96529d1509..c2743d024c11 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -4176,7 +4176,7 @@ int ext4_ext_map_blocks(handle_t *handle, struct inode *inode,
> struct ext4_extent newex, *ex, *ex2;
> struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb);
> ext4_fsblk_t newblock = 0;
> - int free_on_err = 0, err = 0, depth, ret;
> + int err = 0, depth, ret;
> unsigned int allocated = 0, offset = 0;
> unsigned int allocated_clusters = 0;
> struct ext4_allocation_request ar;
> @@ -4374,7 +4374,6 @@ int ext4_ext_map_blocks(handle_t *handle, struct inode *inode,
> goto out2;
> ext_debug("allocate new block: goal %llu, found %llu/%u\n",
> ar.goal, newblock, allocated);
> - free_on_err = 1;
> allocated_clusters = ar.len;
> ar.len = EXT4_C2B(sbi, ar.len) - offset;
> if (ar.len > allocated)
> @@ -4385,23 +4384,28 @@ int ext4_ext_map_blocks(handle_t *handle, struct inode *inode,
> ext4_ext_store_pblock(&newex, newblock + offset);
> newex.ee_len = cpu_to_le16(ar.len);
> /* Mark unwritten */
> - if (flags & EXT4_GET_BLOCKS_UNWRIT_EXT){
> + if (flags & EXT4_GET_BLOCKS_UNWRIT_EXT) {
> ext4_ext_mark_unwritten(&newex);
> map->m_flags |= EXT4_MAP_UNWRITTEN;
> }
>
> - err = 0;
> err = ext4_ext_insert_extent(handle, inode, &path, &newex, flags);
> + if (err) {
> + if (allocated_clusters) {
> + int fb_flags = 0;
>
> - if (err && free_on_err) {
> - int fb_flags = flags & EXT4_GET_BLOCKS_DELALLOC_RESERVE ?
> - EXT4_FREE_BLOCKS_NO_QUOT_UPDATE : 0;
> - /* free data blocks we just allocated */
> - /* not a good idea to call discard here directly,
> - * but otherwise we'd need to call it every free() */
> - ext4_discard_preallocations(inode);
> - ext4_free_blocks(handle, inode, NULL, newblock,
> - EXT4_C2B(sbi, allocated_clusters), fb_flags);
> + /*
> + * free data blocks we just allocated.
> + * not a good idea to call discard here directly,
> + * but otherwise we'd need to call it every free().
> + */
> + ext4_discard_preallocations(inode);
> + if (flags & EXT4_GET_BLOCKS_DELALLOC_RESERVE)
> + fb_flags = EXT4_FREE_BLOCKS_NO_QUOT_UPDATE;
> + ext4_free_blocks(handle, inode, NULL, newblock,
> + EXT4_C2B(sbi, allocated_clusters),
> + fb_flags);
> + }
> goto out2;
> }
>
On Wed, Mar 11, 2020 at 04:50:33PM -0400, Eric Whitney wrote:
> Now that the eofblocks code has been removed, we don't need to assign
> 0 to err before calling ext4_ext_insert_extent() since it will assign
> a return value to ret anyway. The variable free_on_err can be
> eliminated and replaced by a reference to allocated_clusters which
> clearly conveys the idea that newly allocated blocks should be freed
> when recovering from an extent insertion failure. The error handling
> code itself should be restructured so that it errors out immediately on
> an insertion failure in the case where no new blocks have been allocated
> (bigalloc) rather than proceeding further into the mapping code. The
> initializer for fb_flags can also be rearranged for improved
> readability. Finally, insert a missing space in nearby code.
>
> No known bugs are addressed by this patch - it's simply a cleanup.
>
> Signed-off-by: Eric Whitney <[email protected]>
Applied, thanks.
- Ted