Message ID | 20230209194825.511043-5-shikemeng@huaweicloud.com |
---|---|
State | Not Applicable |
Headers | show |
Series | Some bugfix and cleanup to mballoc | expand |
On Fri, Feb 10, 2023 at 03:48:08AM +0800, Kemeng Shi wrote: > We always get ext4_group_desc with group + 1 and ext4_group_info with > group to check if we need do initialize ext4_group_info for the group. > Just get ext4_group_desc with group for ext4_group_info initialization > check. > > Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com> > --- > fs/ext4/mballoc.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c > index 352ac9139fee..f24f80ecf318 100644 > --- a/fs/ext4/mballoc.c > +++ b/fs/ext4/mballoc.c > @@ -2570,13 +2570,13 @@ void ext4_mb_prefetch_fini(struct super_block *sb, ext4_group_t group, > unsigned int nr) > { > while (nr-- > 0) { > - struct ext4_group_desc *gdp = ext4_get_group_desc(sb, group, > - NULL); > - struct ext4_group_info *grp = ext4_get_group_info(sb, group); > + struct ext4_group_desc *gdp; > + struct ext4_group_info *grp; > > if (!group) > group = ext4_get_groups_count(sb); > group--; > + gdp = ext4_get_group_desc(sb, group, NULL); > grp = ext4_get_group_info(sb, group); > > if (EXT4_MB_GRP_NEED_INIT(grp) && > -- > 2.30.0 > This is a duplicate of [1] :) But I'm okay with this going in as that patchseries might take some time to get merged. Feel free to add: Reviewed-by: Ojaswin Mujoo <ojaswin@linux.ibm.com> [1] https://lore.kernel.org/r/85bbcb3774e38de65b737ef0000241ddbdda73aa.1674822311.git.ojaswin@linux.ibm.com
on 2/13/2023 3:03 PM, Ojaswin Mujoo wrote: > On Fri, Feb 10, 2023 at 03:48:08AM +0800, Kemeng Shi wrote: >> We always get ext4_group_desc with group + 1 and ext4_group_info with >> group to check if we need do initialize ext4_group_info for the group. >> Just get ext4_group_desc with group for ext4_group_info initialization >> check. >> >> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com> >> --- >> fs/ext4/mballoc.c | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c >> index 352ac9139fee..f24f80ecf318 100644 >> --- a/fs/ext4/mballoc.c >> +++ b/fs/ext4/mballoc.c >> @@ -2570,13 +2570,13 @@ void ext4_mb_prefetch_fini(struct super_block *sb, ext4_group_t group, >> unsigned int nr) >> { >> while (nr-- > 0) { >> - struct ext4_group_desc *gdp = ext4_get_group_desc(sb, group, >> - NULL); >> - struct ext4_group_info *grp = ext4_get_group_info(sb, group); >> + struct ext4_group_desc *gdp; >> + struct ext4_group_info *grp; >> >> if (!group) >> group = ext4_get_groups_count(sb); >> group--; >> + gdp = ext4_get_group_desc(sb, group, NULL); >> grp = ext4_get_group_info(sb, group); >> >> if (EXT4_MB_GRP_NEED_INIT(grp) && >> -- >> 2.30.0 >> > > This is a duplicate of [1] :) > > But I'm okay with this going in as that patchseries might take some time > to get merged. Feel free to add: > > Reviewed-by: Ojaswin Mujoo <ojaswin@linux.ibm.com> > > [1] https://lore.kernel.org/r/85bbcb3774e38de65b737ef0000241ddbdda73aa.1674822311.git.ojaswin@linux.ibm.com > Hi Ojaswin, Thank you so much to review my code. I 'm sorry that I didn't notice this patch is duplicated and I really appreciate that you allow this one to get merged first. I will add your sign to this patch in next version. Thanks!
On Mon, Feb 13, 2023 at 08:27:32PM +0800, Kemeng Shi wrote: > > > on 2/13/2023 3:03 PM, Ojaswin Mujoo wrote: > > On Fri, Feb 10, 2023 at 03:48:08AM +0800, Kemeng Shi wrote: > >> We always get ext4_group_desc with group + 1 and ext4_group_info with > >> group to check if we need do initialize ext4_group_info for the group. > >> Just get ext4_group_desc with group for ext4_group_info initialization > >> check. > >> > >> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com> > >> --- > >> fs/ext4/mballoc.c | 6 +++--- > >> 1 file changed, 3 insertions(+), 3 deletions(-) > >> > >> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c > >> index 352ac9139fee..f24f80ecf318 100644 > >> --- a/fs/ext4/mballoc.c > >> +++ b/fs/ext4/mballoc.c > >> @@ -2570,13 +2570,13 @@ void ext4_mb_prefetch_fini(struct super_block *sb, ext4_group_t group, > >> unsigned int nr) > >> { > >> while (nr-- > 0) { > >> - struct ext4_group_desc *gdp = ext4_get_group_desc(sb, group, > >> - NULL); > >> - struct ext4_group_info *grp = ext4_get_group_info(sb, group); > >> + struct ext4_group_desc *gdp; > >> + struct ext4_group_info *grp; > >> > >> if (!group) > >> group = ext4_get_groups_count(sb); > >> group--; > >> + gdp = ext4_get_group_desc(sb, group, NULL); > >> grp = ext4_get_group_info(sb, group); > >> > >> if (EXT4_MB_GRP_NEED_INIT(grp) && > >> -- > >> 2.30.0 > >> > > > > This is a duplicate of [1] :) > > > > But I'm okay with this going in as that patchseries might take some time > > to get merged. Feel free to add: > > > > Reviewed-by: Ojaswin Mujoo <ojaswin@linux.ibm.com> > > > > [1] https://lore.kernel.org/r/85bbcb3774e38de65b737ef0000241ddbdda73aa.1674822311.git.ojaswin@linux.ibm.com > > > Hi Ojaswin, Thank you so much to review my code. I 'm sorry that I didn't > notice this patch is duplicated and I really appreciate that you allow this > one to get merged first. I will add your sign to this patch in next version. > Thanks! Hi Kemeng, So I'm not sure what the norm is when dealing with such duplicate patches, but if you do plan to add the Signed-off-by then I'd just like to point out that the patch I linked is mainly from Ritesh Harjani, so you can pick his Signed-off-by rather than mine. Thanks, ojaswin > > -- > Best wishes > Kemeng Shi >
on 2/14/2023 4:14 AM, Ojaswin Mujoo wrote: > On Mon, Feb 13, 2023 at 08:27:32PM +0800, Kemeng Shi wrote: >> >> >> on 2/13/2023 3:03 PM, Ojaswin Mujoo wrote: >>> On Fri, Feb 10, 2023 at 03:48:08AM +0800, Kemeng Shi wrote: >>>> We always get ext4_group_desc with group + 1 and ext4_group_info with >>>> group to check if we need do initialize ext4_group_info for the group. >>>> Just get ext4_group_desc with group for ext4_group_info initialization >>>> check. >>>> >>>> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com> >>>> --- >>>> fs/ext4/mballoc.c | 6 +++--- >>>> 1 file changed, 3 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c >>>> index 352ac9139fee..f24f80ecf318 100644 >>>> --- a/fs/ext4/mballoc.c >>>> +++ b/fs/ext4/mballoc.c >>>> @@ -2570,13 +2570,13 @@ void ext4_mb_prefetch_fini(struct super_block *sb, ext4_group_t group, >>>> unsigned int nr) >>>> { >>>> while (nr-- > 0) { >>>> - struct ext4_group_desc *gdp = ext4_get_group_desc(sb, group, >>>> - NULL); >>>> - struct ext4_group_info *grp = ext4_get_group_info(sb, group); >>>> + struct ext4_group_desc *gdp; >>>> + struct ext4_group_info *grp; >>>> >>>> if (!group) >>>> group = ext4_get_groups_count(sb); >>>> group--; >>>> + gdp = ext4_get_group_desc(sb, group, NULL); >>>> grp = ext4_get_group_info(sb, group); >>>> >>>> if (EXT4_MB_GRP_NEED_INIT(grp) && >>>> -- >>>> 2.30.0 >>>> >>> >>> This is a duplicate of [1] :) >>> >>> But I'm okay with this going in as that patchseries might take some time >>> to get merged. Feel free to add: >>> >>> Reviewed-by: Ojaswin Mujoo <ojaswin@linux.ibm.com> >>> >>> [1] https://lore.kernel.org/r/85bbcb3774e38de65b737ef0000241ddbdda73aa.1674822311.git.ojaswin@linux.ibm.com >>> >> Hi Ojaswin, Thank you so much to review my code. I 'm sorry that I didn't >> notice this patch is duplicated and I really appreciate that you allow this >> one to get merged first. I will add your sign to this patch in next version. >> Thanks! > Hi Kemeng, > > So I'm not sure what the norm is when dealing with such duplicate > patches, but if you do plan to add the Signed-off-by then I'd just like > to point out that the patch I linked is mainly from Ritesh Harjani, so > you can pick his Signed-off-by rather than mine. > Sorry that I miss that there are two Signed-off-bys in patch you have already sent. I will collect both signs to this patch. Thanks!
Kemeng Shi <shikemeng@huaweicloud.com> writes: > We always get ext4_group_desc with group + 1 and ext4_group_info with > group to check if we need do initialize ext4_group_info for the group. > Just get ext4_group_desc with group for ext4_group_info initialization > check. > > Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com> > --- > fs/ext4/mballoc.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c > index 352ac9139fee..f24f80ecf318 100644 > --- a/fs/ext4/mballoc.c > +++ b/fs/ext4/mballoc.c > @@ -2570,13 +2570,13 @@ void ext4_mb_prefetch_fini(struct super_block *sb, ext4_group_t group, > unsigned int nr) > { > while (nr-- > 0) { > - struct ext4_group_desc *gdp = ext4_get_group_desc(sb, group, > - NULL); > - struct ext4_group_info *grp = ext4_get_group_info(sb, group); > + struct ext4_group_desc *gdp; > + struct ext4_group_info *grp; We can even declare these variables at the begining of the function like in [1]. Also I would advise to rearrange any "fixes" patches at the begining of the patch series and "cleanup" patches at the end. e.g. this looks like a fix to me. That way it is sometimes easier for people to cherry-pick any fixes if required in their older kernel trees. ;) [1]: https://lore.kernel.org/all/85bbcb3774e38de65b737ef0000241ddbdda73aa.1674822311.git.ojaswin@linux.ibm.com/ -ritesh > > if (!group) > group = ext4_get_groups_count(sb); > group--; > + gdp = ext4_get_group_desc(sb, group, NULL); > grp = ext4_get_group_info(sb, group); > > if (EXT4_MB_GRP_NEED_INIT(grp) && > -- > 2.30.0
on 2/17/2023 2:46 PM, Ritesh Harjani (IBM) wrote: > Kemeng Shi <shikemeng@huaweicloud.com> writes: > >> We always get ext4_group_desc with group + 1 and ext4_group_info with >> group to check if we need do initialize ext4_group_info for the group. >> Just get ext4_group_desc with group for ext4_group_info initialization >> check. >> >> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com> >> --- >> fs/ext4/mballoc.c | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c >> index 352ac9139fee..f24f80ecf318 100644 >> --- a/fs/ext4/mballoc.c >> +++ b/fs/ext4/mballoc.c >> @@ -2570,13 +2570,13 @@ void ext4_mb_prefetch_fini(struct super_block *sb, ext4_group_t group, >> unsigned int nr) >> { >> while (nr-- > 0) { >> - struct ext4_group_desc *gdp = ext4_get_group_desc(sb, group, >> - NULL); >> - struct ext4_group_info *grp = ext4_get_group_info(sb, group); >> + struct ext4_group_desc *gdp; >> + struct ext4_group_info *grp; > > We can even declare these variables at the begining of the function like > in [1]. Also I would advise to rearrange any "fixes" patches at the > begining of the patch series and "cleanup" patches at the end. > e.g. this looks like a fix to me. > > That way it is sometimes easier for people to cherry-pick any fixes if > required in their older kernel trees. ;) > Hi Ritesh, Thanks for feedback. I declare these variables at the begining of the function in next version. I agree that we should keep bugfix patches at the beginning. Actually, patch 1-8 are all fix patches from my view. So I think current patch sort is fine. Thanks.
diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c index 352ac9139fee..f24f80ecf318 100644 --- a/fs/ext4/mballoc.c +++ b/fs/ext4/mballoc.c @@ -2570,13 +2570,13 @@ void ext4_mb_prefetch_fini(struct super_block *sb, ext4_group_t group, unsigned int nr) { while (nr-- > 0) { - struct ext4_group_desc *gdp = ext4_get_group_desc(sb, group, - NULL); - struct ext4_group_info *grp = ext4_get_group_info(sb, group); + struct ext4_group_desc *gdp; + struct ext4_group_info *grp; if (!group) group = ext4_get_groups_count(sb); group--; + gdp = ext4_get_group_desc(sb, group, NULL); grp = ext4_get_group_info(sb, group); if (EXT4_MB_GRP_NEED_INIT(grp) &&
We always get ext4_group_desc with group + 1 and ext4_group_info with group to check if we need do initialize ext4_group_info for the group. Just get ext4_group_desc with group for ext4_group_info initialization check. Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com> --- fs/ext4/mballoc.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)