Call common helper ext2_group_last_block_no() to
calculate group last block number.
Signed-off-by: Chengguang Xu <[email protected]>
---
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
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 <[email protected]>
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 <[email protected]>
SUSE Labs, CR
---- 在 星期三, 2019-11-06 23:42:36 Jan Kara <[email protected]> 撰写 ----
> 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 <[email protected]>
>
> 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
On Thu 07-11-19 10:54:43, Chengguang Xu wrote:
> ---- 在 星期三, 2019-11-06 23:42:36 Jan Kara <[email protected]> 撰写 ----
> > 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 <[email protected]>
> >
> > 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 <[email protected]>
SUSE Labs, CR
---- 在 星期四, 2019-11-07 17:21:17 Jan Kara <[email protected]> 撰写 ----
> On Thu 07-11-19 10:54:43, Chengguang Xu wrote:
> > ---- 在 星期三, 2019-11-06 23:42:36 Jan Kara <[email protected]> 撰写 ----
> > > 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 <[email protected]>
> > >
> > > 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
On Thu 07-11-19 17:44:23, Chengguang Xu wrote:
> ---- 在 星期四, 2019-11-07 17:21:17 Jan Kara <[email protected]> 撰写 ----
> > On Thu 07-11-19 10:54:43, Chengguang Xu wrote:
> > > ---- 在 星期三, 2019-11-06 23:42:36 Jan Kara <[email protected]> 撰写 ----
> > > > 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 <[email protected]>
> > > >
> > > > 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 <[email protected]>
SUSE Labs, CR