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 PHC | |
Open Source and information security mailing list archives
| ||
|
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