| 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
| ||
|
Message-Id: <20200512113214.309FC4C046@d06av22.portsmouth.uk.ibm.com>
Date: Tue, 12 May 2020 17:02:13 +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: rework map struct instantiation in
ext4_ext_map_blocks()
On 5/10/20 9:28 PM, Eric Whitney wrote:
> The path performing block allocations in ext4_ext_map_blocks() contains
> code trimming the length of a new extent that is repeated later
> in the function. This code is both redundant and unnecessary as the
> exact length of the new extent has already been calculated. Rewrite the
> instantiation of the map struct in this case to use the available
> values, avoiding the overhead of unnecessary conversions and improving
> clarity. Add another map struct instantiation tailored specifically to
> the separate case for an existing written extent. Remove an old comment
> that no longer appears applicable to the current code.
>
> Signed-off-by: Eric Whitney <enwlinux@...il.com>
Yes, the previous code around looks to be confusing.
One thing which I also checked was that, when we insert this new
extent into the tree (via ext4_ext_insert_extent()) and even if this
extent could be merged with a nearby extent, the length or pblock of
this extent is not modified for the caller.
So again doing such below calculations (line 4250 - 4254) were redundant
and it's better that this patch got rid of it. This patch hence looks
logically correct to me.
4250 /* previous routine could use block we allocated */
4251 newblock = ext4_ext_pblock(&newex);
4252 allocated = ext4_ext_get_actual_len(&newex);
4253 if (allocated > map->m_len)
4254 allocated = map->m_len;
Feel free to add:
Reviewed-by: Ritesh Harjani <riteshh@...ux.ibm.com>
> ---
> fs/ext4/extents.c | 50 +++++++++++++++++++++++------------------------
> 1 file changed, 25 insertions(+), 25 deletions(-)
>
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index f2b577b315a0..c01a204ce60b 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -4024,7 +4024,7 @@ int ext4_ext_map_blocks(handle_t *handle, struct inode *inode,
> struct ext4_ext_path *path = NULL;
> struct ext4_extent newex, *ex, *ex2;
> struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb);
> - ext4_fsblk_t newblock = 0;
> + ext4_fsblk_t newblock = 0, pblk;
> int err = 0, depth, ret;
> unsigned int allocated = 0, offset = 0;
> unsigned int allocated_clusters = 0;
> @@ -4040,7 +4040,7 @@ int ext4_ext_map_blocks(handle_t *handle, struct inode *inode,
> if (IS_ERR(path)) {
> err = PTR_ERR(path);
> path = NULL;
> - goto out2;
> + goto out;
> }
>
> depth = ext_depth(inode);
> @@ -4056,7 +4056,7 @@ int ext4_ext_map_blocks(handle_t *handle, struct inode *inode,
> (unsigned long) map->m_lblk, depth,
> path[depth].p_block);
> err = -EFSCORRUPTED;
> - goto out2;
> + goto out;
> }
>
> ex = path[depth].p_ext;
> @@ -4090,8 +4090,14 @@ int ext4_ext_map_blocks(handle_t *handle, struct inode *inode,
> (flags & EXT4_GET_BLOCKS_CONVERT_UNWRITTEN)) {
> err = convert_initialized_extent(handle,
> inode, map, &path, &allocated);
> - goto out2;
> + goto out;
> } else if (!ext4_ext_is_unwritten(ex)) {
> + map->m_flags |= EXT4_MAP_MAPPED;
> + map->m_pblk = newblock;
> + if (allocated > map->m_len)
> + allocated = map->m_len;
> + map->m_len = allocated;
> + ext4_ext_show_leaf(inode, path);
> goto out;
> }
>
> @@ -4102,7 +4108,7 @@ int ext4_ext_map_blocks(handle_t *handle, struct inode *inode,
> err = ret;
> else
> allocated = ret;
> - goto out2;
> + goto out;
> }
> }
>
> @@ -4127,7 +4133,7 @@ int ext4_ext_map_blocks(handle_t *handle, struct inode *inode,
> map->m_pblk = 0;
> map->m_len = min_t(unsigned int, map->m_len, hole_len);
>
> - goto out2;
> + goto out;
> }
>
> /*
> @@ -4151,12 +4157,12 @@ int ext4_ext_map_blocks(handle_t *handle, struct inode *inode,
> ar.lleft = map->m_lblk;
> err = ext4_ext_search_left(inode, path, &ar.lleft, &ar.pleft);
> if (err)
> - goto out2;
> + goto out;
> ar.lright = map->m_lblk;
> ex2 = NULL;
> err = ext4_ext_search_right(inode, path, &ar.lright, &ar.pright, &ex2);
> if (err)
> - goto out2;
> + goto out;
>
> /* Check if the extent after searching to the right implies a
> * cluster we can use. */
> @@ -4217,7 +4223,7 @@ int ext4_ext_map_blocks(handle_t *handle, struct inode *inode,
> ar.flags |= EXT4_MB_USE_RESERVED;
> newblock = ext4_mb_new_blocks(handle, &ar, &err);
> if (!newblock)
> - goto out2;
> + goto out;
> ext_debug("allocate new block: goal %llu, found %llu/%u\n",
> ar.goal, newblock, allocated);
> allocated_clusters = ar.len;
> @@ -4227,7 +4233,8 @@ int ext4_ext_map_blocks(handle_t *handle, struct inode *inode,
>
> got_allocated_blocks:
> /* try to insert new extent into found leaf and return */
> - ext4_ext_store_pblock(&newex, newblock + offset);
> + pblk = newblock + offset;
> + ext4_ext_store_pblock(&newex, pblk);
> newex.ee_len = cpu_to_le16(ar.len);
> /* Mark unwritten */
> if (flags & EXT4_GET_BLOCKS_UNWRIT_EXT) {
> @@ -4252,16 +4259,9 @@ int ext4_ext_map_blocks(handle_t *handle, struct inode *inode,
> EXT4_C2B(sbi, allocated_clusters),
> fb_flags);
> }
> - goto out2;
> + goto out;
> }
>
> - /* previous routine could use block we allocated */
> - newblock = ext4_ext_pblock(&newex);
> - allocated = ext4_ext_get_actual_len(&newex);
> - if (allocated > map->m_len)
> - allocated = map->m_len;
> - map->m_flags |= EXT4_MAP_NEW;
> -
> /*
> * Reduce the reserved cluster count to reflect successful deferred
> * allocation of delayed allocated clusters or direct allocation of
> @@ -4307,14 +4307,14 @@ int ext4_ext_map_blocks(handle_t *handle, struct inode *inode,
> ext4_update_inode_fsync_trans(handle, inode, 1);
> else
> ext4_update_inode_fsync_trans(handle, inode, 0);
> -out:
> - if (allocated > map->m_len)
> - allocated = map->m_len;
> +
> + map->m_flags |= (EXT4_MAP_NEW | EXT4_MAP_MAPPED);
> + map->m_pblk = pblk;
> + map->m_len = ar.len;
> + allocated = map->m_len;
> ext4_ext_show_leaf(inode, path);
> - map->m_flags |= EXT4_MAP_MAPPED;
> - map->m_pblk = newblock;
> - map->m_len = allocated;
> -out2:
> +
> +out:
> ext4_ext_drop_refs(path);
> kfree(path);
>
Powered by blists - more mailing lists