All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] ext4: rework map struct instantiation in ext4_ext_map_blocks()
@ 2020-05-10 15:58 Eric Whitney
  2020-05-12 11:32 ` Ritesh Harjani
  0 siblings, 1 reply; 2+ messages in thread
From: Eric Whitney @ 2020-05-10 15:58 UTC (permalink / raw)
  To: linux-ext4; +Cc: tytso, Eric Whitney

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@gmail.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);
 
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 2+ messages in thread

* Re: [PATCH] ext4: rework map struct instantiation in ext4_ext_map_blocks()
  2020-05-10 15:58 [PATCH] ext4: rework map struct instantiation in ext4_ext_map_blocks() Eric Whitney
@ 2020-05-12 11:32 ` Ritesh Harjani
  0 siblings, 0 replies; 2+ messages in thread
From: Ritesh Harjani @ 2020-05-12 11:32 UTC (permalink / raw)
  To: Eric Whitney, linux-ext4; +Cc: tytso



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@gmail.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@linux.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);
> 

^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, other threads:[~2020-05-12 11:33 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-10 15:58 [PATCH] ext4: rework map struct instantiation in ext4_ext_map_blocks() Eric Whitney
2020-05-12 11:32 ` Ritesh Harjani

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.