[v2,2/2] ext4: cleanup bh release code in ext4_ind_remove_space()
diff mbox series

Message ID 1552633813-42832-3-git-send-email-yi.zhang@huawei.com
State New
Headers show
Series
  • ext4: fix buffer references leak problem
Related show

Commit Message

zhangyi (F) March 15, 2019, 7:10 a.m. UTC
Currently, we are releasing the indirect buffer where we are done with
it in ext4_ind_remove_space(), so we can see the brelse() and
BUFFER_TRACE() everywhere. It seems fragile and hard to read, and we
may probably forget to release the buffer some day. This patch do some
cleanup stuff, put all the releasing code together to the end of this
function.

Signed-off-by: zhangyi (F) <yi.zhang@huawei.com>
---
 fs/ext4/indirect.c | 47 ++++++++++++++++++++++-------------------------
 1 file changed, 22 insertions(+), 25 deletions(-)

Comments

Jan Kara March 18, 2019, 11:10 a.m. UTC | #1
On Fri 15-03-19 15:10:13, zhangyi (F) wrote:
> Currently, we are releasing the indirect buffer where we are done with
> it in ext4_ind_remove_space(), so we can see the brelse() and
> BUFFER_TRACE() everywhere. It seems fragile and hard to read, and we
> may probably forget to release the buffer some day. This patch do some
> cleanup stuff, put all the releasing code together to the end of this
> function.
> 
> Signed-off-by: zhangyi (F) <yi.zhang@huawei.com>

