[PATCH 1/5] ext2: introduce new helper ext2_group_last_block_no()

[PATCH 1/5] ext2: introduce new helper ext2_group_last_block_no()

[Chengguang Xu]

Introduce new helper ext2_group_last_block_no() to calculate
last block num for specific block group, we can replace open
coded logic by calling this common helper.

Signed-off-by: Chengguang Xu <cgxu519@mykernel.net>
---
 fs/ext2/ext2.h | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/fs/ext2/ext2.h b/fs/ext2/ext2.h
index 10ab238de9a6..8178bd38a9d6 100644
--- a/fs/ext2/ext2.h
+++ b/fs/ext2/ext2.h
@@ -813,6 +813,18 @@ ext2_group_first_block_no(struct super_block *sb, unsigned long group_no)
 		le32_to_cpu(EXT2_SB(sb)->s_es->s_first_data_block);
 }
 
+static inline ext2_fsblk_t
+ext2_group_last_block_no(struct super_block *sb, unsigned long group_no)
+{
+	struct ext2_sb_info *sbi = EXT2_SB(sb);
+
+	if (group_no == sbi->s_groups_count - 1)
+		return le32_to_cpu(sbi->s_es->s_blocks_count) - 1;
+	else
+		return ext2_group_first_block_no(sb, group_no) +
+			EXT2_BLOCKS_PER_GROUP(sb) - 1;
+}
+
 #define ext2_set_bit	__test_and_set_bit_le
 #define ext2_clear_bit	__test_and_clear_bit_le
 #define ext2_test_bit	test_bit_le
-- 
2.20.1

[PATCH 2/5] ext2: code cleanup by calling ext2_group_last_block_no()

[Chengguang Xu]

Call common helper ext2_group_last_block_no() to
calculate group last block number.

Signed-off-by: Chengguang Xu <cgxu519@mykernel.net>
---
 fs/ext2/balloc.c | 16 ++++++++--------
 fs/ext2/super.c  |  8 +-------
 2 files changed, 9 insertions(+), 15 deletions(-)

