linux-ext4 - Re: [PATCH] ext4: clean up ext4_ext_insert_extent() call in ext4_ext_map

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20200312094652.51748A405C@b06wcsmtp001.portsmouth.uk.ibm.com>
Date:   Thu, 12 Mar 2020 15:16:51 +0530
From:   Ritesh Harjani <riteshh@...ux.ibm.com>
To:     Eric Whitney <enwlinux@...il.com>, linux-ext4@...r.kernel.org
Cc:     tytso@....edu
Subject: Re: [PATCH] ext4: clean up ext4_ext_insert_extent() call in
 ext4_ext_map_blocks()



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 <enwlinux@...il.com>


Nice cleanup.

Reviewed-by: Ritesh Harjani <riteshh@...ux.ibm.com>


> ---
>   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;
>   	}
>