OK, now when the cleanup is separate, I actually like it. So feel free to
add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/ext4/indirect.c | 47 ++++++++++++++++++++++-------------------------
>  1 file changed, 22 insertions(+), 25 deletions(-)
> 
> diff --git a/fs/ext4/indirect.c b/fs/ext4/indirect.c
> index 9e96a0b..e1801b2 100644
> --- a/fs/ext4/indirect.c
> +++ b/fs/ext4/indirect.c
> @@ -1219,6 +1219,7 @@ int ext4_ind_remove_space(handle_t *handle, struct inode *inode,
>  	ext4_lblk_t offsets[4], offsets2[4];
>  	Indirect chain[4], chain2[4];
>  	Indirect *partial, *partial2;
> +	Indirect *p = NULL, *p2 = NULL;
>  	ext4_lblk_t max_block;
>  	__le32 nr = 0, nr2 = 0;
>  	int n = 0, n2 = 0;
> @@ -1260,7 +1261,7 @@ int ext4_ind_remove_space(handle_t *handle, struct inode *inode,
>  		}
>  
>  
> -		partial = ext4_find_shared(inode, n, offsets, chain, &nr);
> +		partial = p = ext4_find_shared(inode, n, offsets, chain, &nr);
>  		if (nr) {
>  			if (partial == chain) {
>  				/* Shared branch grows from the inode */
> @@ -1285,13 +1286,11 @@ int ext4_ind_remove_space(handle_t *handle, struct inode *inode,
>  				partial->p + 1,
>  				(__le32 *)partial->bh->b_data+addr_per_block,
>  				(chain+n-1) - partial);
> -			BUFFER_TRACE(partial->bh, "call brelse");
> -			brelse(partial->bh);
>  			partial--;
>  		}
>  
>  end_range:
> -		partial2 = ext4_find_shared(inode, n2, offsets2, chain2, &nr2);
> +		partial2 = p2 = ext4_find_shared(inode, n2, offsets2, chain2, &nr2);
>  		if (nr2) {
>  			if (partial2 == chain2) {
>  				/*
> @@ -1321,16 +1320,14 @@ int ext4_ind_remove_space(handle_t *handle, struct inode *inode,
>  					   (__le32 *)partial2->bh->b_data,
>  					   partial2->p,
>  					   (chain2+n2-1) - partial2);
> -			BUFFER_TRACE(partial2->bh, "call brelse");
> -			brelse(partial2->bh);
>  			partial2--;
>  		}
>  		goto do_indirects;
>  	}
>  
>  	/* Punch happened within the same level (n == n2) */
> -	partial = ext4_find_shared(inode, n, offsets, chain, &nr);
> -	partial2 = ext4_find_shared(inode, n2, offsets2, chain2, &nr2);
> +	partial = p = ext4_find_shared(inode, n, offsets, chain, &nr);
> +	partial2 = p2 = ext4_find_shared(inode, n2, offsets2, chain2, &nr2);
>  
>  	/* Free top, but only if partial2 isn't its subtree. */
>  	if (nr) {
> @@ -1387,15 +1384,7 @@ int ext4_ind_remove_space(handle_t *handle, struct inode *inode,
>  					   partial->p + 1,
>  					   partial2->p,
>  					   (chain+n-1) - partial);
> -			while (partial > chain) {
> -				BUFFER_TRACE(partial->bh, "call brelse");
> -				brelse(partial->bh);
> -			}
> -			while (partial2 > chain2) {
> -				BUFFER_TRACE(partial2->bh, "call brelse");
> -				brelse(partial2->bh);
> -			}
> -			return 0;
> +			goto cleanup;
>  		}
>  
>  		/*
> @@ -1410,8 +1399,6 @@ int ext4_ind_remove_space(handle_t *handle, struct inode *inode,
>  					   partial->p + 1,
>  					   (__le32 *)partial->bh->b_data+addr_per_block,
>  					   (chain+n-1) - partial);
> -			BUFFER_TRACE(partial->bh, "call brelse");
> -			brelse(partial->bh);
>  			partial--;
>  		}
>  		if (partial2 > chain2 && depth2 <= depth) {
> @@ -1419,11 +1406,21 @@ int ext4_ind_remove_space(handle_t *handle, struct inode *inode,
>  					   (__le32 *)partial2->bh->b_data,
>  					   partial2->p,
>  					   (chain2+n2-1) - partial2);
> -			BUFFER_TRACE(partial2->bh, "call brelse");
> -			brelse(partial2->bh);
>  			partial2--;
>  		}
>  	}
> +
> +cleanup:
> +	while (p && p > chain) {
> +		BUFFER_TRACE(p->bh, "call brelse");
> +		brelse(p->bh);
> +		p--;
> +	}
> +	while (p2 && p2 > chain2) {
> +		BUFFER_TRACE(p2->bh, "call brelse");
> +		brelse(p2->bh);
> +		p2--;
> +	}
>  	return 0;
>  
>  do_indirects:
> @@ -1431,7 +1428,7 @@ int ext4_ind_remove_space(handle_t *handle, struct inode *inode,
>  	switch (offsets[0]) {
>  	default:
>  		if (++n >= n2)
> -			return 0;
> +			break;
>  		nr = i_data[EXT4_IND_BLOCK];
>  		if (nr) {
>  			ext4_free_branches(handle, inode, NULL, &nr, &nr+1, 1);
> @@ -1439,7 +1436,7 @@ int ext4_ind_remove_space(handle_t *handle, struct inode *inode,
>  		}
>  	case EXT4_IND_BLOCK:
>  		if (++n >= n2)
> -			return 0;
> +			break;
>  		nr = i_data[EXT4_DIND_BLOCK];
>  		if (nr) {
>  			ext4_free_branches(handle, inode, NULL, &nr, &nr+1, 2);
> @@ -1447,7 +1444,7 @@ int ext4_ind_remove_space(handle_t *handle, struct inode *inode,
>  		}
>  	case EXT4_DIND_BLOCK:
>  		if (++n >= n2)
> -			return 0;
> +			break;
>  		nr = i_data[EXT4_TIND_BLOCK];
>  		if (nr) {
>  			ext4_free_branches(handle, inode, NULL, &nr, &nr+1, 3);
> @@ -1456,5 +1453,5 @@ int ext4_ind_remove_space(handle_t *handle, struct inode *inode,
>  	case EXT4_TIND_BLOCK:
>  		;
>  	}
> -	return 0;
> +	goto cleanup;
>  }
> -- 
> 2.7.4
>
Theodore Y. Ts'o March 23, 2019, 3:58 p.m. UTC | #2
On Fri, Mar 15, 2019 at 03:10:13PM +0800, zhangyi (F) wrote:
> Currently, we are releasing the indirect buffer where we are done with
> it in ext4_ind_remove_space(), so we can see the brelse() and
> BUFFER_TRACE() everywhere. It seems fragile and hard to read, and we
> may probably forget to release the buffer some day. This patch do some
> cleanup stuff, put all the releasing code together to the end of this
> function.
> 
> Signed-off-by: zhangyi (F) <yi.zhang@huawei.com>

Thanks, applied.

					- Ted

Patch
diff mbox series

diff --git a/fs/ext4/indirect.c b/fs/ext4/indirect.c
index 9e96a0b..e1801b2 100644
--- a/fs/ext4/indirect.c
+++ b/fs/ext4/indirect.c
@@ -1219,6 +1219,7 @@  int ext4_ind_remove_space(handle_t *handle, struct inode *inode,
 	ext4_lblk_t offsets[4], offsets2[4];
 	Indirect chain[4], chain2[4];
 	Indirect *partial, *partial2;
+	Indirect *p = NULL, *p2 = NULL;
 	ext4_lblk_t max_block;
 	__le32 nr = 0, nr2 = 0;
 	int n = 0, n2 = 0;
@@ -1260,7 +1261,7 @@  int ext4_ind_remove_space(handle_t *handle, struct inode *inode,
 		}
 
 
-		partial = ext4_find_shared(inode, n, offsets, chain, &nr);
+		partial = p = ext4_find_shared(inode, n, offsets, chain, &nr);
 		if (nr) {
 			if (partial == chain) {
 				/* Shared branch grows from the inode */
@@ -1285,13 +1286,11 @@  int ext4_ind_remove_space(handle_t *handle, struct inode *inode,
 				partial->p + 1,
 				(__le32 *)partial->bh->b_data+addr_per_block,
 				(chain+n-1) - partial);
-			BUFFER_TRACE(partial->bh, "call brelse");
-			brelse(partial->bh);
 			partial--;
 		}
 
 end_range:
-		partial2 = ext4_find_shared(inode, n2, offsets2, chain2, &nr2);
+		partial2 = p2 = ext4_find_shared(inode, n2, offsets2, chain2, &nr2);
 		if (nr2) {
 			if (partial2 == chain2) {
 				/*
@@ -1321,16 +1320,14 @@  int ext4_ind_remove_space(handle_t *handle, struct inode *inode,
 					   (__le32 *)partial2->bh->b_data,
 					   partial2->p,
 					   (chain2+n2-1) - partial2);
-			BUFFER_TRACE(partial2->bh, "call brelse");
-			brelse(partial2->bh);
 			partial2--;
 		}
 		goto do_indirects;
 	}
 
 	/* Punch happened within the same level (n == n2) */
-	partial = ext4_find_shared(inode, n, offsets, chain, &nr);
-	partial2 = ext4_find_shared(inode, n2, offsets2, chain2, &nr2);
+	partial = p = ext4_find_shared(inode, n, offsets, chain, &nr);
+	partial2 = p2 = ext4_find_shared(inode, n2, offsets2, chain2, &nr2);
 
 	/* Free top, but only if partial2 isn't its subtree. */
 	if (nr) {
@@ -1387,15 +1384,7 @@  int ext4_ind_remove_space(handle_t *handle, struct inode *inode,
 					   partial->p + 1,
 					   partial2->p,
 					   (chain+n-1) - partial);
-			while (partial > chain) {
-				BUFFER_TRACE(partial->bh, "call brelse");
-				brelse(partial->bh);
-			}
-			while (partial2 > chain2) {
-				BUFFER_TRACE(partial2->bh, "call brelse");
-				brelse(partial2->bh);
-			}
-			return 0;
+			goto cleanup;
 		}
 
 		/*
@@ -1410,8 +1399,6 @@  int ext4_ind_remove_space(handle_t *handle, struct inode *inode,
 					   partial->p + 1,
 					   (__le32 *)partial->bh->b_data+addr_per_block,
 					   (chain+n-1) - partial);
-			BUFFER_TRACE(partial->bh, "call brelse");
-			brelse(partial->bh);
 			partial--;
 		}
 		if (partial2 > chain2 && depth2 <= depth) {
@@ -1419,11 +1406,21 @@  int ext4_ind_remove_space(handle_t *handle, struct inode *inode,
 					   (__le32 *)partial2->bh->b_data,
 					   partial2->p,
 					   (chain2+n2-1) - partial2);
-			BUFFER_TRACE(partial2->bh, "call brelse");
-			brelse(partial2->bh);
 			partial2--;
 		}
 	}
+
+cleanup:
+	while (p && p > chain) {
+		BUFFER_TRACE(p->bh, "call brelse");
+		brelse(p->bh);
+		p--;
+	}
+	while (p2 && p2 > chain2) {
+		BUFFER_TRACE(p2->bh, "call brelse");
+		brelse(p2->bh);
+		p2--;
+	}
 	return 0;
 
 do_indirects:
@@ -1431,7 +1428,7 @@  int ext4_ind_remove_space(handle_t *handle, struct inode *inode,
 	switch (offsets[0]) {
 	default:
 		if (++n >= n2)
-			return 0;
+			break;
 		nr = i_data[EXT4_IND_BLOCK];
 		if (nr) {
 			ext4_free_branches(handle, inode, NULL, &nr, &nr+1, 1);
@@ -1439,7 +1436,7 @@  int ext4_ind_remove_space(handle_t *handle, struct inode *inode,
 		}
 	case EXT4_IND_BLOCK:
 		if (++n >= n2)
-			return 0;
+			break;
 		nr = i_data[EXT4_DIND_BLOCK];
 		if (nr) {
 			ext4_free_branches(handle, inode, NULL, &nr, &nr+1, 2);
@@ -1447,7 +1444,7 @@  int ext4_ind_remove_space(handle_t *handle, struct inode *inode,
 		}
 	case EXT4_DIND_BLOCK:
 		if (++n >= n2)
-			return 0;
+			break;
 		nr = i_data[EXT4_TIND_BLOCK];
 		if (nr) {
 			ext4_free_branches(handle, inode, NULL, &nr, &nr+1, 3);
@@ -1456,5 +1453,5 @@  int ext4_ind_remove_space(handle_t *handle, struct inode *inode,
 	case EXT4_TIND_BLOCK:
 		;
 	}
-	return 0;
+	goto cleanup;
 }