Home » Mailing list archives » linux-ext4 » 2010 » April » 2

[PATCH 2/2] ext4: rename ext4_mb_release_desc() to ext4_mb_unload_buddy()

Previous thread: [Bug 15576] Data Loss (flex_bg and ext4_mb_generate_buddy errors) by bugzilla-daemon on Sunday, March 21, 2010 - 10:34 pm. (1 message)

Next thread: [PATCH] ext4: Fix buffer head ref leaks after calls to ext4_get_inode_loc() by Curt Wohlgemuth on Monday, March 22, 2010 - 8:46 am. (2 messages)
[view original message]
From: jing zhang
Subject: [PATCH] ext4: remove unnecessary operation in ext4_mb_regular_allocator()
Date: Monday, March 22, 2010 - 7:35 am

From: Jing Zhang <zj.barak@gmail.com>

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 <tytso@mit.edu>
Cc: Andreas Dilger <adilger@sun.com>
Cc: Dave Kleikamp <shaggy@linux.vnet.ibm.com>
Signed-off-by: Jing Zhang <zj.barak@gmail.com>

---

--- 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, ...
[ message continues ]
[view original message]
From: tytso
Subject: Re: [PATCH] ext4: remove unnecessary operation in ext4_mb_regular_allocator()
Date: Monday, March 22, 2010 - 11:26 am

Please separate things like this into two separate patches; it makes
them much easier to review.

Thanks,

					- Ted
--


[ message continues ]
[view original message]
From: jing zhang
Subject: Re: [PATCH] ext4: remove unnecessary operation in ext4_mb_regular_allocator()
Date: Tuesday, March 23, 2010 - 5:40 am

---

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);

---

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)
 ...
[ message continues ]
[view original message]
From: tytso
Subject: Re: [PATCH] ext4: remove unnecessary operation in ext4_mb_regular_allocator()
Date: Tuesday, March 23, 2010 - 9:56 am

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
--


[ message continues ]
[view original message]
From: jing zhang
Subject: Re: [PATCH] ext4: remove unnecessary operation in ext4_mb_regular_allocator()
Date: Wednesday, March 24, 2010 - 5:14 am

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
--


[ message continues ]
[view original message]
From: Theodore Ts'o
Subject: [PATCH 1/2] ext4: Remove unnecessary call to ext4_get_group_desc() in mballoc
Date: Thursday, April 1, 2010 - 6:48 pm

From: Jing Zhang <zj.barak@gmail.com>

Signed-off-by: Jing Zhang <zj.barak@gmail.com>
Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
---
 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

--


[ message continues ]
[view original message]
From: Theodore Ts'o
Subject: [PATCH 2/2] ext4: rename ext4_mb_release_desc() to ext4_mb_unload_buddy()
Date: Thursday, April 1, 2010 - 6:48 pm

From: Jing Zhang <zj.barak@gmail.com>

This function cleans up after ext4_mb_load_buddy(), so the renaming
makes the code clearer.

Signed-off-by: Jing Zhang <zj.barak@gmail.com>
Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
---
 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,
 ...
[ message continues ]
[view original message]
From: Aneesh Kumar K. V
Subject: Re: [PATCH] ext4: remove unnecessary operation in ext4_mb_regular_allocator()
Date: Wednesday, March 24, 2010 - 10:23 am

We missed that in commit 75507efb1372b6acf1aa6bf00ebd49ce196fd994
ealier we had an uninit check around that. 


-aneesh
--


[ message continues ]
[view original message]
From: Andreas Dilger
Subject: Re: [PATCH] ext4: remove unnecessary operation in ext4_mb_regular_allocator()
Date: Monday, March 22, 2010 - 8:42 pm

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.

--


[ message continues ]
Previous thread: [Bug 15576] Data Loss (flex_bg and ext4_mb_generate_buddy errors) by bugzilla-daemon on Sunday, March 21, 2010 - 10:34 pm. (1 message)

Next thread: [PATCH] ext4: Fix buffer head ref leaks after calls to ext4_get_inode_loc() by Curt Wohlgemuth on Monday, March 22, 2010 - 8:46 am. (2 messages)