From: Jing Zhang <[email protected]>
Date: Mon Mar 22 22:20:38 2010
1, In one of the hot functions in mballoc.c, unnecessary operation on
struct ext4_group_desc *desc;
is removed.
2, Unlike spin_lock() and spin_unlock(), ext4_mb_load_buddy() is
companied by ext4_mb_release_desc. And it is hard, at least for me, to
get sure that ext4_mb_release_desc has little to do with struct
ext4_group_desc, and for those readers as well, whose native language
is not English. It seems the choice includes
ext4_mb_release_buddy
ext4_mb_put_buddy
ext4_mb_unload_buddy
and I like the last one.
Cc: Theodore Ts'o <[email protected]>
Cc: Andreas Dilger <[email protected]>
Cc: Dave Kleikamp <[email protected]>
Signed-off-by: Jing Zhang <[email protected]>
---
--- linux-2.6.32/fs/ext4/mballoc.c 2009-12-03 11:51:22.000000000 +0800
+++ ext4_mm_leak/mballoc4.c 2010-03-22 22:08:56.000000000 +0800
@@ -1150,7 +1150,7 @@ err:
return ret;
}
-static void ext4_mb_release_desc(struct ext4_buddy *e4b)
+static void ext4_mb_unload_buddy(struct ext4_buddy *e4b)
{
if (e4b->bd_bitmap_page)
page_cache_release(e4b->bd_bitmap_page);
@@ -1618,7 +1618,7 @@ int ext4_mb_try_best_found(struct ext4_a
}
ext4_unlock_group(ac->ac_sb, group);
- ext4_mb_release_desc(e4b);
+ ext4_mb_unload_buddy(e4b);
return 0;
}
@@ -1674,7 +1674,7 @@ int ext4_mb_find_by_goal(struct ext4_all
ext4_mb_use_best_found(ac, e4b);
}
ext4_unlock_group(ac->ac_sb, group);
- ext4_mb_release_desc(e4b);
+ ext4_mb_unload_buddy(e4b);
return 0;
}
@@ -2027,7 +2027,7 @@ repeat:
for (i = 0; i < ngroups; group++, i++) {
struct ext4_group_info *grp;
- struct ext4_group_desc *desc;
+ /* struct ext4_group_desc *desc; */
if (group == ngroups)
group = 0;
@@ -2045,12 +2045,12 @@ repeat:
if (!ext4_mb_good_group(ac, group, cr)) {
/* someone did allocation from this group */
ext4_unlock_group(sb, group);
- ext4_mb_release_desc(&e4b);
+ ext4_mb_unload_buddy(&e4b);
continue;
}
ac->ac_groups_scanned++;
- desc = ext4_get_group_desc(sb, group, NULL);
+ /* desc = ext4_get_group_desc(sb, group, NULL); */
if (cr == 0)
ext4_mb_simple_scan_group(ac, &e4b);
else if (cr == 1 &&
@@ -2060,7 +2060,7 @@ repeat:
ext4_mb_complex_scan_group(ac, &e4b);
ext4_unlock_group(sb, group);
- ext4_mb_release_desc(&e4b);
+ ext4_mb_unload_buddy(&e4b);
if (ac->ac_status != AC_STATUS_CONTINUE)
break;
@@ -2150,7 +2150,7 @@ static int ext4_mb_seq_groups_show(struc
ext4_lock_group(sb, group);
memcpy(&sg, ext4_get_group_info(sb, group), i);
ext4_unlock_group(sb, group);
- ext4_mb_release_desc(&e4b);
+ ext4_mb_unload_buddy(&e4b);
seq_printf(seq, "#%-5u: %-5u %-5u %-5u [", group, sg.info.bb_free,
sg.info.bb_fragments, sg.info.bb_first_free);
@@ -2567,7 +2567,7 @@ static void release_blocks_on_commit(jou
sb_issue_discard(sb, discard_block, entry->count);
kmem_cache_free(ext4_free_ext_cachep, entry);
- ext4_mb_release_desc(&e4b);
+ ext4_mb_unload_buddy(&e4b);
}
mb_debug(1, "freed %u blocks in %u structures\n", count, count2);
@@ -3692,7 +3692,7 @@ out:
ext4_unlock_group(sb, group);
if (ac)
kmem_cache_free(ext4_ac_cachep, ac);
- ext4_mb_release_desc(&e4b);
+ ext4_mb_unload_buddy(&e4b);
put_bh(bitmap_bh);
return free;
}
@@ -3796,7 +3796,7 @@ repeat:
if (bitmap_bh == NULL) {
ext4_error(sb, __func__, "Error in reading block "
"bitmap for %u", group);
- ext4_mb_release_desc(&e4b);
+ ext4_mb_unload_buddy(&e4b);
continue;
}
@@ -3805,7 +3805,7 @@ repeat:
ext4_mb_release_inode_pa(&e4b, bitmap_bh, pa, ac);
ext4_unlock_group(sb, group);
- ext4_mb_release_desc(&e4b);
+ ext4_mb_unload_buddy(&e4b);
put_bh(bitmap_bh);
list_del(&pa->u.pa_tmp_list);
@@ -4069,7 +4069,7 @@ ext4_mb_discard_lg_preallocations(struct
ext4_mb_release_group_pa(&e4b, pa, ac);
ext4_unlock_group(sb, group);
- ext4_mb_release_desc(&e4b);
+ ext4_mb_unload_buddy(&e4b);
list_del(&pa->u.pa_tmp_list);
call_rcu(&(pa)->u.pa_rcu, ext4_mb_pa_callback);
}
@@ -4570,7 +4570,7 @@ do_more:
atomic_add(count, &sbi->s_flex_groups[flex_group].free_blocks);
}
- ext4_mb_release_desc(&e4b);
+ ext4_mb_unload_buddy(&e4b);
*freed += count;
On Mon, Mar 22, 2010 at 10:35:54PM +0800, jing zhang wrote:
> From: Jing Zhang <[email protected]>
>
> Date: Mon Mar 22 22:20:38 2010
>
> 1, In one of the hot functions in mballoc.c, unnecessary operation on
> struct ext4_group_desc *desc;
> is removed.
>
> 2, Unlike spin_lock() and spin_unlock(), ext4_mb_load_buddy() is
> companied by ext4_mb_release_desc. And it is hard, at least for me, to
> get sure that ext4_mb_release_desc has little to do with struct
> ext4_group_desc, and for those readers as well, whose native language
> is not English. It seems the choice includes
> ext4_mb_release_buddy
> ext4_mb_put_buddy
> ext4_mb_unload_buddy
> and I like the last one.
Please separate things like this into two separate patches; it makes
them much easier to review.
Thanks,
- Ted
On 2010-03-22, at 08:35, jing zhang wrote:
> @@ -2027,7 +2027,7 @@ repeat:
>
> for (i = 0; i < ngroups; group++, i++) {
> struct ext4_group_info *grp;
> - struct ext4_group_desc *desc;
> + /* struct ext4_group_desc *desc; */
>
> if (group == ngroups)
> group = 0;
> @@ -2045,12 +2045,12 @@ repeat:
>
> ac->ac_groups_scanned++;
> - desc = ext4_get_group_desc(sb, group, NULL);
> + /* desc = ext4_get_group_desc(sb, group, NULL); */
> if (cr == 0)
> ext4_mb_simple_scan_group(ac, &e4b);
> else if (cr == 1 &&
Rather than commenting these lines out, please just delete them. They
serve no purpose, and leaving them in the code will just add confusion
in the future.
Cheers, Andreas
--
Andreas Dilger
Sr. Staff Engineer, Lustre Group
Sun Microsystems of Canada, Inc.
2010/3/23, [email protected] <[email protected]>:
> On Mon, Mar 22, 2010 at 10:35:54PM +0800, jing zhang wrote:
>> From: Jing Zhang <[email protected]>
>>
>> Date: Mon Mar 22 22:20:38 2010
>>
>> 1, In one of the hot functions in mballoc.c, unnecessary operation on
>> struct ext4_group_desc *desc;
>> is removed.
---
diff --git a/linux-2.6.32/fs/ext4/mballoc.c b/ext4_mm_leak/mballoc5.c
index bba1282..fb18199 100644
--- a/linux-2.6.32/fs/ext4/mballoc.c
+++ b/ext4_mm_leak/mballoc5.c
@@ -2027,7 +2027,6 @@ repeat:
for (i = 0; i < ngroups; group++, i++) {
struct ext4_group_info *grp;
- struct ext4_group_desc *desc;
if (group == ngroups)
group = 0;
@@ -2050,7 +2049,6 @@ repeat:
}
ac->ac_groups_scanned++;
- desc = ext4_get_group_desc(sb, group, NULL);
if (cr == 0)
ext4_mb_simple_scan_group(ac, &e4b);
else if (cr == 1 &&
>> 2, Unlike spin_lock() and spin_unlock(), ext4_mb_load_buddy() is
>> companied by ext4_mb_release_desc. And it is hard, at least for me, to
>> get sure that ext4_mb_release_desc has little to do with struct
>> ext4_group_desc, and for those readers as well, whose native language
>> is not English. It seems the choice includes
>> ext4_mb_release_buddy
>> ext4_mb_put_buddy
>> ext4_mb_unload_buddy
>> and I like the last one.
---
diff --git a/linux-2.6.32/fs/ext4/mballoc.c b/ext4_mm_leak/mballoc4.c
index bba1282..45d78a9 100644
--- a/linux-2.6.32/fs/ext4/mballoc.c
+++ b/ext4_mm_leak/mballoc4.c
@@ -1150,7 +1150,7 @@ err:
return ret;
}
-static void ext4_mb_release_desc(struct ext4_buddy *e4b)
+static void ext4_mb_unload_buddy(struct ext4_buddy *e4b)
{
if (e4b->bd_bitmap_page)
page_cache_release(e4b->bd_bitmap_page);
@@ -1618,7 +1618,7 @@ int ext4_mb_try_best_found(struct
ext4_allocation_context *ac,
}
ext4_unlock_group(ac->ac_sb, group);
- ext4_mb_release_desc(e4b);
+ ext4_mb_unload_buddy(e4b);
return 0;
}
@@ -1674,7 +1674,7 @@ int ext4_mb_find_by_goal(struct
ext4_allocation_context *ac,
ext4_mb_use_best_found(ac, e4b);
}
ext4_unlock_group(ac->ac_sb, group);
- ext4_mb_release_desc(e4b);
+ ext4_mb_unload_buddy(e4b);
return 0;
}
@@ -2045,7 +2045,7 @@ repeat:
if (!ext4_mb_good_group(ac, group, cr)) {
/* someone did allocation from this group */
ext4_unlock_group(sb, group);
- ext4_mb_release_desc(&e4b);
+ ext4_mb_unload_buddy(&e4b);
continue;
}
@@ -2060,7 +2060,7 @@ repeat:
ext4_mb_complex_scan_group(ac, &e4b);
ext4_unlock_group(sb, group);
- ext4_mb_release_desc(&e4b);
+ ext4_mb_unload_buddy(&e4b);
if (ac->ac_status != AC_STATUS_CONTINUE)
break;
@@ -2150,7 +2150,7 @@ static int ext4_mb_seq_groups_show(struct
seq_file *seq, void *v)
ext4_lock_group(sb, group);
memcpy(&sg, ext4_get_group_info(sb, group), i);
ext4_unlock_group(sb, group);
- ext4_mb_release_desc(&e4b);
+ ext4_mb_unload_buddy(&e4b);
seq_printf(seq, "#%-5u: %-5u %-5u %-5u [", group, sg.info.bb_free,
sg.info.bb_fragments, sg.info.bb_first_free);
@@ -2567,7 +2567,7 @@ static void release_blocks_on_commit(journal_t
*journal, transaction_t *txn)
sb_issue_discard(sb, discard_block, entry->count);
kmem_cache_free(ext4_free_ext_cachep, entry);
- ext4_mb_release_desc(&e4b);
+ ext4_mb_unload_buddy(&e4b);
}
mb_debug(1, "freed %u blocks in %u structures\n", count, count2);
@@ -3692,7 +3692,7 @@ out:
ext4_unlock_group(sb, group);
if (ac)
kmem_cache_free(ext4_ac_cachep, ac);
- ext4_mb_release_desc(&e4b);
+ ext4_mb_unload_buddy(&e4b);
put_bh(bitmap_bh);
return free;
}
@@ -3796,7 +3796,7 @@ repeat:
if (bitmap_bh == NULL) {
ext4_error(sb, __func__, "Error in reading block "
"bitmap for %u", group);
- ext4_mb_release_desc(&e4b);
+ ext4_mb_unload_buddy(&e4b);
continue;
}
@@ -3805,7 +3805,7 @@ repeat:
ext4_mb_release_inode_pa(&e4b, bitmap_bh, pa, ac);
ext4_unlock_group(sb, group);
- ext4_mb_release_desc(&e4b);
+ ext4_mb_unload_buddy(&e4b);
put_bh(bitmap_bh);
list_del(&pa->u.pa_tmp_list);
@@ -4069,7 +4069,7 @@ ext4_mb_discard_lg_preallocations(struct super_block *sb,
ext4_mb_release_group_pa(&e4b, pa, ac);
ext4_unlock_group(sb, group);
- ext4_mb_release_desc(&e4b);
+ ext4_mb_unload_buddy(&e4b);
list_del(&pa->u.pa_tmp_list);
call_rcu(&(pa)->u.pa_rcu, ext4_mb_pa_callback);
}
@@ -4570,7 +4570,7 @@ do_more:
atomic_add(count, &sbi->s_flex_groups[flex_group].free_blocks);
}
- ext4_mb_release_desc(&e4b);
+ ext4_mb_unload_buddy(&e4b);
*freed += count;
> Please separate things like this into two separate patches; it makes
> them much easier to review.
>
> Thanks,
>
> - Ted
>
Thanks for separating the patches!
Since you included a Signed-off-by in the earlier version of the
patches, I'll assume it applies two these two patches. For future
reference, though, it saves me time and effort if you follow the
guidelines specified in Documentation/SubmittingPatches, especially
the section 15, ("The canonical patch format") which talks about using
a separate e-mail for each patch (chained together using the
In-reply-to: mail header fields).
Among other reasons, it allows me to keep track of patches using the
patchwork tool (see: http://patchwork.ozlabs.org/project/linux-ext4)
so I don't forget about patches which are sent to me. It also allows
me to use tools such as "git am" to automatically apply patches.
Note that there are tools like "git format-patches" and "git
send-email" which make it very easy to send out patches which are
compliant to the these recomendations. They are not required, but
they do make your job easier.
I'll fix up these patches by hand, so it's no big deal, but in the
future, it streamlines things if you follow the SubmittingPatches
guidelines. (See also the SubmitChecklist in the Documentation
directory of the Linux sources).
Many thanks,
- Ted
2010/3/24, [email protected] <[email protected]>:
> Thanks for separating the patches!
>
> Since you included a Signed-off-by in the earlier version of the
> patches, I'll assume it applies two these two patches. For future
> reference, though, it saves me time and effort if you follow the
> guidelines specified in Documentation/SubmittingPatches, especially
> the section 15, ("The canonical patch format") which talks about using
> a separate e-mail for each patch (chained together using the
> In-reply-to: mail header fields).
>
> Among other reasons, it allows me to keep track of patches using the
> patchwork tool (see: http://patchwork.ozlabs.org/project/linux-ext4)
> so I don't forget about patches which are sent to me. It also allows
> me to use tools such as "git am" to automatically apply patches.
>
> Note that there are tools like "git format-patches" and "git
> send-email" which make it very easy to send out patches which are
> compliant to the these recomendations. They are not required, but
> they do make your job easier.
>
> I'll fix up these patches by hand, so it's no big deal, but in the
> future, it streamlines things if you follow the SubmittingPatches
> guidelines. (See also the SubmitChecklist in the Documentation
> directory of the Linux sources).
>
> Many thanks,
>
> - Ted
>
Evening,
Sorry for my last reply that causes you have to deal the patches by
hand, and thank you, Ted, for your patience.
I will try hard to follow the guidelines specified in
Documentation/SubmittingPatches, and to learn how to git
format-patches.
Best
- zj
On Tue, 23 Mar 2010 20:40:29 +0800, jing zhang <[email protected]> wrote:
> 2010/3/23, [email protected] <[email protected]>:
> > On Mon, Mar 22, 2010 at 10:35:54PM +0800, jing zhang wrote:
> >> From: Jing Zhang <[email protected]>
> >>
> >> Date: Mon Mar 22 22:20:38 2010
> >>
> >> 1, In one of the hot functions in mballoc.c, unnecessary operation on
> >> struct ext4_group_desc *desc;
> >> is removed.
>
> ---
>
> diff --git a/linux-2.6.32/fs/ext4/mballoc.c b/ext4_mm_leak/mballoc5.c
> index bba1282..fb18199 100644
> --- a/linux-2.6.32/fs/ext4/mballoc.c
> +++ b/ext4_mm_leak/mballoc5.c
> @@ -2027,7 +2027,6 @@ repeat:
>
> for (i = 0; i < ngroups; group++, i++) {
> struct ext4_group_info *grp;
> - struct ext4_group_desc *desc;
>
> if (group == ngroups)
> group = 0;
> @@ -2050,7 +2049,6 @@ repeat:
> }
>
> ac->ac_groups_scanned++;
> - desc = ext4_get_group_desc(sb, group, NULL);
> if (cr == 0)
> ext4_mb_simple_scan_group(ac, &e4b);
> else if (cr == 1 &&
>
We missed that in commit 75507efb1372b6acf1aa6bf00ebd49ce196fd994
ealier we had an uninit check around that.
-aneesh
From: Jing Zhang <[email protected]>
Signed-off-by: Jing Zhang <[email protected]>
Signed-off-by: "Theodore Ts'o" <[email protected]>
---
fs/ext4/mballoc.c | 2 --
1 files changed, 0 insertions(+), 2 deletions(-)
diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index 54df209..fcc919f 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -2024,7 +2024,6 @@ repeat:
for (i = 0; i < ngroups; group++, i++) {
struct ext4_group_info *grp;
- struct ext4_group_desc *desc;
if (group == ngroups)
group = 0;
@@ -2047,7 +2046,6 @@ repeat:
}
ac->ac_groups_scanned++;
- desc = ext4_get_group_desc(sb, group, NULL);
if (cr == 0)
ext4_mb_simple_scan_group(ac, &e4b);
else if (cr == 1 &&
--
1.6.6.1.1.g974db.dirty
From: Jing Zhang <[email protected]>
This function cleans up after ext4_mb_load_buddy(), so the renaming
makes the code clearer.
Signed-off-by: Jing Zhang <[email protected]>
Signed-off-by: "Theodore Ts'o" <[email protected]>
---
fs/ext4/mballoc.c | 24 ++++++++++++------------
1 files changed, 12 insertions(+), 12 deletions(-)
diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index fcc919f..f9fe733 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -1149,7 +1149,7 @@ err:
return ret;
}
-static void ext4_mb_release_desc(struct ext4_buddy *e4b)
+static void ext4_mb_unload_buddy(struct ext4_buddy *e4b)
{
if (e4b->bd_bitmap_page)
page_cache_release(e4b->bd_bitmap_page);
@@ -1616,7 +1616,7 @@ int ext4_mb_try_best_found(struct ext4_allocation_context *ac,
}
ext4_unlock_group(ac->ac_sb, group);
- ext4_mb_release_desc(e4b);
+ ext4_mb_unload_buddy(e4b);
return 0;
}
@@ -1671,7 +1671,7 @@ int ext4_mb_find_by_goal(struct ext4_allocation_context *ac,
ext4_mb_use_best_found(ac, e4b);
}
ext4_unlock_group(ac->ac_sb, group);
- ext4_mb_release_desc(e4b);
+ ext4_mb_unload_buddy(e4b);
return 0;
}
@@ -2041,7 +2041,7 @@ repeat:
if (!ext4_mb_good_group(ac, group, cr)) {
/* someone did allocation from this group */
ext4_unlock_group(sb, group);
- ext4_mb_release_desc(&e4b);
+ ext4_mb_unload_buddy(&e4b);
continue;
}
@@ -2055,7 +2055,7 @@ repeat:
ext4_mb_complex_scan_group(ac, &e4b);
ext4_unlock_group(sb, group);
- ext4_mb_release_desc(&e4b);
+ ext4_mb_unload_buddy(&e4b);
if (ac->ac_status != AC_STATUS_CONTINUE)
break;
@@ -2145,7 +2145,7 @@ static int ext4_mb_seq_groups_show(struct seq_file *seq, void *v)
ext4_lock_group(sb, group);
memcpy(&sg, ext4_get_group_info(sb, group), i);
ext4_unlock_group(sb, group);
- ext4_mb_release_desc(&e4b);
+ ext4_mb_unload_buddy(&e4b);
seq_printf(seq, "#%-5u: %-5u %-5u %-5u [", group, sg.info.bb_free,
sg.info.bb_fragments, sg.info.bb_first_free);
@@ -2564,7 +2564,7 @@ static void release_blocks_on_commit(journal_t *journal, transaction_t *txn)
sb_issue_discard(sb, discard_block, entry->count);
}
kmem_cache_free(ext4_free_ext_cachep, entry);
- ext4_mb_release_desc(&e4b);
+ ext4_mb_unload_buddy(&e4b);
}
mb_debug(1, "freed %u blocks in %u structures\n", count, count2);
@@ -3693,7 +3693,7 @@ out:
ext4_unlock_group(sb, group);
if (ac)
kmem_cache_free(ext4_ac_cachep, ac);
- ext4_mb_release_desc(&e4b);
+ ext4_mb_unload_buddy(&e4b);
put_bh(bitmap_bh);
return free;
}
@@ -3797,7 +3797,7 @@ repeat:
if (bitmap_bh == NULL) {
ext4_error(sb, "Error reading block bitmap for %u",
group);
- ext4_mb_release_desc(&e4b);
+ ext4_mb_unload_buddy(&e4b);
continue;
}
@@ -3806,7 +3806,7 @@ repeat:
ext4_mb_release_inode_pa(&e4b, bitmap_bh, pa, ac);
ext4_unlock_group(sb, group);
- ext4_mb_release_desc(&e4b);
+ ext4_mb_unload_buddy(&e4b);
put_bh(bitmap_bh);
list_del(&pa->u.pa_tmp_list);
@@ -4070,7 +4070,7 @@ ext4_mb_discard_lg_preallocations(struct super_block *sb,
ext4_mb_release_group_pa(&e4b, pa, ac);
ext4_unlock_group(sb, group);
- ext4_mb_release_desc(&e4b);
+ ext4_mb_unload_buddy(&e4b);
list_del(&pa->u.pa_tmp_list);
call_rcu(&(pa)->u.pa_rcu, ext4_mb_pa_callback);
}
@@ -4606,7 +4606,7 @@ do_more:
atomic_add(count, &sbi->s_flex_groups[flex_group].free_blocks);
}
- ext4_mb_release_desc(&e4b);
+ ext4_mb_unload_buddy(&e4b);
freed += count;
--
1.6.6.1.1.g974db.dirty