diff --git a/fs/ext2/balloc.c b/fs/ext2/balloc.c
index 19bce75d207b..994a1fd18e93 100644
--- a/fs/ext2/balloc.c
+++ b/fs/ext2/balloc.c
@@ -269,7 +269,7 @@ goal_in_my_reservation(struct ext2_reserve_window *rsv, ext2_grpblk_t grp_goal,
 	ext2_fsblk_t group_first_block, group_last_block;
 
 	group_first_block = ext2_group_first_block_no(sb, group);
-	group_last_block = group_first_block + EXT2_BLOCKS_PER_GROUP(sb) - 1;
+	group_last_block = ext2_group_last_block_no(sb, group);
 
 	if ((rsv->_rsv_start > group_last_block) ||
 	    (rsv->_rsv_end < group_first_block))
@@ -666,22 +666,22 @@ ext2_try_to_allocate(struct super_block *sb, int group,
 			unsigned long *count,
 			struct ext2_reserve_window *my_rsv)
 {
-	ext2_fsblk_t group_first_block;
+	ext2_fsblk_t group_first_block = ext2_group_first_block_no(sb, group);
+	ext2_fsblk_t group_last_block = ext2_group_last_block_no(sb, group);
        	ext2_grpblk_t start, end;
 	unsigned long num = 0;
 
 	/* we do allocation within the reservation window if we have a window */
 	if (my_rsv) {
-		group_first_block = ext2_group_first_block_no(sb, group);
 		if (my_rsv->_rsv_start >= group_first_block)
 			start = my_rsv->_rsv_start - group_first_block;
 		else
 			/* reservation window cross group boundary */
 			start = 0;
 		end = my_rsv->_rsv_end - group_first_block + 1;
-		if (end > EXT2_BLOCKS_PER_GROUP(sb))
+		if (end > group_last_block - group_first_block + 1)
 			/* reservation window crosses group boundary */
-			end = EXT2_BLOCKS_PER_GROUP(sb);
+			end = group_last_block - group_first_block + 1;
 		if ((start <= grp_goal) && (grp_goal < end))
 			start = grp_goal;
 		else
@@ -691,7 +691,7 @@ ext2_try_to_allocate(struct super_block *sb, int group,
 			start = grp_goal;
 		else
 			start = 0;
-		end = EXT2_BLOCKS_PER_GROUP(sb);
+		end = group_last_block - group_first_block + 1;
 	}
 
 	BUG_ON(start > EXT2_BLOCKS_PER_GROUP(sb));
@@ -907,7 +907,7 @@ static int alloc_new_reservation(struct ext2_reserve_window_node *my_rsv,
 	spinlock_t *rsv_lock = &EXT2_SB(sb)->s_rsv_window_lock;
 
 	group_first_block = ext2_group_first_block_no(sb, group);
-	group_end_block = group_first_block + (EXT2_BLOCKS_PER_GROUP(sb) - 1);
+	group_end_block = ext2_group_last_block_no(sb, group);
 
 	if (grp_goal < 0)
 		start_block = group_first_block;
@@ -1114,7 +1114,7 @@ ext2_try_to_allocate_with_rsv(struct super_block *sb, unsigned int group,
 	 * first block is the block number of the first block in this group
 	 */
 	group_first_block = ext2_group_first_block_no(sb, group);
-	group_last_block = group_first_block + (EXT2_BLOCKS_PER_GROUP(sb) - 1);
+	group_last_block = ext2_group_last_block_no(sb, group);
 
 	/*
 	 * Basically we will allocate a new block from inode's reservation
diff --git a/fs/ext2/super.c b/fs/ext2/super.c
index 30c630d73f0f..4cd401a2f207 100644
--- a/fs/ext2/super.c
+++ b/fs/ext2/super.c
@@ -702,13 +702,7 @@ static int ext2_check_descriptors(struct super_block *sb)
 	for (i = 0; i < sbi->s_groups_count; i++) {
 		struct ext2_group_desc *gdp = ext2_get_group_desc(sb, i, NULL);
 		ext2_fsblk_t first_block = ext2_group_first_block_no(sb, i);
-		ext2_fsblk_t last_block;
-
-		if (i == sbi->s_groups_count - 1)
-			last_block = le32_to_cpu(sbi->s_es->s_blocks_count) - 1;
-		else
-			last_block = first_block +
-				(EXT2_BLOCKS_PER_GROUP(sb) - 1);
+		ext2_fsblk_t last_block = ext2_group_last_block_no(sb, i);
 
 		if (le32_to_cpu(gdp->bg_block_bitmap) < first_block ||
 		    le32_to_cpu(gdp->bg_block_bitmap) > last_block)
-- 
2.20.1

[PATCH 3/5] ext2: skip unnecessary operations in ext2_try_to_allocate()

[Chengguang Xu]

Move 'repeat' tag to proper place so that we can
skip unnecessary operations in ext2_try_to_allocate().

Signed-off-by: Chengguang Xu <cgxu519@mykernel.net>
---
 fs/ext2/balloc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/ext2/balloc.c b/fs/ext2/balloc.c
index 994a1fd18e93..a0c22e166682 100644
--- a/fs/ext2/balloc.c
+++ b/fs/ext2/balloc.c
@@ -696,7 +696,6 @@ ext2_try_to_allocate(struct super_block *sb, int group,
 
 	BUG_ON(start > EXT2_BLOCKS_PER_GROUP(sb));
 
-repeat:
 	if (grp_goal < 0) {
 		grp_goal = find_next_usable_block(start, bitmap_bh, end);
 		if (grp_goal < 0)
@@ -713,6 +712,7 @@ ext2_try_to_allocate(struct super_block *sb, int group,
 	}
 	start = grp_goal;
 
+repeat:
 	if (ext2_set_bit_atomic(sb_bgl_lock(EXT2_SB(sb), group), grp_goal,
 			       				bitmap_bh->b_data)) {
 		/*
-- 
2.20.1

[PATCH 4/5] ext2: code cleanup for ext2_try_to_allocate()

[Chengguang Xu]

Code cleanup by removing duplicated code.

Signed-off-by: Chengguang Xu <cgxu519@mykernel.net>
---
 fs/ext2/balloc.c | 32 ++++++++++++++------------------
 1 file changed, 14 insertions(+), 18 deletions(-)

diff --git a/fs/ext2/balloc.c b/fs/ext2/balloc.c
index a0c22e166682..9a9bd566243d 100644
--- a/fs/ext2/balloc.c
+++ b/fs/ext2/balloc.c
@@ -710,29 +710,25 @@ ext2_try_to_allocate(struct super_block *sb, int group,
 				;
 		}
 	}
-	start = grp_goal;
 
-repeat:
-	if (ext2_set_bit_atomic(sb_bgl_lock(EXT2_SB(sb), group), grp_goal,
-			       				bitmap_bh->b_data)) {
-		/*
-		 * The block was allocated by another thread, or it was
-		 * allocated and then freed by another thread
-		 */
-		start++;
-		grp_goal++;
-		if (start >= end)
-			goto fail_access;
-		goto repeat;
-	}
-	num++;
-	grp_goal++;
-	while (num < *count && grp_goal < end
-		&& !ext2_set_bit_atomic(sb_bgl_lock(EXT2_SB(sb), group),
+	while (num < *count && grp_goal < end) {
+		if (ext2_set_bit_atomic(sb_bgl_lock(EXT2_SB(sb), group),
 					grp_goal, bitmap_bh->b_data)) {
+			if (num == 0) {
+				grp_goal++;
+				continue;
+			} else {
+				break;
+			}
+		}
+
 		num++;
 		grp_goal++;
 	}
+
+	if (!num)
+		goto fail_access;
+
 	*count = num;
 	return grp_goal - num;
 fail_access:
-- 
2.20.1

[PATCH 5/5] ext2: fix improper function comment

[Chengguang Xu]

Just fix a improper function comment.

Signed-off-by: Chengguang Xu <cgxu519@mykernel.net>
---
 fs/ext2/balloc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/ext2/balloc.c b/fs/ext2/balloc.c
index 9a9bd566243d..4180467122d0 100644
--- a/fs/ext2/balloc.c
+++ b/fs/ext2/balloc.c
@@ -749,7 +749,7 @@ ext2_try_to_allocate(struct super_block *sb, int group,
  *		but we will shift to the place where start_block is,
  *		then start from there, when looking for a reservable space.
  *
- * 	@size: the target new reservation window size
+ *	@sb: the super block.
  *
  * 	@group_first_block: the first block we consider to start
  *			the real search from
-- 
2.20.1

Re: [PATCH 2/5] ext2: code cleanup by calling ext2_group_last_block_no()

[Jan Kara]

[-- Attachment #1: Type: text/plain, Size: 4200 bytes --]

On Mon 04-11-19 19:40:33, Chengguang Xu wrote:
> Call common helper ext2_group_last_block_no() to
> calculate group last block number.
> 
> Signed-off-by: Chengguang Xu <cgxu519@mykernel.net>

Thanks for the patch! I've applied it (as well as 1/5) and added attached
simplification on top.

								Honza

> ---
>  fs/ext2/balloc.c | 16 ++++++++--------
>  fs/ext2/super.c  |  8 +-------
>  2 files changed, 9 insertions(+), 15 deletions(-)
> 
> diff --git a/fs/ext2/balloc.c b/fs/ext2/balloc.c
> index 19bce75d207b..994a1fd18e93 100644
> --- a/fs/ext2/balloc.c
> +++ b/fs/ext2/balloc.c
> @@ -269,7 +269,7 @@ goal_in_my_reservation(struct ext2_reserve_window *rsv, ext2_grpblk_t grp_goal,
>  	ext2_fsblk_t group_first_block, group_last_block;
>  
>  	group_first_block = ext2_group_first_block_no(sb, group);
> -	group_last_block = group_first_block + EXT2_BLOCKS_PER_GROUP(sb) - 1;
> +	group_last_block = ext2_group_last_block_no(sb, group);
>  
>  	if ((rsv->_rsv_start > group_last_block) ||
>  	    (rsv->_rsv_end < group_first_block))
> @@ -666,22 +666,22 @@ ext2_try_to_allocate(struct super_block *sb, int group,
>  			unsigned long *count,
>  			struct ext2_reserve_window *my_rsv)
>  {
> -	ext2_fsblk_t group_first_block;
> +	ext2_fsblk_t group_first_block = ext2_group_first_block_no(sb, group);
> +	ext2_fsblk_t group_last_block = ext2_group_last_block_no(sb, group);
>         	ext2_grpblk_t start, end;
>  	unsigned long num = 0;
>  
>  	/* we do allocation within the reservation window if we have a window */
>  	if (my_rsv) {
> -		group_first_block = ext2_group_first_block_no(sb, group);
>  		if (my_rsv->_rsv_start >= group_first_block)
>  			start = my_rsv->_rsv_start - group_first_block;
>  		else
>  			/* reservation window cross group boundary */
>  			start = 0;
>  		end = my_rsv->_rsv_end - group_first_block + 1;
> -		if (end > EXT2_BLOCKS_PER_GROUP(sb))
> +		if (end > group_last_block - group_first_block + 1)
>  			/* reservation window crosses group boundary */
> -			end = EXT2_BLOCKS_PER_GROUP(sb);
> +			end = group_last_block - group_first_block + 1;
>  		if ((start <= grp_goal) && (grp_goal < end))
>  			start = grp_goal;
>  		else
> @@ -691,7 +691,7 @@ ext2_try_to_allocate(struct super_block *sb, int group,
>  			start = grp_goal;
>  		else
>  			start = 0;
> -		end = EXT2_BLOCKS_PER_GROUP(sb);
> +		end = group_last_block - group_first_block + 1;
>  	}
>  
>  	BUG_ON(start > EXT2_BLOCKS_PER_GROUP(sb));
> @@ -907,7 +907,7 @@ static int alloc_new_reservation(struct ext2_reserve_window_node *my_rsv,
>  	spinlock_t *rsv_lock = &EXT2_SB(sb)->s_rsv_window_lock;
>  
>  	group_first_block = ext2_group_first_block_no(sb, group);
> -	group_end_block = group_first_block + (EXT2_BLOCKS_PER_GROUP(sb) - 1);
> +	group_end_block = ext2_group_last_block_no(sb, group);
>  
>  	if (grp_goal < 0)
>  		start_block = group_first_block;
> @@ -1114,7 +1114,7 @@ ext2_try_to_allocate_with_rsv(struct super_block *sb, unsigned int group,
>  	 * first block is the block number of the first block in this group
>  	 */
>  	group_first_block = ext2_group_first_block_no(sb, group);
> -	group_last_block = group_first_block + (EXT2_BLOCKS_PER_GROUP(sb) - 1);
> +	group_last_block = ext2_group_last_block_no(sb, group);
>  
>  	/*
>  	 * Basically we will allocate a new block from inode's reservation
> diff --git a/fs/ext2/super.c b/fs/ext2/super.c
> index 30c630d73f0f..4cd401a2f207 100644
> --- a/fs/ext2/super.c
> +++ b/fs/ext2/super.c
> @@ -702,13 +702,7 @@ static int ext2_check_descriptors(struct super_block *sb)
>  	for (i = 0; i < sbi->s_groups_count; i++) {
>  		struct ext2_group_desc *gdp = ext2_get_group_desc(sb, i, NULL);
>  		ext2_fsblk_t first_block = ext2_group_first_block_no(sb, i);
> -		ext2_fsblk_t last_block;
> -
> -		if (i == sbi->s_groups_count - 1)
> -			last_block = le32_to_cpu(sbi->s_es->s_blocks_count) - 1;
> -		else
> -			last_block = first_block +
> -				(EXT2_BLOCKS_PER_GROUP(sb) - 1);
> +		ext2_fsblk_t last_block = ext2_group_last_block_no(sb, i);
>  
>  		if (le32_to_cpu(gdp->bg_block_bitmap) < first_block ||
>  		    le32_to_cpu(gdp->bg_block_bitmap) > last_block)
> -- 
> 2.20.1
> 
> 
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[-- Attachment #2: 0001-ext2-Simplify-initialization-in-ext2_try_to_allocate.patch --]
[-- Type: text/x-patch, Size: 1714 bytes --]

From 33229f6f844afe29a76584922a1c705ba5e88717 Mon Sep 17 00:00:00 2001
From: Jan Kara <jack@suse.cz>
Date: Wed, 6 Nov 2019 16:39:26 +0100
Subject: [PATCH] ext2: Simplify initialization in ext2_try_to_allocate()

Somewhat simplify the logic initializing search start and end in
ext2_try_to_allocate(). No functional change.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ext2/balloc.c | 22 +++++-----------------
 1 file changed, 5 insertions(+), 17 deletions(-)

diff --git a/fs/ext2/balloc.c b/fs/ext2/balloc.c
index 994a1fd18e93..65df001f4cc2 100644
--- a/fs/ext2/balloc.c
+++ b/fs/ext2/balloc.c
@@ -671,29 +671,17 @@ ext2_try_to_allocate(struct super_block *sb, int group,
        	ext2_grpblk_t start, end;
 	unsigned long num = 0;
 
+	start = 0;
+	end = group_last_block - group_first_block + 1;
 	/* we do allocation within the reservation window if we have a window */
 	if (my_rsv) {
 		if (my_rsv->_rsv_start >= group_first_block)
 			start = my_rsv->_rsv_start - group_first_block;
-		else
-			/* reservation window cross group boundary */
-			start = 0;
-		end = my_rsv->_rsv_end - group_first_block + 1;
-		if (end > group_last_block - group_first_block + 1)
-			/* reservation window crosses group boundary */
-			end = group_last_block - group_first_block + 1;
-		if ((start <= grp_goal) && (grp_goal < end))
-			start = grp_goal;
-		else
+		if (my_rsv->_rsv_end < group_last_block)
+			end = my_rsv->_rsv_end - group_first_block + 1;
+		if (grp_goal < start || grp_goal > end)
 			grp_goal = -1;
-	} else {
-		if (grp_goal > 0)
-			start = grp_goal;
-		else
-			start = 0;
-		end = group_last_block - group_first_block + 1;
 	}
-
 	BUG_ON(start > EXT2_BLOCKS_PER_GROUP(sb));
 
 repeat:
-- 
2.16.4

Re: [PATCH 4/5] ext2: code cleanup for ext2_try_to_allocate()

[Jan Kara]

[-- Attachment #1: Type: text/plain, Size: 1678 bytes --]

On Mon 04-11-19 19:40:35, Chengguang Xu wrote:
> Code cleanup by removing duplicated code.
> 
> Signed-off-by: Chengguang Xu <cgxu519@mykernel.net>

Thanks for the patch! I've merged it with a small update to switch the
while() loop into a for() loop which is somewhat more natural in that
situation. Resulting patch attached.

								Honza

> ---
>  fs/ext2/balloc.c | 32 ++++++++++++++------------------
>  1 file changed, 14 insertions(+), 18 deletions(-)
> 
> diff --git a/fs/ext2/balloc.c b/fs/ext2/balloc.c
> index a0c22e166682..9a9bd566243d 100644
> --- a/fs/ext2/balloc.c
> +++ b/fs/ext2/balloc.c
> @@ -710,29 +710,25 @@ ext2_try_to_allocate(struct super_block *sb, int group,
>  				;
>  		}
>  	}
> -	start = grp_goal;
>  
> -repeat:
> -	if (ext2_set_bit_atomic(sb_bgl_lock(EXT2_SB(sb), group), grp_goal,
> -			       				bitmap_bh->b_data)) {
> -		/*
> -		 * The block was allocated by another thread, or it was
> -		 * allocated and then freed by another thread
> -		 */
> -		start++;
> -		grp_goal++;
> -		if (start >= end)
> -			goto fail_access;
> -		goto repeat;
> -	}
> -	num++;
> -	grp_goal++;
> -	while (num < *count && grp_goal < end
> -		&& !ext2_set_bit_atomic(sb_bgl_lock(EXT2_SB(sb), group),
> +	while (num < *count && grp_goal < end) {
> +		if (ext2_set_bit_atomic(sb_bgl_lock(EXT2_SB(sb), group),
>  					grp_goal, bitmap_bh->b_data)) {
> +			if (num == 0) {
> +				grp_goal++;
> +				continue;
> +			} else {
> +				break;
> +			}
> +		}
> +
>  		num++;
>  		grp_goal++;
>  	}
> +
> +	if (!num)
> +		goto fail_access;
> +
>  	*count = num;
>  	return grp_goal - num;
>  fail_access:
> -- 
> 2.20.1
> 
> 
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[-- Attachment #2: 0001-ext2-code-cleanup-for-ext2_try_to_allocate.patch --]
[-- Type: text/x-patch, Size: 1572 bytes --]

From d61650c669a6e3c1347cd3b333e4ae8487757f35 Mon Sep 17 00:00:00 2001
From: Chengguang Xu <cgxu519@mykernel.net>
Date: Mon, 4 Nov 2019 19:40:35 +0800
Subject: [PATCH] ext2: code cleanup for ext2_try_to_allocate()

Code cleanup by removing duplicated code.

Link: https://lore.kernel.org/r/20191104114036.9893-4-cgxu519@mykernel.net
Signed-off-by: Chengguang Xu <cgxu519@mykernel.net>
Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ext2/balloc.c | 29 ++++++++++-------------------
 1 file changed, 10 insertions(+), 19 deletions(-)

diff --git a/fs/ext2/balloc.c b/fs/ext2/balloc.c
index 25bc3a43cd94..d67f7dc1baaa 100644
--- a/fs/ext2/balloc.c
+++ b/fs/ext2/balloc.c
@@ -698,29 +698,20 @@ ext2_try_to_allocate(struct super_block *sb, int group,
 				;
 		}
 	}
-	start = grp_goal;
 
-repeat:
-	if (ext2_set_bit_atomic(sb_bgl_lock(EXT2_SB(sb), group), grp_goal,
-			       				bitmap_bh->b_data)) {
-		/*
-		 * The block was allocated by another thread, or it was
-		 * allocated and then freed by another thread
-		 */
-		start++;
-		grp_goal++;
-		if (start >= end)
-			goto fail_access;
-		goto repeat;
-	}
-	num++;
-	grp_goal++;
-	while (num < *count && grp_goal < end
-		&& !ext2_set_bit_atomic(sb_bgl_lock(EXT2_SB(sb), group),
+	for (; num < *count && grp_goal < end; grp_goal++) {
+		if (ext2_set_bit_atomic(sb_bgl_lock(EXT2_SB(sb), group),
 					grp_goal, bitmap_bh->b_data)) {
+			if (num == 0)
+				continue;
+			break;
+		}
 		num++;
-		grp_goal++;
 	}
+
+	if (num == 0)
+		goto fail_access;
+
 	*count = num;
 	return grp_goal - num;
 fail_access:
-- 
2.16.4

Re: [PATCH 5/5] ext2: fix improper function comment

[Jan Kara]

On Mon 04-11-19 19:40:36, Chengguang Xu wrote:
> Just fix a improper function comment.
> 
> Signed-off-by: Chengguang Xu <cgxu519@mykernel.net>

Thanks, applied! I've also fixed @group_first_block as that should be
@start_block.

								Honza

> ---
>  fs/ext2/balloc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/ext2/balloc.c b/fs/ext2/balloc.c
> index 9a9bd566243d..4180467122d0 100644
> --- a/fs/ext2/balloc.c
> +++ b/fs/ext2/balloc.c
> @@ -749,7 +749,7 @@ ext2_try_to_allocate(struct super_block *sb, int group,
>   *		but we will shift to the place where start_block is,
>   *		then start from there, when looking for a reservable space.
>   *
> - * 	@size: the target new reservation window size
> + *	@sb: the super block.
>   *
>   * 	@group_first_block: the first block we consider to start
>   *			the real search from
> -- 
> 2.20.1
> 
> 
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 2/5] ext2: code cleanup by calling ext2_group_last_block_no()

[Chengguang Xu]

 ---- 在 星期三, 2019-11-06 23:42:36 Jan Kara <jack@suse.cz> 撰写 ----
 > On Mon 04-11-19 19:40:33, Chengguang Xu wrote:
 > > Call common helper ext2_group_last_block_no() to
 > > calculate group last block number.
 > > 
 > > Signed-off-by: Chengguang Xu <cgxu519@mykernel.net>
 > 
 > Thanks for the patch! I've applied it (as well as 1/5) and added attached
 > simplification on top.
 > 

In ext2_try_to_allocate()

+		if (my_rsv->_rsv_end < group_last_block)
+			end = my_rsv->_rsv_end - group_first_block + 1;
+		if (grp_goal < start || grp_goal > end)

Based on original code, shouldn't it be  if (grp_goal < start || grp_goal >=end) ?

Thanks,
Chengguang

Re: [PATCH 4/5] ext2: code cleanup for ext2_try_to_allocate()

[Chengguang Xu]

 ---- 在 星期三, 2019-11-06 23:59:00 Jan Kara <jack@suse.cz> 撰写 ----
 > On Mon 04-11-19 19:40:35, Chengguang Xu wrote:
 > > Code cleanup by removing duplicated code.
 > > 
 > > Signed-off-by: Chengguang Xu <cgxu519@mykernel.net>
 > 
 > Thanks for the patch! I've merged it with a small update to switch the
 > while() loop into a for() loop which is somewhat more natural in that
 > situation. Resulting patch attached.
 > 

Looks fine to me. 


Thanks,
Chengguang

Re: [PATCH 2/5] ext2: code cleanup by calling ext2_group_last_block_no()

[Jan Kara]

On Thu 07-11-19 10:54:43, Chengguang Xu wrote:
>  ---- 在 星期三, 2019-11-06 23:42:36 Jan Kara <jack@suse.cz> 撰写 ----
>  > On Mon 04-11-19 19:40:33, Chengguang Xu wrote:
>  > > Call common helper ext2_group_last_block_no() to
>  > > calculate group last block number.
>  > > 
>  > > Signed-off-by: Chengguang Xu <cgxu519@mykernel.net>
>  > 
>  > Thanks for the patch! I've applied it (as well as 1/5) and added attached
>  > simplification on top.
>  > 
> 
> In ext2_try_to_allocate()
> 
> +		if (my_rsv->_rsv_end < group_last_block)
> +			end = my_rsv->_rsv_end - group_first_block + 1;
> +		if (grp_goal < start || grp_goal > end)
> 
> Based on original code, shouldn't it be  if (grp_goal < start || grp_goal
> >=end) ?

Hum, that's a good point. The original code actually had an off-by-one bug
because 'end' is really the last block that can be used so grp_goal == end
still makes sense. And my cleanup fixed it. Now looking at the code in
ext2_try_to_allocate() we also have a similar bug in the loop allocating
blocks. There we can also go upto 'end' inclusive. Added a patch to fix
that. Thanks for pointing me to this!

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 2/5] ext2: code cleanup by calling ext2_group_last_block_no()

[Chengguang Xu]

 ---- 在 星期四, 2019-11-07 17:21:17 Jan Kara <jack@suse.cz> 撰写 ----
 > On Thu 07-11-19 10:54:43, Chengguang Xu wrote:
 > >  ---- 在 星期三, 2019-11-06 23:42:36 Jan Kara <jack@suse.cz> 撰写 ----
 > >  > On Mon 04-11-19 19:40:33, Chengguang Xu wrote:
 > >  > > Call common helper ext2_group_last_block_no() to
 > >  > > calculate group last block number.
 > >  > > 
 > >  > > Signed-off-by: Chengguang Xu <cgxu519@mykernel.net>
 > >  > 
 > >  > Thanks for the patch! I've applied it (as well as 1/5) and added attached
 > >  > simplification on top.
 > >  > 
 > > 
 > > In ext2_try_to_allocate()
 > > 
 > > +        if (my_rsv->_rsv_end < group_last_block)
 > > +            end = my_rsv->_rsv_end - group_first_block + 1;
 > > +        if (grp_goal < start || grp_goal > end)
 > > 
 > > Based on original code, shouldn't it be  if (grp_goal < start || grp_goal
 > > >=end) ?
 > 
 > Hum, that's a good point. The original code actually had an off-by-one bug
 > because 'end' is really the last block that can be used so grp_goal == end
 > still makes sense. And my cleanup fixed it. Now looking at the code in
 > ext2_try_to_allocate() we also have a similar bug in the loop allocating
 > blocks. There we can also go upto 'end' inclusive. Added a patch to fix
 > that. Thanks for pointing me to this!
 > 

Doesn't it depend on what starting number for grp_block inside block group?
if it starts from 0, is the end number block still available for allocation?

Thanks,
Chengguang

Re: [PATCH 2/5] ext2: code cleanup by calling ext2_group_last_block_no()

[Jan Kara]

On Thu 07-11-19 17:44:23, Chengguang Xu wrote:
>  ---- 在 星期四, 2019-11-07 17:21:17 Jan Kara <jack@suse.cz> 撰写 ----
>  > On Thu 07-11-19 10:54:43, Chengguang Xu wrote:
>  > >  ---- 在 星期三, 2019-11-06 23:42:36 Jan Kara <jack@suse.cz> 撰写 ----
>  > >  > On Mon 04-11-19 19:40:33, Chengguang Xu wrote:
>  > >  > > Call common helper ext2_group_last_block_no() to
>  > >  > > calculate group last block number.
>  > >  > > 
>  > >  > > Signed-off-by: Chengguang Xu <cgxu519@mykernel.net>
>  > >  > 
>  > >  > Thanks for the patch! I've applied it (as well as 1/5) and added attached
>  > >  > simplification on top.
>  > >  > 
>  > > 
>  > > In ext2_try_to_allocate()
>  > > 
>  > > +        if (my_rsv->_rsv_end < group_last_block)
>  > > +            end = my_rsv->_rsv_end - group_first_block + 1;
>  > > +        if (grp_goal < start || grp_goal > end)
>  > > 
>  > > Based on original code, shouldn't it be  if (grp_goal < start || grp_goal
>  > > >=end) ?
>  > 
>  > Hum, that's a good point. The original code actually had an off-by-one bug
>  > because 'end' is really the last block that can be used so grp_goal == end
>  > still makes sense. And my cleanup fixed it. Now looking at the code in
>  > ext2_try_to_allocate() we also have a similar bug in the loop allocating
>  > blocks. There we can also go upto 'end' inclusive. Added a patch to fix
>  > that. Thanks for pointing me to this!
>  > 
> 
> Doesn't it depend on what starting number for grp_block inside block group?
> if it starts from 0, is the end number block still available for allocation?

Argh! You are right, I've misread the initialization of 'end' and that is
really one block past the last one. I've fixed up things in my tree. Thanks
for review!

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR